The use case in mind is that the user does not need to navigate to a certain page in order to authenticate. If the appropriate headers are present Drupal can auto login the user and assign appropriate roles based on configuration. This would be similar to the processing that currently occurs in tamauth_page() but would be done during hook_boot().

This behaviour needs to be controlled by a configuration variable for performance and security reasons.

Patch in progress - will be posted soon.

CommentFileSizeAuthor
#6 tamauth-autologin-ui-logout-roles-6.patch19.06 KBjoel.gilchrist
#4 tamauth-autologin-ui-logout-roles-4.patch18.51 KBjoel.gilchrist
#3 tamauth-autologin-ui-logout-3.patch16.91 KBjoel.gilchrist
#1 tamauth-autologin-2499649-2.patch4.8 KBjoel.gilchrist

Comments

joel.gilchrist’s picture

joel.gilchrist commented
StatusFileSize
new tamauth-autologin-2499649-2.patch4.8 KB

Here's a patch. Please review.

joel.gilchrist’s picture

joel.gilchrist commented
Assigned: joel.gilchrist » Unassigned
Status: Active » Needs review
joel.gilchrist’s picture

joel.gilchrist commented
StatusFileSize
new tamauth-autologin-ui-logout-3.patch16.91 KB

Hi,

During testing I found that a logout capability is needed. TAM needs to be informed about the user logging out and this is done by a redirect to a TAM logout link. So a config value for that is added and a pattern from the redirect_after_logout module is used. Unfortunately that module doesn't quite meet the needs.

I also took the admin ui, security fix and hook from https://www.drupal.org/node/2328555 and merged them into this patch as is it definitely useful for my testing to have all of this in one version of the module.

joel.gilchrist’s picture

joel.gilchrist commented
StatusFileSize
new tamauth-autologin-ui-logout-roles-4.patch18.51 KB

We also have a use case where users may not have meaningful role information in TAM. So I've added a configuration parameter that dictates whether the system should expect and rely upon TAM roles and use the role mapping table or allow for managing roles in Drupal. This means the role mapping table is not used and users need to be pre-configured or after logging in once, an administrator needs to set up their roles.

This might merit it's own issue but it is so dependent upon the other changes in this thread I'm adding it here.

Also changed the validation on the logout redirect url so that it can be relative. Useful for cases where the site is behind a dns or loadbalancer and we just want to point to pkmslogout in a relative way so that if dns changes the site is unaffected.

skwashd’s picture

skwashd commented
Status: Needs review » Needs work

Overall the patch looks really good. There is a few small things that need to be fixed. I don't currently have access to a TAM enabled environment to test these changes.

  1. +++ b/tamauth.admin.inc
@@ -41,22 +125,154 @@ function tamauth_settings_form($form, $form_state) {
 function tamauth_settings_form_submit($form, $form_state) {

    Given all the additional variables, it would be cleaner if we used system_settings_form() and did the field mapping massaging in the validation hook. That would mean we could remove this function. 

  2. +++ b/tamauth.module
@@ -9,6 +9,81 @@
+  if (tamauth_auto_login()) {

    I find code is cleaner if you invert this test and return early. It means the whole block isn't indented. 

  3. +++ b/tamauth.module
@@ -9,6 +9,81 @@
+    if (empty($user->uid) && $name) {

    Again this is another chance to return early. 

  4. +++ b/tamauth.module
@@ -9,6 +9,81 @@
+        echo t("Invalid authentication.");

    print() 

  5. +++ b/tamauth.module
@@ -9,6 +9,81 @@
+      } else {

    }
else {

  6. +++ b/tamauth.module
@@ -9,6 +9,81 @@
+          echo t("Invalid user authentication.");

    print() 

  7. +++ b/tamauth.module
@@ -9,6 +9,81 @@
+    $destination = variable_get('tamauth_logout_redirect', '');

    You shouldn't need the second argument as the default is NULL. 

  8. +++ b/tamauth.module
@@ -56,14 +149,13 @@ function tamauth_page() {
+  $roles_valid = tamauth_manage_roles_in_drupal() || (bool) count(array_intersect($roles, array_keys($role_map)));

    This change makes the code less readable. Please revert it. 

  9. +++ b/tamauth.module
@@ -56,14 +149,13 @@ function tamauth_page() {
+    echo t("Invalid authentication.");

    print()

    I liked the old "Nice try..." message. 

  10. +++ b/tamauth.module
@@ -73,10 +165,22 @@ function tamauth_page() {
+  if (sizeof(module_implements('tamauth_login_event')) > 0) {

    count()

    Please document this hook in tamauth.api.php. Am I in assuming that it calls drupal_goto()? 

  11. +++ b/tamauth.module
@@ -217,3 +321,49 @@ function tamauth_map_roles($tam_roles) {
+ * @return integer - 0 if false, 1 if true

    Given the docs, these really should be booleans. The default value below should be updated to FALSE too. 

  12. +++ b/tamauth.module
@@ -217,3 +321,49 @@ function tamauth_map_roles($tam_roles) {
+ * ¶

    Trailing whitespace. 

  13. +++ b/tamauth.module
@@ -217,3 +321,49 @@ function tamauth_map_roles($tam_roles) {
+  return variable_get('tamauth_logout_redirect', '');

    The default value is NULL, is there any reason why an empty string is more desirable? 

  14. +++ b/tamauth.module
@@ -217,3 +321,49 @@ function tamauth_map_roles($tam_roles) {
+ * ¶

    Trailing whitespace 

  15. +++ b/tamauth.module
@@ -217,3 +321,49 @@ function tamauth_map_roles($tam_roles) {
+  return variable_get('tamauth_manage_roles_in_drupal', '');

    The default value is NULL, is there any reason why an empty string is more desirable?

joel.gilchrist’s picture

joel.gilchrist commented
StatusFileSize
new tamauth-autologin-ui-logout-roles-6.patch19.06 KB

Changes from #5 have been completed and tested. Please review an updated patch.

joel.gilchrist’s picture

joel.gilchrist commented
Status: Needs work » Needs review