Skip to content

Commit 0bb6ab6

Browse files
committed
[server/auth] avoid creating duplicate accounts
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.
1 parent b0f6b66 commit 0bb6ab6

File tree

3 files changed

+41
-5
lines changed

3 files changed

+41
-5
lines changed

components/server/src/auth/generic-auth-provider.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -453,6 +453,7 @@ export class GenericAuthProvider implements AuthProvider {
453453
envVars,
454454
additionalIdentity: githubIdentity,
455455
additionalToken: githubToken,
456+
authHost: this.host,
456457
isBlocked
457458
}
458459
}

components/server/src/terms/tos-flow.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,10 @@ import { saveSession } from "../express-util";
1010

1111

1212
export interface TosFlow {
13+
flowId?: string;
1314
termsAcceptanceRequired?: boolean;
1415
isBlocked?: boolean;
16+
authHost?: string;
1517
}
1618
export namespace TosFlow {
1719
export function is(data?: any): data is TosFlow {
@@ -33,7 +35,6 @@ export namespace TosFlow {
3335
export interface WithUser extends TosFlow {
3436
user: User;
3537
elevateScopes?: string[] | undefined;
36-
authHost?: string;
3738
returnToUrl?: string;
3839
}
3940
export namespace WithUser {

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

Lines changed: 38 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import { LoginCompletionHandler } from "../auth/login-completion-handler";
2727
import { TosCookie } from "./tos-cookie";
2828
import { TosFlow } from "../terms/tos-flow";
2929
import { increaseLoginCounter } from '../../src/prometheusMetrics';
30+
import * as uuidv4 from 'uuid/v4';
3031

3132
@injectable()
3233
export class UserController {
@@ -302,12 +303,17 @@ export class UserController {
302303
user: User.censor(user),
303304
returnToUrl: req.query.returnTo
304305
};
305-
await TosFlow.attach(req.session!, tosFlowInfo);
306306
}
307307

308+
// attaching a random identifier for this web flow to test if it's present in `/tos/proceed` handler
309+
const flowId = uuidv4();
310+
tosFlowInfo.flowId = flowId;
311+
await TosFlow.attach(req.session!, tosFlowInfo);
312+
308313
const isUpdate = !TosFlow.WithIdentity.is(tosFlowInfo);
309314
const userInfo = tosFlowUserInfo(tosFlowInfo);
310315
const tosHints = {
316+
flowId,
311317
isUpdate, // indicate whether to show the "we've updated ..." message
312318
userInfo // let us render the avatar on the dashboard page
313319
};
@@ -356,6 +362,21 @@ export class UserController {
356362
return;
357363
}
358364

365+
// detaching the (random) identifier of this webflow
366+
const flowId = tosFlowInfo.flowId;
367+
delete tosFlowInfo.flowId;
368+
await TosFlow.attach(req.session!, tosFlowInfo);
369+
370+
// let's assume if the form is re-submitted a second time, we need to abort the process, because
371+
// otherwise we potentially create accounts for the same provider identity twice.
372+
//
373+
// todo@alex: check if it's viable to test the flow ids for a single submission, instead of detaching
374+
// from the session.
375+
if (typeof flowId !== "string") {
376+
await redirectOnInvalidSession();
377+
return;
378+
}
379+
359380
const agreeTOS = req.body.agreeTOS;
360381
if (!agreeTOS) {
361382
// The user did not accept the terms.
@@ -364,7 +385,7 @@ export class UserController {
364385
log.info(logContext, '(TOS) User did NOT agree. Redirecting to /logout.', logPayload);
365386

366387
res.redirect(this.env.hostUrl.withApi({ pathname: "/logout" }).toString());
367-
// todo@alex: consider redirecting to a description pages afterwards (returnTo param)
388+
// todo@alex: consider redirecting to a info page (returnTo param)
368389

369390
return;
370391
}
@@ -377,8 +398,21 @@ export class UserController {
377398
await redirectOnInvalidSession();
378399
return;
379400
}
380-
await this.handleTosProceedForNewUser(req, res, authFlow, tosFlowInfo, req.body);
381-
} else if (TosFlow.WithUser.is(tosFlowInfo)) {
401+
402+
// there is a possibility, that a competing browser session already created a new user account
403+
// for this provider identity, thus we need to check again, in order to avoid created unreachable accounts
404+
const user = await this.userService.findUserForLogin({ candidate: tosFlowInfo.candidate });
405+
if (user) {
406+
log.info(`(TOS) User was created in a parallel browser session, let's login...`, { logPayload });
407+
await this.loginCompletionHandler.complete(req, res, { user, authHost: tosFlowInfo.authHost, returnToUrl: authFlow.returnTo });
408+
} else {
409+
await this.handleTosProceedForNewUser(req, res, authFlow, tosFlowInfo, req.body);
410+
}
411+
412+
return;
413+
}
414+
415+
if (TosFlow.WithUser.is(tosFlowInfo)) {
382416
const { user, returnToUrl } = tosFlowInfo;
383417

384418
await this.userService.acceptCurrentTerms(user);

0 commit comments

Comments
 (0)