Skip to content

doc: Custom binding of authenticated user#1524

Merged
sdelamo merged 1 commit intomasterfrom
custom-authentication-binding
Dec 6, 2023
Merged

doc: Custom binding of authenticated user#1524
sdelamo merged 1 commit intomasterfrom
custom-authentication-binding

Conversation

@sdelamo
Copy link
Contributor

@sdelamo sdelamo commented Dec 6, 2023

Close: #1430

@sonarqubecloud
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

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Contributor

@jeremyg484 jeremyg484 left a comment

Choose a reason for hiding this comment

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

While I think showing this particular example is useful in its own right, I don't really think it addresses the specific use case from #1430 wherein they are fully replacing the default Authentication object such that a call to request.getUserPrincipal(AuthenticationWithEmail.class) would be expected to work.

To address that case, I think we'd want to also show that it's possible to just extend AbstractPrincipalArgumentBinder as they did, but explain how that approach only works with something like a custom AuthenticationProvider.

Also as an aside that should probably be addressed separately, I notice that providing a custom implementation of TokenValidator as you have in the tests here seems like a nice simple way to enhance the Authentication attributes, but I don't see it mentioned anywhere in the documentation.

@sdelamo
Copy link
Contributor Author

sdelamo commented Dec 6, 2023

While I think showing this particular example is useful in its own right,

While I think showing this particular example is useful in its own right, I don't really think it addresses the specific use case from #1430 wherein they are fully replacing the default Authentication object such that a call to request.getPrincipal(AuthenticationWithEmail.class) would be expected to work.

They are not fully replacing authentication. They are creating:

class TokenAuthentication implements Authentication {
String token
}

I think this PR adds the docs they would need for they usecase (bind such a class in a controller method parameter).

Happy to do further improvements but I think we should get this merged in.

@sdelamo sdelamo requested a review from jeremyg484 December 6, 2023 16:20
@jeremyg484
Copy link
Contributor

They are not fully replacing authentication. They are creating:

class TokenAuthentication implements Authentication {
String token
}

I think this PR adds the docs they would need for they usecase (bind such a class in a controller method parameter).

In #1430 (comment) they said that simply extending AbstractPrincipalArgumentBinder solved their problem.

i.e., in Java their example would be:

@Singleton
public class TokenAuthenticationArgumentBinder extends AbstractPrincipalArgumentBinder<TokenAuthentication> {

    public TokenAuthenticationArgumentBinder() {
        super(TokenAuthentication.class);
    }
}

The bind method in their implementation is thus going to call httpRequest.getUserPrincipal(TokenAuthentication.class). For that to be working successfully, they have to have somehow replaced the Authentication that is stored in the request, whether with a custom AuthenticationProvider, a custom OpenIdAuthenticationMapper, etc. Thus the example binder shown in this PR is more than what they need for that specific case.

Copy link
Contributor

@jeremyg484 jeremyg484 left a comment

Choose a reason for hiding this comment

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

See my prior comment above. I think this is good in its own right to be merged, but I don't think it addresses the specific case in #1430, so I don't think it should close that issue.

We should address that separately, either with my suggested enhancement in #1522, or by providing a separate documentation example of extending AbstractPrincipalArgumentBinder, or both.

@sdelamo sdelamo merged commit 22dbbc0 into master Dec 6, 2023
@sdelamo sdelamo deleted the custom-authentication-binding branch December 6, 2023 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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