Page MenuHomePhabricator

Provide more flexible management over disabling/changing ldap user accounts
Open, Needs TriagePublic

Description

Our use case here is:

  • Everyone registers via ldap
  • Six months ago we merged our ldap registry into our parent organization, making it necessary for some users to now log in with a different ldap username
  • Now they have two accounts, one of which they can't access anymore

We cleverly went and manipulated records in the external accounts table so that the new ldap credentials would actually log people into their old accounts, I'm waiting for the day this causes some kind of account access armageddon, but that's not my primary concern here.

It would be much better instead, if one or many of the following things were possible:

  • There were some kind of script to wholesale "migrate" a user's content from one external/regular account to another
  • You could merge user accounts together
  • LDAP infrastructure had awareness of this scenario being possible
  • You could at least go in and edit the user profile for a disabled account and cover it with "this is not a real person, use this other account instead" messages

Event Timeline

To explain the hairiness of this a little better in our case, the actual "migration" procedure looks like this:

  • LDAP credentials and IDs were changed by our tech services team, uncoordinated with us (yay!)
  • At random, people would go to log into phabricator with their new username, over a wide stretch of time
  • Or not, maybe for some extended period of time they kept using the cookie from their old account
  • At some point they would message us and complain that they have a different account
  • Our best option is to pull up the external accounts table and change the userPHID of the new external account record to match their old user, so that when they log in moving forward, their new credentials log them into their old account

This leaves some users in a state where they have an arbitrary amount of stuff associated with either their "new" or "old" accounts and don't want it to be that way.

eadler added a subscriber: eadler.Feb 25 2016, 6:42 PM
yelirekim added a comment.EditedFeb 25 2016, 6:50 PM

Probably a really quick partial remedy to the fallout (not root cause) is just exploring ways to make it super obvious to people that they shouldn't be interacting with disabled accounts.

Some possible tools that may form part of a solution here, roughly in order from greatest upstream willingness to greatest upstream resistance:

Making Disabled Accounts More Obvious: My expectation is that this is currently fairly obvious, at least in most cases.

In typeaheads, disabled accounts should only show up if you type an exact match, and should be clearly marked as disabled. They don't show up at all with # autocompletion in remarkup. They don't show up in the main search dropdown. They should render in a "Disabled" state in most tokenizers, and with a "Disabled" grey bullet in handle lists.

Are there cases where this isn't obvious that we're currently missing? The only one that springs to mind is maybe Reviewers: ... from the CLI doesn't have a prompt. The newest iteration of the actual profile page is also maybe not as obvious as it used to be (I think we might have lost the "Disabled" badge), and maybe the newer hovercards could have a stronger callout.

Or are users hitting these cues and just plowing through them, and "super obvious" is, like, flashing lights, a siren, an MFA check, and three prompts where you have to sign the third one in blood?

To the degree that cases exist where a reasonable user can mistake a disabled account for an active one we'd like to fix them, I'm just not sure where these cases are right now.

Linking Multiple External Accounts: This is discussed in greater detail in T2549. Basically, it would let you link multiple LDAP accounts to one Phabricator account, so both alincoln and abrahaml LDAP credentials could be used to log into the same Phabricator account. This is a fair bit of work, but has good alignment with general technical sense, and is work I intend to do eventually. However, it sounds like it might not actually help here at all since accounts were just swapped, not merged additively?

(There's also the sort of partially-separate concern of allowing multiple LDAP providers, which is probably actually straightforward, but presumably totally irrelevant now.)

Some Kind of Profile Editing Script That Writes "IGNORE THIS ACCOUNT" on a Profile: I don't particularly want this in the upstream, but it's completely trivial to write and I'm happy to do that.

Formally Redirect Accounts: We could talk about doing this. I'm resistant to really commit to it fully, but we might be able to get many of the cases with a small amount of work. e.g., bin/auth redirect --from alincoln --to abrahaml makes sure alincoln is disabled, records the "forward" somewhere, and then the UI gives users stronger hints:

  • Profile has a big "go look at alincoln" banner.
  • Typeahead could have more colors/sirens when you try to select the old account?
  • Handle could render as "alincoln (now known as abrahaml)" or something?

But it seems like this only helps if the alincoln account isn't clearly communicated as disabled in the first place. So maybe none of this is necessary if we just shore up whatever's causing confusion there?

(You could get through some of this with a User custom field that just renders big flashing lights on the profile if the user is in some master list of redirected users.)

Content Merge: I'm strongly resistant to this since it's a ton of work and that it imposes a complexity tax on us forever for a feature which is essentially only useful for correcting "enterprise mistakes" (nee "Facebook Problems"; we ran into the same sorts of issues at Facebook when LDAP administrator begin reissuing usernames that had previously belonged to ex-employees (!!!!!)).

Basically, this is super gross, messy and hard to test, and makes developing new features harder forever (since we have to implement user merges and test that user merges work). And we can't really fake our way through this like we do with, say, bin/lipsum, since it's data-lossey and materially bad if this feature doesn't work correctly.

We can probably get most of the way pretty easily, but if you're OK with "most of the way" you can just dump the database, find/replace PHIDs in the raw text dump, and then load it. I think that'll probably work about as well as any partially committed effort we might make here, which is to say that you probably won't lose too much data.


Broadly, it's completely safe to go edit linkages in the external accounts table. From an upstream perspective, the ideal magical fairyland pathway here would have been:

  • We live in a time when T2549 + multiple LDAP providers has been implemented.
  • Tech services coordinates with you and gives you a list of local and global LDAP accounts, and the mapping between them, in advance of the switch/merge/whatever.
  • You add the global LDAP server as a second LDAP provider, and feed the map to some bin/auth script which links up all the global accounts to the same Phabricator accounts that the local accounts are currently linked to. At this point, users can either login with their "Division Credentials" or their "Parent Corporation Credentials" on the login screen, and both take them to the same Phabricator account.
  • In a timely and well-communicated fashion, you disable the local LDAP provider (or disable registration, then later disable login after some time has passed), so users can only use "Parent Corporation Credentials".
  • After a while, perhaps you go unlink all those dead local accounts and delete the local LDAP provider, purely as a matter of good housekeeping.

Obviously that didn't happen, but any indirect process which approximates that is fine/desirable and won't end in armageddon. The linkage between external and Phabricator accounts is intentionally simple and easy to rewrite, and rewriting it is safe and reasonable, in lieu of the ideal multi-phase link + unlink tools. You'll only get into trouble today if you accidentally link two external accounts from the same provider to a single Phabricator account, and can fix that by just fixing the link.

It sounds like that isn't terribly helpful, though.

On external account mangling in particular:

  • It's safe and reasonable to change which Phabricator account an LDAP account (or another external acccount) is linked to by updating userPHID in phabricator_user.user_externalaccount.
  • It's safe and reasonable to change which LDAP account is linked to a Phabricator account by changing accountID in the same table. This isn't completely clean since other fields like profileImagePHID may retain values from the old account, but it should fix itself the next time the user logs in, and if you did this right it should just be a different picture of the same user anyway.
  • The only peril here is if you link one external account to two Phabricator accounts (two rows with the same accountID) or one Phabricator account to two external accounts from the same provider (two rows with the same userPHID). The peril this creates is "minor glitches" peril, not "doom befalls you" peril, and can be fixed by correcting the mistake.

We could provide tools to facilitate this mangling, but my guess is that no two cases are likely to be similar and I'm not sure what tools you'd want. They'd probably boil down to bin/auth change-ldap-for-user --user alincoln --from alincoln --to abrahaml which would just run a SET accountID = ... query with the same parameters you typed. It could perform like one or two extra safety checks, but would pretty much be a paper-thin wrapper around doing it in SQL.

This is the script that we had been using to do this which looks like it aligns cleanly with your second bullet point.

1<?php
2
3class SwitchUserWorkflow extends IntegratorWorkflow {
4
5 protected function didConstruct() {
6 $this
7 ->setName('migrate-external-account')
8 ->setExamples('**migrate-external-account** --username-old __olduser__ '.
9 '--username-new __newuser__')
10 ->setSynopsis(pht(
11 'Associate a new user account with an old one so history is not lost. '.
12 'Only for external accounts.'))
13 ->setArguments([
14 [
15 'name' => 'username-old',
16 'param' => 'string',
17 'help' => pht('Old username that is no longer active.'),
18 ],
19 [
20 'name' => 'username-new',
21 'param' => 'string',
22 'help' => pht('New username for the user to use.'),
23 ],
24 ]);
25 }
26
27 public function execute(PhutilArgumentParser $args) {
28 $old_user = $this->requireArgument($args, 'username-old');
29 $new_user = $this->requireArgument($args, 'username-new');
30
31 $old_user_acct = id(new PhabricatorExternalAccount())
32 ->loadOneWhere('accountId = %s', $old_user);
33
34 $new_user_acct = id(new PhabricatorExternalAccount())
35 ->loadOneWhere('accountID = %s', $new_user);
36
37 $console = PhutilConsole::getConsole();
38
39 if (!$old_user_acct) {
40 throw $this->argumentUsageException('%s does not exist.', $old_user);
41 }
42
43 if (!$new_user_acct) {
44 throw $this->argumentUsageException('%s does not exist.', $new_user);
45 }
46
47 // set the new user account to point to the old user account id
48 $old_user_phid = $old_user_acct->getUserPHID();
49 $new_user_acct
50 ->setUserPHID($old_user_phid)
51 ->save();
52
53 $console->writeOut(pht(
54 "%s has been migrated to %s\n",
55 $old_user,
56 $new_user));
57 }
58
59}

It looks like that can leave two external (LDAP) accounts pointed at the same Phabricator account. If that's working, great. It will be supported explicitly after T2549.

I'm a little surprised it's working and I'd expect some level of glitchiness in the UI at least (e.g., not being able to manage both accounts in "External Accounts"?) -- one earlier issue is documented in T6707. But maybe whatever glitchiness is present doesn't really matter too much.

They do lose access to managing both accounts which has caused a few issues around email addresses. It looks like we could have reduced massive pain on ourselves by letting users unlink ldap accounts and instructing them to link the new account.

cburroughs added a project: Restricted Project.Apr 20 2016, 3:25 PM
cburroughs moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Apr 20 2016, 3:56 PM
pasik added a subscriber: pasik.Jan 30 2017, 3:09 PM
epriestley moved this task from Backlog to LDAP is Special on the Auth board.Dec 12 2018, 7:46 PM