NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Security] Add a JSON authentication listener#18952

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

dunglas wants to merge4 commits intosymfony:masterfromdunglas:json_authentication_listener

Closed

[Security] Add a JSON authentication listener#18952

dunglas wants to merge4 commits intosymfony:masterfromdunglas:json_authentication_listener

Conversation

Copy link

Member

dunglas commentedJun 2, 2016•
edited
Loading

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	n/a
License	MIT
Doc PR	symfony/symfony-docs#7081

Add a new authentication listener allowing to login by sending a JSON document like:

{"_username": "dunglas", "_password": "foo"}.

It is similar to the traditional form login (but take a JSON document as entry) and is convenient for APIs, especially used in combination with JWT.

Seeapi-platform/core#563 andlexik/LexikJWTAuthenticationBundle#123 (comment) for previous discussions.

Add functional tests in security bundle

dunglas added Security SecurityBundle labels

Jun 2, 2016

carsonbot added Status: Needs Review Feature BC Break Deprecation labels

Jun 2, 2016

dunglas removed BC Break Deprecation labels

Jun 2, 2016

This was referencedJun 2, 2016

Allow credentials to be sent as JSONlexik/LexikJWTAuthenticationBundle#123

Closed

LexikJWTAuthentication bridgeapi-platform/core#563

Closed

Copy link

Member

chalasr commentedJun 2, 2016

👍 A great help for JSON APis!

Copy link

Member

GromNaN commentedJun 2, 2016•
edited
Loading

Unless you have to pass the JWT and the login/password in the same request, the standard way for password authentication is HTTP Basic. This is supported out of the box in Symfony and easy to do withjQuery.ajax.
Then a simple controller that return a new JWT can be created as an entry point.

Copy link

Member

chalasr commentedJun 2, 2016•
edited
Loading

@GromNaN I agree about that HTTP Basic and form-data stay the standard way for getting a JWT (& simpler for most cases).
But that's not an absolute rule and, depending on the client's platform/language/framework or simply out of preference, JSON can be the way to go.
Also I think that this could be helpful for a large number of use cases therefore good to have in Symfony

Copy link

Contributor

teohhanhui commentedJun 3, 2016

Does this makehttps://github.com/gfreeau/GfreeauGetJWTBundle obsolete?

Copy link

MemberAuthor

dunglas commentedJun 3, 2016•
edited
Loading

@teohhanhui AFAIK, GfreeauGetJWTBundle is for x-www-formencoded data, not for JSON documents.
@GromNaN it's more common to send JSON documents (or form data) than using HTTP Basic auth to get a JWT token. Most client-side and server-side libs work that way.

Copy link

Contributor

teohhanhui commentedJun 3, 2016•
edited
Loading

@dunglas:

GfreeauGetJWTBundle is for x-www-formencoded data, not for JSON documents

But the usual intention is to avoidform_login, which uses cookies. The fact that it usesx-www-form-urlencoded seems to me simply due to ease of implementation (the code seems to be adapted from Symfony'sUsernamePasswordFormAuthenticationListener).

Copy link

jorge07 commentedJun 3, 2016

Why not a Request body listener like FOSRestBundle has? But that way you can use also XML for example... I'm already using exactly this way to login with Json on 3.0.6 with FOSRestBundle 2

xabbuh reviewed

Jun 5, 2016

View reviewed changes

src/Symfony/Component/Security/Http/Firewall/UsernamePasswordJsonAuthenticationListener.php Outdated

		throw new BadCredentialsException(sprintf('Missing key "%s".', $this->options['password_parameter']));
		}

		$username = $data[$this->options['username_parameter']];

Copy link

Member

xabbuhJun 5, 2016

Choose a reason for hiding this comment

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

What about using property paths here? One might not always have the required information on the top level, but may want to wrap it in some other structure.

Copy link

MemberAuthor

dunglas commentedJun 8, 2016•
edited
Loading

I've done some changes to this PR:

The provider is now stateless (it doesn't use the session)
It uses the PropertyAccess component to access the username and the password as brilliantly suggested by@xabbuh
Functional tests added

It is ready for review.

chalasr reviewed

Jul 1, 2016

View reviewed changes

.../Bundle/SecurityBundle/Tests/Functional/Bundle/JsonLoginBundle/Controller/TestController.php Outdated

		{
		public function loginCheckAction()
		{
		throw new \RuntimeException('loginAction() should never be called.');

Copy link

Member

chalasrJul 1, 2016

Choose a reason for hiding this comment

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

Should beloginCheckAction() should never be called..

Copy link

Contributor

theofidryJul 2, 2016

Choose a reason for hiding this comment

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

could make use of__FUNCTION__ actually

Copy link

Contributor

Simperfit commentedJul 2, 2016

LGTM

Copy link

Member

xabbuh commentedJul 4, 2016

The min version in thecomposer.json file of the SecurityBundle needs to be updated.

@dunglas Can you rebase here to hopefully let Travis run the job on PHP 7 again.

dunglas force-pushed thejson_authentication_listener branch from8475884 tocd86c9eCompare

July 10, 2016 08:33

Copy link

MemberAuthor

dunglas commentedJul 10, 2016

Rebased against master and comments fixed.

dunglas force-pushed thejson_authentication_listener branch from163a36a to653bebbCompare

July 17, 2016 10:02

Copy link

MemberAuthor

dunglas commentedJul 17, 2016

ping @symfony/deciders (HHVM error not related)

Copy link

Member

xabbuh commentedJul 18, 2016•
edited
Loading

Imo you should also add a functional test case covering a config where theusername_path andpassword_path are actually used.

Copy link

MemberAuthor

dunglas commentedAug 16, 2016

@xabbuh test updated.

dunglas force-pushed thejson_authentication_listener branch from5c085a6 toee40718Compare

October 25, 2016 11:05

javiereguiluz reviewed

Oct 25, 2016

View reviewed changes

src/Symfony/Component/Security/Http/Firewall/UsernamePasswordJsonAuthenticationListener.php

		public function handle(GetResponseEvent $event)
		{
		$request = $event->getRequest();
		$data = json_decode($request->getContent());

Copy link

Member

javiereguiluzOct 25, 2016

Choose a reason for hiding this comment

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

Should we protect here ourselves by checking the max. length of the request content in the same way that we check the max username (Security::MAX_USERNAME_LENGTH) and password a bit later?

Copy link

MemberAuthor

dunglasOct 25, 2016•
edited
Loading

Choose a reason for hiding this comment

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

@javiereguiluz but what is the max length of the request? The login and password can be stored in any JSON document. It would be similar to checking the length of the form in the traditional login form and AFAIK we don't do it currently.

Copy link

Member

javiereguiluzOct 25, 2016•
edited
Loading

Choose a reason for hiding this comment

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

I'm not an expert about this topic. I just saw thisjson_decode() that accepts the raw user's input and I was thinking about sending a 100MB request to make the application explode ;)

Copy link

MemberAuthor

dunglasOct 25, 2016

Choose a reason for hiding this comment

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

I'm not a security expert either, but it's the same for all endpoints in this case :) There are some configurations in PHP to avoid such problems (likepost_max_size,max_execution_time andmemory_limit). The check on the username length isn't related to DDOS attacks.

Copy link

MemberAuthor

dunglasOct 25, 2016

Choose a reason for hiding this comment

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

See#18733

dunglas mentioned this pull request

Oct 25, 2016

JSON authentication listener docssymfony/symfony-docs#7081

Merged

Copy link

MemberAuthor

dunglas commentedOct 25, 2016

Docs addedsymfony/symfony-docs#7081

fabpot reviewed

Oct 30, 2016

View reviewed changes

src/Symfony/Component/Security/Http/Firewall/UsernamePasswordJsonAuthenticationListener.php Outdated

		}

		if (!is_string($username)) {
		throw new BadCredentialsException(sprintf('The key "%s" must contain a string.', $this->options['username_path']));

Copy link

Member

fabpotOct 30, 2016

Choose a reason for hiding this comment

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

The key "%s" must be a string.?

src/Symfony/Component/Security/Http/Firewall/UsernamePasswordJsonAuthenticationListener.php Outdated

		}

		if (!is_string($password)) {
		throw new BadCredentialsException(sprintf('The key "%s" must contain a string.', $this->options['password_path']));

Copy link

Member

fabpotOct 30, 2016

Choose a reason for hiding this comment

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

The key "%s" must be a string.?

src/Symfony/Component/Security/Http/Firewall/UsernamePasswordJsonAuthenticationListener.php Outdated

		try {
		$username = $this->propertyAccessor->getValue($data, $this->options['username_path']);
		} catch (AccessException $e) {
		throw new BadCredentialsException(sprintf('Missing key "%s".', $this->options['username_path']));

Copy link

Member

fabpotOct 30, 2016

Choose a reason for hiding this comment

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

The "%s" key must be provided.?

src/Symfony/Component/Security/Http/Firewall/UsernamePasswordJsonAuthenticationListener.php Outdated

		try {
		$password = $this->propertyAccessor->getValue($data, $this->options['password_path']);
		} catch (AccessException $e) {
		throw new BadCredentialsException(sprintf('Missing key "%s".', $this->options['password_path']));

Copy link

Member

fabpotOct 30, 2016

Choose a reason for hiding this comment

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

The "%s" key must be provided.?

dunglas force-pushed thejson_authentication_listener branch fromee40718 to8fd5666Compare

November 2, 2016 14:54

Copy link

Member

fabpot commentedNov 6, 2016

👍 for 3.3

javiereguiluz added this to the3.3 milestone

Nov 6, 2016

dunglas force-pushed thejson_authentication_listener branch from8fd5666 toe0bb4e4Compare

December 2, 2016 08:22

dunglas added3 commits

December 2, 2016 09:25

[Security] Add UsernamePasswordJsonAuthenticationListener class

4261e6a

[SecurityBundle] Register the UsernamePasswordJsonAuthenticationListe…

84114ad

…ner class

Update exception messages

10ecbe1

dunglas force-pushed thejson_authentication_listener branch frome0bb4e4 to10ecbe1Compare

December 2, 2016 08:27

Update composer.json

3477b2d

Copy link

Member

fabpot commentedDec 3, 2016

Thank you@dunglas.

fabpot closed this

Dec 3, 2016

fabpot added a commit that referenced this pull request

Dec 3, 2016

feature#18952[Security] Add a JSON authentication listener (dunglas)

d6e8937

This PR was squashed before being merged into the 3.3-dev branch (closes#18952).Discussion----------[Security] Add a JSON authentication listener| Q | A || --- | --- || Branch? | master || Bug fix? | no || New feature? | yes || BC breaks? | no || Deprecations? | no || Tests pass? | yes || Fixed tickets | n/a || License | MIT || Doc PR |symfony/symfony-docs#7081 |Add a new authentication listener allowing to login by sending a JSON document like: `{"_username": "dunglas", "_password": "foo"}`.It is similar to the traditional form login (but take a JSON document as entry) and is convenient for APIs, especially used in combination with JWT.Seeapi-platform/core#563 andlexik/LexikJWTAuthenticationBundle#123 (comment) for previous discussions.- [x] Add functional tests in security bundleCommits-------02178bc [Security] Add a JSON authentication listener

dunglas deleted the json_authentication_listener branch

December 3, 2016 14:23

xabbuh added a commit to symfony/symfony-docs that referenced this pull request

Dec 14, 2016

feature#7081JSON authentication listener docs (dunglas, javiereguiluz)

bad6664

This PR was merged into the master branch.Discussion----------JSON authentication listener docsDocs forsymfony/symfony#18952.Commits-------b192ab3 Fixed a syntax issue16ae3f6 Reworded and simplified the article820f28e Fixed the name of the "username" property6d018f6 Fixed a syntax errorab5259b Removed a tip which is not too relevant for the articled2dd895 Fixed the JSON format (this time for real)6ff24da Fixed the JSON format0961128 Show the simple example first and then explain the complex use case68bd9a5 JSON authentication listener docs