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

DefaultSecurityService should use RolesFinder #328

Merged
merged 6 commits into from
Jul 24, 2020

Conversation

sdelamo
Copy link
Contributor

@sdelamo sdelamo commented Jul 23, 2020

I agree with you @dstepanov DefaultSecurityServiceshould use RolesFinder. Could you check this PR and let me know if it aligns with what you are looking for.

Aside: I've created a RolesFinder::hasAnyRequiredRoles so that we have a single place which can be override to support for example Case Insensitive roles

@sdelamo sdelamo added this to In progress in Micronaut Development via automation Jul 23, 2020
@sdelamo sdelamo self-assigned this Jul 23, 2020
@dstepanov
Copy link
Contributor

Looks good, maybe it would be better to have RolesFinder::hasAnyRequiredRoles(Claims claims, requiredRoles)
so this code can be reduced to one call:

.map(rolesFinder::findInClaims)
.map(grantedRoles -> rolesFinder.hasAnyRequiredRoles(Collections.singletonList(role), grantedRoles))

BTW: Can you please take a look at #289? I can redo PR for master.

@sdelamo
Copy link
Contributor Author

sdelamo commented Jul 24, 2020

I've created a RolesFinder::hasAnyRequiredRoles(List<String> requiredRoles, Authentication authentication) to simplify things further.

@sdelamo sdelamo added the type: bug Something isn't working label Jul 24, 2020
@sdelamo sdelamo merged commit 0d0b587 into master Jul 24, 2020
Micronaut Development automation moved this from In progress to Done Jul 24, 2020
@sdelamo sdelamo deleted the pullrequests/dstepanov/roles-f branch July 24, 2020 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants