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

[Security] Decoupling password from UserInterface #23081

Open
Vinorcola opened this Issue Jun 6, 2017 · 7 comments

Comments

Projects
None yet
7 participants
@Vinorcola

Vinorcola commented Jun 6, 2017

Q A
Bug report? no
Feature request? yes
BC Break report? no
RFC? no
Symfony version 3.x

I think we should decouple password from UserInterface (having a PasswordInterface for example) in order to be able to use them with the password encoding system. Currently, you need to pass a whole UserInterface for encoding password, while a simple password/salt is enough.

This would be usefull to implement some system where you manager password history. Right now, you need the "PasswordHistory" object to implement UserInterface in order to be used with the PasswordEncoder. But having a username or roles into a PasswordHistory object doesn't make any sense...

@Hanmac

This comment has been minimized.

Show comment
Hide comment
@Hanmac

Hanmac Jun 6, 2017

i find your idea interesting but i would have used the UserName with the Password in the Password history like:
[id] [user] [password] [salt]

for that i would have used a UserToken that just implements UserInterface.
getRoles returns empty string and eraseCredentials does nothing.

hm with that you can also use it to tell the user that he already used a password like that (with same hash)

Hanmac commented Jun 6, 2017

i find your idea interesting but i would have used the UserName with the Password in the Password history like:
[id] [user] [password] [salt]

for that i would have used a UserToken that just implements UserInterface.
getRoles returns empty string and eraseCredentials does nothing.

hm with that you can also use it to tell the user that he already used a password like that (with same hash)

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Jun 6, 2017

Member

@Vinorcola : The PasswordEncoderInterface does not require a UserInterface instance on the contrary of the UserPasswordEncoderInterface.

Instead of using UserPasswordEncoderInterface or the security.password_encoder service, try using the security.encoder_factory to create the encoder you expect to inject in your services/controller:

services:
    Symfony\Component\Security\Core\Encoder\PasswordEncoderInterface:
        factory: 'Symfony\Component\Security\Core\Encoder\EncoderFactoryInterface:getEncoder'
        # or
        factory: 'security.encoder_factory:getEncoder'
        arguments: ['AppBundle\Entity\User']
Member

ogizanagi commented Jun 6, 2017

@Vinorcola : The PasswordEncoderInterface does not require a UserInterface instance on the contrary of the UserPasswordEncoderInterface.

Instead of using UserPasswordEncoderInterface or the security.password_encoder service, try using the security.encoder_factory to create the encoder you expect to inject in your services/controller:

services:
    Symfony\Component\Security\Core\Encoder\PasswordEncoderInterface:
        factory: 'Symfony\Component\Security\Core\Encoder\EncoderFactoryInterface:getEncoder'
        # or
        factory: 'security.encoder_factory:getEncoder'
        arguments: ['AppBundle\Entity\User']
@iltar

This comment has been minimized.

Show comment
Hide comment
@iltar

iltar Jun 7, 2017

Contributor

Personally, I'd rather see the UserInterface disappear. Beyond the username, you don't really need anything anymore besides for authentication.

Contributor

iltar commented Jun 7, 2017

Personally, I'd rather see the UserInterface disappear. Beyond the username, you don't really need anything anymore besides for authentication.

@xabbuh xabbuh added the Security label Jun 7, 2017

@Hanmac

This comment has been minimized.

Show comment
Hide comment
@Hanmac

Hanmac Jun 9, 2017

@Vinorcola for that do you currently plan to use "PasswordHistory"?
do you plan in using it in such a "you already used that password before" mechanic?

because i would be interested in such thing too

Hanmac commented Jun 9, 2017

@Vinorcola for that do you currently plan to use "PasswordHistory"?
do you plan in using it in such a "you already used that password before" mechanic?

because i would be interested in such thing too

@Vinorcola

This comment has been minimized.

Show comment
Hide comment
@Vinorcola

Vinorcola Jun 9, 2017

@Hanmac Yes that's excatly why I want to use it. I don't need the username since PasswordHistory has a relation with the user account entity.

It is not that diffcult: when the user change his password, before registering it, you just try to match it to all its password history. If one match, then you refuse the given password.

@ogizanagi Thanks, good idea to create a service with a factory!

Vinorcola commented Jun 9, 2017

@Hanmac Yes that's excatly why I want to use it. I don't need the username since PasswordHistory has a relation with the user account entity.

It is not that diffcult: when the user change his password, before registering it, you just try to match it to all its password history. If one match, then you refuse the given password.

@ogizanagi Thanks, good idea to create a service with a factory!

@Hanmac

This comment has been minimized.

Show comment
Hide comment
@Hanmac

Hanmac Jun 9, 2017

@Vinorcola interesting, would like to see if its ready to bundle
hm i would probalby still use my idea there with, and implement UserInterface
[id] [user] [password] [salt]
getUsernamefor example could point be done like getUser()->getUsername()

would it work with FOSUserbundle too?

Hanmac commented Jun 9, 2017

@Vinorcola interesting, would like to see if its ready to bundle
hm i would probalby still use my idea there with, and implement UserInterface
[id] [user] [password] [salt]
getUsernamefor example could point be done like getUser()->getUsername()

would it work with FOSUserbundle too?

@curry684

This comment has been minimized.

Show comment
Hide comment
@curry684

curry684 Jun 30, 2017

Contributor

Big upvote in general, and highly agree with @iltar that UserInterface in general should be refactored or even removed. It hurts my purist mind that even the documentation lightly suggests to just implement getPassword, getSalt etc. as empty functions, without seeing the implication that this means the base mechanism is broken there.

In a modern application with OAuth or other SSO mechanisms the Symfony authentication layer is enforcing way too much crap on the developer that should be opt-in.

Contributor

curry684 commented Jun 30, 2017

Big upvote in general, and highly agree with @iltar that UserInterface in general should be refactored or even removed. It hurts my purist mind that even the documentation lightly suggests to just implement getPassword, getSalt etc. as empty functions, without seeing the implication that this means the base mechanism is broken there.

In a modern application with OAuth or other SSO mechanisms the Symfony authentication layer is enforcing way too much crap on the developer that should be opt-in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment