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 3ed0fdfb73.

* 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

@@ -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,
+            ),
         ]);
     });
 

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 () => {

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<object>} Resolves to the returned state.
      * @private
      */
-    private static fetchWellKnownObject(url: string): Promise<IWellKnownConfig> {
-        return new Promise(function(resolve) {
+    private static fetchWellKnownObject(uri: string): Promise<IWellKnownConfig> {
+        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,
                         });
                     }
                 },

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 }
+            : {});
     }
 
     /**

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);
     }

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 {

src/webrtc/call.ts

@@ -1865,10 +1865,10 @@ export class MatrixCall extends TypedEventEmitter<CallEvent, CallEventHandlerMap
         const shouldTerminate = (
             // reject events also end the call if it's ringing: it's another of
             // our devices rejecting the call.
-            ([CallState.InviteSent, CallState.Ringing].includes(this.state)) ||
+            [CallState.InviteSent, CallState.Ringing].includes(this.state) ||
             // also if we're in the init state and it's an inbound call, since
             // this means we just haven't entered the ringing state yet
-            this.state === CallState.Fledgling && this.direction === CallDirection.Inbound
+            (this.state === CallState.Fledgling && this.direction === CallDirection.Inbound)
         );
 
         if (shouldTerminate) {