8000 [RFC][Security] Deprecate AdvancedUserInterface · Issue #23292 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[RFC][Security] Deprecate AdvancedUserInterface #23292

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
Closed
linaori opened this issue Jun 25, 2017 · 11 comments
Closed

[RFC][Security] Deprecate AdvancedUserInterface #23292

linaori opened this issue Jun 25, 2017 · 11 comments
Labels
RFC RFC = Request For Comments (proposals about features that you want to be discussed) Security

Comments

@linaori
Copy link
Contributor
linaori commented Jun 25, 2017
Q A
Bug report? no
Feature request? no
BC Break report? no
RFC? yes
Symfony version 3.4.0

Currently the AdvancedUserInterface is not used internally other than:

User Checker

One of the reasons I added the feature for custom user-checkers, was to avoid using the AdvancedUserInterface, as it does not adhere to my business rules (needed different checks). With this customization, the checks it performs are no longer needed to be in the core, as developers can easily define this now.

AbstractToken::hasUserChanged

Can implement the EquatableInterface and add your own logic. I think this is a good practice to do in the first place. The method also compares the salt and password, which would always fail if you don't store the password in the session during refreshing.

EquatableInterface comment

Also implementation should consider that $user instance may implement the extended user interface AdvancedUserInterface.

So if your User implements the interface, you will still have to do the manual checks, which makes the check in the abstract token obsolete.

InMemory user provider

Probably most often added via the security config. It's easy to use for a bunch of things, especially when you have only 1 or 2 users that can manage content. However, it's 10x easier to comment or remove a user from this list, than using one of those flags to prevent a log in. If the idea is that a user could still be found but not logged in, this feature should be contained to this particular user object and not use the AdvancedUserInterface.

Method naming

isAccountNonExpired, isAccountNonLocked, isCredentialsNonExpired naming is not ideal. I would expect isAccountExpired, isAccountLocked, areCredentialsExpired. https://www.refactoring.com/catalog/removeDoubleNegative.html

However, I'm of opinion that renaming doesn't really solve the issue of trying to introduce certain business logic into the framework.

RFC

I propose to completely deprecate the AdvancedUserInterface in 3.4 and remove it with 4.0 as it no longer serves a purpose. This would mean the following actions:

  • Currently user checkers are documented, but this bit of documentation would have to be updated to explain integration with user checkers.
  • Add deprecations when loading the AdvancedUserInterface
  • Optionally add deprecations run-time in the abstract token and user checker (Listed, but I don't think needed)
  • Add user user-checker checks for the in-memory user to keep behavior the same without deprecations for this particular case

For 4.0 the following actions would be taken:

  • Remove the checks in the abstract token
  • Remove the interface check in the UserChecker and only register it on the in memory user
  • Remove the reference to AdvancedUserInterface in the EquatableInterface
  • Remove the AdvancedUserInterface
  • Remove interface implementation of the in memory user (functionality can be kept)
@linaori
Copy link
Contributor Author
linaori commented Jun 26, 2017

User curtchan on Slack pointed me on the FOS User Bundle using this implementation. The fix for this would be something in line of adding a user-checker that decorates the default Symfony one and also executes the checks on here. Additionally, the equatable interface should be used to verify the additional fields, as this is not the responsibility of the token, but the user.

@xabbuh xabbuh added RFC RFC = Request For Comments (proposals about features that you want to be discussed) Security labels Jun 26, 2017
@Hanmac
Copy link
Contributor
Hanmac commented Jun 27, 2017

@iltar what about adding a Trait that does that what AbstractToken#hasUserChanged does with EquatableInterface?

i mean this part AbstractToken#hasUserChanged as part of a Trait that uses isEqualTo?

@linaori
Copy link
Contributor Author
linaori commented Jun 27, 2017

It's a solution but I don't see the benefit of adding this, as it would only be used in the in memory user.

@stof
Copy link
Member
stof commented Jun 27, 2017

The only method of AdvancedUserInterface actually used in FOSUserBundle is isEnabled. All other methods return hard-coded false (I'm talking about version 2 of the bundle. 1.3 is irrelevant here, as it supports only Symfony 2.x anyway).
We use isEnabled to prevent using a user for login when email is not confirmed yet (in case the email confirmation feature is used)

@linaori
Copy link
Contributor Author
linaori commented Jun 27, 2017

So for FOSUserBundle it would only require a user-checker that checks whether or not the client is enabled. This could already be fixed without this PR. If I have some time, I can help out with that.

@Hanmac
Copy link
Contributor
Hanmac commented Jun 27, 2017

so FOSUserBundle can use EquatableInterface or do i get that wrong?
i just don't know what it should do with UserCheckerInterface

@linaori
Copy link
Contributor Author
linaori commented Jun 27, 2017

@Hanmac everyone can use that interface. That interface is literally to compare User A with User B to see if it's the same user. I only identify based on the Username (identifier), so that's my personal check. If you want you can even compare a timestamp if that pleases you.

@Hanmac
Copy link
Contributor
Hanmac commented Jun 27, 2017

@iltar i was thinking when this RFC got done and AdvancedUserInterface got removed how FOSUserBundle should be changed because of that.

i was thinking about AbstractToken::hasUserChanged and that the FOSUser might use EquatableInterface too to check if the user got changed.

@linaori linaori mentioned this issue Jun 30, 2017
Closed
@curry684
Copy link
Contributor

isAccountNonExpired, isAccountNonLocked, isCredentialsNonExpired naming is not ideal.

I believe the rationale there is that forgetting the implementation of the interface method will cause null to be implicitly returned, thus causing the account to be incorrectly blocked all the time instead of allowed all the time. Err on the side of being overly secure.

@apfelbox
Copy link
Contributor

@curry684 afair the rationale was that returning true should always mean "okay, allow the user" and returning false should be "deny access". And that's why all methods are isNon...

@linaori linaori mentioned this issue Jul 14, 2017
Merged
nicolas-grekas added a commit that referenced this issue Feb 4, 2018
@nicolas-grekas
feature #23508 Deprecated the AdvancedUserInterface (iltar)
32cc2e0 
This PR was merged into the 4.1-dev branch.

Discussion
----------

Deprecated the AdvancedUserInterface

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #23292
| License       | MIT
| Doc PR        | ~

This PR deprecates the usages of the `AdvancedUserInterface`.

Commits
-------

8456f3b Deprecated the AdvancedUserInterface
@linaori
Copy link
Contributor Author
linaori commented Mar 1, 2018

Closing this issue as it was fixed in #23508

@linaori linaori closed this as completed Mar 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
RFC RFC = Request For Comments (proposals about features that you want to be discussed) Security
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@Hanmac @stof @apfelbox @curry684 @linaori @xabbuh
0