Stop moving provider identities between account of a user #3032

AlexTugarev · 2021-01-27T04:33:19Z

With these change Gitpod stops moving identities between accounts of a user.

What's inside?

Access Control

On /access-control you now see the provider identity you've connected with, and you get the option to disconnect.

Confirm disconnecting a provider from your account:

Last (or the single) provider identity is a special one, and account deletion sounds is the way how to proceed, otherwise we end up with unreachable accounts.

Instead of moving provider identities between accounts.

Let's guide the user, and give them the options to select and manage accounts.

When we see that a provider identity is connected to a second account, we provide as much details as ~~possible~~necessary to let them decide on the next step.

in case it wasn't clear that two accounts were created, we provide names to these provider identities
in general we encourage to review the accounts and decide on a main account to proceed with, i.e. disconnect from providers or even let accounts go, if they aren't wanted.

in a situation where the browser agent might submit the terms form more than once, we should avoid creating new accounts. instead, we need to select the recently created account for a login in a parallel session.

AlexTugarev · 2021-01-29T12:52:39Z

components/dashboard/public/styles.css

@@ -826,7 +826,6 @@ footer .logo-icon {


 .access-control__card-container {
-    width: 31%;


fixing a bogus line wrap occurring with longer hostnames of self-hosted git providers.

AlexTugarev · 2021-01-29T12:54:57Z

components/dashboard/src/components/tos/terms-of-service.tsx

@@ -148,13 +158,15 @@ export class TermsOfService extends React.Component<TermsOfServiceProps, TermsOf
                                    variant='text'
                                    color={'secondary'}
                                    onClick={this.onDecline}
+                                    disabled={this.state.submitted}


to prevent resubmission if we have a long line to the proxy or dashboard, i.e. make it impossible to resubmit.

AlexTugarev · 2021-01-29T12:59:33Z

components/server/src/user/user-controller.ts

+                    log.info(`(TOS) User was created in a parallel browser session, let's login...`, { logPayload });
+                    await this.loginCompletionHandler.complete(req, res, { user, authHost: tosFlowInfo.authHost, returnToUrl: authFlow.returnTo });
+                } else {
+                    await this.handleTosProceedForNewUser(req, res, authFlow, tosFlowInfo, req.body);


fyi, this is the previous default code path

AlexTugarev · 2021-01-29T13:01:41Z

components/server/src/user/user-controller.ts

+                const user = await this.userService.findUserForLogin({ candidate: tosFlowInfo.candidate });
+                if (user) {
+                    log.info(`(TOS) User was created in a parallel browser session, let's login...`, { logPayload });
+                    await this.loginCompletionHandler.complete(req, res, { user, authHost: tosFlowInfo.authHost, returnToUrl: authFlow.returnTo });


fyi, this is new: if we retry searching for a user and find one, we definitely should not create new account, but login in into the existing one.

gtsiolis

Thanks for tackling this issue @AlexTugarev! It took me longer than expected to reproduce this on the preview environment but here're some thoughts on the UX here. Let me know what you think. 🏀

gtsiolis · 2021-01-29T12:52:36Z

components/server/src/terms/terms-provider.ts

 `,
    content: `
-# Before we proceed
+# Create a new Gitpod account with {{AUTH_HOST}}


suggestion: We're certainly going to improve this page later on but let me know if you think thee following seems like an improvement that can fit into this iteration. Feel free to leave this out of the scope of this PR and create a follow-up issue.

BEFORE AFTER ALTERNATIVE

We'll get to this later 🙏🏻

components/dashboard/src/components/access-control/access-control.tsx

gtsiolis · 2021-01-29T13:15:08Z

components/dashboard/src/components/access-control/access-control.tsx

+        const otherIdentitiesOfUser = user.identities.filter(i => i.authProviderId !== authProvider.authProviderId);
+        if (otherIdentitiesOfUser.length === 0) {
+            message = (<DialogContentText>
+                Disconnecting the single remaining provider would make your account unreachable. Please go the settings, if you want to delete the account.


question: Do you think it could be practical to offer account deletion in this step?

I'd rather not do this. Adding some extra cycles to avoid deleting by accident could be helpful in some cases.

Also, this is something I'd like to push to a follow-up discussion and PR.

Awesome! 🤝

gtsiolis · 2021-01-29T14:48:47Z

components/dashboard/src/proceed-with-account.tsx

+            <ApplicationFrame service={service}>
+                <div className='content content-area'>
+
+                    <h3>Cannot connect with {otherUser.authHost}</h3>


suggestion: What do you think of the following? We can keep the verbs like switch if you think this describes more accurate what's going on.

BEFORE AFTER

Created ${days} days ago and the recommendation on the Primary account is postponed. We'd discuss this separately.

For now we agreed on Other account, as we're missing some infos from server to do it.

OTOH, many thanks on the layout work, @gtsiolis! Awesome!

csweichel

Code-wise LGTM.

* allow our users to disconnect provider identities from their accounts * when a user tries to connect with a provider, for which there is already a connection to anther account, we redirect to an assistance page. a summary should help to review both accounts. in the end, any user may decide to move to a single account by disconnecting the provider identities from the other account. this way we can guarantee to not automagically lock out users from accounts with subscriptions or any meaningful data. * show `Connected as` * update terms renderer Signed-off-by: Alex Tugarev <[email protected]>

AlexTugarev · 2021-02-01T06:40:47Z

Testing once again before merging 🙏🏻

AlexTugarev linked an issue Jan 27, 2021 that may be closed by this pull request

Moving identities results in dead acounts #2629

Closed

AlexTugarev force-pushed the at/identities branch 2 times, most recently from 6acfca5 to 4b8a5c8 Compare January 27, 2021 11:40

AlexTugarev requested a review from gtsiolis January 27, 2021 13:47

AlexTugarev marked this pull request as ready for review January 27, 2021 13:48

AlexTugarev requested a review from csweichel January 28, 2021 08:13

AlexTugarev force-pushed the at/identities branch from 2cd22ec to ce6d7fa Compare January 29, 2021 10:25

AlexTugarev added 2 commits January 29, 2021 11:43

[terms/dashboard] avoid resubmitting twice

3673362

[server/auth] avoid creating duplicate accounts

f89fb4a

in a situation where the browser agent might submit the terms form more than once, we should avoid creating new accounts. instead, we need to select the recently created account for a login in a parallel session.

AlexTugarev force-pushed the at/identities branch 3 times, most recently from cb848b0 to 16c6d11 Compare January 29, 2021 11:22

AlexTugarev commented Jan 29, 2021

View reviewed changes

AlexTugarev requested a review from geropl January 29, 2021 13:02

AlexTugarev added this to the January 2021 milestone Jan 29, 2021

gtsiolis reviewed Jan 29, 2021

View reviewed changes

csweichel approved these changes Jan 29, 2021

View reviewed changes

AlexTugarev force-pushed the at/identities branch 4 times, most recently from 4ae48a1 to 4fb4a03 Compare January 29, 2021 16:56

AlexTugarev force-pushed the at/identities branch from 4fb4a03 to 2e95e8d Compare February 1, 2021 06:39

AlexTugarev merged commit e8adb24 into master Feb 1, 2021

AlexTugarev deleted the at/identities branch February 1, 2021 07:06

AlexTugarev mentioned this pull request Feb 1, 2021

Update manual tests #3067

Merged

AlexTugarev mentioned this pull request Feb 2, 2021

Improve "Create Account" and "Select Account" pages #3072

Closed

		@@ -826,7 +826,6 @@ footer .logo-icon {


		.access-control__card-container {
		width: 31%;

Stop moving provider identities between account of a user #3032

Stop moving provider identities between account of a user #3032

Uh oh!

Conversation

AlexTugarev commented Jan 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's inside?

Access Control

Instead of moving provider identities between accounts.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gtsiolis left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gtsiolis Jan 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csweichel left a comment

Choose a reason for hiding this comment

Uh oh!

AlexTugarev commented Feb 1, 2021 • edited by werft-gitpod-dev-com bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

AlexTugarev commented Jan 27, 2021 •

edited

Loading

gtsiolis left a comment •

edited

Loading

gtsiolis Jan 29, 2021 •

edited

Loading

AlexTugarev commented Feb 1, 2021 •

edited by werft-gitpod-dev-com bot

Loading