From 3f6f5b69c7a10145dc562f3b470385b195ef71c9 Mon Sep 17 00:00:00 2001 From: 3nprob <74199244+3nprob@users.noreply.github.com> Date: Thu, 11 Aug 2022 14:29:53 +0000 Subject: [PATCH] Improve test coverage and modernize style for interactive-auth (#2574) * style: address no-mixed-operators errors,minor style improvements * test: Fix async interactive-auth tests, add test case * tests: Fix incorrectly stringified mock response * pushprocessor: style update * use async primitives in interactive-auth-spec * lint * fixup: remove duplicate test * add test case for no-flow-with-session for interactive-auth * interactive-auth: handle non-existing error.data * async test fix * test: add dummyauth test * add testing for errcode * Revert "pushprocessor: style update" This reverts commit 3ed0fdfb73ae55b725aa7c74d9cab35fb96c9178. * add testcase for missing error data * test: move sessionId assignment * Add tests to improve coverage for interactive-auth * pushprocessor: style update --- spec/unit/autodiscovery.spec.ts | 35 ++-- spec/unit/interactive-auth.spec.ts | 318 ++++++++++++++++++++++++++--- src/autodiscovery.ts | 52 ++--- src/interactive-auth.ts | 28 ++- src/pushprocessor.ts | 10 +- src/utils.ts | 30 +-- src/webrtc/call.ts | 4 +- 7 files changed, 374 insertions(+), 103 deletions(-) diff --git a/spec/unit/autodiscovery.spec.ts b/spec/unit/autodiscovery.spec.ts index 71b48ef41..939f47797 100644 --- a/spec/unit/autodiscovery.spec.ts +++ b/spec/unit/autodiscovery.spec.ts @@ -153,27 +153,26 @@ describe("AutoDiscovery", function() { ]); }); - it("should return FAIL_PROMPT when .well-known returns not-JSON", function() { + it("should return FAIL_PROMPT when .well-known returns not-JSON", async () => { const httpBackend = getHttpBackend(); - httpBackend.when("GET", "/.well-known/matrix/client").respond(200, "abc"); + httpBackend.when("GET", "/.well-known/matrix/client").respond(200, "abc", true); + const expected = { + "m.homeserver": { + state: "FAIL_PROMPT", + error: AutoDiscovery.ERROR_INVALID, + base_url: null, + }, + "m.identity_server": { + state: "PROMPT", + error: null, + base_url: null, + }, + }; return Promise.all([ httpBackend.flushAllExpected(), - AutoDiscovery.findClientConfig("example.org").then((conf) => { - const expected = { - "m.homeserver": { - state: "FAIL_PROMPT", - error: AutoDiscovery.ERROR_INVALID, - base_url: null, - }, - "m.identity_server": { - state: "PROMPT", - error: null, - base_url: null, - }, - }; - - expect(conf).toEqual(expected); - }), + AutoDiscovery.findClientConfig("example.org").then( + expect(expected).toEqual, + ), ]); }); diff --git a/spec/unit/interactive-auth.spec.ts b/spec/unit/interactive-auth.spec.ts index b21a9296a..8005f1179 100644 --- a/spec/unit/interactive-auth.spec.ts +++ b/spec/unit/interactive-auth.spec.ts @@ -32,8 +32,8 @@ class FakeClient { const getFakeClient = (): MatrixClient => new FakeClient() as unknown as MatrixClient; -describe("InteractiveAuth", function() { - it("should start an auth stage and complete it", function() { +describe("InteractiveAuth", () => { + it("should start an auth stage and complete it", async () => { const doRequest = jest.fn(); const stateUpdated = jest.fn(); @@ -59,7 +59,7 @@ describe("InteractiveAuth", function() { }); // first we expect a call here - stateUpdated.mockImplementation(function(stage) { + stateUpdated.mockImplementation((stage) => { logger.log('aaaa'); expect(stage).toEqual(AuthType.Password); ia.submitAuthDict({ @@ -69,23 +69,130 @@ describe("InteractiveAuth", function() { // .. which should trigger a call here const requestRes = { "a": "b" }; - doRequest.mockImplementation(function(authData) { + doRequest.mockImplementation(async (authData) => { logger.log('cccc'); expect(authData).toEqual({ session: "sessionId", type: AuthType.Password, }); - return Promise.resolve(requestRes); + return requestRes; }); - return ia.attemptAuth().then(function(res) { - expect(res).toBe(requestRes); - expect(doRequest).toBeCalledTimes(1); - expect(stateUpdated).toBeCalledTimes(1); - }); + const res = await ia.attemptAuth(); + expect(res).toBe(requestRes); + expect(doRequest).toBeCalledTimes(1); + expect(stateUpdated).toBeCalledTimes(1); }); - it("should make a request if no authdata is provided", function() { + it("should handle auth errcode presence ", 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.Password] }, + ], + errcode: "MockError0", + params: { + [AuthType.Password]: { param: "aa" }, + }, + }, + }); + + expect(ia.getSessionId()).toEqual("sessionId"); + expect(ia.getStageParams(AuthType.Password)).toEqual({ + param: "aa", + }); + + // first we expect a call here + stateUpdated.mockImplementation((stage) => { + logger.log('aaaa'); + expect(stage).toEqual(AuthType.Password); + ia.submitAuthDict({ + type: AuthType.Password, + }); + }); + + // .. which should trigger a call here + const requestRes = { "a": "b" }; + doRequest.mockImplementation(async (authData) => { + logger.log('cccc'); + expect(authData).toEqual({ + session: "sessionId", + type: AuthType.Password, + }); + return requestRes; + }); + + const res = await ia.attemptAuth(); + expect(res).toBe(requestRes); + expect(doRequest).toBeCalledTimes(1); + expect(stateUpdated).toBeCalledTimes(1); + }); + + it("should handle set emailSid for email flow", async () => { + const doRequest = jest.fn(); + const stateUpdated = jest.fn(); + const requestEmailToken = jest.fn(); + + const ia = new InteractiveAuth({ + doRequest, + stateUpdated, + requestEmailToken, + matrixClient: getFakeClient(), + emailSid: 'myEmailSid', + authData: { + session: "sessionId", + flows: [ + { stages: [AuthType.Email, AuthType.Password] }, + ], + params: { + [AuthType.Email]: { param: "aa" }, + [AuthType.Password]: { param: "bb" }, + }, + }, + }); + + expect(ia.getSessionId()).toEqual("sessionId"); + expect(ia.getStageParams(AuthType.Email)).toEqual({ + param: "aa", + }); + + // first we expect a call here + stateUpdated.mockImplementation((stage) => { + logger.log('husky'); + expect(stage).toEqual(AuthType.Email); + ia.submitAuthDict({ + type: AuthType.Email, + }); + }); + + // .. which should trigger a call here + const requestRes = { "a": "b" }; + doRequest.mockImplementation(async (authData) => { + logger.log('barfoo'); + expect(authData).toEqual({ + session: "sessionId", + type: AuthType.Email, + }); + return requestRes; + }); + + const res = await ia.attemptAuth(); + expect(res).toBe(requestRes); + expect(doRequest).toBeCalledTimes(1); + expect(stateUpdated).toBeCalledTimes(1); + expect(requestEmailToken).toBeCalledTimes(0); + expect(ia.getEmailSid()).toBe("myEmailSid"); + }); + + it("should make a request if no authdata is provided", async () => { const doRequest = jest.fn(); const stateUpdated = jest.fn(); const requestEmailToken = jest.fn(); @@ -101,7 +208,7 @@ describe("InteractiveAuth", function() { expect(ia.getStageParams(AuthType.Password)).toBe(undefined); // first we expect a call to doRequest - doRequest.mockImplementation(function(authData) { + doRequest.mockImplementation((authData) => { logger.log("request1", authData); expect(authData).toEqual(null); // first request should be null const err = new MatrixError({ @@ -119,7 +226,7 @@ describe("InteractiveAuth", function() { // .. which should be followed by a call to stateUpdated const requestRes = { "a": "b" }; - stateUpdated.mockImplementation(function(stage) { + stateUpdated.mockImplementation((stage) => { expect(stage).toEqual(AuthType.Password); expect(ia.getSessionId()).toEqual("sessionId"); expect(ia.getStageParams(AuthType.Password)).toEqual({ @@ -127,13 +234,13 @@ describe("InteractiveAuth", function() { }); // submitAuthDict should trigger another call to doRequest - doRequest.mockImplementation(function(authData) { + doRequest.mockImplementation(async (authData) => { logger.log("request2", authData); expect(authData).toEqual({ session: "sessionId", type: AuthType.Password, }); - return Promise.resolve(requestRes); + return requestRes; }); ia.submitAuthDict({ @@ -141,14 +248,76 @@ describe("InteractiveAuth", function() { }); }); - return ia.attemptAuth().then(function(res) { - expect(res).toBe(requestRes); - expect(doRequest).toBeCalledTimes(2); - expect(stateUpdated).toBeCalledTimes(1); - }); + const res = await ia.attemptAuth(); + expect(res).toBe(requestRes); + expect(doRequest).toBeCalledTimes(2); + expect(stateUpdated).toBeCalledTimes(1); }); - it("should start an auth stage and reject if no auth flow", function() { + it("should make a request if authdata is null", async () => { + const doRequest = jest.fn(); + const stateUpdated = jest.fn(); + const requestEmailToken = jest.fn(); + + const ia = new InteractiveAuth({ + authData: null, + matrixClient: getFakeClient(), + stateUpdated, + doRequest, + requestEmailToken, + }); + + expect(ia.getSessionId()).toBe(undefined); + expect(ia.getStageParams(AuthType.Password)).toBe(undefined); + + // first we expect a call to doRequest + doRequest.mockImplementation((authData) => { + logger.log("request1", authData); + expect(authData).toEqual(null); // first request should be null + const err = new MatrixError({ + session: "sessionId", + flows: [ + { stages: [AuthType.Password] }, + ], + params: { + [AuthType.Password]: { param: "aa" }, + }, + }); + err.httpStatus = 401; + throw err; + }); + + // .. which should be followed by a call to stateUpdated + const requestRes = { "a": "b" }; + stateUpdated.mockImplementation((stage) => { + expect(stage).toEqual(AuthType.Password); + expect(ia.getSessionId()).toEqual("sessionId"); + expect(ia.getStageParams(AuthType.Password)).toEqual({ + param: "aa", + }); + + // submitAuthDict should trigger another call to doRequest + doRequest.mockImplementation(async (authData) => { + logger.log("request2", authData); + expect(authData).toEqual({ + session: "sessionId", + type: AuthType.Password, + }); + return requestRes; + }); + + ia.submitAuthDict({ + type: AuthType.Password, + }); + }); + + const res = await ia.attemptAuth(); + expect(res).toBe(requestRes); + expect(doRequest).toBeCalledTimes(2); + expect(stateUpdated).toBeCalledTimes(1); + }); + + it("should start an auth stage and reject if no auth flow", async () => { const doRequest = jest.fn(); const stateUpdated = jest.fn(); const requestEmailToken = jest.fn(); @@ -160,7 +329,7 @@ describe("InteractiveAuth", function() { requestEmailToken, }); - doRequest.mockImplementation(function(authData) { + doRequest.mockImplementation((authData) => { logger.log("request1", authData); expect(authData).toEqual(null); // first request should be null const err = new MatrixError({ @@ -174,9 +343,108 @@ describe("InteractiveAuth", function() { throw err; }); - return ia.attemptAuth().catch(function(error) { - expect(error.message).toBe('No appropriate authentication flow found'); + await expect(ia.attemptAuth.bind(ia)).rejects.toThrow( + new Error('No appropriate authentication flow found'), + ); + }); + + it("should start an auth stage and reject if no auth flow but has session", async () => { + const doRequest = jest.fn(); + const stateUpdated = jest.fn(); + const requestEmailToken = jest.fn(); + + const ia = new InteractiveAuth({ + matrixClient: getFakeClient(), + doRequest, + stateUpdated, + requestEmailToken, + authData: { + }, + sessionId: "sessionId", }); + + doRequest.mockImplementation((authData) => { + logger.log("request1", authData); + expect(authData).toEqual({ "session": "sessionId" }); // has existing sessionId + const err = new MatrixError({ + session: "sessionId", + flows: [], + params: { + [AuthType.Password]: { param: "aa" }, + }, + error: "Mock Error 1", + errcode: "MOCKERR1", + }); + err.httpStatus = 401; + throw err; + }); + + await expect(ia.attemptAuth.bind(ia)).rejects.toThrow( + new Error('No appropriate authentication flow found'), + ); + }); + + it("should handle unexpected error types without data propery set", async () => { + const doRequest = jest.fn(); + const stateUpdated = jest.fn(); + const requestEmailToken = jest.fn(); + + const ia = new InteractiveAuth({ + matrixClient: getFakeClient(), + doRequest, + stateUpdated, + requestEmailToken, + authData: { + session: "sessionId", + }, + }); + + doRequest.mockImplementation((authData) => { + logger.log("request1", authData); + expect(authData).toEqual({ "session": "sessionId" }); // has existing sessionId + const err = new Error('myerror'); + (err as any).httpStatus = 401; + throw err; + }); + + await expect(ia.attemptAuth.bind(ia)).rejects.toThrow( + new Error("myerror"), + ); + }); + + it("should allow dummy auth", async () => { + const doRequest = jest.fn(); + const stateUpdated = jest.fn(); + const requestEmailToken = jest.fn(); + + const ia = new InteractiveAuth({ + matrixClient: getFakeClient(), + doRequest, + stateUpdated, + requestEmailToken, + authData: { + session: 'sessionId', + flows: [ + { stages: [AuthType.Dummy] }, + ], + params: {}, + }, + }); + + const requestRes = { "a": "b" }; + doRequest.mockImplementation((authData) => { + logger.log("request1", authData); + expect(authData).toEqual({ + session: "sessionId", + type: AuthType.Dummy, + }); + return requestRes; + }); + + const res = await ia.attemptAuth(); + expect(res).toBe(requestRes); + expect(doRequest).toBeCalledTimes(1); + expect(stateUpdated).toBeCalledTimes(0); }); describe("requestEmailToken", () => { @@ -247,7 +515,7 @@ describe("InteractiveAuth", function() { doRequest, stateUpdated, requestEmailToken, }); - expect(async () => await ia.requestEmailToken()).rejects.toThrowError("unspecific network error"); + await expect(ia.requestEmailToken.bind(ia)).rejects.toThrowError("unspecific network error"); }); it("only starts one request at a time", async () => { diff --git a/src/autodiscovery.ts b/src/autodiscovery.ts index 58c4cb157..71cbd2105 100644 --- a/src/autodiscovery.ts +++ b/src/autodiscovery.ts @@ -17,6 +17,8 @@ limitations under the License. /** @module auto-discovery */ +import { ServerResponse } from "http"; + import { IClientWellKnown, IWellKnownConfig } from "./client"; import { logger } from './logger'; @@ -409,39 +411,41 @@ export class AutoDiscovery { * @return {Promise} Resolves to the returned state. * @private */ - private static fetchWellKnownObject(url: string): Promise { - return new Promise(function(resolve) { + private static fetchWellKnownObject(uri: string): Promise { + return new Promise((resolve) => { // eslint-disable-next-line const request = require("./matrix").getRequest(); if (!request) throw new Error("No request library available"); request( - { method: "GET", uri: url, timeout: 5000 }, - (err, response, body) => { - if (err || response && - (response.statusCode < 200 || response.statusCode >= 300) - ) { - let action = AutoDiscoveryAction.FAIL_PROMPT; - let reason = (err ? err.message : null) || "General failure"; - if (response && response.statusCode === 404) { - action = AutoDiscoveryAction.IGNORE; - reason = AutoDiscovery.ERROR_MISSING_WELLKNOWN; - } - resolve({ raw: {}, action: action, reason: reason, error: err }); - return; + { method: "GET", uri, timeout: 5000 }, + (error: Error, response: ServerResponse, body: string) => { + if (error || response?.statusCode < 200 || response?.statusCode >= 300) { + const result = { error, raw: {} }; + return resolve(response?.statusCode === 404 + ? { + ...result, + action: AutoDiscoveryAction.IGNORE, + reason: AutoDiscovery.ERROR_MISSING_WELLKNOWN, + } : { + ...result, + action: AutoDiscoveryAction.FAIL_PROMPT, + reason: error?.message || "General failure", + }); } try { - resolve({ raw: JSON.parse(body), action: AutoDiscoveryAction.SUCCESS }); - } catch (e) { - let reason = AutoDiscovery.ERROR_INVALID; - if (e.name === "SyntaxError") { - reason = AutoDiscovery.ERROR_INVALID_JSON; - } - resolve({ + return resolve({ + raw: JSON.parse(body), + action: AutoDiscoveryAction.SUCCESS, + }); + } catch (err) { + return resolve({ + error: err, raw: {}, action: AutoDiscoveryAction.FAIL_PROMPT, - reason: reason, - error: e, + reason: err?.name === "SyntaxError" + ? AutoDiscovery.ERROR_INVALID_JSON + : AutoDiscovery.ERROR_INVALID, }); } }, diff --git a/src/interactive-auth.ts b/src/interactive-auth.ts index d5e10427e..dab48a944 100644 --- a/src/interactive-auth.ts +++ b/src/interactive-auth.ts @@ -250,12 +250,9 @@ export class InteractiveAuth { if (!this.data?.flows) { this.busyChangedCallback?.(true); // use the existing sessionId, if one is present. - let auth = null; - if (this.data.session) { - auth = { - session: this.data.session, - }; - } + const auth = this.data.session + ? { session: this.data.session } + : null; this.doRequest(auth).finally(() => { this.busyChangedCallback?.(false); }); @@ -312,7 +309,7 @@ export class InteractiveAuth { * @return {string} session id */ public getSessionId(): string { - return this.data ? this.data.session : undefined; + return this.data?.session; } /** @@ -477,6 +474,9 @@ export class InteractiveAuth { logger.log("Background poll request failed doing UI auth: ignoring", error); } } + if (!error.data) { + error.data = {}; + } // if the error didn't come with flows, completed flows or session ID, // copy over the ones we have. Synapse sometimes sends responses without // any UI auth data (eg. when polling for email validation, if the email @@ -539,19 +539,17 @@ export class InteractiveAuth { return; } - if (this.data && this.data.errcode || this.data.error) { + if (this.data?.errcode || this.data?.error) { this.stateUpdatedCallback(nextStage, { - errcode: this.data.errcode || "", - error: this.data.error || "", + errcode: this.data?.errcode || "", + error: this.data?.error || "", }); return; } - const stageStatus: IStageStatus = {}; - if (nextStage == EMAIL_STAGE_TYPE) { - stageStatus.emailSid = this.emailSid; - } - this.stateUpdatedCallback(nextStage, stageStatus); + this.stateUpdatedCallback(nextStage, nextStage === EMAIL_STAGE_TYPE + ? { emailSid: this.emailSid } + : {}); } /** diff --git a/src/pushprocessor.ts b/src/pushprocessor.ts index 950f395b7..4e736c3a1 100644 --- a/src/pushprocessor.ts +++ b/src/pushprocessor.ts @@ -362,13 +362,9 @@ export class PushProcessor { return false; } - let regex; - - if (cond.key == 'content.body') { - regex = this.createCachedRegex('(^|\\W)', cond.pattern, '(\\W|$)'); - } else { - regex = this.createCachedRegex('^', cond.pattern, '$'); - } + const regex = cond.key === 'content.body' + ? this.createCachedRegex('(^|\\W)', cond.pattern, '(\\W|$)') + : this.createCachedRegex('^', cond.pattern, '$'); return !!val.match(regex); } diff --git a/src/utils.ts b/src/utils.ts index 8ebafc1ae..63e3475b8 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -328,22 +328,28 @@ export function escapeRegExp(string: string): string { } export function globToRegexp(glob: string, extended?: any): string { - extended = typeof(extended) === 'boolean' ? extended : true; // From // https://github.com/matrix-org/synapse/blob/abbee6b29be80a77e05730707602f3bbfc3f38cb/synapse/push/__init__.py#L132 // Because micromatch is about 130KB with dependencies, // and minimatch is not much better. - let pat = escapeRegExp(glob); - pat = pat.replace(/\\\*/g, '.*'); - pat = pat.replace(/\?/g, '.'); - if (extended) { - pat = pat.replace(/\\\[(!|)(.*)\\]/g, function(match, p1, p2, offset, string) { - const first = p1 && '^' || ''; - const second = p2.replace(/\\-/, '-'); - return '[' + first + second + ']'; - }); - } - return pat; + const replacements: ([RegExp, string | ((substring: string, ...args: any[]) => string) ])[] = [ + [/\\\*/g, '.*'], + [/\?/g, '.'], + extended !== false && [ + /\\\[(!|)(.*)\\]/g, + (_match: string, neg: string, pat: string) => [ + '[', + neg ? '^' : '', + pat.replace(/\\-/, '-'), + ']', + ].join(''), + ], + ]; + return replacements.reduce( + // https://github.com/microsoft/TypeScript/issues/30134 + (pat, args) => args ? pat.replace(args[0], args[1] as any) : pat, + escapeRegExp(glob), + ); } export function ensureNoTrailingSlash(url: string): string { diff --git a/src/webrtc/call.ts b/src/webrtc/call.ts index ebc7464f0..b772d5d97 100644 --- a/src/webrtc/call.ts +++ b/src/webrtc/call.ts @@ -1865,10 +1865,10 @@ export class MatrixCall extends TypedEventEmitter