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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lelizondo’s picture

Status: Active » Needs review
FileSize
1.61 KB

And the patch

lelizondo’s picture

FileSize
1.6 KB

The patch in #1 is not working, use this one.

anarcat’s picture

Status: Needs review » Needs work

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.

markwk’s picture

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.

paranojik’s picture

I think this #314781 is the issue where most of the work has already been done. We may close this as duplicate. (?)

anarcat’s picture

Status: Needs work » Closed (duplicate)

Indeed, see #314781: Access rules for realms, even though I think the two features could be orthogonal.

markwk’s picture

I'm a bit behind on this issue. It's been awhile. Why wouldn't this approach about allowing certain sites as automatically accepted?

anarcat’s picture

I think you're missing a verb there. ;) I don't understand what you mean...

markwk’s picture

What I mean is: Why wouldn't this approach about allowing certain sites as automatically accepted work?

anarcat’s picture

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.