Skip to content

[Security] Add callable type to CustomCredentials #60245

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 22, 2025
Merged

[Security] Add callable type to CustomCredentials #60245

merged 1 commit into from
Apr 22, 2025

Conversation

BenMorel
Copy link
Contributor

Q A
Branch? 7.3
Bug fix? no
New feature? no
Deprecations? no
Issues /
License MIT

@BenMorel BenMorel requested a review from chalasr as a code owner April 20, 2025 23:36
@carsonbot carsonbot added this to the 7.3 milestone Apr 20, 2025
@carsonbot carsonbot changed the title Add callable type to CustomCredentials [Security] Add callable type to CustomCredentials Apr 21, 2025
@nicolas-grekas
Copy link
Member

Thank you @BenMorel.

@nicolas-grekas nicolas-grekas merged commit fe94cfb into symfony:7.3 Apr 22, 2025
2 checks passed
@nicodemuz
Copy link
Contributor

nicodemuz commented Aug 14, 2025 •
edited
Loading

Phan is reporting an issue for this:

src/.../Security/GoogleAuthenticator.php:61 PhanTypeMismatchArgumentSuperType Argument 1 ($customCredentialsChecker) is (fn) of type Closure(...\Security\GoogleCredentials,\Symfony\Component\Security\Core\User\UserInterface):bool but \Symfony\Component\Security\Http\Authenticator\Passport\Credentials\CustomCredentials::__construct() takes callable(mixed,\Symfony\Component\Security\Core\User\UserInterface):void defined at vendor/symfony/security-http/Authenticator/Passport/Credentials/CustomCredentials.php:34 (expected type to be the same or a subtype, but saw a supertype instead)

To compare:

Closure (\...\Security\GoogleCredentials,\Symfony\Component\Security\Core\User\UserInterface):bool
callable(mixed,                          \Symfony\Component\Security\Core\User\UserInterface):void

It's suggesting I change the return type from bool to void, which is incorrect.

Should we add the correct return type?

param callable(mixed, UserInterface)
->
param callable(mixed, UserInterface): bool

@xabbuh
Copy link
Member

xabbuh commented Aug 14, 2025

@nicodemuz Looks reasonable to me, can you send a PR?

@nicodemuz nicodemuz mentioned this pull request Aug 14, 2025
Merged
@nicodemuz
Copy link
Contributor

@nicodemuz Looks reasonable to me, can you send a PR?

@xabbuh See #61409

fabpot added a commit that referenced this pull request Aug 16, 2025
@fabpot
minor #61409 [Security] Add bool return type to CustomCredentials cal…
138777a 
…lable parameter (Nico Hiort af Ornäs)

This PR was merged into the 7.3 branch.

Discussion
----------

[Security] Add bool return type to CustomCredentials callable parameter

| Q             | A
| ------------- | ---
| Branch?       | 7.3 for bug fixes
| Bug fix?      | yes
| New feature?  | no <!-- if yes, also update src/**/CHANGELOG.md -->
| Deprecations? | no <!-- if yes, also update UPGRADE-*.md and src/**/CHANGELOG.md -->
| Issues        | Fixes comment in [#60245](#60245 (comment))
| License       | MIT

<!--
🛠️ Replace this text with a concise explanation of your change:
- What it does and why it's needed
- A simple example of how it works (include PHP, YAML, etc.)
- If it modifies existing behavior, include a before/after comparison

Contributor guidelines:
- ✅ Add tests and ensure they pass
- 🐞 Bug fixes must target the **lowest maintained** branch where they apply
  https://symfony.com/releases#maintained-symfony-branches
- ✨ New features and deprecations must target the **feature** branch
  and must add an entry to the changelog file of the patched component:
  https://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry
- 🔒 Do not break backward compatibility:
  https://symfony.com/bc
-->

Commits
-------

cf02add Add bool return type to CustomCredentials callable parameter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chalasr chalasr Awaiting requested review from chalasr chalasr is a code owner

Assignees
No one assigned
Labels
Security Status: Needs Review
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.
6 participants
@BenMorel @nicolas-grekas @nicodemuz @xabbuh @OskarStark @carsonbot