Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[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

Merged
merged 1 commit into from
Nov 24, 2022

Conversation

Jeroeny
Copy link
Contributor

@Jeroeny Jeroeny commented Nov 22, 2022

Q A
Branch? 6.2
Bug fix? no
New feature? yes
Deprecations? no
License MIT
Doc PR -

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-jwthandler

In Symfony's authentication system, retrieving the userIdentifier and the User is a two-step process. However with both the userIdentifier and the User 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.

@carsonbot
Copy link

Hey!

Thanks for your PR. You are targeting branch "6.2" but it seems your PR description refers to branch "6.3".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

@stof
Copy link
Member

stof commented Nov 22, 2022

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).
Note that it might not even be a special interface. We could have getUserIdentifierFrom(string $accessToken): string|UserBadge allowing to return a UserBadge directly for the advanced case.

@chalasr
Copy link
Member

chalasr commented Nov 22, 2022

Note that it might not even be a special interface. We could have getUserIdentifierFrom(string $accessToken): string|UserBadge allowing to return a UserBadge directly for the advanced case.

Looks sensible to me.

@Jeroeny
Copy link
Contributor Author

Jeroeny commented Nov 22, 2022

I've changed it to use the string|UserBadge return types instead.

@stof
Copy link
Member

stof commented Nov 22, 2022

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

@Jeroeny
Copy link
Contributor Author

Jeroeny commented Nov 22, 2022

@stof I thought a string returntype in implementations of the interface would be covariant with string|UserBadge?

@wouterj
Copy link
Member

wouterj commented Nov 22, 2022

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

@stof
Copy link
Member

stof commented Nov 22, 2022

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

@chalasr
Copy link
Member

chalasr commented Nov 22, 2022

Since some on-going implementations already need this, I'd be fine merging this on 6.2 as a feature-freeze adjustment.

@Jeroeny
Copy link
Contributor Author

Jeroeny commented Nov 22, 2022

Ah or course, for consumers. Allowing it in 6.2 would be cool.

Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

For 6.2

Copy link
Member

@dunglas dunglas left a 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.

@Jeroeny
Copy link
Contributor Author

Jeroeny commented Nov 23, 2022

Should private readonly UserProviderInterface $userProvider of AccessTokenAuthenticator become nullable? What UserProviderInterface would you inject when you are already retrieving the UserBadge from the AccessTokenHandlerInterface ?

@chalasr
Copy link
Member

chalasr commented Nov 23, 2022

None indeed, can you make the change in this PR?

vincentchalamon added a commit to vincentchalamon/symfony that referenced this pull request Nov 25, 2022
@fabpot fabpot mentioned this pull request Nov 25, 2022
vincentchalamon added a commit to vincentchalamon/symfony that referenced this pull request Nov 25, 2022
vincentchalamon added a commit to vincentchalamon/symfony that referenced this pull request Nov 26, 2022
vincentchalamon added a commit to vincentchalamon/symfony that referenced this pull request Dec 1, 2022
vincentchalamon added a commit to vincentchalamon/symfony that referenced this pull request Dec 8, 2022
wouterj added a commit to wouterj/symfony that referenced this pull request Dec 10, 2022
vincentchalamon added a commit to vincentchalamon/symfony that referenced this pull request Dec 12, 2022
vincentchalamon added a commit to vincentchalamon/symfony that referenced this pull request Dec 13, 2022
chalasr pushed a commit to wouterj/symfony that referenced this pull request Dec 18, 2022
chalasr pushed a commit to wouterj/symfony that referenced this pull request Dec 18, 2022
chalasr added a commit that referenced this pull request Dec 18, 2022
…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
symfony-splitter pushed a commit to symfony/security-bundle that referenced this pull request Dec 18, 2022
vincentchalamon added a commit to vincentchalamon/symfony that referenced this pull request Dec 19, 2022
vincentchalamon added a commit to vincentchalamon/symfony that referenced this pull request Dec 27, 2022
vincentchalamon added a commit to vincentchalamon/symfony that referenced this pull request Jan 4, 2023
vincentchalamon added a commit to vincentchalamon/symfony that referenced this pull request Jan 5, 2023
vincentchalamon added a commit to vincentchalamon/symfony that referenced this pull request Jan 9, 2023
bitgandtter pushed a commit to timgchile/symfony that referenced this pull request Jan 9, 2023
vincentchalamon added a commit to vincentchalamon/symfony that referenced this pull request Jan 16, 2023
vincentchalamon added a commit to vincentchalamon/symfony that referenced this pull request Jan 21, 2023
vincentchalamon added a commit to vincentchalamon/symfony that referenced this pull request Feb 8, 2023
vincentchalamon added a commit to vincentchalamon/symfony that referenced this pull request Feb 13, 2023
vincentchalamon added a commit to vincentchalamon/symfony that referenced this pull request Feb 28, 2023
vincentchalamon added a commit to vincentchalamon/symfony that referenced this pull request Mar 21, 2023
vincentchalamon added a commit to vincentchalamon/symfony that referenced this pull request Mar 27, 2023
vincentchalamon added a commit to vincentchalamon/symfony that referenced this pull request Apr 11, 2023
fabpot added a commit that referenced this pull request Apr 14, 2023
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.