Posted by Dane Powell on August 7, 2009 at 7:02pm
| Project: | CAS |
| Version: | 6.x-2.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | metzlerd |
| Status: | closed (fixed) |
Issue Summary
If a user somehow ends up at user/login, they will not see an option to log in via CAS. Can a link to CAS login be added to this page?
Comments
#1
There are some people who want to be able to log in both ways, and there are ample ways to deal with this problem in CAS.
In the cas redirection settings require cas login for pages that are at user* or user/login. If the user visits the user/login page then they would automatically be redirected to the cas login pages.
Also cas provides both login blocks and menu items that can be enabled on these pages if you want to. Also, you can use the formdefaults module to add insturctions to the login page with a link if you so desire.
#2
In my case I can't use redirection settings, because half of our users log in traditionally or using OpenID. I understand this can be overcome using other modules, but why not simply add an option to include a CAS login link on forms, the same way that OpenID does? I think it would be trivial to implement with hook_form_alter. If you don't want to do this yourself would you accept a patch?
#3
Actually, after looking into this it wouldn't be as trivial as I thought. You could simply provide a link to the CAS login page on the form, but this would break compatibility with Persistent Login. I think the "proper" way to do it would be to emulate OpenID - have a link called "Log in via CAS" that uses Javascript to hide the user name and password boxes and replace them with instructional text - i.e., "Click 'Log in' to authenticate via CAS". We then use a form redirect to do our business. Does that sound reasonable?
#4
Why not simply put the form provided by the cas login block on a page somewhere? You could stick it in a panel or put it in the content area on your theme, and give the user the option, (along with the other login blocks provided by other modules).
It's difficult to forsee every place that someone might like a link, and the language surrounding the link would be incredibly site specific This is why this is done in a login block in CAS, cause you can put that anywhere. Login via CAS doesn't speak to anyone except the site maintainer. This is why I think it really should be done in some generic way with formdefaults or some such thing.
If you want to submit a patch putting the cas login form (provided by the block) at a url, that makes sense to me, and I'd hapilly commit that.
Using javascript is not appropriate for this simple problem, and wouldn't fly for the accessibility standards at many university. I'm willing to accept a patch to provide a link to a separate cas provided login form, but I do so reluctantly knowing that the language will be debated, and that we'll then need a "setting" to indicate that link language, and this feels like cruft to me. If you want to tweak a form provided by another module, well that code belongs with the other module, not CAS.
OpenID is a bit more general, since you don't own the accounts, so I can see how you could get away with the same language for all sties. This approach will not likely be acceptible for CAS, since no one really has a "CAS" account but an account at X university or some drupal site or something else.
#5
I understand your concerns; however, I feel like this still might be workable and in many ways more elegant than the current solution. I already have a proof-of-concept that works very much like the OpenID login - anywhere a login form appears, there is a link to log in via CAS below that (I'm sure we can find some way to customize this language) that simply hides the user name and password fields and informs users that they will be redirected to CAS log in when clicking "Log in". This has the advantages of working with other login modules (such as Persistent Login) without the need for any extra code, and taking up much less page real estate. I don't think there are any accessibility issues, because there is actually a checkbox that causes the login to redirect to cas on validation. This checkbox normally gets hidden by JS when the page loads and checked/unchecked if the user clicks the link, but if a user has Javascript disabled they'll just see the checkbox so no problem.
I know it sounds a bit convoluted but really it does seem more elegant to me. I'll try to finish a proof-of-concept and provide a screenshot.
#6
Well I'm willing to be convinced otherwise. Do forward on the screen shots.
I don't think you alleviate the need for code to support persistent login, because cas still has to alter the attribute on the login/account creation. You just alleviate the need for the ui in cas login block.
Understand that some CAS users want the user/login to be "back door". So make sure you support such a configuration.
#7
Okay, I have a working example (user/login). I have a patch but I figured screenshots and a demo would be easier until everything is ironed out.
All login forms will have a small link added to the end (see login1.jpg). When you click this link, it uses jQuery to hide the user/pass textfields and check a hidden checkbox. When you then click "Log in" (see login2.jpg), this checkbox causes Drupal to redirect to CAS on form validation. If a user doesn't have JS enabled, they will simply see the checkbox that causes the login to redirect to CAS (see login_no_js.jpg).
It would be a fairly simple matter to let administrators customize the text you see here, and to make CAS login default. We could even probably use JS to submit the form when the user initially clicks "Log in via CAS", thus saving an extra click.
Let me know if you honestly think this is an improvement. I think I will use it on my site because it takes up less real estate, but there's no reason that it can't coexist with the current system.
#8
Ok, It looks slick enought that I'm ready to review a patch. Do you think it will work ok with screen readers? If you're willing to put in a patch that has appropriate end user customizations for the messages, I'd be willing to see it into the code base, provided it's implemented in a sane way.
Dave
#9
This is awesome, and exactly the kind of solution we are looking for too. Any update on whether a patch was eventually submitted?
#10
No, I have been rather busy and haven't had time to roll a patch. I'll try to get one up within the week.
#11
Okay, here is a preliminary patch. This adds two options to
admin/user/cas: the first lets you choose whether or not to add a CAS login link to all standard login forms, and whether to make CAS login default on these forms. The second lets you name your CAS credentials in order to make the login text more intuitive to average users (for instance, at our school we call our CAS credentials "NetIDs", so if I set this option to "NetID" the login text will appear as "Click here to log in with your NetID" as opposed to "Click here to log in with your CAS username").Note that you must patch cas.module as well as adding four new files to the cas module directory - cas_link.css, cas_default.css, cas_link.js, and cas_default.js. Also note that this patch is against 6.x-2.x-dev. Finally, if you don't see changes to login forms after enabling these options, remember to clear your caches.
#12
The patch generally looks good, but is there really a need for two CSS files? Could these be combined and have the html be the piece that changes (different classes or ids?). That would seem to follow drupal best practices.
#13
In general, yes, there are probably better ways of doing this :) I can look into ways of consolidating these files.
#14
Hello,
Very useful patch.
I had to transform the patch file format from dos to unix to work on linux system.
Thank you
#15
I attempted to consolidate the JS and CSS files, but I quickly realized that it would really be more trouble than it's worth. If we do combine them, all we are going to get is two big files rather than four smaller ones. Also, I think we would have to embed "styling" into the HTML - instead of having ids and classes like "cas-login-form" or whatever, we'd have to have ones like "cas-display-block" or "cas-display-none". These classes would change depending on what the default login preference is. I don't think this is the "Drupal" way to do things - I think it's better to change the CSS and leave the HTML untouched in order to change the appearance of the login form.
Let me know if you disagree or if more work needs to be done. If someone wants to take a crack at consolidating these files, you're welcome to, but I don't think it will be easy... :)
#16
At minimum you must resubmit the patches rolled against the current head version... Too much has changed since then. But i'm concerned about the lack of interest in refactoring this code.
Having the classes set by the theming layer, and not changing the css file plugs in much better to the theming layer of drupal, because site admins can override the different classes using the theme layer. It's possbile that you can adjust the class of the existing html using javascript, which may be a better approach.
If the html is the same, and therefore the classes are the same, the themer cannot adjust any of this without hacking the cas module. I haven't yet had time to look at your approach to see if there's an easy way to organize the css or classes, and I'm not a JSS expert so I have less to say about that. I'm less concerned about two java script files than I am about two CSS files for the reasons stated above.
Please resubmit a patch against head if you're not going to be willing to refactor this code. I'll look more at it, but I'm concerned about the lack of theme integration and will need to spend a lot more time reverse engineering your solution before I'm comfortable committing a change that may encourage people to hack CAS in order to get the stuff to work right.
#17
Okay, here is a revised patch that greatly cleans things up. Just one tiny CSS and one JS file. Let me know what you think.
#18
Whoops, forgot to include a default argument for a variable_get call. Doesn't break anything, just produces watchdog errors. Use this patch and the tarball in 17.
#19
Updated to take into account a fix in #663834: OpenID links with anchor encoded as %2523 which also applies to code in this patch. Can anyone review? I've been using this in production ever since it was submitted with no problems.
#20
Hey Dane. I'm up to my eyeballs right now, but that's the only reason I haven't reviewed it. I promise you're next in line. There won't be any more commits until I review this patch.
#21
This is really close. Just some cosmetics to take care of. I want to strip down the CSS so that it themes everything consistent witht he stock login block (remove padding stuff that doesn't really work with the default drupal theme) and I want to make it so all the messages are fully configurable rather than just what people call the logins.
I'm going to take a stab at rolling a patch with those changes in the interest of getting this done rapidly.
#22
Here's a revised patch against Head. Dane can you look at this and tell me if it meets your needs?
#23
It looks good at first glance, but I can't test it as-is because of the following lines (since I don't have the ldap_integration module and I'm pretty sure this will throw an error)
- module_load_include('module', 'ldap_integration', 'ldapauth');+ include_once('modules/ldap_integration/ldapauth.module');
#24
IT shouldn't because its using include_once instead of require_once. I don't have the LDAP integration module either and have tested this patch without it. I'll look into how that code got changed though, since the way you have it seems more appropriate to me. Is that something you patched manually, or is it something that changed since you last downloaded head?
#25
Nevermind I see it in the patch, now. I thought you were doing diffs a differnt way. It's unrelated so I'll have to figure out how it got in there....
#26
Here's the revised patch without the unrelated code.
#27
Oh, I forgot to mention. I revised the CSS to take away some theme specific padding you appeared to be doing. Here's the css file as I have it in it's entirety.
#edit-cas-identifier {background-position: 0% 50%;
background-repeat: no-repeat;
padding-left: 20px;
}
html #user-login-form li.cas-link,
html.js #user-login li.cas-link {
display: none;
}
html #user-login-form li.uncas-link,
html #user-login li.uncas-link {
display: none;
}
html.js #user-login-form li.cas-link,
html.js #user-login li.cas-link {
list-style: none;
}
#28
Okay, that looks good!
#29
Commited to head and 6.x-2x.-dev. This should be available for further testing in the next dev snapshot.
Dane thanks again for this patch, and for your patience, flexibility and persistence in getting this to RTBC status.
#30
No problem, thanks for being a responsive and helpful maintainer!
#31
Automatically closed -- issue fixed for 2 weeks with no activity.