Skip to content
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

Update "How to Authenticate Users with API Keys" #5252

Closed
mvar opened this issue May 13, 2015 · 3 comments
Closed

Update "How to Authenticate Users with API Keys" #5252

mvar opened this issue May 13, 2015 · 3 comments
Labels
hasPR A Pull Request has already been submitted for this issue. Security

Comments

@mvar
Copy link
Contributor

mvar commented May 13, 2015

While reading How to Authenticate Users with API Keys cookbook few ideas just came into my mind.

Is there any reason why this cookbook suggests to [manually] inject custom user provider (ApiKeyUserProvider) into authenticator service (ApiKeyAuthenticator)? Using this solution I see two minor issues that can be solved another way. First, we need to inject another service... Second, $userProvider argument of ApiKeyAuthenticator :: authenticateToken() method is left unused.

If I'm right, we can achieve the same goal by eliminating injection and additiong few lines to security.yml:

security:
    providers:
        # ...
        api_key_user_provider: # This on is already registered, but not used
            id: api_key_user_provider
    firewalls:
        secured_area:
            pattern: ^/admin
            stateless: false
            simple_preauth:
                authenticator: apikey_authenticator
            provider: api_key_user_provider # Shouldn't we set provider here instead of injecting it?

If we inject ApiKeyUserProvider manually, then there is no reason to register it as provider or even implement UserProviderInterface.

What do you think?


Also in few places ApiKeyAuthentication should be renamed to ApiKeyAuthenticator.

@weaverryan
Copy link
Member

@mvar Nice thinking!

I definitely agree with the ApiKeyAuthentication typo - let's fix that immediately!

And avoiding manual injection of the UserProvider also makes perfect sense to me - I'm guessing this was just an oversight. 👍 !

@mvar
Copy link
Contributor Author

mvar commented May 14, 2015

@weaverryan thanks for your feedback.

I've submitted a PR with discussed changes.

Actually I have one more idea. How bad it would be to suggest to treat API key as a username? I.e. to use loadUserByUsername() to load user by API key. Personally I wouldn't create getUsernameForApiKey() because it sounds like additional pointless [SQL] query for me.

@xabbuh xabbuh added hasPR A Pull Request has already been submitted for this issue. Security labels May 14, 2015
@xabbuh
Copy link
Member

xabbuh commented May 14, 2015

Actually I have one more idea. How bad it would be to suggest to treat API key as a username? I.e. to use loadUserByUsername() to load user by API key. Personally I wouldn't create getUsernameForApiKey() because it sounds like additional pointless [SQL] query for me.

This sounds too confusing for me. Often your users will have both a username and an API key (or even more than one API key, one for each authorised third-party application, for example), won't they?

weaverryan added a commit that referenced this issue May 23, 2015
…ction (mvar)

This PR was submitted for the 2.5 branch but it was merged into the 2.6 branch instead (closes #5255).

Discussion
----------

[Cookbook] Use configured user provider instead of injection

| Q             | A
| ------------- | ---
| Doc fix?      | yes
| New docs?     | no
| Applies to    | 2.5, 2.6
| Fixed tickets | #5252

Commits
-------

8556ae2 Improve invalid user provider exception message
77fdbbe Check user provider type
f7d7f81 Use configured user provider instead of injection
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hasPR A Pull Request has already been submitted for this issue. Security
Projects
None yet
Development

No branches or pull requests

3 participants