Proposal - Implement basic multiple authentication (1.6)

Proposal - Implement basic multiple authentication (1.6)

by Howard Miller -
Number of replies: 24
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers

Something I have been working on...

'Basic' implementation of multiple authentication. Highlights...

  • New admin screen for authentication - like the 1.6dev filters screen. You can select more than one authentication method, the order in which they are applied and it shows links to their settings.
  • The 'enabled' methods are applied one after the other for a new/unknown user. Once they are in the database, we know which one to use.
  • authentication libraries converted to classes

...is this useful? I've done some basic work, but it needs a lot more work for 'real'.

Any comments appreciated smile

Average of ratings: -
In reply to Howard Miller

Re: Proposal - Implement basic multiple authentication (1.6)

by Martín Langhoff -

Howard,

I'm doing some maintenance on the auth/ldap and auth/db plugins, as well as enrolment, so I think I should speak up... I had a plan drafted that I now re-read and ... is identical to yours big grin Great!

Can I ask you to do this on a branch off HEAD? There's a chance for peer review before it gets into HEAD and, if it turns out to be risky or need more work, we can hold off. Put a tag in the place where you're branching off from, so we can diff it easily.

My extra notes are:

  • authentication libs to classes
    • use enrol as inspiration
    • but simpler than enrolment: there should be no base class or the base class should be fallback only (that is, mostly empty)
  • we need to handle self-registration user creation
  • having this will allow users to actually disable moodle internal auth: we need to handle (internal) admin user acct
  • migrate all per-plugin config settings to config_plugins table.
  • "Once they are in the database, we know which one to use" yes, but if that plugin is disabled, block access.

This complements with similar work that Patrick Li and I are doing on the enrolment plugins area (each course gets an 'enrol' field, just like users have an 'auth' field) to support OSCommerce integration more smoothly. I've been saying that 1.5 was the "leap ahead" release, and it's starting to sound like 1.6 is going to be the "rocket forward" release. Wow.

Average of ratings: -
In reply to Martín Langhoff

Re: Proposal - Implement basic multiple authentication (1.6)

by Howard Miller -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
Martin,

Thanks for your interest and support. Doing this in a branch is a very wise idea smile Off to read up on branches in CVS!!

Notes on your notes...
* Classes - I was going to have a base class that is basically manual-only authentication. 'Almost' nothing in it, except the manual database lookup for auth. I wouldn't not have a bass class - that's throwing away a framework that provides great documentation.
* self-registration - not completely sure what you mean. Are you just talking about email registration?
* yes - admin will always need to be a special case for 'safety'
* Migrate settings - my initial idea was *not* to do this. I realise this means only 1 ldap only 1 pop3 etc.. However, you've now got me thinking that maybe that would be half a job. On the other hand migrating the settings makes this a completely different scale of enterprise. I'm erring toward just doing it properly though smile
* Know which one to use - yes, good point.

I'll keep you posted....
Average of ratings: -
In reply to Howard Miller

Re: Proposal - Implement basic multiple authentication (1.6)

by Martín Langhoff -

Howard,

Cool -- I'm happy to set up the branch for you, well tagged and tidy, if you want. Then all you need to do is a checkout to a separate dirrectory or "switch" an existing checkout to the branch. Both trivial operations. Let me know if you want me to do this (and then I'll later be better prepared to help you to merge it into HEAD). (edit: I see you've got it done)

Now, with regards to the stuff we're discussing...

Classes

If you're after self-documenting code, we can have manual auth be the 'reference' and make sure it has all the API methods, even if commented out, with generous comment blocks.

Using base classes + inheritance backfires often, and this is a textbook scenario. This is an API that grows organically, and that tolerates (and even encourages) people to maintain their private plugins, like those that interoperate with UglyLegacyStudentMgmtSystem. I maintain a few of them.

So, we define the API today, and provide 'default' methods in the base class, which doubles up as 'manual/internal enrolment'. So 100 plugins are written against it, that count on the current API behaviour, 20 of those are official, 80 'private', local customizations.

Now, in 1.8, we decide to grow the API adding a new series of methods. Normally, when you're growing an API like this, you add methods that are optional so it's backwards-compatible (or forward-compatible, depending on where you are standing). But with a strategy of "it's always in the base class" we're fsck'ed.

We will add them to the base class, because (a) the code expects them, (b) for self-documentation, and (c) because it implements 'manual auth' which can truly make good use of them. And as soon as we do that, all the plugins will magically do the same thing that the base class does even if it doesn't make any sense at all and breaks them badly.

The workaround usually involves adding more methods to the API, where each module reports what it knows how to do, or what 'features' it has, and is generally awful, because you check whether you should call the method before you call it, lest inheritance does its job. Oh my!.

So, the best way is to have manual as a normal class, have no base class, and wrap calls to methods that are optional in the API with method-exists().

This is important, and is something I'll have to get around to fixing in the enrolment plugin architecture, but it hasn't been a problem so far (because we haven't changed the API... but we're about to sad )

Inheritance is tricky business, and defines an 'internal' API between base class and subclass that is full of funny interactions. All good for an in-house project, but hell for this loosely organized tribe.

(In other words, inheritance is too tighly coupled, we need loose coupling for a plugin API).

Edited for clarity and typos.

Average of ratings: -
In reply to Martín Langhoff

Re: Proposal - Implement basic multiple authentication (1.6)

by Howard Miller -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
Martin,

I'm not entirely sure I understand your argument. However...

I would still prefer - as I always do - to implement an abstract bass class. This can be completely non-functional (ie, even the manual auth inherits from it).

My big objection to not having it is that it would involve lots of unpleasant 'method_exists' functions, rather than simply calling a 'do nothing' method declared in the abstract bass class. In addition, the base class clearly defines the API as it stands, and the code that used that API will be much more elegant and readable.

If one modifies the inherited class, then non-api methods are effectively private methods of the class. The only way that you could effectively use these additional methods elsewhere in the code is to 'break' the api anyway - in which case all bets are off.

Your point about adding methods in 1.8 - if we require additional functionality that requires more API calls you either need them or you don't. It doesn't matter if there is a base class or not - you would either *have* to add the method in every (non inherited) function or you wouldn't. The base class method would either say "ignore this if you don't want the functionality" or "you *must* override this method" which is surely clearer and more elegant? It's a shame PHP doesn't enforce more of this stuff.... maybe smile

EDIT:
I managed to set up the branch myself as per the docs - hopefully correctly.
Average of ratings: -
In reply to Howard Miller

Re: Proposal - Implement basic multiple authentication (1.6)

by Martín Langhoff -

hi Howard,

I would still prefer - as I always do - to implement an abstract bass class. This can be completely non-functional (ie, even the manual auth inherits from it).

Cool. An 'empty' base class is better than a functional one, but let me try to explain why no base class for the plugin API is even better.

I am not against inheritance at all. In fact, I think some classes (manual, for instance) should be written so that other classes can inherit from them if they want.

But that is an option of the 'new' plugin author. APIs are the point where we provide for loose coupling and inheritance is way too tight. We end up with a brittle API.

Using an empty base class only buys you not having to call method-exists(), but that also means not being able to call method-exists() and get a meaningful answer. The short version is that inheritance is not for that, and we would be misusing it.

The long version goes...

Imagine you want to offer an option in user/edit.php only if the auth supports it. Impossible to tell because method-exists() isn't useful.

Of course, you can start implementing methods that report back what the object knows how to do, a poor man's substitute for method-exists(). Yuck.

And having

   if (method-exists($obj, 'danceforme')) {

for optional methods is good because it is self-documenting. No-one has to guess whether

   if ($obj->danceforme()) {

Returned true because it's implemented and did dance correctly, or because it's not implemented and we hit a dummy method. If knowing this is important (and often is) we're fsck'ed mixed

The base class method would either say "ignore this if you don't want the functionality" or "you must override this method" which is surely clearer and more elegant?

That approach is valid for in-house development (and still doesn't scale very well along code size and team size) of a few tightly controlled classes. But it's not elegant in an API where you don't have any control on your plugin implementors and want to offer backwards compat.

I started doing OOP about 9 years ago now, and used it in 4 languages with different approaches and teams (and specially, team sizes of up to ~25). Inheritance is tightly bound, and brittle, and you can see things go wrong with large codebases, large teams and long project history. Large FOSS projects using OO this way spend a lot of time and effort coordinating the smallest changes in their base classes, partly because backwards compat is almost impossible unless you version your class names.

Sorry for the rant, as you can surely tell, I don't want to go that way wink -- and I'll probably be stuck maintaining ext auth plugins for a few seasons to come, too sad

Average of ratings: -
In reply to Martín Langhoff

Re: Proposal - Implement basic multiple authentication (1.6)

by Howard Miller -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
Mmmm.... Ok..... Mmmm....

I don't think that I'm *quite* there, but I'm starting to see your points smile My day job has been software development since the dinosaurs were running about and we used Fortran on CP/M machines, but I have generally had the luxury of doing my own thing. I know that there is much for me to learn about developing in open-source communities.

I will go forward with your recommendations!

My only remaining worry is the documentation aspect. A well commented abstract class gives a strong blueprint for developers wishing to implement a new authentication plugin. This, IMHO, is why developing a new block is near trivial and developing, even a trivial, new module is a nightmare. The former is based on a clear, well thought out class model and the latter is rather old-school and fragmented (I don't mean that as a criticism by the way). I guess we could embellish the manual auth class with stubs, or just comments, for the 'optional' parts of the API.
Average of ratings: -
In reply to Martín Langhoff

Re: Proposal - Implement basic multiple authentication (1.6)

by Howard Miller -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
Thinking some more... as there is clearly a lot to do here, I hope you won't object if I make a start using a base class. However, I think these things often resolve themselves in the process - so we will keep an eye on it and change course if need be. I believe that it will be easier to illustrate any possible problems when there is something concrete to point at.

I'm going to try and start with a limited admin screen and just a couple of options - manual and the bare bones of ldap probably, and work forward from there. Try and get 'something' working.
Average of ratings: -
In reply to Howard Miller

Re: Proposal - Implement basic multiple authentication (1.6)

by Martín Langhoff -

Promise! no inheritance bashing in this post! ;)

I had forgotten about one important API change that we'll need to have multi-auth working well. I've worked/programmed a lot with other systems that implement multi-auth, namely PAM and Apache's internal auth system (which is pretty sophisticated and it's a crying hsmae that PHP doesn't hook into it at all).

These systems both implement an Auth API very much like Moodle, and you can configure each module and put them in order. Man, they were reading our minds. And through a time-machine too! ;)

The difference, though, is that for the auth handler (in our case, auth-user-login()) they accept 3 return codes, rather than just boolean. They are usually constants defined like:

  • AUTH_OK - user/password verified
  • AUTH_DENIED - an authoritative "we don't like the user", stronger than just user/pw don't match (perhaps they don't match and this plugin is authoritative with regards to this user acct)
  • AUTH_DECLINED - non-authoritative "no". Means that user/pw didn't match, or I don't know about this user. Try the next plugin.

and sometimes

  • AUTH_ERROR - something went wrong (ext auth server not there?)

The control loop is still simple, but becomes much more powerful. You breakout with a 'true' on thefirst OK you see, and with a 'false' on the first DENIED you see. If you get to the end of the foreach and all you've seen are DECLINED, then it's 'false'.

Examples (taken from Apache and PAM plugins):

  • As auth/plugins are 'stackable' you can implement a 'blacklist' plugin, and put it 'on top'.
  • Plugin modules that actually can read the password can choose to issue a DENIED if the user matches, but the pw doesn't.

What do you reckon?

Average of ratings: -
In reply to Martín Langhoff

Re: Proposal - Implement basic multiple authentication (1.6)

by Howard Miller -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
Martin,

AUTH_OK etc...
Yes excellent point - hadn't thought of that. This is much more flexible. I guess we may want an option on what to do if AUTH_DENIED is returned - stop or try something else.

I just wanted to surface the matter, and make some sort of decision, on multiple authentication of the same type. It is obviously an order of magnitude more complex as soon as we allow, say, more than one LDAP account. This will also require some modification to the mdl_user tables to indicate which ldap instance - and all the fun/games of correctly maintaining these links. I guess this is the 'right' way to go though.
Average of ratings: -
In reply to Howard Miller

Re: Proposal - Implement basic multiple authentication (1.6)

by Martín Langhoff -
> I guess we may want an option on what to do if AUTH_DENIED is
> returned - stop or try something else.

Interesting. Plugins should return DECLINED 99% of the time. A module should only return DENIED when it means it. And I think we should respect that at least by (strong) default.

Perhaps this should be intra-plugin? The plugin can then be asked to pass DECLINED even if it has strong misgivings about this authentication attempt and would normally pass DENIED. Other plugins (such as blacklists or tarpitting plugins) should rather be disabled.

> It is obviously an order of magnitude more complex as soon as we
> allow, say, more than one LDAP account.

Yes, multiple plugin instances are a whole new game. Your changes are going to set the table for it to happen, but it is probably too much to hack in one go.

Perhaps for the mythical Moodle 2.0?
Average of ratings: -
In reply to Martín Langhoff

Re: Proposal - Implement basic multiple authentication (1.6)

by Howard Miller -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
Good! Crazy multiple plugin instances idea out of the window smile The more I thought about it the more frightening it became.

Ok - I'll get on with it!
Average of ratings: -
In reply to Martín Langhoff

Re: Proposal - Implement basic multiple authentication (1.6)

by Iñaki Arenaza -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
I like the stackable way PAM and Apache work. I think it's flexible enough for most (¿all?) cases, while still being simple enough for the typical scenario: just one source for authentication info.

Saludos. Iñaki.
Average of ratings: -
In reply to Iñaki Arenaza

Re: Proposal - Implement basic multiple authentication (1.6)

by Martín Langhoff -
Yes - it's a great approach.

MD set the stage when he designed the Moodle auth and enrolment plugin architectures. This is just some sugar on top wink

Howard, how is this going? Patrick seems to have the multi-enrolment stuff mostly done, and it's looking really good...
Average of ratings: -
In reply to Martín Langhoff

Re: Proposal - Implement basic multiple authentication (1.6)

by Howard Miller -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
A bit slow... but I'm getting there. I've been hit by the beginning of the new academic year here and the user support side of my job is taking up most of my time. I hope to make some progress this week - meetings permitting.
Average of ratings: -
In reply to Martín Langhoff

Re: Proposal - Implement basic multiple authentication (1.6)

by Howard Miller -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
I have set up the branch...

MOODLE_16_AUTHREFACTOR
Average of ratings: -
In reply to Howard Miller

Re: Proposal - Implement basic multiple authentication (1.6)

by Iñaki Arenaza -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
Is the code available anywhere (even if it's not finished)? I've had a look at the CVS branch you mention, but I can't see any activity since 2005.10.19 or so.

I'm quite interested in this functionality, and would like to help if needed.

Saludos. Iñaki.
Average of ratings: -
In reply to Iñaki Arenaza

Re: Proposal - Implement basic multiple authentication (1.6)

by Martín Langhoff -
Hola Inaki!

I don't think the branch has seen much action, so I guess it's a bit orphaned. The plan is rather well laid out, and we have a matching implementation of multi-enrol that I will be looking into merging soon (like, next week).

Do you want to give me a hand with this? wink

Average of ratings: -
In reply to Martín Langhoff

Re: Proposal - Implement basic multiple authentication (1.6)

by Iñaki Arenaza -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
Sure! cool

Saludos. Iñaki.
Average of ratings: -
In reply to Iñaki Arenaza

Re: Proposal - Implement basic multiple authentication (1.6)

by Martín Langhoff -
Hey, I've just added a new auth module that came from Clive Gould. I hope you can merge it into what you are doing. And we should coordinate something as to how to merge this whole into HEAD cleanly wink
Average of ratings: -
In reply to Martín Langhoff

Re: Proposal - Implement basic multiple authentication (1.6)

by Iñaki Arenaza -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
In fact, I haven't started coding yet. This is why I wanted to have a look at what Howard had done, so I don't have to do all the analysis and design from scratch.  But it seems I'll have to do it anyway mixed.

Anyway, I'm thinking about doing it multi-instance (doesn't seem it will be that difficult) and PAM-like behaved (as discussed in this same thread above).

But this will take time. I don't know moodle enough yet and will be doing mostly in my spare time.

I'm thinking about using 'frozen' (serialized) objects to store each authentication instance's settings. Is this a heresy? This should help with configuration namespace pollution in the case of multi-instance plugins.

Saludos. Iñaki.
Average of ratings: -
In reply to Howard Miller

Re: Proposal - Implement basic multiple authentication (1.6)

by Howard Miller -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
Update...

I've been horribly delayed getting on with this. My current (revised) plan is to simply get the multiple authentication part (and associated administration screen) with the existing auth libraries and subtly modified settings screens.

When this is ok I'll put this into 1.6dev.

After a wait to allow for complaints and comments, I'll do a part2 branch to refactor the libraries and convert the settings into database tables.
Average of ratings: -
In reply to Howard Miller

Re: Proposal - Implement basic multiple authentication (1.6)

by Martín Langhoff -
Howard,

Good things take time. Can you keep it in the branch you've created until it's all done? That way HEAD won't be broken while you work. From what I can understand, between the 1st and 2nd stage you are describing, auth will probably not work at all, as you can't do multi-auth without turning all the modules into OOP.

Cheers.
Average of ratings: -
In reply to Martín Langhoff

Re: Proposal - Implement basic multiple authentication (1.6)

by Howard Miller -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
Oh no, not my intention, but you could be right all the same smile I would only merge it back in if it really did work. I don't want things to diverge too far from the authentication libraries and end up with a grim job merging it back.
Average of ratings: -
In reply to Martín Langhoff

Re: Proposal - Implement basic multiple authentication (1.6)

by Howard Miller -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
Yet another stupid CVS question....

Can I (should I) merge the changes that have happened since I did the enrolment branch back into my local copy. For example there have been some changes in the LDAP libraries in 1.6dev that I don't have in my local copy - before I go and change it.
Average of ratings: -