Moodle
  1. Moodle
  2. MDL-8973

Improve OOP model of new multi auth plugins

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.8
    • Fix Version/s: 1.8
    • Component/s: Authentication
    • Labels:
      None
    • Affected Branches:
      MOODLE_18_STABLE
    • Fixed Branches:
      MOODLE_18_STABLE
    • Rank:
      28960

      Description

      Fix OOP model of new multi auth plugins; add hooks for pre-login and pre-logout actions (needed for sso plugins)

      patch ready: testing...

      1. auth_oop_hooks.patch
        56 kB
        Petr Škoda
      2. patchfile.txt
        0.6 kB
        Donal McMullan

        Issue Links

          Activity

          Hide
          Petr Škoda added a comment -

          also fix creators unassignments

          Show
          Petr Škoda added a comment - also fix creators unassignments
          Hide
          Petr Škoda added a comment -

          and update dev docs

          Show
          Petr Škoda added a comment - and update dev docs
          Hide
          Petr Škoda added a comment -

          attaching raw patch, needs a lot of testing,
          should solve all problems above and some more

          Show
          Petr Škoda added a comment - attaching raw patch, needs a lot of testing, should solve all problems above and some more
          Hide
          Petr Škoda added a comment -

          committed into cvs, please reopen or comment in case of any problems or regressions, thanks.

          Show
          Petr Škoda added a comment - committed into cvs, please reopen or comment in case of any problems or regressions, thanks.
          Hide
          Martín Langhoff added a comment -

          Petr - I am really not happy about not being told about this - there are good reasons why the API was NOT made as an extendable class. It is stupid to change this without consultation.

          Can you please revert it and discuss?

          I've added myself as a watcher now.

          Show
          Martín Langhoff added a comment - Petr - I am really not happy about not being told about this - there are good reasons why the API was NOT made as an extendable class. It is stupid to change this without consultation. Can you please revert it and discuss? I've added myself as a watcher now.
          Hide
          Petr Škoda added a comment -

          Hi Martín,

          it all started when I noticed that the conversion to multi auth was not finished and many things did not work. First I was reporting bugs then gave up and instead fixed the code. Now we are very close to the release, I would prefer to keep the code the way it is now because I can not work on it anymore.

          I will fix any bugs and regressions reported ASAP in the current auth code.

          My last commit is no big change - the api is the same, I have eliminated all method_exist calls, implemented proper OOP, fixed inline docs, found some bugs and did test the code. I believe that this solution will prevent possible future headaches and allows easier implementation of 3rd party auth plugins without constant hacking of core and cvs conflicts.

          I will be out of town during the weekend, if you prepare a reasonable patch with improvements I would be more than happy to commit it. Any testing of the current code and bug reports would be really appreciated too.

          The API is not an extendable class - extending here only helps with implementation. You do not have to extend the class - you only have to implement the same methods as the base (good old copy/paste). I know you will say we had a nice method_exists() function - no, this not a proper way to do OOP and it was a PITA for me to study+fix the old code during the first round of cleanup. Proper API in OOP is defined using interfaces which we unfortunately can not use now. All docs and instructions are now placed in the base class, you can find all needed info there without reading README, README2 and inline comments in several plugins.

          Could you please describe your good reasons?
          Could you give some real world examples?
          What exactly does the current implementation break for you?
          What problems do you think it will cause in the future?

          skodak

          Show
          Petr Škoda added a comment - Hi Martín, it all started when I noticed that the conversion to multi auth was not finished and many things did not work. First I was reporting bugs then gave up and instead fixed the code. Now we are very close to the release, I would prefer to keep the code the way it is now because I can not work on it anymore. I will fix any bugs and regressions reported ASAP in the current auth code. My last commit is no big change - the api is the same, I have eliminated all method_exist calls, implemented proper OOP, fixed inline docs, found some bugs and did test the code. I believe that this solution will prevent possible future headaches and allows easier implementation of 3rd party auth plugins without constant hacking of core and cvs conflicts. I will be out of town during the weekend, if you prepare a reasonable patch with improvements I would be more than happy to commit it. Any testing of the current code and bug reports would be really appreciated too. The API is not an extendable class - extending here only helps with implementation. You do not have to extend the class - you only have to implement the same methods as the base (good old copy/paste). I know you will say we had a nice method_exists() function - no, this not a proper way to do OOP and it was a PITA for me to study+fix the old code during the first round of cleanup. Proper API in OOP is defined using interfaces which we unfortunately can not use now. All docs and instructions are now placed in the base class, you can find all needed info there without reading README, README2 and inline comments in several plugins. Could you please describe your good reasons? Could you give some real world examples? What exactly does the current implementation break for you? What problems do you think it will cause in the future? skodak
          Hide
          Petr Škoda added a comment -

          I agree the description in README was not good, I have committed some more explanation there.

          Also please note that I am the maintainer of all user editing, password changing, singup, forget_password, login page, user setup and the like. These areas were rewritten/changed/improved in 1.8 and actually those are the main "users" of auth plugins - I did some changes in auth in order to get things implemented properly in other parts.

          Show
          Petr Škoda added a comment - I agree the description in README was not good, I have committed some more explanation there. Also please note that I am the maintainer of all user editing, password changing, singup, forget_password, login page, user setup and the like. These areas were rewritten/changed/improved in 1.8 and actually those are the main "users" of auth plugins - I did some changes in auth in order to get things implemented properly in other parts.
          Hide
          Petr Škoda added a comment -

          ... the "old way" with method_exists() was not compatible with phpdocs, you would learn nothing from reading the docs; now you can find out a lot about auth system just from reading of auth_plugin_base class description

          Show
          Petr Škoda added a comment - ... the "old way" with method_exists() was not compatible with phpdocs, you would learn nothing from reading the docs; now you can find out a lot about auth system just from reading of auth_plugin_base class description
          Hide
          Martin Dougiamas added a comment -

          /me watches from sidelines.

          Petr, can you link those bugs to this one, please?

          Martín, can you explain what you mean about the good reasons?

          Show
          Martin Dougiamas added a comment - /me watches from sidelines. Petr, can you link those bugs to this one, please? Martín, can you explain what you mean about the good reasons?
          Hide
          Martin Dougiamas added a comment -

          Reopening until Martín L gets back on this one.

          Show
          Martin Dougiamas added a comment - Reopening until Martín L gets back on this one.
          Hide
          Petr Škoda added a comment -

          MD, you you mean list of bugs fixed by the last patch?
          1/ change password - wrong checks if the users auth plugin allows changing of password
          2/ typo in condition // just change auth and skip update when using advanced edit - changes not saved in ldap (my regression)
          3/ fixed creators syn - now only removes assignment if previously assigned by plugin
          4/ updated docs

          The previous cleanup is described in MDL-8590 , some method_exists() were already removed at that time too.

          Show
          Petr Škoda added a comment - MD, you you mean list of bugs fixed by the last patch? 1/ change password - wrong checks if the users auth plugin allows changing of password 2/ typo in condition // just change auth and skip update when using advanced edit - changes not saved in ldap (my regression) 3/ fixed creators syn - now only removes assignment if previously assigned by plugin 4/ updated docs The previous cleanup is described in MDL-8590 , some method_exists() were already removed at that time too.
          Hide
          Martín Langhoff added a comment -

          Hi Petr,

          thanks for reopening. I have no problem with the bugfixes applied. However, the API changes to have a base class that implements all the methods is a bad approach. There is no need to "fix" something that was done the right way

          Here is why.

          First - from a theory and fuzzy handwaving POV, OOP does not require use of subclassing/inheritance. Subclassing/inheritance makes for very tight coupling. Which is fine for some areas you might be programming, but not for an API. This is when you actually have the base-class doing something, manipulating data / state. Subclassing is tight coupling because the contact surface is all the data private to the class - a trivial change in how you initialise variables in the subclass might break a subclass. "Internal" variable renames will break a subclass. Obviously this is tight coupling, and not suitable for an API where people will want to maintain "private" plugins.

          So, first part: we need loose coupling here, and subclassing is tight coupling.

          Second part - we actually have an empty base class, which does not make use of (the good aspects of) using inheritance. The only thing it does it that – by virtue of implementing empty methods, it breaks method_exists() for no good reason. It is important and useful to know if a plugin implements method X, which is different from failure.

          if (method_exists($plugin, 'foo'))

          { // it can do foo print button to offer to do fo }

          if (method_exists($plugin, 'foo') && $formdata->dofoo === 'yes') {
          if ($plugin->foo($param))

          { print fooed successfully! }

          else

          { print error fooing, the remote server may be frobbed }

          }

          Having a base class with methods that exist but do nothing serves no purpose. Actually – haviung to write method_exists() makes it really clear to a programmer what is happening: we are asking whether the plugin can do foo(), and acting accordingly. It may mean doing something else instead, of showing the end user something different.

          If I see $plugin->dofoo() retunring false, I interpret it as a failure. Not as "method not implemented". If the method is not implemented, that is tested with method_exists() It is a matter of using the good semantics that PHP has.

          So - my point #2 is that semantics matter, and we had the right semantics.

          In terms of moving forward, I am not too stressed if you REALLY want to have a base class for the weird case that sometime in the hopefully never future we really need to add a method there. That is fine , however, what I think really matters is that if you want to list all the methods in that base class as documentation for programmers... let's have them commented out, as mere examples.

          Then,

          • calls to all optional methods are wrapped in method_exists() which makes it clear that they may or may not be there, and that the caller has to handle things accordingly
          • mandatory methods don't need to be wrapped, and will fail correctly- even better, PHP error reporting will report the right thing! (missing method)
          • programmers used to going to the base class for the "reference" implementation will find what they are expecting, in nice sample methods that are commented out

          My day is a bit crazy today but I'll put together a sample patch for this.

          Show
          Martín Langhoff added a comment - Hi Petr, thanks for reopening. I have no problem with the bugfixes applied. However, the API changes to have a base class that implements all the methods is a bad approach. There is no need to "fix" something that was done the right way Here is why. First - from a theory and fuzzy handwaving POV, OOP does not require use of subclassing/inheritance. Subclassing/inheritance makes for very tight coupling. Which is fine for some areas you might be programming, but not for an API. This is when you actually have the base-class doing something , manipulating data / state. Subclassing is tight coupling because the contact surface is all the data private to the class - a trivial change in how you initialise variables in the subclass might break a subclass. "Internal" variable renames will break a subclass. Obviously this is tight coupling, and not suitable for an API where people will want to maintain "private" plugins. So, first part: we need loose coupling here, and subclassing is tight coupling. Second part - we actually have an empty base class, which does not make use of (the good aspects of) using inheritance. The only thing it does it that – by virtue of implementing empty methods, it breaks method_exists() for no good reason. It is important and useful to know if a plugin implements method X, which is different from failure. if (method_exists($plugin, 'foo')) { // it can do foo print button to offer to do fo } if (method_exists($plugin, 'foo') && $formdata->dofoo === 'yes') { if ($plugin->foo($param)) { print fooed successfully! } else { print error fooing, the remote server may be frobbed } } Having a base class with methods that exist but do nothing serves no purpose. Actually – haviung to write method_exists() makes it really clear to a programmer what is happening: we are asking whether the plugin can do foo(), and acting accordingly. It may mean doing something else instead, of showing the end user something different. If I see $plugin->dofoo() retunring false, I interpret it as a failure. Not as "method not implemented". If the method is not implemented, that is tested with method_exists() It is a matter of using the good semantics that PHP has. So - my point #2 is that semantics matter, and we had the right semantics. In terms of moving forward, I am not too stressed if you REALLY want to have a base class for the weird case that sometime in the hopefully never future we really need to add a method there. That is fine , however, what I think really matters is that if you want to list all the methods in that base class as documentation for programmers... let's have them commented out, as mere examples. Then, calls to all optional methods are wrapped in method_exists() which makes it clear that they may or may not be there, and that the caller has to handle things accordingly mandatory methods don't need to be wrapped, and will fail correctly- even better, PHP error reporting will report the right thing! (missing method) programmers used to going to the base class for the "reference" implementation will find what they are expecting, in nice sample methods that are commented out My day is a bit crazy today but I'll put together a sample patch for this.
          Hide
          Martin Dougiamas added a comment -

          Thanks a lot Martin, appreciate your time on this, I know you're flat out. Especially on something you thought was done.

          If we can get to a happy place by a small move forward rather than a big move back that would be best.

          Show
          Martin Dougiamas added a comment - Thanks a lot Martin, appreciate your time on this, I know you're flat out. Especially on something you thought was done. If we can get to a happy place by a small move forward rather than a big move back that would be best.
          Hide
          Martín Langhoff added a comment -

          Petr, MartinD - thanks for you patience on what bit of a whine

          (that I think makes sense, however, otherwise I wouldn't want to waste anyone's time or patience)

          I'll prep a patch to illustrate what I'd like to happen, and one other thing I wanted to mention is... if I drop the ball (or the Catalyst team drops the ball does) or we generally cause frustration... please let us know. I am trying hard to organise things here so we are all a lot more Moodle.org-facing and generally pulling our own weight. Not just Penny & me! But as any process... it's leaky and imperfect, and we might get distracted. Yell at us everyonce in a while. Good for the spirit

          Show
          Martín Langhoff added a comment - Petr, MartinD - thanks for you patience on what bit of a whine (that I think makes sense, however, otherwise I wouldn't want to waste anyone's time or patience) I'll prep a patch to illustrate what I'd like to happen, and one other thing I wanted to mention is... if I drop the ball (or the Catalyst team drops the ball does) or we generally cause frustration... please let us know. I am trying hard to organise things here so we are all a lot more Moodle.org-facing and generally pulling our own weight. Not just Penny & me! But as any process... it's leaky and imperfect, and we might get distracted. Yell at us everyonce in a while. Good for the spirit
          Hide
          Martin Dougiamas added a comment -

          Just a quick note, the latest 1.8 CVS gives this:

          PHP Fatal error: Cannot redeclare class auth_plugin_base:auth_plugin_manual in /home/moodtest/public_html/auth/manual/auth.php on line 23

          Show
          Martin Dougiamas added a comment - Just a quick note, the latest 1.8 CVS gives this: PHP Fatal error: Cannot redeclare class auth_plugin_base:auth_plugin_manual in /home/moodtest/public_html/auth/manual/auth.php on line 23
          Hide
          Martin Dougiamas added a comment -

          That's on admin/index.php

          Show
          Martin Dougiamas added a comment - That's on admin/index.php
          Hide
          Petr Škoda added a comment -

          Hi!

          1/ I agree the use of can_do_something() methods is not good, but using method_exists is IMO worse. I guess we would agree that the correct approach would be to use interfaces that define what each function can do and later testing if object instance has interace.

          And then there is the case when the capability is not hardcoded - see example from ldap:
          function can_change_password()

          { return !empty($this->config->stdchangepassword) or !empty($this->config->changepasswordurl); }

          How would you solve this with interfaces?? You may have a class that can do all of these, but only when given the correct config settings or enabled. The result is that can_ is more flexible that method exists or interfaces, which is IMHO exactly what we want here.

          We could discuss theory for hours, but do you think that having can_change_password(), can_confirm(), can_reset_password() and can_signup() methods makes the code of auth classes harder to understand? Does it make code that uses auth methods harder to read? Is an abstract class good for ppl starting that are starting coding of auth plugin?

          2/ empty base class is usually called abstract class - you can use it or you do not have to use it, it is just a semi product that helps with implementation; the only problem is that you can not write abstract class for objects with lots of changing interfaces.

          _____________________

          Anyway I think that the current code is easier to read+understand and is easier to maintain.

          For 3rd party code it can be either worse or better depending on what they do:
          a/ if they add new methods and do some fundamental changes - it may be worse or better depending on coding style
          b/ for basic plugin that extend abstract class - it is IMO better now

          I will respect any decision by MD, the backup plan would be to remove those four can_ functions, move out the conditional methods (around 10) from abstract class and reintroduce the tons of if(methodexists) + write docs explaining why some methods are not required. (And then complain silently each time I have to change something in code that uses auth methods.)

          Show
          Petr Škoda added a comment - Hi! 1/ I agree the use of can_do_something() methods is not good, but using method_exists is IMO worse. I guess we would agree that the correct approach would be to use interfaces that define what each function can do and later testing if object instance has interace. And then there is the case when the capability is not hardcoded - see example from ldap: function can_change_password() { return !empty($this->config->stdchangepassword) or !empty($this->config->changepasswordurl); } How would you solve this with interfaces?? You may have a class that can do all of these, but only when given the correct config settings or enabled. The result is that can_ is more flexible that method exists or interfaces, which is IMHO exactly what we want here. We could discuss theory for hours, but do you think that having can_change_password(), can_confirm(), can_reset_password() and can_signup() methods makes the code of auth classes harder to understand? Does it make code that uses auth methods harder to read? Is an abstract class good for ppl starting that are starting coding of auth plugin? 2/ empty base class is usually called abstract class - you can use it or you do not have to use it, it is just a semi product that helps with implementation; the only problem is that you can not write abstract class for objects with lots of changing interfaces. _____________________ Anyway I think that the current code is easier to read+understand and is easier to maintain. For 3rd party code it can be either worse or better depending on what they do: a/ if they add new methods and do some fundamental changes - it may be worse or better depending on coding style b/ for basic plugin that extend abstract class - it is IMO better now I will respect any decision by MD, the backup plan would be to remove those four can_ functions, move out the conditional methods (around 10) from abstract class and reintroduce the tons of if(methodexists) + write docs explaining why some methods are not required. (And then complain silently each time I have to change something in code that uses auth methods.)
          Hide
          Martín Langhoff added a comment -

          I haven't finished the proposed patch - it's late here and my brain is a wreck. Will finish it tomorrow. A quick note though to Petr: I think there is a bit of confusion - in most cases, the plugin can or cannot do something - and method_exists() is the right way of asking that question. That is what PHP has, and it is well implemented.

          There is another case – the plugin knows how to do it, but it may not be configured to do it. Example:

          • auth/imap cannot change passwords - so it has no method for it
          • auth/ldap can change password, but may not be configured to do it (maybe it doesn't have enough privileges) so we check for can_change_password()
          Show
          Martín Langhoff added a comment - I haven't finished the proposed patch - it's late here and my brain is a wreck. Will finish it tomorrow. A quick note though to Petr: I think there is a bit of confusion - in most cases, the plugin can or cannot do something - and method_exists() is the right way of asking that question. That is what PHP has, and it is well implemented. There is another case – the plugin knows how to do it, but it may not be configured to do it. Example: auth/imap cannot change passwords - so it has no method for it auth/ldap can change password, but may not be configured to do it (maybe it doesn't have enough privileges) so we check for can_change_password()
          Hide
          Petr Škoda added a comment -

          "..in MOST cases, the plugin can or cannot do something - and method_exists() is the right way of asking that question."

          Hmm, the "most cases" in previous sentence is THE exact reason why we need can_do_something() instead of method_exists(), interfaces would not help here too.

          Imagine somebody wants to write plugin that:
          1/ may or may not allow changing of passwords
          2/ may or may not allow signup
          3/ may or may not allow password reset
          depending on auth plugin configuration options.

          How would you solve this with method_exists()? Are you going to add/remove methods on the fly? I hope not.

          I do not understand the end of the previous comment, but if you are proposing to use both method_exists() and can_do_something() for those 3 points above - my opinion is that it would be the worst solution.

          =====================

          The *_hook methods are different, I agree that those might be used with method_exists(). But anyway in most cases sso plugins and other advanced types will override them in most cases anyway - so what is the benefit in this case?

          Show
          Petr Škoda added a comment - "..in MOST cases, the plugin can or cannot do something - and method_exists() is the right way of asking that question." Hmm, the "most cases" in previous sentence is THE exact reason why we need can_do_something() instead of method_exists(), interfaces would not help here too. Imagine somebody wants to write plugin that: 1/ may or may not allow changing of passwords 2/ may or may not allow signup 3/ may or may not allow password reset depending on auth plugin configuration options. How would you solve this with method_exists()? Are you going to add/remove methods on the fly? I hope not. I do not understand the end of the previous comment, but if you are proposing to use both method_exists() and can_do_something() for those 3 points above - my opinion is that it would be the worst solution. ===================== The *_hook methods are different, I agree that those might be used with method_exists(). But anyway in most cases sso plugins and other advanced types will override them in most cases anyway - so what is the benefit in this case?
          Hide
          Martin Dougiamas added a comment -

          BTW, ignore my error report above, it was caused by faulty PHP caching on the server and was not a code problem.

          Show
          Martin Dougiamas added a comment - BTW, ignore my error report above, it was caused by faulty PHP caching on the server and was not a code problem.
          Hide
          Martin Dougiamas added a comment -

          I have to say, looking at the code, I do prefer the style of the "can do" functions.

          It seems more like most of the class-oriented code I've ever seen and gives the subclass maximum flexibility to decide what to return.

          Petr makes a good point that flags could change per-instance and that just checking for a method's existence does not take that into account.

          Show
          Martin Dougiamas added a comment - I have to say, looking at the code, I do prefer the style of the "can do" functions. It seems more like most of the class-oriented code I've ever seen and gives the subclass maximum flexibility to decide what to return. Petr makes a good point that flags could change per-instance and that just checking for a method's existence does not take that into account.
          Hide
          Martín Langhoff added a comment -

          I agree with the "use can_do_something() for all optional methods" argument. Then the base class should implement all the can_do_xx() functions returning false so we can test for it explicitly.

          I personally think implementing a null function in the base class is pointless, but hey. Won't stop anyone at this stage :-/

          The current state of things is that all "if (method_exists($p, 'xx'))" have been replaced with an unconditionall call to $p->xx(), counting on the base class to fail silently – and therefore missing the chance to do error handling.

          Should we move to address that aspect of the API then? I can put together a patch that brings back those checks.

          (The "if (method_exists($p, 'xx') && $p->can_do_xx())" tests still check for can_do_xx() so that's fine)

          And please, with sugar on top, can we discuss API changes before they get into the tree? I do make a point of discussing API changes with MartinD with time and patience. Blood does flow quickly to the head when reading moodle-cvs is the only way to find out about these things...

          Show
          Martín Langhoff added a comment - I agree with the "use can_do_something() for all optional methods" argument. Then the base class should implement all the can_do_xx() functions returning false so we can test for it explicitly. I personally think implementing a null function in the base class is pointless, but hey. Won't stop anyone at this stage :-/ The current state of things is that all "if (method_exists($p, 'xx'))" have been replaced with an unconditionall call to $p->xx(), counting on the base class to fail silently – and therefore missing the chance to do error handling. Should we move to address that aspect of the API then? I can put together a patch that brings back those checks. (The "if (method_exists($p, 'xx') && $p->can_do_xx())" tests still check for can_do_xx() so that's fine) And please, with sugar on top, can we discuss API changes before they get into the tree? I do make a point of discussing API changes with MartinD with time and patience. Blood does flow quickly to the head when reading moodle-cvs is the only way to find out about these things...
          Hide
          Martin Dougiamas added a comment -

          Yes, we'll endeavour to communicate better about these things in future. Everyone's always busy, that's all, sometimes we need to trust each other to do the right things (it normally works too!)

          Since there would be a lot of this pattern:

          if (method_exists($p, 'xx') && $p->can_xx())

          can we just make the base "can" functions also check if the method exists?

          Something like this inside can_xx() at the beginning:

          if (!method_exists($this, 'xx'))

          { return false; }
          Show
          Martin Dougiamas added a comment - Yes, we'll endeavour to communicate better about these things in future. Everyone's always busy, that's all, sometimes we need to trust each other to do the right things (it normally works too!) Since there would be a lot of this pattern: if (method_exists($p, 'xx') && $p->can_xx()) can we just make the base "can" functions also check if the method exists? Something like this inside can_xx() at the beginning: if (!method_exists($this, 'xx')) { return false; }
          Hide
          Martín Langhoff added a comment -

          MartinD says:
          > Since there would be a lot of this pattern:
          > if (method_exists($p, 'xx') && $p->can_xx())

          Actually - I think we've agreed that the preferred use is:

          if ($p->can_xx()) {
          if ($p->xx())

          { // success }

          else

          { // oops }

          }

          backed by can_xx() being in the base class is better. Then the base class can just say

          function can_xx()

          { return false; }

          /* maybe implement xx(), maybe just show it
          in a comment
          function xx()

          { // do stuff return $something; }

          and the subclass can then override can_xx() as it sees fit. In one fell swoop, I let go of inheritance-less OOP and method_exists().

          > sometimes we need to trust each other to do the right things

          I trust you guys Perhaps what worries me is that I hope you trust me enough to talk to me about a major change on an API I've worked on for a while. A large commit late in the road to 1.8, – without communication - is a bit unfriendly.

          Letting people know doesn't hurt... a post in Auth forum or GDF, an email to me. The tracker is great but doesn't help make things like this visible. (MoodleHQ is fun but it's restricted to those that can be reading it every day. Miss more than 2 days, and it's impossible - and actually it is soaking up a lot of discussions that should happen on GDF - the all the niggly details of the roles infrastructure have only ever been discussed in MoodleHQ).

          Show
          Martín Langhoff added a comment - MartinD says: > Since there would be a lot of this pattern: > if (method_exists($p, 'xx') && $p->can_xx()) Actually - I think we've agreed that the preferred use is: if ($p->can_xx()) { if ($p->xx()) { // success } else { // oops } } backed by can_xx() being in the base class is better. Then the base class can just say function can_xx() { return false; } /* maybe implement xx(), maybe just show it in a comment function xx() { // do stuff return $something; } and the subclass can then override can_xx() as it sees fit. In one fell swoop, I let go of inheritance-less OOP and method_exists(). > sometimes we need to trust each other to do the right things I trust you guys Perhaps what worries me is that I hope you trust me enough to talk to me about a major change on an API I've worked on for a while. A large commit late in the road to 1.8, – without communication - is a bit unfriendly. Letting people know doesn't hurt... a post in Auth forum or GDF, an email to me. The tracker is great but doesn't help make things like this visible. (MoodleHQ is fun but it's restricted to those that can be reading it every day. Miss more than 2 days, and it's impossible - and actually it is soaking up a lot of discussions that should happen on GDF - the all the niggly details of the roles infrastructure have only ever been discussed in MoodleHQ).
          Hide
          Petr Škoda added a comment -

          1/ there is already pattern in present code
          if ($auth->can_do_something())

          { $auth->do_something(); }

          if not, it is a bug

          2/ the do_something methods already have comment stating that they are needed only if can_do_something() resturns true

          I agree that we should communicate more about major changes in API on moodle.org site.

          Sometimes it is hard to propose API changes without actually writing a code that uses the API, and this was exactly the case - I was fixing bugs in code that uses auth API and had to change the API in order to make it work. I committed it as one big patch because I needed to test it all together, the changes crawled in one by one over days and I did not realize it would be such a big change for other ppl.

          I have one last pending patch for hooks - I would like to create loginpage_hook() and lougoutpage_hook() (instead of pre.._hooks) and new login_hook() (previously authenticated user) and logout_hook() (with cas and mnet stuff from require_logout()) - I think they might be called not only for $USER->auth, but for all enabled plugins to make it more flexible. Will attach the patch later today.

          Show
          Petr Škoda added a comment - 1/ there is already pattern in present code if ($auth->can_do_something()) { $auth->do_something(); } if not, it is a bug 2/ the do_something methods already have comment stating that they are needed only if can_do_something() resturns true I agree that we should communicate more about major changes in API on moodle.org site. Sometimes it is hard to propose API changes without actually writing a code that uses the API, and this was exactly the case - I was fixing bugs in code that uses auth API and had to change the API in order to make it work. I committed it as one big patch because I needed to test it all together, the changes crawled in one by one over days and I did not realize it would be such a big change for other ppl. I have one last pending patch for hooks - I would like to create loginpage_hook() and lougoutpage_hook() (instead of pre.._hooks) and new login_hook() (previously authenticated user) and logout_hook() (with cas and mnet stuff from require_logout()) - I think they might be called not only for $USER->auth, but for all enabled plugins to make it more flexible. Will attach the patch later today.
          Hide
          Petr Škoda added a comment -

          Another issue that bugs me is the content of /sso/hive that is used for hive only, I would like to move it into /auth/hivesso, then I could remove the ugly hack for it from the base class hook and start using method_exists() for all hooks.

          Show
          Petr Škoda added a comment - Another issue that bugs me is the content of /sso/hive that is used for hive only, I would like to move it into /auth/hivesso, then I could remove the ugly hack for it from the base class hook and start using method_exists() for all hooks.
          Hide
          Martín Langhoff added a comment -

          +1 for deprecating /sso/hive in favour of auth/hivesso. In terms of testing, IIRC the main end users are Moodle.com clients - though I don't know who exactly.

          Show
          Martín Langhoff added a comment - +1 for deprecating /sso/hive in favour of auth/hivesso. In terms of testing, IIRC the main end users are Moodle.com clients - though I don't know who exactly.
          Hide
          Martin Dougiamas added a comment -

          errrr .... sso != auth

          sso/hive LOGS IN to an external hive system and creates a cookie there for later

          Show
          Martin Dougiamas added a comment - errrr .... sso != auth sso/hive LOGS IN to an external hive system and creates a cookie there for later
          Hide
          Martín Langhoff added a comment -

          One of Petr's additions to the API is a post-login hook that does the job for sso. Before multi-auth, it wouldn't have made any sense, but with multi-auth, you can have an auth/hive plugin that doesn't really let anyone login, but uses the API to manage its configuration, and to get itself registered for the post-login hook.

          (In a way, auth/mnet is a hybrid in that it let's users login (sort of) and it also does sso outbound).

          The sso API never matured - and the auth API's gotten to a point where it offers that hook. I think it makes sense to let /sso be absorbed by auth.

          Show
          Martín Langhoff added a comment - One of Petr's additions to the API is a post-login hook that does the job for sso. Before multi-auth, it wouldn't have made any sense, but with multi-auth, you can have an auth/hive plugin that doesn't really let anyone login, but uses the API to manage its configuration, and to get itself registered for the post-login hook. (In a way, auth/mnet is a hybrid in that it let's users login (sort of) and it also does sso outbound). The sso API never matured - and the auth API's gotten to a point where it offers that hook. I think it makes sense to let /sso be absorbed by auth.
          Hide
          Petr Škoda added a comment -

          Thanks Martín, I could not describe it better. One of my aims was to allow anybody to implement SSO by addding only one directory into /auth/ , I already have unfinished cookie+webservice auth plugin for one institution. Other thing I had in mind while implementing the hooks was to remove all hacks for cas, weblinkauth, fc and hive. Mnet is special case but shares a lot with auth and could IMHO benefit a bit from these changes too.

          Show
          Petr Škoda added a comment - Thanks Martín, I could not describe it better. One of my aims was to allow anybody to implement SSO by addding only one directory into /auth/ , I already have unfinished cookie+webservice auth plugin for one institution. Other thing I had in mind while implementing the hooks was to remove all hacks for cas, weblinkauth, fc and hive. Mnet is special case but shares a lot with auth and could IMHO benefit a bit from these changes too.
          Hide
          Petr Škoda added a comment -

          ... I also agree with Martín that the shared configuration page/framework for sso and auth is a big bonus.

          Show
          Petr Škoda added a comment - ... I also agree with Martín that the shared configuration page/framework for sso and auth is a big bonus.
          Hide
          Martin Dougiamas added a comment -

          "you can have an auth/hive plugin that doesn't really let anyone login"

          This is the part that I find a bit problematic, as it will be listed together with all the plugins that do.

          However, I totally agree sso is a bit of a black sheep so let's deprecate it. The auth/hive plugin could possibly do double duty by also (optionally) allowing authentication against Hive.

          DON'T delete it OR the SSO hook for it yet, though, as there are a lot of Moodle/hive sites relying it on it! And possibly others using their own mambo/joomla/whatever plugins as well.

          (I am really getting keen on a proper event-driven architecture now as Eloy proposed - it will make all these kind of "hooks" standard. Will write it up soon for discussion in the forums)

          Show
          Martin Dougiamas added a comment - "you can have an auth/hive plugin that doesn't really let anyone login" This is the part that I find a bit problematic, as it will be listed together with all the plugins that do. However, I totally agree sso is a bit of a black sheep so let's deprecate it. The auth/hive plugin could possibly do double duty by also (optionally) allowing authentication against Hive. DON'T delete it OR the SSO hook for it yet, though, as there are a lot of Moodle/hive sites relying it on it! And possibly others using their own mambo/joomla/whatever plugins as well. (I am really getting keen on a proper event-driven architecture now as Eloy proposed - it will make all these kind of "hooks" standard. Will write it up soon for discussion in the forums)
          Hide
          Martin Dougiamas added a comment -

          How soon can we close this. I really want to try for 1.8 release on friday.

          Show
          Martin Dougiamas added a comment - How soon can we close this. I really want to try for 1.8 release on friday.
          Hide
          Petr Škoda added a comment -

          I would like to close this today, my plan is to:
          1/ rename the hooks as described somewhere above
          2/ keep the sso/hive as is for now with hardcoded hack in base auth class - add notices about plans to deprecate SSO and move it into auth plugin

          Show
          Petr Škoda added a comment - I would like to close this today, my plan is to: 1/ rename the hooks as described somewhere above 2/ keep the sso/hive as is for now with hardcoded hack in base auth class - add notices about plans to deprecate SSO and move it into auth plugin
          Hide
          Petr Škoda added a comment -

          reclosing, the sso and weblink auth has new issues

          thanks for all input and sorry for any trouble caused by these changes

          Show
          Petr Škoda added a comment - reclosing, the sso and weblink auth has new issues thanks for all input and sorry for any trouble caused by these changes
          Hide
          Martín Langhoff added a comment -

          Reopening - Donal is reporting that MNET login/logout is broken in 1.8 with this.

          Show
          Martín Langhoff added a comment - Reopening - Donal is reporting that MNET login/logout is broken in 1.8 with this.
          Hide
          Donal McMullan added a comment -

          Hey Petr:
          >> First I was reporting bugs then gave up
          Your reports never hit my radar, so I'm sorry that we were unresponsive. I generally respond to email, pizza and beer. But not in that order.

          If you're not getting feedback from Catalyst people about stuff we should be following up, you can always try me as a third string after Martín and Penny. I don't hang out in Moodle HQ anymore though.

          Your big refactor is a PITA for me, because I'm the jerk that will have to re-test the old functionality, and I've already spotted a couple of issues, so we're off to a bad start. I need about four hours to set up candidates, test, tweak, test, etc. and I can't see that happening this week.

          Did you write any unit tests for the new API? That'd be really handy.

          Cheers guys

          Show
          Donal McMullan added a comment - Hey Petr: >> First I was reporting bugs then gave up Your reports never hit my radar, so I'm sorry that we were unresponsive. I generally respond to email, pizza and beer. But not in that order. If you're not getting feedback from Catalyst people about stuff we should be following up, you can always try me as a third string after Martín and Penny. I don't hang out in Moodle HQ anymore though. Your big refactor is a PITA for me, because I'm the jerk that will have to re-test the old functionality, and I've already spotted a couple of issues, so we're off to a bad start. I need about four hours to set up candidates, test, tweak, test, etc. and I can't see that happening this week. Did you write any unit tests for the new API? That'd be really handy. Cheers guys
          Hide
          Martin Dougiamas added a comment -

          +100 for some unit tests for auth modules as they are so important. Petr can you do this?

          Donal, can you give us more information about the problems/errors you're seeing?

          Show
          Martin Dougiamas added a comment - +100 for some unit tests for auth modules as they are so important. Petr can you do this? Donal, can you give us more information about the problems/errors you're seeing?
          Hide
          Martín Langhoff added a comment -

          He's in a meeting right now – if I understand correctly it's on the logout phase that he spotted a problem. We'll get it fixed, np, (unless you guys sort it out first), but not right today, as he's finishing a Mahara-Moodle SSO implementation (that relies on this code as well) and the deadline is on us.

          Show
          Martín Langhoff added a comment - He's in a meeting right now – if I understand correctly it's on the logout phase that he spotted a problem. We'll get it fixed, np, (unless you guys sort it out first), but not right today, as he's finishing a Mahara-Moodle SSO implementation (that relies on this code as well) and the deadline is on us.
          Hide
          Petr Škoda added a comment -

          Donal, I am sorry for the trouble - I have already explained it above. I expect that the maintainer of auth plugins should be watching this tracker carefully and also monitoring moodle.org forums - I am doing it now because nobody did it. You can not commit code and then go away and wait till the bugs knock on your door, sorry.

          It is possible to do some basic unit tests, but AFAIK they can not cover browser interaction (session, cookies, etc.) == the login process ittself.

          Reclosing now, filing new issue for unit tests. Please report the mnet problem as a new issue, I am sure I can fix it soon if you describe it.

          Show
          Petr Škoda added a comment - Donal, I am sorry for the trouble - I have already explained it above. I expect that the maintainer of auth plugins should be watching this tracker carefully and also monitoring moodle.org forums - I am doing it now because nobody did it. You can not commit code and then go away and wait till the bugs knock on your door, sorry. It is possible to do some basic unit tests, but AFAIK they can not cover browser interaction (session, cookies, etc.) == the login process ittself. Reclosing now, filing new issue for unit tests. Please report the mnet problem as a new issue, I am sure I can fix it soon if you describe it.
          Hide
          Donal McMullan added a comment -

          >> You can not commit code and then go away and wait till the bugs knock on your door, sorry.
          If you're having difficulty with assigning bugs, or with adding watchers to issues in the tracker, let me know and I'll be happy to walk you through it.

          I'm going to attach a patch - it removes come code that would prevent an important call:
          $this->kill_children($USER->username, sha1($_SERVER['HTTP_USER_AGENT']));
          What this call does, is contact all remote servers that your local user has roamed to, and it ends his remote sessions on those servers. I think you might also have created (elsewhere) a redirect-on-logout??? I'm not sure though, and I don't know if it breaks the MNET cleanup stuff, so I need to check/test the code some more.

          Personally, I would not take on a refactor at this stage in the release cycle without some unit tests to make sure the changes I'm making under the hood don't affect the upper tiers. The browser/session stuff can be caught by Selenium, if you're changing front-end code, but I think you were mostly working on the object model?

          I have to get back to my "proper OO" homework now or my teacher will be cross with me.

          Show
          Donal McMullan added a comment - >> You can not commit code and then go away and wait till the bugs knock on your door, sorry. If you're having difficulty with assigning bugs, or with adding watchers to issues in the tracker, let me know and I'll be happy to walk you through it. I'm going to attach a patch - it removes come code that would prevent an important call: $this->kill_children($USER->username, sha1($_SERVER ['HTTP_USER_AGENT'] )); What this call does, is contact all remote servers that your local user has roamed to, and it ends his remote sessions on those servers. I think you might also have created (elsewhere) a redirect-on-logout??? I'm not sure though, and I don't know if it breaks the MNET cleanup stuff, so I need to check/test the code some more. Personally, I would not take on a refactor at this stage in the release cycle without some unit tests to make sure the changes I'm making under the hood don't affect the upper tiers. The browser/session stuff can be caught by Selenium, if you're changing front-end code, but I think you were mostly working on the object model? I have to get back to my "proper OO" homework now or my teacher will be cross with me.
          Hide
          Donal McMullan added a comment -

          Removing an early return that would prevent 'kill_children' from executing. Not tested - please test before committing.

          Show
          Donal McMullan added a comment - Removing an early return that would prevent 'kill_children' from executing. Not tested - please test before committing.
          Hide
          Petr Škoda added a comment -

          I do not want to argue with anyone - the auth plugins were not properly maintained, I am trying to fix that. If anybody wants to be the maintainer of auth plugins and everything that is related (user editing, session, cookies, login code, login page CSS, etc.) please, please let me know, I would be glad to work more on modules, groups, security or other unmaintained stuff.

          Show
          Petr Škoda added a comment - I do not want to argue with anyone - the auth plugins were not properly maintained, I am trying to fix that. If anybody wants to be the maintainer of auth plugins and everything that is related (user editing, session, cookies, login code, login page CSS, etc.) please, please let me know, I would be glad to work more on modules, groups, security or other unmaintained stuff.
          Hide
          Petr Škoda added a comment -

          Updating title, "fix" was not the correct word.

          Show
          Petr Škoda added a comment - Updating title, "fix" was not the correct word.
          Hide
          Donal McMullan added a comment -

          I just committed some changes that should restore the "complete logout" feature, so that a user who logs out of a Moodle he has roamed to will be logged out of:
          That remote Moodle
          His home Moodle, where he actually logs in
          Any other child sessions of his home Moodle that he has also roamed to

          Show
          Donal McMullan added a comment - I just committed some changes that should restore the "complete logout" feature, so that a user who logs out of a Moodle he has roamed to will be logged out of: That remote Moodle His home Moodle, where he actually logs in Any other child sessions of his home Moodle that he has also roamed to
          Hide
          Petr Škoda added a comment -

          Please next time file a new bug before committing and mention it in the commit message.
          We are using the tracker to prepare the release notes and this commit/fix will not be included

          Show
          Petr Škoda added a comment - Please next time file a new bug before committing and mention it in the commit message. We are using the tracker to prepare the release notes and this commit/fix will not be included
          Hide
          Donal McMullan added a comment -

          Sorry Petr - I'll try to remember.
          Cheers - D

          Show
          Donal McMullan added a comment - Sorry Petr - I'll try to remember. Cheers - D
          Hide
          Petr Škoda added a comment -

          I just reviewed the changes, this is not good. The problem is that you have changed how auth API works in stable branch and you did not even update the inline docs in authlib. Also I had a reason to do it exactly this way, I understand that it might caused problems for mnet and I want to fix it too, but you should IMHO dicuss these changes in API before commit into stable. The problem is that your code breaks (not yet implemented) hive sso.

          Please:
          1/ revert the change related to prelogout_hook()
          2/ file a new bug
          3/ describe why we should not call other enabled auth plugins or better try to find a way around it in mnet code

          I hope we find solution soon.

          There might also be a problem with logout code not executed when users log in without previous logout - did you think about this potential problem?

          Show
          Petr Škoda added a comment - I just reviewed the changes, this is not good. The problem is that you have changed how auth API works in stable branch and you did not even update the inline docs in authlib. Also I had a reason to do it exactly this way, I understand that it might caused problems for mnet and I want to fix it too, but you should IMHO dicuss these changes in API before commit into stable. The problem is that your code breaks (not yet implemented) hive sso. Please: 1/ revert the change related to prelogout_hook() 2/ file a new bug 3/ describe why we should not call other enabled auth plugins or better try to find a way around it in mnet code I hope we find solution soon. There might also be a problem with logout code not executed when users log in without previous logout - did you think about this potential problem?
          Hide
          Donal McMullan added a comment -

          Sorry Petr - I didn't realise the API had moved in this direction. It seems strange to call the prelogout_hook method on plugins that the $USER doesn't have a relationship with... is the rationale documented somewhere? I'll have a look for the hive SSO docs or code and try to make something that's compatible.

          >> There might also be a problem with logout code not executed when users log in without previous logout - did you think about this potential problem?
          This used to work... do you think it's broken now?

          I'll think about this tomorrow - I can't work any more today.

          Show
          Donal McMullan added a comment - Sorry Petr - I didn't realise the API had moved in this direction. It seems strange to call the prelogout_hook method on plugins that the $USER doesn't have a relationship with... is the rationale documented somewhere? I'll have a look for the hive SSO docs or code and try to make something that's compatible. >> There might also be a problem with logout code not executed when users log in without previous logout - did you think about this potential problem? This used to work... do you think it's broken now? I'll think about this tomorrow - I can't work any more today.
          Hide
          Petr Škoda added a comment -

          I do not think it worked, but I might be wrong - I will look at it and file a bug report if it does not now and cc you.

          the SSO bug is linked above, there are docs and some hints how to use the auth methods in lib/authlib.php

          Show
          Petr Škoda added a comment - I do not think it worked, but I might be wrong - I will look at it and file a bug report if it does not now and cc you. the SSO bug is linked above, there are docs and some hints how to use the auth methods in lib/authlib.php
          Hide
          Petr Škoda added a comment -

          The code was made as much flexible as possible to allow easy implementation of shibolleth, cas, weblink, hive sso, etc. without the need to modify anything outside of /auth/pluginname/; it seems it worked.

          In 1.9 I am planning to convert weblink (or deprecate?) and hive sso and improve the ldap, extdb plugins - password changing/reset and signup. I hope ML will commit ext db improvements soon.

          In anycase I am going to file bugs first and discuss it a bit

          Thanks for working on improving of Moodle code!

          Show
          Petr Škoda added a comment - The code was made as much flexible as possible to allow easy implementation of shibolleth, cas, weblink, hive sso, etc. without the need to modify anything outside of /auth/pluginname/; it seems it worked. In 1.9 I am planning to convert weblink (or deprecate?) and hive sso and improve the ldap, extdb plugins - password changing/reset and signup. I hope ML will commit ext db improvements soon. In anycase I am going to file bugs first and discuss it a bit Thanks for working on improving of Moodle code!

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: