Details

    • Improvement
    • Status: Resolved
    • Major
    • Resolution: Won't Fix
    • 1.3.14
    • None
    • core
    • None

    Description

      The TokenLoginModule and specifically TokenProviderImpl only look at SimpleCredentials.getUserID() when creating a token.

      However, in certain situations, such as with the ExternalLoginModule and non-username/password credentials, the SimpleCredentials are used but don't have a user id as the real user id is determined not by the caller of Repository.login(), but by the external identity provider inside the ExternalLoginModule (and the credentials might not include any kind of user id, say an opaque token from an external service). In this case, SimpleCredentials.getUserID() returns null and the token implementation fails to create a token and does not return it in the .token attribute of the credentials.

      Instead, the TokenLoginModule should look at the shared javax.security.auth.login.name attribute, which can de-facto override a SimpleCredentials.getUserID(), as it happens in the ExternalLoginModule.

      Attachments

        Issue Links

        Activity

          alexander.klimetschek Alexander Klimetschek added a comment - - edited

          Attached a patch (against oak 1.2.2 release), that basically takes the shared attribute javax.security.auth.login.name over the SimpleCredentials.getUserID() if it's not null.

          It was a bit tricky, as this logic including the write-back-token-to-credentials happens inside the TokenProviderImpl. The use of the TokenProvider.createToken(Credentials) method prevents one from passing the separate shared login name. Thus I had to move this logic to the TokenLoginModule, and effectively deprecating TokenProvider.createToken(Credentials) (being called from the TokenLoginModule was it's only usage within Oak at least).

          I think this makes sense: the token provider should be concerned only with creating & managing tokens in the repository (or whatever persistence is wanted). Parsing jcr Credentials and sending back the new token in the .token attribute if it's a SimpleCredentials is orthogonal and better left with the TokenLoginModule anyway.

          alexander.klimetschek Alexander Klimetschek added a comment - - edited Attached a patch (against oak 1.2.2 release), that basically takes the shared attribute javax.security.auth.login.name over the SimpleCredentials.getUserID() if it's not null. It was a bit tricky, as this logic including the write-back-token-to-credentials happens inside the TokenProviderImpl. The use of the TokenProvider.createToken(Credentials) method prevents one from passing the separate shared login name. Thus I had to move this logic to the TokenLoginModule, and effectively deprecating TokenProvider.createToken(Credentials) (being called from the TokenLoginModule was it's only usage within Oak at least). I think this makes sense: the token provider should be concerned only with creating & managing tokens in the repository (or whatever persistence is wanted). Parsing jcr Credentials and sending back the new token in the .token attribute if it's a SimpleCredentials is orthogonal and better left with the TokenLoginModule anyway.

          To explain our use case a bit:

          We have a custom external identity provider (for an external user & authentication system). One authentication mechanism is using oauth, in which an authorization code is sent with the request after a login page, and we pass this code through from an Apache Sling AuthenticationHandler to the repository login. This is done using SimpleCredentials (because a. the ExternalLoginModule only supports this at this time and b. the retrieve-token-back feature of the TokenLoginModule only works with SimpleCredentials as well), which gets a null userId, empty password and a special attribute containing the code.

          After this initial login using the code, we want to continue the browser "session" with the oak login token. Because of that, the SimpleCredentials also gets the .token attribute set in the authentication handler so after the session login the token is present there and can be set as a cookie on the same response for subsequent requests. This would make the whole process seamless, avoiding extra (privileged) sessions.

          Workaround:

          I can use a utility (granite TokenUtil) to create the token after the session was created and use the session user id to use for the token. This creates 2 extra sessions as it uses impersonation from a privileged session, which is overhead I would like to avoid.

          alexander.klimetschek Alexander Klimetschek added a comment - To explain our use case a bit: We have a custom external identity provider (for an external user & authentication system). One authentication mechanism is using oauth, in which an authorization code is sent with the request after a login page, and we pass this code through from an Apache Sling AuthenticationHandler to the repository login. This is done using SimpleCredentials (because a. the ExternalLoginModule only supports this at this time and b. the retrieve-token-back feature of the TokenLoginModule only works with SimpleCredentials as well), which gets a null userId, empty password and a special attribute containing the code. After this initial login using the code, we want to continue the browser "session" with the oak login token. Because of that, the SimpleCredentials also gets the .token attribute set in the authentication handler so after the session login the token is present there and can be set as a cookie on the same response for subsequent requests. This would make the whole process seamless, avoiding extra (privileged) sessions. Workaround: I can use a utility (granite TokenUtil) to create the token after the session was created and use the session user id to use for the token. This creates 2 extra sessions as it uses impersonation from a privileged session, which is overhead I would like to avoid.

          Updated the patch with a unit test in TokenLoginModuleTest. This is a bit elaborate, since I had to add a second test LoginModule that simulates the scenario where another LoginModule sets the javax.security.auth.login.name to a different user name.

          alexander.klimetschek Alexander Klimetschek added a comment - Updated the patch with a unit test in TokenLoginModuleTest. This is a bit elaborate, since I had to add a second test LoginModule that simulates the scenario where another LoginModule sets the javax.security.auth.login.name to a different user name.

          sorry... but that patch cannot be applied. it's on purpose that the TokenLoginModule doesn't care about the nature of the shared credentials because the loginmodule doesn't know what type of credentials are supported and respected by the TokenProvider implementations (note: there might be multiple and the default one might simply not be relevant). So, we are not going to bake the SimpleCredentials into the TokenLoginModule.

          angela Angela Schreiber added a comment - sorry... but that patch cannot be applied. it's on purpose that the TokenLoginModule doesn't care about the nature of the shared credentials because the loginmodule doesn't know what type of credentials are supported and respected by the TokenProvider implementations (note: there might be multiple and the default one might simply not be relevant). So, we are not going to bake the SimpleCredentials into the TokenLoginModule .

          If you want to put it into the TokenProvider, you have to change its API (a method to pass in credentials plus user id), which I think is putting too much complexity into the TokenProvider and breaks existing custom TokenProviders (if there are any).

          I see the separation this way:

          TokenProvider: token generation & storage
          TokenLoginModule: mapping from credentials, extra validation

          As an application, I don't think it's common thatyou want to switch the token generation to sonething else (too many security/crypto considerations that applications builders can't or don't want to make).

          alexander.klimetschek Alexander Klimetschek added a comment - If you want to put it into the TokenProvider, you have to change its API (a method to pass in credentials plus user id), which I think is putting too much complexity into the TokenProvider and breaks existing custom TokenProviders (if there are any). I see the separation this way: TokenProvider: token generation & storage TokenLoginModule: mapping from credentials, extra validation As an application, I don't think it's common thatyou want to switch the token generation to sonething else (too many security/crypto considerations that applications builders can't or don't want to make).

          I am sorry, but I don't share you interpretation. Furthermore, we already have different implementations of the TokenProvider in production. If you as an application don't have the need for a custom implementation of the TokenProvider I am obviously happy that the code is used and will try to meet you needs but I hope you understand that I don't want to apply a hack that makes things work for you but breaks it for others by introducing backwards incompatible behavior (btw, neither in the TokenLoginModule nor in the TokenProvider). Oak is an implementation of the JCR specification that allows for any type of Credentials and IMO it's important that reflect that fact throughout the various authentication related parts. otherwise the API would have probably defined to only take SimpleCredentials.

          angela Angela Schreiber added a comment - I am sorry, but I don't share you interpretation. Furthermore, we already have different implementations of the TokenProvider in production. If you as an application don't have the need for a custom implementation of the TokenProvider I am obviously happy that the code is used and will try to meet you needs but I hope you understand that I don't want to apply a hack that makes things work for you but breaks it for others by introducing backwards incompatible behavior (btw, neither in the TokenLoginModule nor in the TokenProvider ). Oak is an implementation of the JCR specification that allows for any type of Credentials and IMO it's important that reflect that fact throughout the various authentication related parts. otherwise the API would have probably defined to only take SimpleCredentials .

          AFAICS, my patch is not breaking any existing behaviour, that's exactly why I built it this way.

          alexander.klimetschek Alexander Klimetschek added a comment - AFAICS, my patch is not breaking any existing behaviour, that's exactly why I built it this way.

          Just to be sure: will OAK-4129 support setting the shared login name by an ExternalIdentityProvider? (The description doesn't particularly mention it, but maybe it's implied)

          Custom credential support for ext id providers in itself does not solve the original problem, that some do not know the user id / login name up front, but only in the middle of the login module process when they are called.

          Since it's the same logic for all ext id providers (use ExternalIdentity.getId()), it makes sense to have this handling in the ExternalLoginModule, instead of having each provider reimplement it.

          alexander.klimetschek Alexander Klimetschek added a comment - Just to be sure: will OAK-4129 support setting the shared login name by an ExternalIdentityProvider? (The description doesn't particularly mention it, but maybe it's implied) Custom credential support for ext id providers in itself does not solve the original problem, that some do not know the user id / login name up front, but only in the middle of the login module process when they are called. Since it's the same logic for all ext id providers (use ExternalIdentity.getId()), it makes sense to have this handling in the ExternalLoginModule, instead of having each provider reimplement it.

          Ping.

          alexander.klimetschek Alexander Klimetschek added a comment - Ping.

          Created OAK-6345 with the recommended steps from above.

          alexander.klimetschek Alexander Klimetschek added a comment - Created OAK-6345 with the recommended steps from above.

          People

            angela Angela Schreiber
            alexander.klimetschek Alexander Klimetschek
            Votes:
            0 Vote for this issue
            Watchers:
            Start watching this issue

            Dates

              Created:
              Updated:
              Resolved:

              Slack