Posted by bfroehle on February 12, 2011 at 11:09pm
3 followers
| Project: | CAS |
| Version: | 7.x-1.x-dev |
| Component: | Code |
| Category: | task |
| Priority: | normal |
| Assigned: | bfroehle |
| Status: | closed (fixed) |
Issue Summary
Lots of helper modules would like to get their hands on the CAS user data before users are created. This would allow integration with LDAP (e.g., populating user roles and profile fields based upon LDAP data), SAML, etc.
We need to develop a list of locations in the code where these hooks need to be provided. Some hooks already exist (#1059204: Rename hooks) and these should be reconsider as well.
Comments
#1
One hook suggestion in #1011848-3: Add Testing Routines -- offer a hook which allows modules to directly alter the phpCAS object before we ask for authentication.
#2
But the cas user data isn't there when this alter would be called, it would seem that this would need to be after the redirect/ticket processing is done before it would be truly useful. I agree that hooks should be reconsidered. Here's what I'm currently thinking:
hook_cas_object_alter - Alters the cas object prior to performing authentication. I'm not sure that this is really needed. It would seem to me that if we were going to completely refactor cas auth we'd want a mock phpcas object, not a mock server object. (it wouldn't actually do the auth to perform the tests) maybe this discussion should continue on #1011848: Add Testing Routines.
hook_cas_login_user_alter - Alters a user object prior to login. In this case the cas authentication has been performed, but nothing else has been done. Perhaps a property called deny or unsetting the object would have the affect of aborting the login process the way that auth_filter currently works. This would let you alter the cas user name object,etc. We might want to create our own object rather than referencing phpCAS, but I'm open to suggestions here.
hook_cas_user_presave - Alters a drupal user object prior to saving info. May only be needed for the 6.x version of the code, because this can be handled via the user_presave hook, isn't that correct?
#3
This is useful to allow modules to do things like call phpCAS::callSomeSpecialRoutine(). Even if there isn't a clear use yet it'd be nice to have this hook so admins could implement advanced CAS features without waiting on us to implement a UI. We might want to call it hook_cas_phpcas_alter() [since object might refer to many different things].
Or call it hook_cas_user_login_alter() ? Drupal seems to like putting the verb(s) last in the hook names.
Maybe the object could have the properties:
Would anything else fit in here? Default for register_denied could be taken from the CAS preferences.
We need to decide if this should be called when we register users? Or every time a CAS user logs in? We don't want to duplicate other user hooks. Another possibility is trying to add something to the global $user to indicate that this is a CAS user --- like setting $user->cas_name.
Primary use case is going to be fetching LDAP data or integrating SAML data into the $user before we create the user. Or maybe every time the user logs in.
#4
So we don't forget, there are some preliminary hook renaming patches #1059204: Rename hooks.
#5
I thought about this a bit today. In the attached patch, I've created three hooks:
function hook_cas_user_alter(&$cas_user) {// Modify CAS user properties before the user is logged in.
}
function hook_cas_user_register_alter(&$edit) {// Modify a Drupal user account before a CAS user is registered.
}
function hook_cas_phpcas_alter() {// Modify phpCAS authentication properties.
}
Any comments? Don't commit this yet — I want some time to think about these design choices and proofread my comments.
#6
Regarding 1 - I'm not sure we still don't need it allow login. It means that if someones account got removed from the ldap group that allowed them to register they'd still have access to the site, unless someone remembered to go into the drupal site to update them. This seems desireable still to me.
Regarding 2 - So if you were going to try and alter say an email attribute, and it changed on the server, it wouldn't change on the drupal site? I think it would be desirable to synchronize certain elements of profile information from the contributing modules. Email might be a minimum (since we might hide that sometime). Also you can't do role synchronization if you only save on register.
#7
Re #1: Okay, didn't think of that. I'll add back in the login functionality.
Re #2: This would necessitate a different hook. How about hook_cas_user_login(), which would allow modules to react to a CAS user logging in?
#8
Changes from the previously uploaded patch:
- added a 'login' property to $cas_user in hook_cas_user_alter().
- added a
function hook_cas_user_login($cas_user) {which is called immediately after a Drupal user is logged in from CAS. Users of the hook are expected to pull the Drupal user data from the global $user object. This hook can be used to synchronize the Drupal object with a LDAP server, for example. Note that this is essentially a duplicate of hook_user_login() which is only called for CAS users.Comments? (I still want some more time to really think about this before committing it).
#10
On second thought, It is possible that we don't need a separate hook from user_login if we make sure that extra data is loaded into the user object to indicate a cas based account. Maybe by implementing the user_load hook in cas to add the fact that we have logged in via cas? Then we would add the cas user name to the user object. Actually, we could just make sure that data was in the user object before we invoke the login hook? Come to think of it, is there a good reason not to use the same approach and just invoke user_register hook for that hook? Either way I suppose that we should do it consistently. Iguess the big problem with that approach is that each module has to do its own separate user save operation, but that makes the hook more general. Each approach seems to have drawbacks.
What do you think? How does openid do this or not?
#11
There aren't really good hook for this that get called before the database saves. There is a hook_user_insert() which gets called after a user is inserted, i.e., after a user is registered. And it is the same for hook_user_update() which gets called after a user is saved (which we do every login when re-update the user roles).
As for hook_cas_user_login(), we should probably remove this function (as it is duplicitous to hook_user_login()), and instead guarantee that we set $edit['cas_name'] to be the CAS username.
Perhaps we want instead a hook which fires right before we call user_finalize_login() to allow for one more shot at editing the account before we login? A
hook_cas_user_presave(&$edit, $account), where we guarantee to save any changes to$editand then log the user in. And we'll stuff 'cas_name' in $edit?#12
I agree, so does that leave us with:
hook_cas_phpcas_alter -- Called after we instantiate the phpCAS object, but before we call any validation methods?
hook_cas_user_alter -- Called before we determine who the matching drupal user is
hook_cas_user_presave -- Called just before we save the user and finalize login.
in rough order of execution with the mods to make sure there's a cas_name property in all hook_user calls? I think we don't need hook_cas_user_register_alter as I think that's also a duplicate with just firing hook_user_register?
#13
Yes, I think that looks good. I'm still a bit on the fence regarding the hook_cas_user_register_alter() call, as for example, my LDAP server is slow and I'd like to only query it for the user's real name when the user is first registered.
However, if we were to set $edit['cas_first_login'] or some such to indicate this, and then hook_cas_user_presave() would work great for this functionality. I think it's good in that it reduces the number of hook calls, and eliminates code duplication in other modules which implement these types of hooks. (i.e., now they can do
if ($sync_every_time || $edit['cas_first_login']) { /* do sync procedure */ }which is slick.I am flexible to change the 'cas_first_login' key name if you have any suggestions.
#14
I've attempted to implement what we discussed in #12. See attached. We will need to review this some more before it gets committed.
#15
This is very close, I think, to commit ready. A couple of minor things:
There's an un-indented code block between 267 and 289.
The error messages are a bit too stringent in the case where check-authentication is called and not force authentication. I'm not sure we want to alert users strongly when they don't pass the login check criteria. If they access a page that requires auth, there going to get an access denied message anyway, but in many cases anonymous access might be fine, and since we're using the CheckAuthentication method in many cases, any user just visiting a site would get an error message. Even if they didn't "intend" to log in. Does that make sense?
#16
Aha, you are totally correct. We need to look at
$cas_force_loginto see if we forced authentication. Display an error if we did force authenticate, but fail silently if we did not.#17
Okay, patch implementing #16. Changes since patch in #14.
global $account;since we never use that variable.What do you think? As soon as we're happy with it I can try to backport it to 6.x-3.x (which should be straightforward). There seems to be zero action on the CVS Application queue these days, so I have no idea when that'll happen.
#18
Stylistic notes to myself:
+++ cas.api.phpundefined@@ -0,0 +1,116 @@
+ * machine readable. This field is not the Drupal name of the user. Instead, ¶
trailing whitespace.
+++ cas.api.phpundefined@@ -0,0 +1,116 @@
+ * The 'register' parameter controls whether an account should be created if ¶
trailing whitespace.
+++ cas.moduleundefined@@ -200,66 +154,57 @@ function cas_login_check() {
+ // Bail out if a module denied login access for this user or unset the user name.
break at 80 chars.
Powered by Dreditor.
#19
I think they're probably holding for the GIT migration.... which should happen Thursday? Looks like I'll need to ramp up on GIT pretty darn quick.
I think we're basically there. I'm going to write some hooks in a test module and run it through it's paces tomorrow and try and get this commit in before Thursday's outage. If I don't get to it, we'll touch base on Friday and see where we are. I'll need to get my GIT environment set up before I can commit any more patches after Thursday.
Regarding the port... whatever you think is easiest.... We still have some references ton CAS_AUTHMAP_INTERNAL that need to be removed, and other patches to get the UI going. I'm happy to flesh this out and do a more significant backport patch if you think that would be easier for you, but if you'd rather keep the two DEV branches in sync, I can understand.
#21
Okay, please do not commit these. I've been working on incorporating a lot of changes into a sandbox project http://drupal.org/sandbox/bfroehle/1075074 and it'll probably be easier to just pull changes from there instead.
#22
Development code in a sandbox:
See http://drupal.org/sandbox/bfroehle/1075074
Due to the interdependency with other issues, it's hard to come up with a single commit that resolves this issue. Many (or all) of the commits in the sandbox should be able to be pushed to the primary CAS repository.
#23
Committed to 7.x-1.x and 6.x-3.x branches.
#24
Automatically closed -- issue fixed for 2 weeks with no activity.