-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Security] Support loading UserBadge directly from accessToken #48285
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
Conversation
Hey! Thanks for your PR. You are targeting branch "6.2" but it seems your PR description refers to branch "6.3". Cheers! Carsonbot |
The proposal here makes the UserProvider responsible for implementing all the token validation. To me, this risks encouraging bad implementations because the UserProvider is currently responsible for none of the validation. I'd rather have a specific interface to be implemented by the AccessTokenHandler to return a UserBadge directly instead of only the identifier (or something like that). |
Looks sensible to me. |
I've changed it to use the |
Widening the return type of the interface will be a BC break for consumers of the interface though. So if we delay that to 6.3, we'll have to use a separate method |
@stof I thought a |
@Jeroeny yes, it's OK for implementations, but consumers have to take care of another type when calling the method - which would be a BC break. |
@Jeroeny the BC break concerns consumers, not implementors. For implementors, the covariance rule indeed makes it BC. Btw, that's the reason why a child class is not allowed to widen a return type as it would break consumers. And also the main consumer is in the core and will be updated, any decorator class is both an implementation and a consumer and will be impacted by the widened type. |
Since some on-going implementations already need this, I'd be fine merging this on 6.2 as a feature-freeze adjustment. |
Ah or course, for consumers. Allowing it in 6.2 would be cool. |
src/Symfony/Component/Security/Http/Authenticator/AccessTokenAuthenticator.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For 6.2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for 6.2 too.
src/Symfony/Component/Security/Http/AccessToken/AccessTokenHandlerInterface.php
Outdated
Show resolved
Hide resolved
Should |
None indeed, can you make the change in this PR? |
src/Symfony/Bundle/SecurityBundle/Resources/config/security_authenticator_access_token.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Http/Authenticator/AccessTokenAuthenticator.php
Outdated
Show resolved
Hide resolved
…at don't need a user provider (wouterj) This PR was merged into the 6.3 branch. Discussion ---------- [SecurityBundle] Improve support for authenticators that don't need a user provider | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | Ref #48285, #48272 | License | MIT | Doc PR | - This builds on top of the self-contained token feature added in 6.2 (#48285). While that PR allows access token handlers to load the user from the access token without user provider, it was still required to configure a user provider in the code. With this PR, the bundle allows a user provider to not be configured when: 1. The firewall is `stateless`, otherwise we still need the user provider to refresh the user 2. The authenticator factory implements `StatelessAuthenticatorFactoryInterface` (i.e. declares compatibility with no user provider) This can help with simplifying the code in #48272 (comment) , as we no longer have to define a special user badge and provider. cc `@Jeroeny` Commits ------- 5464c57 [SecurityBundle] Improve support for authenticators that don't need a user provider
…vincentchalamon) This PR was squashed before being merged into the 6.3 branch. Discussion ---------- [Security] Add OidcUserInfoTokenHandler and OidcUser | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | N/A | License | MIT | Doc PR | symfony/symfony-docs#17463 Hi, This PR aims to complete [the previous one](#46428) from `@Spomky` with an AccessTokenHandler ready-to-use with an OIDC server (Keycloak, Auth0). ## TODO - [x] Rebase from 6.3 - [x] Rebase from #48285 - [x] Rebase from #48594 - [x] Write doc (symfony/symfony-docs#17463) - [x] Add TokenHandlerFactory - [x] Add ServiceTokenHandlerFactory for BC layer - [x] Add OidcUserInfoTokenHandlerFactory - [x] Add OidcTokenHandlerFactory (using web-token/jwt-*) - [x] Implement OidcUser to keep user claims from OIDC server - [x] Update doc PR about claims usage in a custom UserProvider - [x] ~Update doc PR about OidcUserProvider usage~ (abandonned) ## Usage ```yaml # usage with a custom client security: firewalls: main: pattern: ^/ access_token: token_handler: oidc_user_info: client: oidc.client ``` ```yaml # usage with generic HttpClient security: firewalls: main: pattern: ^/ access_token: token_handler: oidc_user_info: claim: email client: base_uri: https://www.example.com/realms/demo/protocol/openid-connect/userinfo ``` ```yaml # usage with token decode (no call to OIDC server) security: firewalls: main: pattern: ^/ access_token: token_handler: oidc: signature: # Algorithm used to sign the JWS algorithm: 'HS256' # A JSON-encoded JWK key: '{"kty":"...","k":"..."}' ``` ```php # usage with a custom UserProvider class CustomUserProvider implements UserProviderInterface { public function loadUserByIdentifier(string $identifier, array $claims = []): UserInterface { // do some magic } } ``` Commits ------- 99a35f0 [Security] Add OidcUserInfoTokenHandler and OidcUser
I attended @welcoMattic 's nice talk at SymfonyCon Paris last week, after which we and @wouterj discussed the way tokens need to be parsed to obtain a
userIdentifier
. With JWT tokens, there is usually no need to make a call to a database to instantiate a User object. Because the token is signed, we can guarantee the user details inside it are valid. See example code: https://github.com/welcoMattic/painless-authentication-with-access-token/blob/main/slides/slides.md#62-jwthandlerIn Symfony's authentication system, retrieving the
userIdentifier
and theUser
is a two-step process. However with both theuserIdentifier
and theUser
object coming from the token, we either have to parse the token twice or store it in memory.I think this can be improved by allowing a single step to go from $token (string) -> parsed token (implementation specific) -> User object.
The suggested change in this PR is probably not yet the way to go, but I'd like to hear from the Security contributers if they have any ideas.