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] getUsername in UserInterface is confusing #10316

Open
ged15 opened this Issue Feb 23, 2014 · 18 comments

Comments

Projects
None yet
@ged15
Copy link
Contributor

ged15 commented Feb 23, 2014

Symfony Security component's UserInterface defines a method getUsername. The purpose of this method is to provide an identifier for the user. In some cases however, a username is not used as the identifier for a user (i.e. in Facebook you create a username only after registration, the email is used instead to identify the user; e-commerce solutions, in order to simplify user registration, also use the email to identify).
IMO, the user interface should not define a getUsername method, but rather a getIdentifier. I believe that this was the original intention of the getUsername method, but the naming turned out to be somewhat confusing.
If there would be interest and +1s from the community and the maintainers, I would gladly submit a PR respecting BC.

@jakzal

This comment has been minimized.

Copy link
Member

jakzal commented Feb 23, 2014

Same problem as we have with the UserProviderInterface (see #5678).

How would you see this improved without breaking the BC (see Changing Interfaces)?

@jakzal jakzal added the Security label Feb 23, 2014

@ged15

This comment has been minimized.

Copy link
Contributor

ged15 commented Feb 23, 2014

@jakzal I would suggest @deprecating the method along with the loadByUsername from the UserProviderInterface and adding alternative methods.
I do not fully agree with the comment by @lmammino "that's not a real issue" as this hinders model readability or sometimes even violates domain language (what if there IS such a property as username and it IS important to the model, but it should not be used to identify the user).

@gnugat

This comment has been minimized.

Copy link
Contributor

gnugat commented Mar 2, 2014

It is confusing indeed, but adding a new method to an interface isn't possible without breaking BC (every project would fail because they don't implement this new method).

@lmammino

This comment has been minimized.

Copy link
Contributor

lmammino commented Mar 2, 2014

Why don't schedule the fix for the version 3.0 (or some next version for which a BC would be acceptable)?

@thewilkybarkid

This comment has been minimized.

Copy link
Contributor

thewilkybarkid commented Jun 5, 2014

getPassword(), getSalt() and eraseCredentials() are also redundant when using external authentication methods (we currently have to return empty strings for the first two and leave the last as an empty method).

@linaori

This comment has been minimized.

Copy link
Contributor

linaori commented Aug 23, 2014

What about things such as:

  • PasswordAwareInterface
  • SaltedPasswordAwareInterface extends PasswordAwareInterface
  • etc.

They could be used mainly as marker interfaces to execute additional methods:

if ($user instanceof PasswordAwareInterface) {
    $this->password_encoder->etc
}

This all could be combined in certain User interfaces.

@lmammino

This comment has been minimized.

Copy link
Contributor

lmammino commented Aug 23, 2014

Would it be reasonable to decouple authentication and user? I mean handling authentication mechanisms(eg. password+salt) and users with separate interfaces/classes/objects and also with separate storage facilities (different tables for ORM or different documents for ODM)...

@linaori

This comment has been minimized.

Copy link
Contributor

linaori commented Aug 23, 2014

The problem is that people start storing entities in their session which causes additional issues and serialization problems. The current implementation is fine in my opinion, it just forces you to implement more than you need.

@JHGitty

This comment has been minimized.

Copy link

JHGitty commented Dec 27, 2014

rel #13131

@backbone87

This comment has been minimized.

Copy link
Contributor

backbone87 commented Mar 7, 2016

The registration is called "username/password" not "identifier/password" or "email/password". you do have a username and thats the email. return $this->getEmail();

A better clarification imho would be to rename the UserInterface to UsernamePasswordIdentityInterface, which also hints the fact, that this should not be your entity/model, but something that is derived from it. it also tributes to the fact, that a user can have multiple identities (aliases or actual different sets of permissions) under that he wants to operate. According to that we could rename UserProviderInterface to IdentityProviderInterface.

@JHGitty

This comment has been minimized.

Copy link

JHGitty commented Mar 11, 2016

Identity sounds better! 👍

@Tarhab

This comment has been minimized.

Copy link

Tarhab commented Jun 16, 2017

Is this still an issue?

@linaori

This comment has been minimized.

Copy link
Contributor

linaori commented Jun 17, 2017

As far as I know, yes. I recently started rewriting from Simple* to Guard. I ended up using the getUsername for the identifier of the authenticated visitor. I also ended up having to implement a salt, password, roles etc. Now I've ended up with several objects, that basically receive an identifier in the constructor and have a getUsername to return it.

@ossinkine

This comment has been minimized.

Copy link
Contributor

ossinkine commented Mar 13, 2018

@iltar How is your work going? Are there any dates when we can see the result? Is there an understanding of how this should look in the next milestone?

@linaori

This comment has been minimized.

Copy link
Contributor

linaori commented Mar 13, 2018

I have no opensource code to illustrate the solution, but the fact that it's called a username is still an issue. It's not something I've been working on to fix in Symfony as of yet.

@ricknox

This comment has been minimized.

Copy link
Contributor

ricknox commented Mar 18, 2018

In my honest opinion I think that the whole UserInterface is redundant. But to sum up: I use, and I see it a lot, it breaks down to this when you only use e-mail addresses:

<?php declare(strict_types=1);

class Foo implements UserInterface, \Serializable
{
    /**
     * @var string
     */
    private $email;

    // ...

    /**
     * {@inheritdoc}
     */
    public function getUsername()
    {
        return $this->email;
    }

    // ...
}

For this issue, I see the ->getUsername() is used in many places. Making it superfluous will be tricky. I think renaming it to getIdentity would be better, however, it will cause "Why is this code here" when looking at this part of the code:

/**
* Returns a user representation.
*
* @return mixed Can be a UserInterface instance, an object implementing a __toString method,
* or the username as a regular string
*
* @see AbstractToken::setUser()
*/
public function getUser();
To sum up, this is really tricky as getUsername is everywhere. I am really interested to see what @iltar has in mind.

Oh and @wouterj already did something some time ago in #21068 but @javiereguiluz ruled it as to wait for the survey of 2017 and then the issue got closed. Time to shred some light on this issue as the security component really needs some love.

@sstok

This comment has been minimized.

Copy link
Contributor

sstok commented Mar 18, 2018

Basically the token should be concerned with the password (if any) and Roles/attributes, the "user" could be anything the implementer wants to use, a User Entity, a string or a UUID Value object.

We can keep the UserInterface (but deprecate it), to ensure existing implementations keep working while allowing more flexibility. There is however one problem, if you start using an incompatible value any existing implementation could break when this relies on a UserInterface to be returned.

@linaori

This comment has been minimized.

Copy link
Contributor

linaori commented Mar 18, 2018

I'm still of opinion that the UserInterface should probably never be used outside anything related to the security component/bundle. It could be a data container during the (re-)authentication process each request, but never leak into the session or application. Instead, I would like to make it easier to obtain the domain object representing the current user (Argument Value Resolver). If you need custom values that help identify the current user, you can use token attributes for that.

Obtaining the current domain object could be done with an abstraction.

// names are probably bad
final class TokenToDomainObjectResolver implements TokenObjectResolverInterface
{
    // ... constructor and dependencies
    public function makeDomainObject(TokenInterface $token): mixed
    {
        return $this->userRepository->find($token->getIdentityValue()); // new method?

        // or use attributes
        return $this->userRepository->findForLogin(
            $token->getIdentifyingValue(), 
            $token->getAttribute('company')
        );
    }
}

// allows this in controller
public function fooAction(User $user);
security:
    # ...
    firewall:
        main:
            # ...
            # again, the name is bad
            token_to_domain_object_resolver: App\Security\TokenToDomainObjectResolver

This way it should be possible to never expose the UserInterface in your application. However, this has 2 downsides.

  • While it's a bad practice, it'll be harder to obtain the User if it's not passed along methods (event listeners for example). This could in turn be resolved by creating a service that uses the same converter, should be 2 lines of code.
  • It'll be an extra query to query user (but might be cached for example)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment