Page MenuHomePhabricator

Remove the highly suspect "Import from LDAP" workflow
ClosedPublic

Authored by epriestley on Feb 6 2019, 11:19 PM.

Details

Summary

Depends on D20109. Ref T6703. This flow was contributed in 2012 and I'm not sure it ever worked, or at least ever worked nondestructively. For now, get rid of it. We'll do importing and external sync properly at some point (T3980, T13190).

Test Plan

Grepped for ldap/, grepped for controller.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Feb 6 2019, 11:19 PM
epriestley requested review of this revision.Feb 6 2019, 11:21 PM
epriestley added inline comments.Feb 6 2019, 11:21 PM
src/applications/people/controller/PhabricatorPeopleLdapController.php
93–98

This manual, unconditional construction of external accounts without a related ProviderConfig is the main thing I'm trying to get rid of. I could associate them with the provider instead, but I'm not convinced I can actually test LDAP.

FWIW, I definitely used this import flow in a previous life and it worked for me. I'd like to have some other technique for bulk user creation before totally blowing this code away.

This workflow can fail and/or do dangerous things in what seems like a lot of cases (see inlines). Given that we haven't really seen reports about it, I suspect it's seeing very little use in the wild?

You can currently bulk-create users with scripts/user/add_user.php, although this does not link accounts to providers.

Do you think we should build T5953 before removing this? I worry that's a very large amount of work.

I'd expect self-registration to be reasonable in most cases -- I think the UX usually shouldn't be too much different in most cases except that you can't @mention other users right away. Was there some context where self-registration wouldn't have been a reasonable approach, and bulk import was required?

src/applications/people/controller/PhabricatorPeopleLdapController.php
67

Email addresses are read from the client, so anyone who can use this workflow can associate any username with any email address.

82

This does not validate usernames. Invalid LDAP usernames will fail during user creation with no ability to correct the error.

85–87

Email addresses are verified unconditionally, so any user with access to this workflow can launch a private LDAP server and verify control of any email address. Because we trust the client to provide the email address, they don't have to launch an LDAP server: they can just submit whatever they want.

98

This may fail after the save() on line 91 succeeds. This will leave the user with a User record and no ExternalAccount record. This can't be fixed because the save() on line 91 will fail next time, since a user with the username already exists.

amckinley accepted this revision.Feb 7 2019, 7:56 PM

Do you think we should build T5953 before removing this? I worry that's a very large amount of work.

Probably not. I didn't realize how actually broken this was; I'm ok with nuking it now.

Was there some context where self-registration wouldn't have been a reasonable approach, and bulk import was required?

Users can never be trusted to maintain username consistency on their own. I wish I had a dollar for every script I've written to reconcile usernames across different suites of internal tools.

This revision is now accepted and ready to land.Feb 7 2019, 7:56 PM

Historically, we had various kinds of "you aren't allowed to change your username" stuff, although I think much of it fell by the wayside. account.editable may have actually extended all the way to username registration at some point? And there were some very old event hooks for mangling accounts -- actually, it looks like TYPE_AUTH_WILLREGISTERUSER still exists.

So it's posssible to listen for TYPE_AUTH_WILLREGISTERUSER, prevent username changes in the event hook, and effectively enforce that your Phabricator username is your LDAP username even with self-registration.

However, this is quite a mess and it looks like it has only come up a couple times in the last several years (T10700, T11716) and at least one of those was LDAP and the import workflow presumably wasn't a good fit so who knows.

(I do want actual import tools at some point, but ideally not today.)

This revision was automatically updated to reflect the committed changes.

Very very disappointed. "Import from LDAP" worked very well and we use that almost every week for importing newly added employees. Though it's ok that employees register themself, but it changed our existing workflow in our company.