[BC Break][Security] Renamed equals to isSameUser in the UserInterface #2669

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
Member

stof commented Nov 17, 2011

As discussed today during the IRC meeting, here is the PR changing the name of the method to what was preferred by most people that were there.

All: suggest better names if you have some.

Contributor

willdurand commented Nov 17, 2011

+1 for isSameUser

Contributor

schmittjoh commented Nov 17, 2011

I don't think this is worthwhile.

function equals(UserInterface $user);
function isSameUser(UserInterface $user);

$user1->equals($user2);
$user1->isSameUser($user2);

The equals method fits better with the natural language "user1 equals user2" whereas isSameUser does not make sense "user1 is same user user2". In my experience, people have more a problem with the implementation of this method because it's not so common in PHP. However, that is a documentation issue as the corresponding cookbook entry has not been written yet.

Considering that this adds no real value, I'm inclined to close this PR unless someone comes up with a significantly better name.

Contributor

willdurand commented Nov 17, 2011

How about isSameAs() ?

Member

stof commented Nov 17, 2011

The issue with equals is that I saw some people implementing it with === even if the phpdoc explicitly warns against it because this is how they thought about equality.

And btw, the context in which this method is used is to determine if the user has changed between the instance coming from the session and the refreshed one. The name of the method in the interface does not make it obvious at all (thus the addition of an explanation in the phpdoc to avoid having to read the Security component source code to know it). Do someone has a better name by taking it into account ?

Contributor

schmittjoh commented Nov 17, 2011

isSameAs() is better to read than isSameUser(), but I still don't see why it's better than equals()?

$user1->isSameAs($user2);
$user1->equals($user2);
Member

stof commented Nov 17, 2011

@schmittjoh From @couac POV, it is better because it won't conflict with Propel anymore so he will be able to remove the need of proxifying the model class (which is a real pain).

But isSameAs does not explain the case where we should consider the user to be the same (for instance, if you changed the role of a user and don't re-authenticate, the old roles will still be used so you may want to compare them here)

Contributor

canni commented Nov 17, 2011

-1 from me, equals(UserInteface $user) is more than enough, we should not introduce BC breaks just because someone may not read interface docs, besides equals is a well known industry standard for object comparison.

Member

stof commented Nov 17, 2011

@canni One of the issue is here too: as it is a well known standard, it is likely to conflict with other interfaces or with different implementations. A good example already exists: Propel models cannot implement this interface because the base class contains a equals method already but without the typehint and for a different need.

Contributor

fzaninotto commented Nov 17, 2011

+1 as well for isSameUser. equals() is too generic and may (does) conflict with other libraries.

Contributor

canni commented Nov 17, 2011

@stof, good point, so my +1 vote goes for equalsUser(UserInterface $user) e.g.: $john->equalsUser($alice), isSameUser seems weird even for non english native speaker like me :) my secondary vote (+0.5 ;) ) goes for isSameAs

Contributor

schmittjoh commented Nov 17, 2011

After reading through the discussion of the IRC meeting, the general agreement was to only make a change to avoid confusion not for compatibility with other libraries. I agree here. The proposed alternatives isSameUser or isSameAs do not have that goal though.

Maybe we should first try to determine what causes the confusion?

Member

stof commented Nov 17, 2011

One point here is that the goal is not only to check if the user is the same but to check if the user can still be considered as valid for the authentication and this part is not taken into account properly when naming the method.

marijn commented Nov 18, 2011

-1. This is about semantics and documentation...

Contributor

trompette commented Nov 18, 2011

I think @stof as a good point here. If the semantics behind the method is not just comparing objects, the method name has to be changed. Let's call it isSameAsValidAutenticatedUser or something like that. It even helps code readibility.

Contributor

schmittjoh commented Nov 18, 2011

We really want the implementor to check for equality here. Based on his decision, we change the authentication status of the token. It does not make sense to name the method something like isStillConsideredAsValidForAuthentication.

$user1->isStillConsideredAsValidForAuthentication($user2);
$user1->isSameAsValidAuthenticatedUser($user2);

Such a method name might make sense if it is on the token itself, but then the implementation of such a check would not be the responsibility of the developer anymore.

Anyway, feel free to make more suggestions, but I think it's more about providing better documentation (since we have none on this) than about the method name. In its current form, it cannot be merged though.

Contributor

canni commented Nov 18, 2011

Maybe we should move this comparison logic into token itself?

Owner

fabpot commented Nov 18, 2011

The
equals method name comes from Java where it compares values for equality. This method is defined in the Object class, from which all other classes are derived. So, it is automatically defined for every Java class and as such, it is a very well-know method in the Java world.

In Java, the equals method should compare state, not identity. For instance, if you have a class that has a single string property, two different objects might be considered equals if the string is the same for the two objects even if the objects themselves are different (different memory location). It can be seen as a subtle difference, but actually it's not, and that's exactly the problem we face here (as explained above by @stof and in the PHPDoc of the method itself).

As most PHP developers do not have this background (myself included as the two previous paragraphs are mostly a summary of what I've just read from blog posts I have found on the Web), we cannot assume that they all know about the semantic of this method, which has, of course, no special meaning in PHP.

So, IMO, we need to rename the equals method for two reasons:

  • Change the method name to something more specific to make its goal/intention very clear and avoid confusion (this is important as ambiguity is not an option when it comes to Security);
  • Make the method name more specific to avoid collisions with other existing classes. This one is also very important because one of the best feature of the Security component is that it does not force you to use any specific User class; just implement UserInterface and you are all set. So, UserInterface should be useable on any class, and so its methods should be as unique and as specific as possible.

For me, the renaming of the equals method is mandatory and the only question at this point is what is the best method name for it.

Contributor

canni commented Nov 18, 2011

Check to see if an user can still be considered as valid for the authentication is not concern of logic of UserInterface this logic in my opinion belongs to TokenInterface. Lets leave UserInterface free of equals method, as equality (state or identity) implementation should be left to developer hands, to define in classes (like in Propel models)

j commented Nov 18, 2011

+1 for $user1->equalsUser($user2)

Member

stof commented Nov 18, 2011

@canni the issue is that it also depends of the User class. As one of the user concerned by the comparison is the unserialized user, the comparison can only involve properties that are serialized otherwise it will fail.

Contributor

schmittjoh commented Nov 19, 2011

@canni, I think your idea is not so bad. We could provide a default implementation that checks for equality based on the other methods that we have on the UserInterface (getUsername, getSalt, getPassword, etc.) and the AdvancedUserInterface. Then we could remove the equals method from the UserInterface.

Optionally, we could add another interface EquatableInterface with a method hasEqualPropertyValues which could be used if someone needs to check additional properties that are not covered by either the UserInterface, or the AdvancedUserInterface.

I think this way, we really address the confusion issue as the default implementation is provided by the framework and the user does not have to worry about this anymore. He can still change it by implementing the EquatableInterface, but that would be optional. What do you think?

Contributor

mvrhov commented Nov 19, 2011

@schmittjoh: I have a problem with what has been serialized. I really don't like that the password and salt get serialized and stored into a session. Please note that most of shared hosting providers have set up sessions in such way that all sites on their serer write them to specific directory. e.g /tmp, /var/lib/php/sessions as this dir is ussualy not restricted by openbasedir because then you cannot write sessions, so any other host can grab them. Now you have access to the username hashed pwd and salt.

Contributor

trompette commented Nov 19, 2011

@schmittjoh, any idea that ease develpment for non power users is a big step forward towards wide adoption.

I don't like the chosen names EquatableInterface and hasEqualPropertyValues, but your idea seems great.

Contributor

canni commented Nov 19, 2011

@schmittjoh that is exactly what I have proposed, UserInterface must be implemented, and has defined methods, that we can use to check "equality" outside of UserInterface (e.g. in AbstractToken or somewhere else).

That leaves UserInterface free of problematic method name and optionally enables developers to define equality in way they want it.

Interface name doesn't matter for me, but method name of EquatableIntrerface could be named simply by generic: isEqualTo

Contributor

schmittjoh commented Nov 22, 2011

@stof, are you interested/do you have time to update your PR?

Member

stof commented Nov 22, 2011

@schmittjoh I will work on it this evening.

Owner

fabpot commented Dec 1, 2011

@stof: any news on this PR?

Member

stof commented Dec 1, 2011

no, I haven't had time to implement an Equatable interface instead of the current renaming. If someone else wants to work on it, feel free to do so. I won't be able to do it myself until this weekend.

Contributor

willdurand commented Dec 13, 2011

@stof what's the status of that PR ?

Member

stof commented Dec 14, 2011

I haven't has time to work on the alternate implementation. If someone else wants to do it to have it more quickly, feel free.

Contributor

willdurand commented Dec 14, 2011

Do we have to write an interface ?

Member

stof commented Dec 14, 2011

@willdurand the way to go is to refactor the code using the equals method to use an external comparator

Contributor

willdurand commented Dec 14, 2011

Oh right, missed @schmittjoh's comment..

Contributor

flevour commented Dec 15, 2011

If I read the whole thread correctly, I have the impression that after @schmittjoh comment above, the discussion has derailed from the point @fabpot stated above: find a good name for equals.

Member

stof commented Dec 15, 2011

@flevour @schmittjoh's comment has been done after the IRC meeting which agreed on changing the way it works

Contributor

flevour commented Dec 15, 2011

As per today IRC meeting (https://gist.github.com/1482008) the direction to follow is the implementation of a UserComparator with a method to test the equality of 2 UserInterface objects. No one has stepped up to write this implementation.

Contributor

schmittjoh commented Dec 15, 2011

Note that we do not want to add another class, the implementation can be embedded in the AbstractToken.

Member

stof commented Dec 24, 2011

Closing in favor of #2927

@stof stof closed this Dec 24, 2011

fabpot added a commit that referenced this pull request Jan 13, 2012

merged branch canni/user_comparable_interface2 (PR #2927)
Commits
-------

e23d452 Add info about BC Break to CHANGELOG-2.1
d7ffeb5 Add some more tests, and enforce boolean return value of interface implementations.
9d3a49f When method name is `hasUserChanged` the return boolean should be true (to match question semantics) and false when user has not changed, this commits inverts return statements.
c57b528 Add note about `AdvancedUserInterface`.
3682f62 Refactor `isUserChanged` to `hasUserChanged`
56db4a1 Change names to Equatable
680b108 Suggested fixes ;)
9386583 [BC Break][Security] Moved user comparsion logic out of UserInterface As discussed on IRC meetings and in PR #2669 I came up with implementation. This is option2, I think more elegant.

Discussion
----------

[BC Break][Security][Option2] Moved user comparsion logic out of UserInterface

As discussed on IRC meetings and in PR #2669 I came up with implementation.
This is option2, I think more elegant.

BC break: yes
Feature addition: no/feature move
Symfony2 test pass: yes
Symfony2 test written: yes
Todo: decide about naming

[![Build Status](https://secure.travis-ci.org/canni/symfony.png)](http://travis-ci.org/canni/symfony)

---------------------------------------------------------------------------

by schmittjoh at 2011-12-19T19:33:24Z

This looks much better than the previous PR. Thanks!

One thing, we also discussed this on Doctrine, the name "comparable" is used in most programming languages to perform a real compare operation that is ">", "<", or "=". In this case though, we are specifically interested in equality of two objects (we cannot establish a natural order between these objects). Java has no such interface as all objects naturally have an equals() method, .NET uses "Equatable" which looks a bit odd. Not sure if there are better names.

---------------------------------------------------------------------------

by canni at 2011-12-19T19:34:52Z

I think this is best of "both worlds" we have nice full-featured implementation suitable for most, and if someone needs advanced compare logic just implements interface. @stof @schmittjoh, what do you think?

---------------------------------------------------------------------------

by stof at 2011-12-19T19:36:55Z

@canni I already commented on the code, and I agree with @schmittjoh that the naming can be confusing

---------------------------------------------------------------------------

by jmikola at 2011-12-20T17:33:22Z

I don't mean to bikeshed, but I strongly agree with @schmittjoh about implications of "compare". I'm not concerned with the interface name so much as I am with `compareUser()`. Given that this method returns a boolean, I think it's best to prefix it with `is` (e.g. `isSameUser`, `isUserEqualTo`) or `equals` (e.g. `equalsUser`).

In this PR, the Token class is implementing the interface, so I think having "User" in the method name is a good idea. Naturally, if the interface was intended for User classes, we could do without it.

---------------------------------------------------------------------------

by canni at 2011-12-20T19:00:00Z

@jmikola in this PR Token class does not implement any additional interface, and `compareUser` is `private` and used internally. I don't stand still after this names, I'll update PR as soon as some decision about naming will be done.

---------------------------------------------------------------------------

by jmikola at 2011-12-21T02:29:59Z

@canni: My mistake, I got confused between the Token method and interface method, which you've since renamed in canni/symfony@fcfcd10.

---------------------------------------------------------------------------

by mvrhov at 2011-12-21T06:09:45Z

hm. Now I'm going to bike shed. Wouldn't the proper function name be hasUserChanged?

---------------------------------------------------------------------------

by stof at 2011-12-21T10:58:38Z

it would probably be bettter. The meaning of ``true`` and ``false`` would then be the opposite of the current ones but this is not an issue IMO as it is a different method

---------------------------------------------------------------------------

by jstout24 at 2011-12-27T18:08:49Z

@canni nice job

---------------------------------------------------------------------------

by fabpot at 2011-12-30T14:59:11Z

The method `isUserChanged()` must be rename. What about `hasUserChanged()` as @mvrhov suggested or `isUserDifferent()`?

---------------------------------------------------------------------------

by canni at 2012-01-02T11:44:05Z

@fabpot done.

---------------------------------------------------------------------------

by fabpot at 2012-01-02T18:13:40Z

The only missing thing I can think of is adding some unit tests.

---------------------------------------------------------------------------

by canni at 2012-01-10T20:16:25Z

@fabpot is there anything more you think that should done in this PR?

---------------------------------------------------------------------------

by stof at 2012-01-10T20:38:46Z

@canni can you rebase your branch ? it conflicts with the current master according to github

---------------------------------------------------------------------------

by canni at 2012-01-10T20:56:55Z

@stof done.

---------------------------------------------------------------------------

by fabpot at 2012-01-12T18:06:00Z

@canni: Can you just add some information in the CHANGELOG and in the UPGRADE file? That's all I need to merge this PR now. Thanks a lot.

---------------------------------------------------------------------------

by canni at 2012-01-12T18:16:32Z

@fabpot done, and no problem :)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment