Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prioritise entirely supported flows for UIA #3402

Merged
merged 5 commits into from
May 24, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions spec/unit/interactive-auth.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -517,4 +517,47 @@ describe("InteractiveAuth", () => {
expect(ia.getEmailSid()).toEqual(sid);
});
});

it("should prioritise shorter flows", async () => {
const doRequest = jest.fn();
const stateUpdated = jest.fn();

const ia = new InteractiveAuth({
matrixClient: getFakeClient(),
doRequest: doRequest,
stateUpdated: stateUpdated,
requestEmailToken: jest.fn(),
authData: {
session: "sessionId",
flows: [{ stages: [AuthType.Recaptcha, AuthType.Password] }, { stages: [AuthType.Password] }],
params: {},
},
});

// @ts-ignore
ia.chooseStage();
expect(ia.getChosenFlow()?.stages).toEqual([AuthType.Password]);
});

it("should prioritise flows with entirely supported stages", async () => {
const doRequest = jest.fn();
const stateUpdated = jest.fn();

const ia = new InteractiveAuth({
matrixClient: getFakeClient(),
doRequest: doRequest,
stateUpdated: stateUpdated,
requestEmailToken: jest.fn(),
authData: {
session: "sessionId",
flows: [{ stages: ["com.devture.shared_secret_auth"] }, { stages: [AuthType.Password] }],
params: {},
},
supportedStages: [AuthType.Password],
});

// @ts-ignore
ia.chooseStage();
expect(ia.getChosenFlow()?.stages).toEqual([AuthType.Password]);
});
});
29 changes: 25 additions & 4 deletions src/interactive-auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ const EMAIL_STAGE_TYPE = "m.login.email.identity";
const MSISDN_STAGE_TYPE = "m.login.msisdn";

export interface UIAFlow {
stages: AuthType[];
stages: Array<AuthType | string>;
}

export interface IInputs {
Expand Down Expand Up @@ -156,6 +156,11 @@ interface IOpts {
*/
emailSid?: string;

/**
* If specified will prefer flows which entirely consist of supported stages to avoid needing the fallback mechanism
t3chguy marked this conversation as resolved.
Show resolved Hide resolved
*/
supportedStages?: string[];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this not an AuthType[] ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
supportedStages?: string[];
supportedStages?: Array<AuthType | string>;

Because the app may support custom types which the js-sdk does not know about

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. It might be nice to say in the comment "These should normally be AuthTypes, but may be strings if the application supports custom stages"?


/**
* Called with the new auth dict to submit the request.
* Also passes a second deprecated arg which is a flag set to true if this request is a background request.
Expand All @@ -176,7 +181,7 @@ interface IOpts {
* m.login.email.identity:
* * emailSid: string, the sid of the active email auth session
*/
stateUpdated(nextStage: AuthType, status: IStageStatus): void;
stateUpdated(nextStage: AuthType | string, status: IStageStatus): void;

/**
* A function that takes the email address (string), clientSecret (string), attempt number (int) and
Expand Down Expand Up @@ -216,6 +221,7 @@ export class InteractiveAuth {
private readonly busyChangedCallback?: IOpts["busyChanged"];
private readonly stateUpdatedCallback: IOpts["stateUpdated"];
private readonly requestEmailTokenCallback: IOpts["requestEmailToken"];
private readonly supportedStages?: Set<string>;

private data: IAuthData;
private emailSid?: string;
Expand Down Expand Up @@ -243,6 +249,7 @@ export class InteractiveAuth {
if (opts.sessionId) this.data.session = opts.sessionId;
this.clientSecret = opts.clientSecret || this.matrixClient.generateClientSecret();
this.emailSid = opts.emailSid;
if (opts.supportedStages) this.supportedStages = new Set(opts.supportedStages);
t3chguy marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand Down Expand Up @@ -571,7 +578,7 @@ export class InteractiveAuth {
* @returns login type
* @throws {@link NoAuthFlowFoundError} If no suitable authentication flow can be found
*/
private chooseStage(): AuthType | undefined {
private chooseStage(): AuthType | string | undefined {
if (this.chosenFlow === null) {
this.chosenFlow = this.chooseFlow();
}
Expand All @@ -581,6 +588,16 @@ export class InteractiveAuth {
return nextStage;
}

// Returns a low number for flows we consider best, counts increase for longer flows and even more so
t3chguy marked this conversation as resolved.
Show resolved Hide resolved
// for flows which contain stages we do not have built-in support for.
t3chguy marked this conversation as resolved.
Show resolved Hide resolved
private scoreFlow(flow: UIAFlow): number {
let score = flow.stages.length;
if (this.supportedStages) {
t3chguy marked this conversation as resolved.
Show resolved Hide resolved
score += flow.stages.filter((stage) => !this.supportedStages!.has(stage)).length * 10;
t3chguy marked this conversation as resolved.
Show resolved Hide resolved
}
return score;
}

/**
* Pick one of the flows from the returned list
* If a flow using all of the inputs is found, it will
Expand All @@ -603,6 +620,10 @@ export class InteractiveAuth {
const haveEmail = Boolean(this.inputs.emailAddress) || Boolean(this.emailSid);
const haveMsisdn = Boolean(this.inputs.phoneCountry) && Boolean(this.inputs.phoneNumber);

// Flows are not represented in a significant order, so we can choose any we support best
// Sort flows based on how many unsupported stages they contain ascending
flows.sort((a, b) => this.scoreFlow(a) - this.scoreFlow(b));

for (const flow of flows) {
let flowHasEmail = false;
let flowHasMsisdn = false;
Expand Down Expand Up @@ -633,7 +654,7 @@ export class InteractiveAuth {
* @internal
* @returns login type
*/
private firstUncompletedStage(flow: UIAFlow): AuthType | undefined {
private firstUncompletedStage(flow: UIAFlow): AuthType | string | undefined {
const completed = this.data.completed || [];
return flow.stages.find((stageType) => !completed.includes(stageType));
}
Expand Down