Skip to content

Fixed description of session storage of the ApiKeyAuthenticator #4076

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Fixed description of session storage of the ApiKeyAuthenticator #4076

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 46 additions & 1 deletion cookbook/security/api_key_authentication.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ you can use to create an error ``Response``.

class ApiKeyAuthenticator implements SimplePreAuthenticatorInterface, AuthenticationFailureHandlerInterface
{
//...
// ...

public function onAuthenticationFailure(Request $request, AuthenticationException $exception)
{
Expand Down Expand Up @@ -427,6 +427,51 @@ configuration or set it to ``false``:
),
));

Even though the token is being stored in the session, the credentials - in this
case the API key (i.e. ``$token->getCredentials()``) - are not stored in the session
for security reasons. To take advantage of the session, update ``ApiKeyAuthenticator``
to see if the stored token has a valid User object that can be used::

Copy link
Member

Choose a reason for hiding this comment

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

You have to start a code block:

.. code-block:: php

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I have added this to the other section of the document now as well.

// src/Acme/HelloBundle/Security/ApiKeyAuthenticator.php
// ...

class ApiKeyAuthenticator implements SimplePreAuthenticatorInterface
Copy link
Member

Choose a reason for hiding this comment

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

the namespace and use directives are missing

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 have left them away since I was referencing the above mentioned class. I dont think I need to duplicate all.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. That's a valid reason. But I would then place a placeholder (// ...) there (just in the line below the file name).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just updated.

{
// ...
public function authenticateToken(TokenInterface $token, UserProviderInterface $userProvider, $providerKey)
{
$apiKey = $token->getCredentials();
$username = $this->userProvider->getUsernameForApiKey($apiKey);

// User is the Entity which represents your user
$user = $token->getUser();
if ($user instanceof User) {
return new PreAuthenticatedToken(
$user,
$apiKey,
$providerKey,
$user->getRoles()
);
}

if (!$username) {
throw new AuthenticationException(
sprintf('API Key "%s" does not exist.', $apiKey)
);
}

$user = $this->userProvider->loadUserByUsername($username);

return new PreAuthenticatedToken(
$user,
$apiKey,
$providerKey,
$user->getRoles()
);
Copy link
Member

Choose a reason for hiding this comment

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

This is exactly the same return as above. Can't we do something like below?

$user = $token->getUser();

if (!$user instanceof User) {
    if (!$username) {
        throw new AuthenticationException(
            sprintf('API Key "%s" does not exists.', $apiKey)
        );
    }

    $user = $this->userProvider->loadUserByUsername($username);
}

return new PreAuthenticatedToken(
    $user,
    $apiKey,
    $providerKey,
    $user->getRoles()
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To pick up your previous argument, docs should be simple. I wanted to highlight the
small code addition without changing the entire structure. And I find it personally also
easier to read through code if I am able to avoid nested divs.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, this is a better experience for the readers, even if it's a bit of a worse experience for maintainers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I assume we take care about the readers first? Or should I change this?

Copy link
Member

Choose a reason for hiding this comment

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

Haha, yea, I care about the readers first :). Don't change it imo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All right :)

}
// ...
}

Storing authentication information in the session works like this:

#. At the end of each request, Symfony serializes the token object (returned
Expand Down