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

vincentchalamon
Copy link
Contributor

@vincentchalamon vincentchalamon 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).

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.

👍 when tests will be added

@vincentchalamon vincentchalamon changed the base branch from master to 2.8 October 10, 2018 12:46
@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
Copy link
Contributor

Koc 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
Copy link
Member

@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
Copy link
Member

fabpot commented Oct 17, 2018

Thank you @vincentchalamon.

@fabpot fabpot merged commit 4503ac8 into symfony:2.8 Oct 17, 2018
fabpot added a commit that referenced this pull request Oct 17, 2018
…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
Copy link

Rebolon commented Oct 18, 2018

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

@dunglas
Copy link
Member

dunglas 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
Copy link

gitnik 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
Copy link

ganoch 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
Copy link

gitnik 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 😉

@Rebolon
Copy link

Rebolon commented Aug 8, 2019

There is the same kind of missing feature with this Symfony Exception:
Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
Symfony always return a 500 instead of a 403

I opened an issue in ApiPlatform https://github.com/api-platform/api-platform/issues/1213
But i think tht the problem is on Symfony

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.

None yet

10 participants