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.

Sign up for GitHub

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

Jump to bottom

registration: add function to re-request email token #2357

Merged
merged 2 commits into from
May 11, 2022
Merged

registration: add function to re-request email token #2357

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
58a0a7e
registration: add function to re-request email token
justjanne May 10, 2022
8e1f2f3
registration: add tests for re-request email token function
justjanne May 11, 2022
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
105 changes: 105 additions & 0 deletions spec/unit/interactive-auth.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ limitations under the License.
import { logger } from "../../src/logger";
import { InteractiveAuth } from "../../src/interactive-auth";
import { MatrixError } from "../../src/http-api";
import { sleep } from "../../src/utils";
import { randomString } from "../../src/randomstring";

// Trivial client object to test interactive auth
// (we do not need TestClient here)
Expand Down Expand Up @@ -172,4 +174,107 @@ describe("InteractiveAuth", function() {
expect(error.message).toBe('No appropriate authentication flow found');
});
});

describe("requestEmailToken", () => {
it("increases auth attempts", async () => {
const doRequest = jest.fn();
const stateUpdated = jest.fn();
const requestEmailToken = jest.fn();
requestEmailToken.mockImplementation(async () => ({ sid: "" }));

const ia = new InteractiveAuth({
matrixClient: new FakeClient(),
doRequest, stateUpdated, requestEmailToken,
});

await ia.requestEmailToken();
expect(requestEmailToken).toHaveBeenLastCalledWith(undefined, ia.getClientSecret(), 1, undefined);
requestEmailToken.mockClear();
await ia.requestEmailToken();
expect(requestEmailToken).toHaveBeenLastCalledWith(undefined, ia.getClientSecret(), 2, undefined);
requestEmailToken.mockClear();
await ia.requestEmailToken();
expect(requestEmailToken).toHaveBeenLastCalledWith(undefined, ia.getClientSecret(), 3, undefined);
requestEmailToken.mockClear();
await ia.requestEmailToken();
expect(requestEmailToken).toHaveBeenLastCalledWith(undefined, ia.getClientSecret(), 4, undefined);
requestEmailToken.mockClear();
await ia.requestEmailToken();
expect(requestEmailToken).toHaveBeenLastCalledWith(undefined, ia.getClientSecret(), 5, undefined);
});

it("increases auth attempts", async () => {
const doRequest = jest.fn();
const stateUpdated = jest.fn();
const requestEmailToken = jest.fn();
requestEmailToken.mockImplementation(async () => ({ sid: "" }));

const ia = new InteractiveAuth({
matrixClient: new FakeClient(),
doRequest, stateUpdated, requestEmailToken,
});

await ia.requestEmailToken();
expect(requestEmailToken).toHaveBeenLastCalledWith(undefined, ia.getClientSecret(), 1, undefined);
requestEmailToken.mockClear();
await ia.requestEmailToken();
expect(requestEmailToken).toHaveBeenLastCalledWith(undefined, ia.getClientSecret(), 2, undefined);
requestEmailToken.mockClear();
await ia.requestEmailToken();
expect(requestEmailToken).toHaveBeenLastCalledWith(undefined, ia.getClientSecret(), 3, undefined);
requestEmailToken.mockClear();
await ia.requestEmailToken();
expect(requestEmailToken).toHaveBeenLastCalledWith(undefined, ia.getClientSecret(), 4, undefined);
requestEmailToken.mockClear();
await ia.requestEmailToken();
expect(requestEmailToken).toHaveBeenLastCalledWith(undefined, ia.getClientSecret(), 5, undefined);
});

it("passes errors through", async () => {
const doRequest = jest.fn();
const stateUpdated = jest.fn();
const requestEmailToken = jest.fn();
requestEmailToken.mockImplementation(async () => {
throw new Error("unspecific network error");
});

const ia = new InteractiveAuth({
matrixClient: new FakeClient(),
doRequest, stateUpdated, requestEmailToken,
});

expect(async () => await ia.requestEmailToken()).rejects.toThrowError("unspecific network error");
});

it("only starts one request at a time", async () => {
const doRequest = jest.fn();
const stateUpdated = jest.fn();
const requestEmailToken = jest.fn();
requestEmailToken.mockImplementation(() => sleep(500, { sid: "" }));

const ia = new InteractiveAuth({
matrixClient: new FakeClient(),
doRequest, stateUpdated, requestEmailToken,
});

await Promise.all([ia.requestEmailToken(), ia.requestEmailToken(), ia.requestEmailToken()]);
expect(requestEmailToken).toHaveBeenCalledTimes(1);
});

it("stores result in email sid", async () => {
const doRequest = jest.fn();
const stateUpdated = jest.fn();
const requestEmailToken = jest.fn();
const sid = randomString(24);
requestEmailToken.mockImplementation(() => sleep(500, { sid }));

const ia = new InteractiveAuth({
matrixClient: new FakeClient(),
doRequest, stateUpdated, requestEmailToken,
});

await ia.requestEmailToken();
expect(ia.getEmailSid()).toEqual(sid);
});
});
});
51 changes: 32 additions & 19 deletions src/interactive-auth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,8 @@ export class InteractiveAuth {
private chosenFlow: IFlow = null;
private currentStage: string = null;

private emailAttempt = 1;

// if we are currently trying to submit an auth dict (which includes polling)
// the promise the will resolve/reject when it completes
private submitPromise: Promise<void> = null;
Expand Down Expand Up @@ -408,6 +410,34 @@ export class InteractiveAuth {
this.emailSid = sid;
}

/**
* Requests a new email token and sets the email sid for the validation session
*/
public requestEmailToken = async () => {
Copy link
Member

Choose a reason for hiding this comment

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

out of interest is there any reason this is an arrow func?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to use this in the function, and while I personally would prefer to use a regular function and binding it in the constructor, across the codebase I’ve found arrow functions to be used more commonly to accomplish the same.

Copy link
Member

Choose a reason for hiding this comment

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

Sure but the function is always called with the correct this context already (L493) so would work in either way. Its not being copied to lose its bound context anywhere yet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But it is? It’s a public function, so it can be called with any this context. And it is called like with a different context as onClick handler in matrix-org/matrix-react-sdk#8554.

Copy link
Member

Choose a reason for hiding this comment

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

The SDK as a whole makes the assumption that the caller will always call with the correct this context, we tend to use arrow funcs for when we want to use it as a local handler. Arrow functions here are not wrong by any means, was just curious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that’s an interesting assumption! Good to know, I’ve never seen a codebase that assumed this before.

Copy link
Member

Choose a reason for hiding this comment

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

It is very much a bad assumption to have, but one we make anyway. If you feel strongly about it we could make a rule that all public methods have to be arrow functions to remedy this

if (!this.requestingEmailToken) {
logger.trace("Requesting email token. Attempt: " + this.emailAttempt);
// If we've picked a flow with email auth, we send the email
// now because we want the request to fail as soon as possible
// if the email address is not valid (ie. already taken or not
// registered, depending on what the operation is).
this.requestingEmailToken = true;
try {
const requestTokenResult = await this.requestEmailTokenCallback(
this.inputs.emailAddress,
this.clientSecret,
this.emailAttempt++,
this.data.session,
);
this.emailSid = requestTokenResult.sid;
logger.trace("Email token request succeeded");
} finally {
this.requestingEmailToken = false;
}
} else {
logger.warn("Could not request email token: Already requesting");
}
};

/**
* Fire off a request, and either resolve the promise, or call
* startAuthStage.
Expand Down Expand Up @@ -458,24 +488,9 @@ export class InteractiveAuth {
return;
}

if (
!this.emailSid &&
!this.requestingEmailToken &&
this.chosenFlow.stages.includes(AuthType.Email)
) {
// If we've picked a flow with email auth, we send the email
// now because we want the request to fail as soon as possible
// if the email address is not valid (ie. already taken or not
// registered, depending on what the operation is).
this.requestingEmailToken = true;
if (!this.emailSid && this.chosenFlow.stages.includes(AuthType.Email)) {
try {
const requestTokenResult = await this.requestEmailTokenCallback(
this.inputs.emailAddress,
this.clientSecret,
1, // TODO: Multiple send attempts?
this.data.session,
);
this.emailSid = requestTokenResult.sid;
await this.requestEmailToken();
// NB. promise is not resolved here - at some point, doRequest
// will be called again and if the user has jumped through all
// the hoops correctly, auth will be complete and the request
Expand All @@ -491,8 +506,6 @@ export class InteractiveAuth {
// send the email, for whatever reason.
this.attemptAuthDeferred.reject(e);
this.attemptAuthDeferred = null;
} finally {
this.requestingEmailToken = false;
}
}
}
Expand Down