Skip to content

Make single definition of defaultRolePrefix and rolePrefix#3956

Closed
eddumelendez wants to merge 1 commit intospring-projects:masterfrom
eddumelendez:gh-3701
Closed

Make single definition of defaultRolePrefix and rolePrefix#3956
eddumelendez wants to merge 1 commit intospring-projects:masterfrom
eddumelendez:gh-3701

Conversation

@eddumelendez
Copy link
Copy Markdown
Contributor

@eddumelendez eddumelendez commented Jul 2, 2016 •
edited
Loading

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
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
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 added the status: waiting-for-feedback We need additional information before we can continue label Jul 7, 2016
@rwinch rwinch self-assigned this Jul 7, 2016
@eddumelendez
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Member

rwinch commented Aug 30, 2016 •
edited
Loading

@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).

@eddumelendez
Make single definition of defaultRolePrefix and rolePrefix
5ae8e1d 
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 spring-projectsgh-3701
@eddumelendez
Copy link
Copy Markdown
Contributor Author

eddumelendez commented Sep 19, 2016

@rwinch PR has been updated

@rwinch rwinch added in: config An issue in spring-security-config type: enhancement A general enhancement status: duplicate A duplicate of another issue and removed status: waiting-for-feedback We need additional information before we can continue labels Sep 19, 2016
@rwinch
Copy link
Copy Markdown
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

Reviewers

No reviews

Assignees

@rwinch rwinch

Labels

in: config An issue in spring-security-config status: duplicate A duplicate of another issue type: enhancement A general enhancement

Projects

None yet

Milestone

4.2.0 M1

Development

Successfully merging this pull request may close these issues.

Create single point of defining the default role prefix

2 participants

@eddumelendez @rwinch