Support login by email even when not using UUID userids fixes #26

[gyst]

It took me a dozen hours to prove this one-line fix :-(.

Summary: when using email logins without UUID, as is required when working with LDAP, dexterity.membrane breaks. Users are not enumerated, because the enumeration queries on exact_getUserId, which relies on the indexed return value of getUserId, which returns the email address instead of the actual userid. To further complicate matters, the userid is stored in a field called username but the username (login name) in this scenario is actually the email address stored in the email field.

[mauritsvanrees]

Current status: confused. But that has more to do with the current code than with your pull request.

Your change makes sense. The user id should be stable, preferably even not change at all ever. So linking it to a changeable email address is not good. So pick a user id once and stick to it.

My confusion:

• Where is self.context.username ever set in our current code? It looks like this does not happen. On a current test Plone 5 site with the default dexterity.membrane settings (use uuid as id, and email as login), a test user works fine when I look at the Users and Groups panel. When I change email as login to False, the Users and Groups panel fails: well, the getUserName call fails with an AttributeError on self.context.username. This error is then swallowed by PAS. Using self.context.getId() seems better then.
• Do we really have settings for uuid and email login in both the standard Security control panel and the dexterity.membrane control panel? It may make sense, but I currently don't quite see it. Well, I guess in some cases you want different settings...
• I wonder if we call any code when our settings change. I am pretty sure Plone itself does that when the Security panel options change.

I haven't looked at the git history yet to see if that makes things clearer.

Okay, change in current status: going home before it starts raining. :-)

[gyst]

Some extra info:

• username is not actually defined in the dx.membrane codebase. That's #19

• The "right" way would be to actually replace any improper username with userid. However: downstream code implementations (like ploneintranet) actually depend on the current faulty semantics. So that would imply a major version bump.

• The reason downstream implementations use the provided "example" behavior is that this code is rather complex, with both a behavior and an adapter using the same base class, and registering replacements for both downstream results in the weird situation that the dx.membrane variants are still being called instead of the downstream subclasses. Appartently there's some lookup happening within dx.membrane that defies refining the implementation downstream. Have not fully investigated.

• To further muddy the waters, I've been trying as part of this issue to move the whole ploneintranet code base over from username as id to userid. This then swiftly runs into the problem that dx.membrane is not the only code base to suffer semantic confusion between username and userid. Just look at the plone.api.user API contracts where only .get() supports both userid and username but all other methods use username for what semantically is the user id.

• Yes we really have settings in both dx.membrane and the site control panel, and they control different things. It beats me why one would want those to diverge, except:

• Yes the security control panel option has a subscriber. Which, when I ticked the checkbox and hit 'save', had the horrific effect of starting to move all my user profiles to new object ids. That was before I developed this PR, so the fact that this PR makes getUserId() far more stable may fix that, haven't tested it. But it was pretty scary.

So, in summary: the confusion between userid and username is a case of deep rottenness in the Plone code base.

I don't think that's fixable. Even in the context of the dx.membrane schema, the actual getUserName() may be the email address while the actual getUserId() may be the UUID, leaving any field which is called either userid or username in a limbo where toggling a registry setting renders it semantically incorrect. Indeed, in the default config both getters bypass schema attributes completely, which explains how this code base can exist without actually having a username attribute.

This PR is essentially a one-line fix, where getUserId() defaults to returning username instead of email. While that may not be perfect, it's clearly superior to the existing situation.

[gyst]

Hold the merge. After doing some more code auditing, I'm reaching the conclusion we should just return self.context.id or self.context.getId() instead of self.context.username.

As to changing settings, that requires a reindex of the profiles. I should add utility methods for that.

[gyst]

OK this is now ready for merge. Username and Userid accessors are now completely decoupled.

[gyst]

As to adding migration utility calls: I've wasted enough of my life in this rabbithole by now :-(. Maybe in another life...

[mauritsvanrees]

Perfect! Thanks a lot. I will merge manually, so I don't include the internal version bumps.

I will add a note in the readme that the user may need to reindex manually.

Also, some of the tests don't work on Python 2.6. I will add a note that we drop support for that officially. Unofficially it may still work: the non-test code should still be fine. But we are only running Travis tests on Python 2.7.

[mauritsvanrees]

Merged. I have released dexterity.membrane 1.2 with this.