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.

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

Improve @AuthenticationPrincipal meta-annotations #15344

Merged
merged 6 commits into from
Aug 10, 2024
Merged

Improve @AuthenticationPrincipal meta-annotations #15344

merged 6 commits into from
Aug 10, 2024

Conversation

kse-music
Copy link
Contributor

Closes gh-15286

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 2, 2024
jzheaux
jzheaux requested changes Jul 3, 2024
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 for contributing this, @kse-music! I've left some initial feedback inline.

.../org/springframework/security/messaging/context/AuthenticationPrincipalArgumentResolver.java Outdated Show resolved Hide resolved
.../org/springframework/security/messaging/context/AuthenticationPrincipalArgumentResolver.java Outdated Show resolved Hide resolved
.../org/springframework/security/messaging/context/AuthenticationPrincipalArgumentResolver.java Outdated Show resolved Hide resolved
@kse-music kse-music force-pushed the gh-15286 branch 4 times, most recently from 0d0f8ec to 7bb8a85 Compare July 3, 2024 09:48
@jzheaux jzheaux self-assigned this Jul 9, 2024
@jzheaux jzheaux added in: core An issue in spring-security-core type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 9, 2024
@kse-music
Copy link
Contributor Author

Thanks,@jzheaux ,all feedback is done

@jzheaux jzheaux force-pushed the gh-15286 branch 2 times, most recently from 7def1ce to 6f8cf72 Compare July 19, 2024 00:19
@jzheaux
Copy link
Contributor

jzheaux commented Jul 19, 2024

Thanks, @kse-music! Given that this needs to be placed in the other AuthenticationPrincipalArgumentResolvers, I extracted the logic in AuthorizationAnnotationUtils into an API. Please see if it works for you. If so, please also update this PR to change the other non-deprecated instances of AuthenticationPrincipalArgumentResolver in the project.

@kse-music kse-music force-pushed the gh-15286 branch 3 times, most recently from 747299c to e5f2230 Compare July 23, 2024 09:46
@kse-music
Copy link
Contributor Author

Thanks, @kse-music! Given that this needs to be placed in the other AuthenticationPrincipalArgumentResolvers, I extracted the logic in AuthorizationAnnotationUtils into an API. Please see if it works for you. If so, please also update this PR to change the other non-deprecated instances of AuthenticationPrincipalArgumentResolver in the project.

All non-deprecated instances of AuthenticationPrincipalArgumentResolver in the project has done.

@kse-music kse-music force-pushed the gh-15286 branch 2 times, most recently from c627031 to 25edd68 Compare July 23, 2024 11:59
@jzheaux
Copy link
Contributor

jzheaux commented Jul 29, 2024

Thanks, @kse-music, this is taking shape very nicely.

I believe our last step is to make this simple to configure by picking up the AnnotationTemplateExpressionDefaults bean in configuration.

I've added documentation so you can see the expectation that someone should be able to do:

@Bean
public AnnotationTemplateExpressionDefaults templateExpressionDefaults() {
    return new AnnotationTemplateExpressionDefaults();
}

And now they will be able to use placeholders in any @AuthenticationPrincipal meta-annotation definition.

Are you able to add this to the following spring-security-config classes: WebMvcSecurityConfiguration, ServerHttpSecurityConfiguration, and WebSocketMessageBrokerSecurityConfiguration? It's a little harder, however, if you are able, it would be great to add to WebSocketMessageBrokerSecurityBeanDefinitionParser, too.

@kse-music
Copy link
Contributor Author

@jzheaux ,I tried to add it

@kse-music kse-music force-pushed the gh-15286 branch 2 times, most recently from 69aced4 to 7cf93db Compare July 30, 2024 04:24
@kse-music
Copy link
Contributor Author

@jzheaux I have added the code, please help to review it.Thank you

jzheaux added 2 commits August 10, 2024 11:10
@jzheaux
Deprecate PrePostTemplateDefaults
041d8f6 
Since there is nothing specific to configuring pre/post
annotations, there is no need for the extra class.

If a need like this does arise in the future,
either AnnotationTemplateExpressionDefaults can be sub-
classed, or it can have introduced a Map field holding
custom properties.

Issue spring-projectsgh-15286
@jzheaux
Add Config Tests for AuthenticationPrincipal Templates
e13471f 
Issue spring-projectsgh-15286
@jzheaux jzheaux merged commit 2b33f6f into spring-projects:main Aug 10, 2024
4 checks passed
@jzheaux
Copy link
Contributor

jzheaux commented Aug 10, 2024

Thanks for all your help with this PR, @kse-music! I added a couple of commits to add more testing and also to deprecate PrePostTemplateDefaults. It's now merged into main.

@jzheaux jzheaux added this to the 6.4.0-M2 milestone Aug 10, 2024
@jzheaux
Copy link
Contributor

jzheaux commented Aug 10, 2024

If you are interested, it would be nice to do the same with @CurrentSecurityContext. I've added #15551 to address that. That PR should go a lot more quickly.

@kse-music
Copy link
Contributor Author

If you are interested, it would be nice to do the same with @CurrentSecurityContext. I've added #15551 to address that. That PR should go a lot more quickly.

Yes,I have added PR #15553

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: core An issue in spring-security-core type: enhancement A general enhancement
Projects
Spring Security Team
Archived in project
Milestone
6.4.0-M2
Development

Successfully merging this pull request may close these issues.

Improve @AuthenticationPrincipal meta-annotations
3 participants
@kse-music @jzheaux @spring-projects-issues