Skip to content

Allow to set default securityContextRepository for each authenticatio… #7275

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 3, 2019
Merged

Allow to set default securityContextRepository for each authenticatio… #7275

merged 1 commit into from
Sep 3, 2019

Conversation

eddumelendez
Copy link
Contributor

…n mechanisms

Fixes gh-7249

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 18, 2019
@eddumelendez
Copy link
Contributor Author

@jzheaux how should securityContextRepositoryWebFilter() resolve the securityContextRepository? Since the new approach is:

  1. Use securityContextRepository in the authentication mechanism
  2. Use securityContextRepository in the global value
  3. Use the default one

My current PR is setting the global securityContextRepository in the build() method by each authentication mechanism which I assume is wrong but didn't notice until running integration tests with configs with multiple authentication mechanism. Should the global value remain with an instance of WebSessionServerSecurityContextRepository?

Note: there have been some changes in other tests since http basic and auth2 login never read the securityContextRepository and there were some of them changing that value but it was ignored.

Thanks in advance

@eddumelendez eddumelendez marked this pull request as ready for review August 18, 2019 09:40
@jzheaux jzheaux added in: web An issue in web modules (web, webmvc) status: duplicate A duplicate of another issue type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Aug 22, 2019
@jzheaux jzheaux self-assigned this Aug 22, 2019
@jzheaux jzheaux added this to the 5.2.0.RC1 milestone Aug 22, 2019
@jzheaux
Copy link
Contributor

jzheaux commented Aug 22, 2019

@eddumelendez Good observations.

Should the global value remain with an instance of WebSessionServerSecurityContextRepository?

No, good catch, I think that this should be set to null, and then also change securityContextRepositoryWebFilter() to use WebSessionServerSecurityContextRepository by default instead of returning null.

jzheaux
jzheaux requested changes Aug 27, 2019
View reviewed changes
Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @eddumelendez! I've left some feedback inline.

config/src/test/java/org/springframework/security/config/web/server/FormLoginTests.java Outdated

homePage.assertAt();

verify(defaultSecContextRepository, times(3)).load(any());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the times(3) criteria will make this brittle. For this test, I think we should simply confirm that the application used the configured repository.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am change it for atLeastOnce(), is it ok?

@eddumelendez
Copy link
Contributor Author

@jzheaux I have added a new commit with the changes. Thanks again for the review. Once get the approval I will rebase it and squash the commits

jzheaux
jzheaux requested changes Aug 30, 2019
View reviewed changes
Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eddumelendez Thanks for the updates. I have just one more question inline.

@eddumelendez
Copy link
Contributor Author

@jzheaux thanks again for all your feedback. I have submitted a new commit

@jzheaux
Copy link
Contributor

jzheaux commented Sep 2, 2019

Great, @eddumelendez, go ahead and squash your commits in preparation for merging.

@eddumelendez
Copy link
Contributor Author

@jzheaux done :)

@jzheaux jzheaux merged commit 8773c79 into spring-projects:master Sep 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jzheaux jzheaux jzheaux approved these changes

Assignees

@jzheaux jzheaux

Labels
in: web An issue in web modules (web, webmvc) status: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
None yet
Milestone
5.2.0.RC1
Development

Successfully merging this pull request may close these issues.

Authentication Mechanisms Should Default their ServerSecurityContextRepository
3 participants
@eddumelendez @jzheaux @spring-projects-issues