Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Remove unnecesary catch in AuthenticationProviderManager #36822

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

Conversation

umpirsky
Copy link
Contributor

Q A
Branch? master
Bug fix? no
New feature? no
Deprecations? no
License MIT
Doc PR symfony/symfony-docs#13666 is related, but nothing that should concert this PR

Separate catch is not needed as AccountStatusException extends AuthenticationException.

@wouterj
Copy link
Member

wouterj commented May 15, 2020

These catch blocks are not equal and that is exactly why there is an AccountStatusException:

  • In case of an AuthenticationException, the next authentication provider is executed
  • In case of AccountStatusException, the authentication provider has already indicated that it is able to authenticate this request (but there were issues with the user checker). So no further authentication provider is tried and authentication fails directly.

@stof
Copy link
Member

stof commented May 15, 2020

@wouterj the fact that Travis is green tells me that we don't have tests covering your expectation to prevent regressions.

@umpirsky
Copy link
Contributor Author

Oh, I see. But yes, the tests are passing.

@umpirsky umpirsky closed this May 15, 2020
@umpirsky umpirsky deleted the AuthenticationProviderManager-remove-catch branch May 15, 2020 15:44
wouterj added a commit to wouterj/symfony that referenced this pull request May 15, 2020
wouterj added a commit to wouterj/symfony that referenced this pull request May 15, 2020
@wouterj
Copy link
Member

wouterj commented May 15, 2020

Agreed, proposed a test change in #36829

nicolas-grekas added a commit that referenced this pull request May 16, 2020
…havior (wouterj)

This PR was merged into the 3.4 branch.

Discussion
----------

[Security] Update test to test AccountStatusException behavior

| Q             | A
| ------------- | ---
| Branch?       | 3.4 (behavior is this way since 2.0)
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

See #36822

This PR updates the `AccountStatusException` test to test the expected behavior of this exception (and its difference from `AuthenticationException`).

Commits
-------

08fbfcf Added regression test for AccountStatusException behavior (ref #36822)
nicolas-grekas added a commit that referenced this pull request May 16, 2020
* 3.4:
  [VarDumper] fix for change in PHP 7.4.6
  Added regression test for AccountStatusException behavior (ref #36822)
  embed resource name in error message
  [Serializer] fix issue with PHP 8
  [Yaml] Fix escaped quotes in quoted multi-line string
nicolas-grekas added a commit that referenced this pull request May 16, 2020
* 4.4:
  [VarDumper] fix for change in PHP 7.4.6
  Added regression test for AccountStatusException behavior (ref #36822)
  [HttpClient] fix PHP warning + accept status code >= 600
  [Security/Core] fix compat of `NativePasswordEncoder` with pre-PHP74 values of `PASSWORD_*` consts
  embed resource name in error message
  [FrameworkBundle] fix stringable annotation
  Change priority of KernelEvents::RESPONSE subscriber
  Fix register event listeners compiler pass
  Missing description in `messenger:setup-transports` command
  [Serializer] fix issue with PHP 8
  [WebProfiler] Remove 'none' when appending CSP tokens
  [TwigBundle] FormExtension does not have a constructor anymore since sf 4.0
  [Yaml] Fix escaped quotes in quoted multi-line string
nicolas-grekas added a commit that referenced this pull request May 16, 2020
* 5.0:
  [VarDumper] fix for change in PHP 7.4.6
  Added regression test for AccountStatusException behavior (ref #36822)
  [HttpClient] fix PHP warning + accept status code >= 600
  [Security/Core] fix compat of `NativePasswordEncoder` with pre-PHP74 values of `PASSWORD_*` consts
  embed resource name in error message
  [FrameworkBundle] fix stringable annotation
  Change priority of KernelEvents::RESPONSE subscriber
  Fix register event listeners compiler pass
  Missing description in `messenger:setup-transports` command
  [Serializer] fix issue with PHP 8
  [WebProfiler] Remove 'none' when appending CSP tokens
  [TwigBundle] FormExtension does not have a constructor anymore since sf 4.0
  [Yaml] Fix escaped quotes in quoted multi-line string
hultberg pushed a commit to hultberg/symfony that referenced this pull request Sep 17, 2021
* 3.4:
  [VarDumper] fix for change in PHP 7.4.6
  Added regression test for AccountStatusException behavior (ref symfony#36822)
  embed resource name in error message
  [Serializer] fix issue with PHP 8
  [Yaml] Fix escaped quotes in quoted multi-line string
hultberg pushed a commit to hultberg/symfony that referenced this pull request Sep 17, 2021
* 4.4:
  [VarDumper] fix for change in PHP 7.4.6
  Added regression test for AccountStatusException behavior (ref symfony#36822)
  [HttpClient] fix PHP warning + accept status code >= 600
  [Security/Core] fix compat of `NativePasswordEncoder` with pre-PHP74 values of `PASSWORD_*` consts
  embed resource name in error message
  [FrameworkBundle] fix stringable annotation
  Change priority of KernelEvents::RESPONSE subscriber
  Fix register event listeners compiler pass
  Missing description in `messenger:setup-transports` command
  [Serializer] fix issue with PHP 8
  [WebProfiler] Remove 'none' when appending CSP tokens
  [TwigBundle] FormExtension does not have a constructor anymore since sf 4.0
  [Yaml] Fix escaped quotes in quoted multi-line string
hultberg pushed a commit to hultberg/symfony that referenced this pull request Sep 17, 2021
* 5.0:
  [VarDumper] fix for change in PHP 7.4.6
  Added regression test for AccountStatusException behavior (ref symfony#36822)
  [HttpClient] fix PHP warning + accept status code >= 600
  [Security/Core] fix compat of `NativePasswordEncoder` with pre-PHP74 values of `PASSWORD_*` consts
  embed resource name in error message
  [FrameworkBundle] fix stringable annotation
  Change priority of KernelEvents::RESPONSE subscriber
  Fix register event listeners compiler pass
  Missing description in `messenger:setup-transports` command
  [Serializer] fix issue with PHP 8
  [WebProfiler] Remove 'none' when appending CSP tokens
  [TwigBundle] FormExtension does not have a constructor anymore since sf 4.0
  [Yaml] Fix escaped quotes in quoted multi-line string
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.

4 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.