Make single definition of `defaultRolePrefix` and `rolePrefix` #3956

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
@eddumelendez
Contributor

eddumelendez commented Jul 2, 2016

Previous to this commit, role prefix had to be set in every class
causing repetition. Now, bean GrantedAuthorityDefaults can be used to
define the role prefix in a single point.

Fixes gh-3701

@eddumelendez

This comment has been minimized.

Show comment
Hide comment
@eddumelendez

eddumelendez Jul 3, 2016

Contributor

@rwinch is it ok to replace all classes involved in this commit? Early feedback is welcome. Thanks in advance.

Contributor

eddumelendez commented Jul 3, 2016

@rwinch is it ok to replace all classes involved in this commit? Early feedback is welcome. Thanks in advance.

@rwinch

This comment has been minimized.

Show comment
Hide comment
@rwinch

rwinch Jul 7, 2016

Member

@eddumelendez Thanks for the PR! I think I'd prefer that any of the domain object avoid adding ApplicationContextAware. Instead, the configuration specific classes would try to detect the default role prefix and then set that on the domain object. This decouples the way the defaults are provided from the domain object and keeps the logic in the configuration.

Member

rwinch commented Jul 7, 2016

@eddumelendez Thanks for the PR! I think I'd prefer that any of the domain object avoid adding ApplicationContextAware. Instead, the configuration specific classes would try to detect the default role prefix and then set that on the domain object. This decouples the way the defaults are provided from the domain object and keeps the logic in the configuration.

@rwinch rwinch self-assigned this Jul 7, 2016

@eddumelendez

This comment has been minimized.

Show comment
Hide comment
@eddumelendez

eddumelendez Jul 14, 2016

Contributor

@rwinch Thanks for feedback.

I think I'd prefer that any of the domain object avoid adding ApplicationContextAware

Just to clarify, you meant RoleVoter, right?

Contributor

eddumelendez commented Jul 14, 2016

@rwinch Thanks for feedback.

I think I'd prefer that any of the domain object avoid adding ApplicationContextAware

Just to clarify, you meant RoleVoter, right?

@rwinch rwinch modified the milestone: 4.2.0 M1 Aug 15, 2016

@rwinch

This comment has been minimized.

Show comment
Hide comment
@rwinch

rwinch Aug 30, 2016

Member

@eddumelendez Thanks for the fast response.

I mean unless a class is in the spring-security-config module this PR should not add implements ApplicationContextAware to it. Some examples of classes that should NOT implement ApplicationContextAware are: Jsr250MethodSecurityMetadataSource, SecurityExpressionRoot, DefaultMethodSecurityExpressionHandler, RunAsManagerImpl, RoleVoter, JdbcDaoImpl, etc.

For now we should not add GrantedAuthorityDefaults to any of these classes either (instead just invoke the setter for default role prefix on each object).

Member

rwinch commented Aug 30, 2016

@eddumelendez Thanks for the fast response.

I mean unless a class is in the spring-security-config module this PR should not add implements ApplicationContextAware to it. Some examples of classes that should NOT implement ApplicationContextAware are: Jsr250MethodSecurityMetadataSource, SecurityExpressionRoot, DefaultMethodSecurityExpressionHandler, RunAsManagerImpl, RoleVoter, JdbcDaoImpl, etc.

For now we should not add GrantedAuthorityDefaults to any of these classes either (instead just invoke the setter for default role prefix on each object).

Make single definition of `defaultRolePrefix` and `rolePrefix`
Previous to this commit, role prefix had to be set in every class
causing repetition. Now, bean `GrantedAuthorityDefaults` can be used to
define the role prefix in a single point.

Fixes gh-3701
@eddumelendez

This comment has been minimized.

Show comment
Hide comment
@eddumelendez

eddumelendez Sep 19, 2016

Contributor

@rwinch PR has been updated

Contributor

eddumelendez commented Sep 19, 2016

@rwinch PR has been updated

@rwinch

This comment has been minimized.

Show comment
Hide comment
@rwinch

rwinch Sep 22, 2016

Member

Thanks for the PR! This is merged via eabeaf3 I applied some Polish in b443bae for your review. The main goal was to continue with the suggestion from #3956 (comment)

Member

rwinch commented Sep 22, 2016

Thanks for the PR! This is merged via eabeaf3 I applied some Polish in b443bae for your review. The main goal was to continue with the suggestion from #3956 (comment)

@rwinch rwinch closed this Sep 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment