Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

micronaut-security-jwt: scopes are not converted to list/iterable when remapping the rolesfinder #787

Closed
frehov opened this issue Sep 24, 2021 · 6 comments · Fixed by #796

Comments

@frehov
Copy link
Contributor

frehov commented Sep 24, 2021

Expected Behavior

When remapping DefaultRolesFinder to read roles from the scope attribute with security.token.roles-name=scope I expect to find multiple roles attached to the authentication object.

Actual Behaviour

When remapping DefaultRolesFinder to read roles from the scope attribute with security.token.roles-name=scope I find only one entry in the list of roles.

Steps To Reproduce

Running with cognito user pools with jwks validation set up.
Create a resource server in cognito with multiple scopes.
Create an app client and grant access to multiple scopes from the resource server

Set up a basic micronaut app with security and and add micronaut-security-jwt to enable jwt-validation.
Use security.token.roles-name=scope to remap the DefaultRolesFinder to generate roles from scopes attribute.
Secure one endpoint with @RolesAllowed(<a single scope from the resource server>)

Request a token with multiple scopes with postman following the client_credentials flow.

Hit the secured endpoint with postman and the token requested, and watch the request get a 403 Forbidden as there are no matching roles.

Debugging the DefaultRolesFinder it easy to see what happens.
image

The scopes are required to be space separated in the client credentials flow, and as such they're not mapped to an iterable since the attribute is seen as a single string.
But it should be treated as a space separated list, and thus be an iterable.

The following workaround (kotlin) has been used to successfully split the scopes, but it's not the most elegant solution, as we were relying on builtin configuration to resolve these issues.

    override fun resolveRoles(attributes: MutableMap<String, Any>?): MutableList<String> {
        return super.resolveRoles(attributes)
            .flatMap { scopes -> scopes.split(" ") }
            .toMutableList()

    }

Environment Information

  • Windows 10
  • JDK 16

Example Application

No response

Version

3.0.1

@sdelamo
Copy link
Contributor

sdelamo commented Sep 28, 2021

I don't think this is a bug.

However, we could add some flexibility via configuration.

Maybe we could add a new configuration property security.token.roles-separator (defaul to null to avoid being a breaking change).

And if in your case you could set it to

micronaut: 
     security:
         token:
             roles-name: 'scope'
             roles-separator: ' '

@frehov what do you think?

@frehov
Copy link
Contributor Author

frehov commented Sep 28, 2021

I think that would work.
I had something more extensive in mind, but this is probably the most elegant solution.

It's probably not a bug per se, as you're only relying on what the nimbuds library is parsing and returning as the ClaimsSet

I can most likely have a PR for this up within the day if that's of any interest?

@sdelamo
Copy link
Contributor

sdelamo commented Sep 28, 2021

Yes, PRs are welcome. I will review it and get it merged.

@sdelamo sdelamo removed their assignment Sep 28, 2021
@frehov
Copy link
Contributor Author

frehov commented Sep 28, 2021

I do have one question though.
Should the splitting be part of the DefaultRolesFinder, or should it provide a consistent experience so that every RolesFinder will get the same inital map of attributes, i.e not having to implement the splitting yourself when implementing a RolesFinder

I believe the best option is going for the consistent feel, and thus doing the separation in DefaultJwtAuthenticationFactory just after getting the attributes from the jwt so that the DefaultRolesFinder still works without modifications.

@sdelamo
Copy link
Contributor

sdelamo commented Sep 28, 2021

I think it probably best to have it in the DefaultRolesFinder. Users may not be using non JWT authentication.

@frehov
Copy link
Contributor Author

frehov commented Sep 28, 2021

Alright, I've submitted the PR with the change on the DefaultRolesFinder

Micronaut Developers Work Coordination automation moved this from To do to Done Sep 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants