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.

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

Add binder for annotated custom Principal #1522

Closed
wants to merge 6 commits into from

Conversation

jeremyg484
Copy link
Contributor

@jeremyg484 jeremyg484 commented Dec 5, 2023

A new @User annotation is added that can be used to bind a custom Principal object to a method parameter for the
currently active login.

This allows for users to provide their own custom authentication object and still be able to bind it to @Controller
method parameters as long as the parameter is annotated with @User and matches the type of the authentication
stored in the request.

For example:

@Post("/{var1}/method")
fun method(
        @PathVariable var1: String,
        @Body request: SomeOurRequest,
        @User userToken: our.package.TokenAuthentication,
    ): HttpResponse<Unit>

would work without requiring the user to supply their own RequestArgumentBinder

This resolves #1430

A new `@User` annotation is added that can be used to bind a custom
`Principal` object to a method argument for the currently active login.
@jeremyg484 jeremyg484 changed the title Add binder for annotated custom Authentication Add binder for annotated custom Principal Dec 5, 2023
@jeremyg484 jeremyg484 requested a review from sdelamo December 5, 2023 18:49
@jeremyg484 jeremyg484 self-assigned this Dec 5, 2023
@jeremyg484 jeremyg484 added the type: enhancement New feature or request label Dec 5, 2023
@jeremyg484
Copy link
Contributor Author

@sdelamo Note that I have not added documentation for this enhancement yet - wanted to see what you think of the idea first.

@sdelamo sdelamo marked this pull request as draft December 6, 2023 06:07
Copy link
Contributor

@sdelamo sdelamo left a comment

Choose a reason for hiding this comment

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

I am not sure about this. it will require to create a custom authentication response.

@@ -95,7 +103,7 @@ class AuthorizationSpec extends EmbeddedServerSpecification {

void "Authentication Argument Binders binds Authentication if return type is Single"() {
expect:
embeddedServer.applicationContext.getBean(PrincipalArgumentBinder.class)
embeddedServer.applicationContext.getBean(AuthenticationArgumentBinder.class)
Copy link
Contributor

Choose a reason for hiding this comment

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

why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seemed more appropriate since that is the binder that gets used when the method argument is Authentication.


@Get("/single-server-authentication")
@SingleResult
Publisher<String> singleServerAuthentication(@User ServerAuthentication authentication) {
Copy link
Contributor

Choose a reason for hiding this comment

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

all of these methods return Publisher. we could just return String. Returning Single is unrelated with what is under test.

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 agree. I wasn't sure if the original spec author was explicitly trying to make sure it worked with Publisher for some non-obvious reason since it was explicitly being called out in the spec method descriptions.

@sdelamo
Copy link
Contributor

sdelamo commented Dec 6, 2023

I have created a PR with docs #1524 for the current behaviour.

jeremyg484 and others added 4 commits December 6, 2023 09:53
Copy link

sonarqubecloud bot commented Dec 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

80.0% 80.0% Coverage
0.0% 0.0% Duplication

…uthorizationSpec.groovy

Co-authored-by: Sergio del Amo <[email protected]>
@jeremyg484
Copy link
Contributor Author

I am not sure about this. it will require to create a custom authentication response.

@sdelamo I wouldn't say it requires it so much as it supports it. I assume that must be what the user in #1430 is already doing in order to generate their custom Authentication subtype, right?

@sdelamo
Copy link
Contributor

sdelamo commented Feb 5, 2024

Closing this for now. I am not sure about this feature.

@sdelamo sdelamo closed this Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Subtypes of Authentication are not injected as expected
2 participants