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

Convert InsufficientAuthenticationException to HttpException with 401 status code #28801

Merged
merged 1 commit into from Oct 17, 2018

Conversation

Projects
None yet
10 participants
@vincentchalamon
Copy link
Contributor

commented Oct 10, 2018

Q A
Branch? 2.8
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed ticket #8467
License MIT

I was trying to implement the json_login authentication and test it with an API Platform project. When I call a secured endpoint without authentication, an InsufficientAuthenticationException is thrown with a 500 status code instead of a 401.

After some researches with @dunglas, there is no default entrypoint on the security firewall. As one already exists for form_login in the FormLoginFactory, this component might need a default one to convert this 500 exception to a correct 401 HTTP error.

This fixes #25806 (comment).

@dunglas
Copy link
Member

left a comment

👍 when tests will be added

@vincentchalamon vincentchalamon force-pushed the vincentchalamon:master branch from e709469 to df9cf29 Oct 10, 2018

@vincentchalamon vincentchalamon changed the base branch from master to 2.8 Oct 10, 2018

@vincentchalamon vincentchalamon force-pushed the vincentchalamon:master branch from df9cf29 to 4503ac8 Oct 10, 2018

@vincentchalamon vincentchalamon changed the title [WIP] Convert InsufficientAuthenticationException to HttpException with 401 status code Convert InsufficientAuthenticationException to HttpException with 401 status code Oct 10, 2018

@Koc

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2018

seems like this PR also fixes #8467, but not sure

@nicolas-grekas nicolas-grekas added this to the 2.8 milestone Oct 17, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

@Koc I'm linking your issue as being fixed by this PR. I'll let you reopen an issue if that's not the case.

@fabpot

fabpot approved these changes Oct 17, 2018

@fabpot

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

Thank you @vincentchalamon.

@fabpot fabpot merged commit 4503ac8 into symfony:2.8 Oct 17, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Oct 17, 2018

bug #28801 Convert InsufficientAuthenticationException to HttpExcepti…
…on with 401 status code (vincentchalamon)

This PR was merged into the 2.8 branch.

Discussion
----------

Convert InsufficientAuthenticationException to HttpException with 401 status code

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed ticket | #8467
| License       | MIT

I was trying to implement the `json_login` authentication and test it with an API Platform project. When I call a secured endpoint without authentication, an InsufficientAuthenticationException is thrown with a 500 status code instead of a 401.

After some researches with @dunglas, there is no default `entrypoint` on the security firewall. As one already exists for `form_login` in the FormLoginFactory, this component might need a default one to convert this 500 exception to a correct 401 HTTP error.

This fixes #25806 (comment).

Commits
-------

4503ac8 Convert InsufficientAuthenticationException to HttpException
@Rebolon

This comment has been minimized.

Copy link

commented Oct 18, 2018

The problem also exist in 4+ branch, does the fix solve the issue in recent version of Sf ?

@dunglas

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

@Rebolon I think so, old branches are merged in the newer ones on a regular basis.

This was referenced Nov 3, 2018

@gitnik

This comment has been minimized.

Copy link

commented Nov 22, 2018

Hey guys, this actually broke some of our code as we caught this specific exception and converted it to a 401 ourselves. With this change it turned into a 500 essentially reversing our scenario.
I would definitely consider throwing a different exception to be BC breaking.

@ganoch

This comment has been minimized.

Copy link

commented Mar 14, 2019

@gitnik nice, your case was bound to happen as this issue has been present for quite some time now. I must say your original fix is now the root of your problem. Do you mind telling how you fixed your new issue?

@gitnik

This comment has been minimized.

Copy link

commented Mar 14, 2019

Now we just catch the generic HttpException and check for status 401. But it took quite some time figuring this out 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.