-
Notifications
You must be signed in to change notification settings - Fork 122
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
docs: improve documentation about SensitiveEndpointRule replacement #619
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approved, it hurts that it could not be done with my PR because I did it so that it was not necessary to create a new class to specify the role but from configurations (from applicaiton.yml). anyway thank you very much
Boolean sensitive = endpointMethods.get(method); | ||
if (sensitive) { | ||
if (claims == null) { | ||
return checkSensitiveAnonymous(request, method); | ||
} | ||
return checkSensitiveAuthenticated(request, claims, method); | ||
} | ||
return checkNotSensitive(request, claims, method); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is the best way to encapsulate everything calling the threes protected methods from here. All three methods are the same in the sense that they log a message and return the appropriate SecurityRuleResult
but they don't check any condition. Everything is decided in this method.
Did you create those three methods in other for users to be able to override them without override this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, three protected methods makes easier the replacement. See example, in the tests which I show in the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
thanks @oscarcitoz for pointing us to improving this part of the security module. Really valuable feedback. |
close: #606
close: #589
@oscarcitoz this PR refactors
SensitiveEndpointRule
to ease replacement. Moreover, it documents how to restrict management endpoints for a particular role.