ModeShape
  1. ModeShape
  2. MODE-1451

Anonymous Authentication in providers array ranks above custom provider

    Details

    • Type: Bug Bug
    • Status: Closed Closed (View Workflow)
    • Priority: Major Major
    • Resolution: Done
    • Affects Version/s: 2.8.0.Final
    • Fix Version/s: 2.8.1.Final, 2.8.1.GA
    • Component/s: Common
    • Labels:
      None
    • Environment:
      n/a
    • Workaround Description:
      Hide

      Remove <anonymousUserRoles jcr:primaryType="mode:option" mode:value="" /> from config. Doesn't throw exception but appears to then set String rawAnonRoles = 'admin' (JcrRepository line 963), never getting to custom provider so not actually authenticated.

      Show
      Remove <anonymousUserRoles jcr:primaryType="mode:option" mode:value="" /> from config. Doesn't throw exception but appears to then set String rawAnonRoles = 'admin' (JcrRepository line 963), never getting to custom provider so not actually authenticated.
    • Estimated Difficulty:
      Low
    • Steps to Reproduce:
      Hide

      Run with anonymous provider in config, then take it out.

      Show
      Run with anonymous provider in config, then take it out.
    • Similar Issues:
      Show 10 results 

      Description

      Iteration over providers starts with anonymousProvider even if there is a custom provider.

      This means if an anonymousProvider with empty ACL value is specified in the config, a non-null result will get returned by org.modeshape.jcr.security.AuthenticationProviders.authenticate().

      Therefore configuration is fragile, in that you need to either use custom OR anonymous, and specifying <anonymousUserRoles jcr:primaryType="mode:option" mode:value="" /> causes the custom provider to be ignored (commenting out fixes the problem).

      I believe the code that populates the providers array should push the anonymous provider to the bottom. I think this can be done in org.modeshape.jcr.JcrRepository constructor (line 984: // Set up any custom AuthenticationProvider classes ...) by moving that statement block to run before anonymousprovider (line 962), however there is also an issue where if no anonymousProvider is specified in the config, one seems to get created anyway (with 'admin' privileges) so it doesn't look like it is possible to actually authenticate as the user, and therefore any custom SecurityContext is never used.

      Removes ambiguity in configuration: I discovered if I commented out the anonymous provider then the 'null' result moves onto custom, but I'd expect Modeshape to try custom then anonymous before throwing a login exception.

      Could be documented but think having custom read/write + anonymous readonly access is a feature Modeshape should provide, and I believe this prevents that from happening.

        Activity

        Hide
        Randall Hauch
        added a comment -

        You're right: the anonymous is registered before the custom authenticators. Thanks for catching and reporting this! I'll get this fixed right now.

        Show
        Randall Hauch
        added a comment - You're right: the anonymous is registered before the custom authenticators. Thanks for catching and reporting this! I'll get this fixed right now.
        Hide
        Randall Hauch
        added a comment -

        BTW, I just checked the 3.0 codebase, and it already is adding the anonymous authenticator last.

        Show
        Randall Hauch
        added a comment - BTW, I just checked the 3.0 codebase, and it already is adding the anonymous authenticator last.
        Hide
        Randall Hauch
        added a comment - - edited

        I just debugged this test case, which does set the anonymous roles to an empty value (see this line in the configuration). What I found is that in this case the rawAnonRoles value is set (on JcrRepository line 963) to an empty string, which means that the if-block on line 964 is entered but the if-block on line 969 should not be entered, and the anonymous authentication provider is not registered. (See the end of this comment for more.)

        It is true that the default for the anonymous roles is "admin", but this default value is replaced if the actual value is non-null (even an empty string is considered non-null) earlier in the constructor (on line 700). So, when the option is obtained on line 963:

        String rawAnonRoles = this.options.get(Option.ANONYMOUS_USER_ROLES);
        

        the rawAnonRoles is actually the non-null value (and in this case, the empty string) from the configuration file.

        However, there is another bug that (with the incorrect order of the anonymous authenticator) does explain why you couldn't disable the anonymous provider: even when rawAnonRoles is an empty string, we are incorrectly adding that empty string to the roles set on line 967. Thus, the code does incorrectly enter the if-block on line 969.

        (This last issue also does not exist in the 3.0 codebase, since our use of JSON for our configuration means that the "security/anonymous/roles" field is expected to be an array of values, and if this array is empty then we assume the anonymous provider is not to be used.)

        Again, thanks for reporting this and helping us get to the bottom of the problems!

        Show
        Randall Hauch
        added a comment - - edited I just debugged this test case , which does set the anonymous roles to an empty value (see this line in the configuration). What I found is that in this case the rawAnonRoles value is set (on JcrRepository line 963 ) to an empty string, which means that the if-block on line 964 is entered but the if-block on line 969 should not be entered, and the anonymous authentication provider is not registered. (See the end of this comment for more.) It is true that the default for the anonymous roles is "admin", but this default value is replaced if the actual value is non-null (even an empty string is considered non-null) earlier in the constructor (on line 700 ). So, when the option is obtained on line 963: String rawAnonRoles = this .options.get(Option.ANONYMOUS_USER_ROLES); the rawAnonRoles is actually the non-null value (and in this case, the empty string) from the configuration file. However, there is another bug that (with the incorrect order of the anonymous authenticator) does explain why you couldn't disable the anonymous provider: even when rawAnonRoles is an empty string, we are incorrectly adding that empty string to the roles set on line 967 . Thus, the code does incorrectly enter the if-block on line 969 . (This last issue also does not exist in the 3.0 codebase, since our use of JSON for our configuration means that the "security/anonymous/roles" field is expected to be an array of values, and if this array is empty then we assume the anonymous provider is not to be used.) Again, thanks for reporting this and helping us get to the bottom of the problems!
        Hide
        Randall Hauch
        added a comment -

        The anonymous authentication provider is now being initialized after the chain of custom authentication providers (rather than before). Also, the anonymous provider is no longer being initialized when the anonymous roles is set to an empty string.

        All unit and integration tests pass with these changes.

        These changes need to be merged onto '2.x' and cherry-picked onto '2.8.x'.

        Show
        Randall Hauch
        added a comment - The anonymous authentication provider is now being initialized after the chain of custom authentication providers (rather than before). Also, the anonymous provider is no longer being initialized when the anonymous roles is set to an empty string. All unit and integration tests pass with these changes. These changes need to be merged onto '2.x' and cherry-picked onto '2.8.x'.
        Hide
        Randall Hauch
        added a comment -

        Merged onto the '2.x' branch and cherry-picked onto the '2.8.x' branch.

        Show
        Randall Hauch
        added a comment - Merged onto the '2.x' branch and cherry-picked onto the '2.8.x' branch.

          People

          • Assignee:
            Randall Hauch
            Reporter:
            David Green
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: