Posted by lelizondo on July 13, 2011 at 4:21pm
4 followers
| Project: | OpenID Provider |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (duplicate) |
Issue Summary
In some sites, it could be confusing for some users being asked if they want to login "just this time" or if they want to login "always".
I'm attaching a patch to enable/disable the possibility to skip the openid_provider_form and avoid the question.
Comments
#1
And the patch
#2
The patch in #1 is not working, use this one.
#3
A little bit of review.
+++ b/openid_provider.moduleundefined@@ -185,6 +185,13 @@ function openid_provider_admin_settings() {
+ '#title' => t("Enable automatic redirection without asking"),
Sounds redundant - "automatic redirection" is always "without asking", isn't it? :)
+++ b/openid_provider.moduleundefined@@ -185,6 +185,13 @@ function openid_provider_admin_settings() {
+ );
Watch out for extra whitespace in your patch.
+++ b/openid_provider.moduleundefined@@ -282,7 +289,13 @@ function openid_provider_form_submit(&$form, $form_state, $auto_release = FALSE)
+ $automatic_auto_release = variable_get("openid_provider_submit_always", '');
+ if($automatic_auto_release == 1) {
Don't use an extra variable here, just test if (variable_get(..)). Also, need a space after that if statement.
+++ b/openid_provider.moduleundefined@@ -282,7 +289,13 @@ function openid_provider_form_submit(&$form, $form_state, $auto_release = FALSE)
+ _openid_provider_rp_save($user->uid, $form_state['storage']['realm'], TRUE);
Do we really need to save that here? Can't we just bypass the checks?
I guess that would be my main objection to this patch - I would prefer if that setting wouldn't be persistent in two places in the database - if it is enabled, I would expect the users settings to *not* be saved to the database. Exposing the user to the database internals also doesn't seem very usable, so at the very least I would make it so that the table is effectively cleared when the setting is disabled again. But even that is too crude a solution.
Let's just not save the setting please.
I otherwise agree with the feature in principle.
#4
I didn't see this patch / feature earlier should I built my own feature that ties in the openid_sso_provider module. Here's the patch to provide the automatic login.
#5
I think this #314781 is the issue where most of the work has already been done. We may close this as duplicate. (?)
#6
Indeed, see #314781: Access rules for realms, even though I think the two features could be orthogonal.
#7
I'm a bit behind on this issue. It's been awhile. Why wouldn't this approach about allowing certain sites as automatically accepted?
#8
I think you're missing a verb there. ;) I don't understand what you mean...
#9
What I mean is: Why wouldn't this approach about allowing certain sites as automatically accepted work?
#10
I am not saying it wouldn't work, i am saying it overlaps with the work done in #314781: Access rules for realms. Furthermore from what I can tell your patch depends on a third party table that is not created by the openid_provider module, so it just wouldn't work by itself.