Skip to content

Conversation

@Spomky
Copy link
Contributor

@Spomky Spomky commented May 21, 2022

Q A
Branch? 6.2
Bug fix? yes
New feature? yes
Deprecations? no
Tickets Fix #45844
License MIT
Doc PR symfony/symfony-docs#16819

Hi,

This PR aims at fixing #45844.
It adds a new authenticator that is able to fetch a token in the request header and retrieve the associated user identifier.

The authenticator delegates the token loading to a handler. This handler could manage opaque tokens (random strings stored in a database) or self-contained tokens such as JWT, Paseto, SAML...

Firewall Configuration

This PR adds a new authenticator that covers the RFC6750: access_token.
Also, it adds the possibility to extract the token from anywhere in the request.

Basic Configuration

security:
    firewalls:
        main:
            pattern: ^/
            access_token:
                token_handler: access_token.access_token_handler

Complete Configuration

security:
    firewalls:
        main:
            pattern: ^/
            access_token:
                user_provider: 'dedicate_user_provider_for_this_firewall'
                success_handler: 'custom_success_handler'
                failure_handler: 'custom_failure_handler'
                token_handler: access_token.access_token_handler
                token_extractors:
                    - 'security.access_token_extractor.query_string'
                    - 'security.access_token_extractor.request_body'
                    - 'security.access_token_extractor.header'
                    - 'custom_access_token_extractor'

Token Handler

This authenticator relies on a Token Handler. Its responsability is to

  • load the token
  • check the token (revocation, expiration time, digital signature...)
  • return the user ID associated to it

Tokens could be of any kind: opaque strings or self-contained tokens such as JWT, Paseto, SAML2...

Example: from a repository

<?php

namespace App\Security;

use App\Repository\AccessTokenRepository;
use Symfony\Component\Security\Core\Exception\BadCredentialsException;
use Symfony\Component\Security\Http\Authenticator\AccessTokenHandler as AccessTokenHandlerAliasInterface;

class AccessTokenHandler implements AccessTokenHandlerAliasInterface
{
    public function __construct(private readonly AccessTokenRepository $repository)
    {
    }

    public function getUserIdentifierFrom(string $token): string
    {
        $accessToken = $this->repository->findOneByValue($token);
        if ($accessToken === null || !$accessToken->isValid()) {
            throw new BadCredentialsException('Invalid credentials.');
        }

        return $accessToken->getUserId();
    }
}

Example: from a JWT

<?php

namespace App\Security;

use App\Security\JWTLoader;
use App\Security\JWTValidator;
use Symfony\Component\Security\Core\Exception\BadCredentialsException;
use Symfony\Component\Security\Http\Authenticator\AccessTokenHandler as AccessTokenHandlerAliasInterface;

class AccessTokenHandler implements AccessTokenHandlerAliasInterface
{
    public function __construct(
        private readonly JWTLoader $loader,
        private readonly JWTValidator $validator
    )
    {
    }

    public function getUserIdentifierFrom(string $token): string
    {
        try {
            $token = $this->loader->loadJWT($token);
            $this->validator->validate($token);

            return $token->getClaim('sub');
        } catch (\Throwable $e) {
            throw new BadCredentialsException('Invalid credentials.', $e->getCode, $e);
        }
    }
}

@Spomky Spomky requested review from chalasr and wouterj as code owners May 21, 2022 16:09
@carsonbot carsonbot added this to the 6.2 milestone May 21, 2022
@Spomky Spomky marked this pull request as draft May 21, 2022 16:09
@chalasr
Copy link
Member

chalasr commented May 21, 2022

I like this approach.

@Spomky Spomky marked this pull request as ready for review May 21, 2022 19:12
@Spomky
Copy link
Contributor Author

Spomky commented May 21, 2022

@Spomky Spomky marked this pull request as draft May 22, 2022 07:40
@wouterj
Copy link
Member

wouterj commented May 22, 2022

Thanks for checking all tests! (btw, I'll look at the PR somewhere after 6.1 is released)

PHPUnit / Tests (8.2) (pull_request): the error reported here: https://github.com/symfony/symfony/runs/6538975401?check_suite_focus=true#step:8:2110 is related to another (unchanged) component. Should I fix it?

We'll ignore it for this PR (but if you want, a separate PR with a fix is always welcome 😉 )

PHPUnit / Tests (8.1, low-deps) (pull_request): Error: Class "Symfony\Component\Security\Http\Authenticator\AccessTokenAuthenticator" not found I don't understand this issue. The class is present in the PR

See https://symfony.com/doc/current/contributing/code/pull_requests.html#automated-tests You must update the minimum versions in e.g. the SecurityBundle.

@Spomky Spomky marked this pull request as ready for review May 22, 2022 16:19
@Spomky
Copy link
Contributor Author

Spomky commented May 23, 2022

Hi,

Many thanks for your comments.
I updated the authenticator that is now based on token "extractors".

security:
    firewalls:
        main:
            pattern: ^/
            access_token:
                token_handler: access_token.access_token_handler
#                token_extractors:
#                    header: 'security.access_token_extractor.header' # This extrator is set by default

You can add any extractor you want. Three already exist:

  • security.access_token_extractor.header
  • security.access_token_extractor.query_string
  • security.access_token_extractor.request_body

You can create as many extractors you need. They shall implement Symfony\Component\Security\Http\Authenticator\AccessToken\AccessTokenExtractorInterface and the method extractToken.

Other parameters are also present:

security:
    firewalls:
        main:
            pattern: ^/
            access_token:
                token_handler: access_token.access_token_handler
                success_handler: access_token.success_handler
                failure_handler: access_token.failure_handler
                user_provider: 'another.user_provider'
                token_extractors:
                    - 'security.access_token_extractor.header'
                    - 'security.access_token_extractor.query_string'
                    - 'security.access_token_extractor.request_body'
                    - 'App\Security\CustomExtractor'

I have not created Exceptions. I revised my mind as the existing ones seem sufficient.
WDYT?

Best regards.

Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

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

Thank you for your PR. In this review, I focussed mainly on code style and best practises. I'll do a more in-depth review later.

Copy link
Member

@wouterj wouterj left a comment

Choose a reason for hiding this comment

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

Very cool. Thanks for taking on this adventure @Spomky! (and wow, never knew this would be your first code contribution to Symfony core)

I've reviewed the component part for now. I really like how this PR is strict enough to completely follow the RFC, but yet keep enough open to be useful for many custom-build API authenticators.

@Spomky
Copy link
Contributor Author

Spomky commented Jul 16, 2022

Hi all,

Would you mind to review the changes I made and if the comments you raised have correctly been addressed?
Let me know if you see other modifications I should consider.

Many thanks.
Regards.

@wouterj
Copy link
Member

wouterj commented Jul 16, 2022

@Spomky if you click on the "load more ...", you'll see a review from me. I guess GitHub immediately hid it, so it got lost 🙂

@lyrixx
Copy link
Member

lyrixx commented Aug 1, 2022

Super feature 👍🏼 Thanks for the work 👏🏼 . I let some comments

@Spomky
Copy link
Contributor Author

Spomky commented Aug 1, 2022

Many thanks for the review and comments.
I think I addressed all of them (including those hidden by github).
Let me know if you have any others.

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.

🤩

Copy link
Member

@lyrixx lyrixx left a comment

Choose a reason for hiding this comment

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

Some comments about the CS, but I'm OK with the global design 👍🏼

Comment on lines 34 to 33
public function __construct(private readonly string $parameter = 'access_token')
{
}
Copy link
Member

Choose a reason for hiding this comment

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

You don't always use the same CS. This one ⬆️ VS

    public function __construct(
        private readonly iterable $accessTokenExtractors,
    ) {
    }

In my applications (at work), I use the latest: more scalable, and reduce the diff when adding a new param.

However, I don't know what's the standard in Symfony (cc @nicolas-grekas @fabpot)

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 ran php-cs-fixer but many files got updated. I just focussed on the one from this PR but the gaps you spotted were not found by the tool.
Anyway, I updated them to be consistent.

Copy link
Member

@wouterj wouterj left a comment

Choose a reason for hiding this comment

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

Hi there! I finally got the chance to review this a bit more in-depth and I'm really liking the feature :)

I've pushed 2 commits to your branch that fix the last 2 big remaining comments. It would be nice if you could squash all your commits into one and after that, it's ready to be merged in my opinion.

Thanks a lot for all the work!

@Spomky
Copy link
Contributor Author

Spomky commented Aug 9, 2022

Hi there! I finally got the chance to review this a bit more in-depth and I'm really liking the feature :)

I've pushed 2 commits to your branch that fix the last 2 big remaining comments. It would be nice if you could squash all your commits into one and after that, it's ready to be merged in my opinion.

Thanks a lot for all the work!

Excellent! Many thanks to you and all the other reviewers.
I edited the few things you spotted and squashed the PR.
This is a big step for me and I really hope it will help many developers to integrate token-based authentication.

@wouterj wouterj added the Ready label Aug 9, 2022
@chalasr
Copy link
Member

chalasr commented Aug 10, 2022

Thank you @Spomky.

@chalasr chalasr merged commit dfcf900 into symfony:6.2 Aug 10, 2022
@Spomky
Copy link
Contributor Author

Spomky commented Aug 11, 2022

So cool! Many thanks for merging this PR.

@Spomky Spomky deleted the features/access-token-authenticator branch August 11, 2022 11:10
@derrabus
Copy link
Member

Awesome! 🚀

@vincentchalamon
Copy link
Contributor

@Spomky Question regarding the configuration of this feature: why adding user_provider option, and not rely on the firewall user provider?

@Spomky
Copy link
Contributor Author

Spomky commented Nov 29, 2022

@vincentchalamon this allows setting a specific user provider for this firewall. If not set, the default user provider is used.

@chalasr
Copy link
Member

chalasr commented Nov 29, 2022

See #48385

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.

[Security] Add Bearer Authenticator

10 participants