Moodle
  1. Moodle
  2. MDL-20772

move forward with stackable auth modules

    Details

    • Type: New Feature New Feature
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 2.0
    • Fix Version/s: DEV backlog
    • Component/s: Authentication
    • Labels:
      None
    • Affected Branches:
      MOODLE_20_STABLE
    • Rank:
      5647

      Description

      Background discussion here: http://moodle.org/mod/forum/discuss.php?d=136675

      I've attached a first attempt at a patch for actually using the AUTH_* definitions already in lib/authlib.php. This code respects AUTH_DENIED so that it will be possible to create stackable blacklist authentication modules in addition to the stackable whitelist modules we already have.

      I wrote this with the idea of keeping the patch as small as possible, but I think it would be better if we made it possible to provide more information back to the user. For example, it would be useful if, while configuring your authentication modules stack, you could turn on some more verbosity and be told "login denied for XYZ reason by ABC module" instead of simply "Login denied".

      Is there any interest in considering this for inclusion in 2.0?

      Cheers,
      Matt

      1. moquist_auth_fail3.patch
        4 kB
        Matt Oquist
      2. more-stackable.diff
        3 kB
        Matt Oquist
      3. skodak_auth_fail2.patch
        5 kB
        Petr Škoda

        Activity

        Hide
        Anthony Borrow added a comment -

        Matt - Looks good. My only suggestion for improvement has to do with communication with the user. Might it be helpful to indicate a reason(s) why the access is being denied. For example, Login access denied: Blacklisted IP. I realize that the more information you give the easier it allows some folks to get back in; however, looking at it from the perspective of most good users it may be helpful to specify a more exact cause for the denial. In any case, just something to consider. Peace - Anthony

        Show
        Anthony Borrow added a comment - Matt - Looks good. My only suggestion for improvement has to do with communication with the user. Might it be helpful to indicate a reason(s) why the access is being denied. For example, Login access denied: Blacklisted IP. I realize that the more information you give the easier it allows some folks to get back in; however, looking at it from the perspective of most good users it may be helpful to specify a more exact cause for the denial. In any case, just something to consider. Peace - Anthony
        Hide
        Petr Škoda added a comment -

        why not use loginpage_hook() instead, you may disable login page completely from some IP addresses?

        Show
        Petr Škoda added a comment - why not use loginpage_hook() instead, you may disable login page completely from some IP addresses?
        Hide
        Petr Škoda added a comment -

        attaching patch with some more minor modifications, please review

        Show
        Petr Škoda added a comment - attaching patch with some more minor modifications, please review
        Hide
        Matt Oquist added a comment -

        skodak_auth_fail2.patch looks good to me.

        Should we make AUTH_OK 1 and AUTH_FAIL 0? That way they'll still correspond to the 'true' and 'false' that are being used now. I've attached another variant that does this.

        Show
        Matt Oquist added a comment - skodak_auth_fail2.patch looks good to me. Should we make AUTH_OK 1 and AUTH_FAIL 0? That way they'll still correspond to the 'true' and 'false' that are being used now. I've attached another variant that does this.
        Hide
        Matt Oquist added a comment -

        This swaps the values of AUTH_FAIL and AUTH_OK so they correspond to 'false' and 'true'.

        Show
        Matt Oquist added a comment - This swaps the values of AUTH_FAIL and AUTH_OK so they correspond to 'false' and 'true'.
        Hide
        Iñaki Arenaza added a comment -

        Matt,

        while your last change makes certain sense, I'd explicitly code a check for AUTH_FAIL or 'false' in the last hunk of the patch. This would make the code more obvious to anyone having a look at it, which is a good thing when dealing with security related code Other than that, it looks good to me.

        Saludos
        Iñaki.

        Show
        Iñaki Arenaza added a comment - Matt, while your last change makes certain sense, I'd explicitly code a check for AUTH_FAIL or 'false' in the last hunk of the patch. This would make the code more obvious to anyone having a look at it, which is a good thing when dealing with security related code Other than that, it looks good to me. Saludos Iñaki.

          People

          • Votes:
            2 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated: