Details
- Reviewers
amckinley - Maniphest Tasks
- T6703: Allow multiple copies of the same auth provider type
- Commits
- rP378a43d09c1f: Remove the highly suspect "Import from LDAP" workflow
Grepped for ldap/, grepped for controller.
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| 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. | |
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.
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.)
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.