8000 [Security] Deprecate UserInterface & TokenInterface's `eraseCredentials()` by nicolas-grekas · Pull Request #59682 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Security] Deprecate UserInterface & TokenInterface's eraseCredentials() #59682

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 2 commits into from
Feb 4, 2025
Merged

[Security] Deprecate UserInterface & TokenInterface's eraseCredentials() #59682

merged 2 commits into from
Feb 4, 2025

Conversation

nicolas-grekas
Copy link
Member
@nicolas-grekas nicolas-grekas commented Feb 3, 2025 •
edited
Loading
Q A
Branch? 7.3
Bug fix? no
New feature? yes
Deprecations? yes
Issues Fix #57842
License MIT

As promised, this PR adds a commit on top of #59106 to improve the BC layer. This approach didn't fit in a review comment :) /cc @chalasr

This PR leverages the new #[\Deprecated] attribute to decide if some eraseCredentials() method is to be called or not.

My target DX here is to save us all (the community) from having to add erase_credentials: false configuration in all our apps.

So, instead of having to opt-out from the deprecation using this config option, the opt-out is done by adding the attribute on the method:

Before:

public function eraseCredentials(): void
{
}

After:

#[\Deprecated]
public function eraseCredentials(): void
{
}

// If your eraseCredentials() method was used to empty a "password" property:
public function __serialize(): array
{
    $data = (array) $this;
    unset($data["\0".self::class."\0password"]);

    return $data;
}

This should provide a smoother upgrade path (and maker-bundle could be updated right-away).

@carsonbot carsonbot added this to the 7.3 milestone Feb 3, 2025
@nicolas-grekas nicolas-grekas mentioned this pull request Feb 3, 2025
Closed
@nicolas-grekas nicolas-grekas force-pushed the deprecate-erase-credentials branch 3 times, most recently from 012a13f to 2069f53 Compare February 3, 2025 21:33
chalasr
chalasr requested changes Feb 3, 2025
View reviewed changes
Copy link
Member
@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

Thanks!

@nicolas-grekas nicolas-grekas force-pushed the deprecate-erase-credentials branch from 2069f53 to 40b3696 Compare February 4, 2025 07:21
@nicolas-grekas
Copy link
Member Author

Comments addressed thanks.

@nicolas-grekas nicolas-grekas force-pushed the deprecate-erase-credentials branch 2 times, most recently from b7d3929 to 5df85e4 Compare February 4, 2025 07:25
chalasr
chalasr approved these changes Feb 4, 2025
View reviewed changes
Copy link
Member
@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

This kind of BC layer feels new to me, refreshing :) 👍 with some more minor catches

@nicolas-grekas nicolas-grekas force-pushed the deprecate-erase-credentials branch from 5df85e4 to e5c94e6 Compare February 4, 2025 08:23
@nicolas-grekas
Copy link
Member Author

Thank you @chalasr.

@nicolas-grekas nicolas-grekas merged commit 5a09d52 into symfony:7.3 Feb 4, 2025
8 of 12 checks passed
@nicolas-grekas nicolas-grekas mentioned this pull request Feb 4, 2025
Closed
@nicolas-grekas nicolas-grekas deleted the deprecate-erase-credentials branch February 4, 2025 14:30
@fabpot fabpot mentioned this pull request May 2, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@welcoMattic welcoMattic welcoMattic approved these changes

@chalasr chalasr chalasr approved these changes

Assignees
No one assigned
Labels
Deprecation Feature Security Status: Reviewed
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.

Move or retire UserInterface::eraseCredentials
4 participants
@nicolas-grekas @welcoMattic @chalasr @carsonbot
0