From 9ecdf1e12b6d8e5b601822ad1b92aac442802307 Mon Sep 17 00:00:00 2001 From: Sonny Date: Thu, 16 Jan 2025 17:51:16 +0100 Subject: [PATCH] fast: Delete token when it is invalid (#1055) --- eslint.config.js | 17 ++- packages/client-core/src/fast/README.md | 9 +- packages/client-core/src/fast/fast.js | 49 ++++++-- .../client-core/src/fast/isTokenValid.test.js | 22 ++-- packages/client-core/test/fast.js | 118 ++++++++++++++++++ packages/error/test.js | 1 - packages/events/index.js | 7 ++ packages/reconnect/test.js | 1 - packages/sasl2/index.js | 32 ++--- packages/starttls/starttls.test.js | 1 - .../stream-management/stream-features.test.js | 6 +- packages/tls/test.js | 1 - test/client.test.js | 1 - 13 files changed, 211 insertions(+), 54 deletions(-) create mode 100644 packages/client-core/test/fast.js diff --git a/eslint.config.js b/eslint.config.js index 73236769..ef276398 100644 --- a/eslint.config.js +++ b/eslint.config.js @@ -58,8 +58,6 @@ export default [ // node // https://github.com/eslint-community/eslint-plugin-n/ "n/no-unpublished-require": "off", // doesn't play nice with monorepo - "n/no-extraneous-require": ["error", { allowModules: ["@xmpp/test"] }], - "n/no-extraneous-import": ["error", { allowModules: ["@xmpp/test"] }], "n/hashbang": "off", // promise @@ -109,6 +107,21 @@ export default [ // }, // ], "promise/no-callback-in-promise": "off", + // "n/no-extraneous-require": ["error", { allowModules: ["@xmpp/test"] }], + "n/no-extraneous-import": [ + "error", + { + allowModules: [ + "@xmpp/test", + "@xmpp/time", + "@xmpp/xml", + "@xmpp/connection", + "@xmpp/websocket", + "selfsigned", + "@xmpp/events", + ], + }, + ], }, }, ]; diff --git a/packages/client-core/src/fast/README.md b/packages/client-core/src/fast/README.md index 8940b9a3..48c23d5c 100644 --- a/packages/client-core/src/fast/README.md +++ b/packages/client-core/src/fast/README.md @@ -6,7 +6,7 @@ By default `@xmpp/fast` stores the token in memory and as such fast authenticati You can supply your own functions to store and retrieve the token from a persistent database. -If fast authentication fails, regular authentication with `credentials` will happen. +If fast authentication fails, regular authentication will happen. ## Usage @@ -26,10 +26,9 @@ client.fast.saveToken = async (token) => { await secureStorage.set("token", JSON.stringify(token)); } -// Debugging only -client.fast.on("error", (error) => { - console.log("fast error", error); -}) +client.fast.removeToken = async () => { + await secureStorage.del("token"); +} ``` ## References diff --git a/packages/client-core/src/fast/fast.js b/packages/client-core/src/fast/fast.js index 6fb629d1..1599b948 100644 --- a/packages/client-core/src/fast/fast.js +++ b/packages/client-core/src/fast/fast.js @@ -1,5 +1,6 @@ import { EventEmitter } from "@xmpp/events"; import { getAvailableMechanisms } from "@xmpp/sasl"; +import SASLError from "@xmpp/sasl/lib/SASLError.js"; import xml from "@xmpp/xml"; import SASLFactory from "saslmechanisms"; @@ -20,6 +21,9 @@ export default function fast({ sasl2, entity }) { async fetchToken() { return token; }, + async deleteToken() { + token = null; + }, async save(token) { try { await this.saveToken(token); @@ -34,17 +38,31 @@ export default function fast({ sasl2, entity }) { entity.emit("error", err); } }, + async delete() { + try { + await this.deleteToken(); + } catch (err) { + entity.emit("error", err); + } + }, saslFactory, async auth({ authenticate, entity, userAgent, - token, credentials, streamFeatures, features, }) { + // Unavailable + if (!fast.mechanism) { + return false; + } + + const { token } = credentials; + // Invalid or unavailable token if (!isTokenValid(token, fast.mechanisms)) { + requestToken(streamFeatures); return false; } @@ -68,20 +86,29 @@ export default function fast({ sasl2, entity }) { }); return true; } catch (err) { + if ( + err instanceof SASLError && + ["not-authorized", "credentials-expired"].includes(err.condition) + ) { + await this.delete(); + requestToken(streamFeatures); + return false; + } entity.emit("error", err); return false; } }, - _requestToken(streamFeatures) { - streamFeatures.push( - xml("request-token", { - xmlns: NS, - mechanism: fast.mechanism, - }), - ); - }, }); + function requestToken(streamFeatures) { + streamFeatures.push( + xml("request-token", { + xmlns: NS, + mechanism: fast.mechanism, + }), + ); + } + function reset() { fast.mechanism = null; fast.mechanisms = []; @@ -121,13 +148,17 @@ export default function fast({ sasl2, entity }) { } export function isTokenValid(token, mechanisms) { + if (!token) return false; + // Avoid an error round trip if the server does not support the token mechanism anymore if (!mechanisms.includes(token.mechanism)) { return false; } + // Avoid an error round trip if the token is already expired if (new Date(token.expiry) <= new Date()) { return false; } + return true; } diff --git a/packages/client-core/src/fast/isTokenValid.test.js b/packages/client-core/src/fast/isTokenValid.test.js index dc20d811..94cf91ff 100644 --- a/packages/client-core/src/fast/isTokenValid.test.js +++ b/packages/client-core/src/fast/isTokenValid.test.js @@ -1,5 +1,4 @@ import { isTokenValid } from "./fast.js"; -// eslint-disable-next-line n/no-extraneous-import import { datetime } from "@xmpp/time"; const tomorrow = new Date(); @@ -12,40 +11,45 @@ it("returns false if the token.mechanism is not available", async () => { expect( isTokenValid( { - expires: datetime(tomorrow), + expiry: datetime(tomorrow), mechanism: "bar", }, ["foo"], ), - ); + ).toBe(false); }); it("returns true if the token.mechanism is available", async () => { expect( - isTokenValid({ expires: datetime(tomorrow), mechanism: "foo" }, ["foo"]), - ); + isTokenValid({ expiry: datetime(tomorrow), mechanism: "foo" }, ["foo"]), + ).toBe(true); }); it("returns false if the token is expired", async () => { expect( isTokenValid( { - expires: datetime(yesterday), + expiry: datetime(yesterday), mechanism: "foo", }, ["foo"], ), - ); + ).toBe(false); }); it("returns true if the token is not expired", async () => { expect( isTokenValid( { - expires: datetime(tomorrow), + expiry: datetime(tomorrow), mechanism: "foo", }, ["foo"], ), - ); + ).toBe(true); +}); + +it("returns false if the token is nullish", async () => { + expect(isTokenValid(null)).toBe(false); + expect(isTokenValid(undefined)).toBe(false); }); diff --git a/packages/client-core/test/fast.js b/packages/client-core/test/fast.js new file mode 100644 index 00000000..7ad4cc5b --- /dev/null +++ b/packages/client-core/test/fast.js @@ -0,0 +1,118 @@ +import { tick } from "@xmpp/events"; +import { mockClient } from "@xmpp/test"; +import { datetime } from "@xmpp/time"; +import { Element } from "@xmpp/xml"; + +const mechanism = "HT-SHA-256-NONE"; + +test("requests and saves token if server advertises fast", async () => { + const { entity, fast } = mockClient(); + + const spy_saveToken = jest.spyOn(fast, "saveToken"); + + entity.mockInput( + + + PLAIN + + + {mechanism} + + + + , + ); + + const authenticate = await entity.catchOutgoing(); + expect(authenticate.is("authenticate", "urn:xmpp:sasl:2")).toBe(true); + const request_token = authenticate.getChild( + "request-token", + "urn:xmpp:fast:0", + ); + expect(request_token.attrs.mechanism).toBe(mechanism); + + const token = "secret-token:fast-HZzFpFwHTy4nc3C8Y1NVNZqYef_7Q3YjMLu2"; + const expiry = "2025-02-06T09:58:40.774329Z"; + + expect(spy_saveToken).not.toHaveBeenCalled(); + + entity.mockInput( + + + + username@localhost/rOYwkWIywtnF + + , + ); + + expect(spy_saveToken).toHaveBeenCalledWith({ token, expiry, mechanism }); +}); + +async function setupFast() { + const { entity, fast } = mockClient(); + + const d = new Date(); + d.setFullYear(d.getFullYear() + 1); + const expiry = datetime(d); + + fast.fetchToken = async () => { + return { + mechanism, + expiry, + token: "foobar", + }; + }; + + entity.mockInput( + + + PLAIN + + + {mechanism} + + + + , + ); + + expect(fast.mechanism).toBe(mechanism); + + const authenticate = await entity.catchOutgoing(); + expect(authenticate.is("authenticate", "urn:xmpp:sasl:2")); + expect(authenticate.attrs.mechanism).toBe(mechanism); + expect(authenticate.getChild("fast", "urn:xmpp:fast:0")).toBeInstanceOf( + Element, + ); + + return entity; +} + +test("deletes the token if server replies with not-authorized", async () => { + const entity = await setupFast(); + const spy_deleteToken = jest.spyOn(entity.fast, "deleteToken"); + + expect(spy_deleteToken).not.toHaveBeenCalled(); + entity.mockInput( + + + , + ); + await tick(); + expect(spy_deleteToken).toHaveBeenCalled(); +}); + +test("deletes the token if server replies with credentials-expired", async () => { + const entity = await setupFast(); + const spy_deleteToken = jest.spyOn(entity.fast, "deleteToken"); + + // credentials-expired + expect(spy_deleteToken).not.toHaveBeenCalled(); + entity.mockInput( + + + , + ); + await tick(); + expect(spy_deleteToken).toHaveBeenCalled(); +}); diff --git a/packages/error/test.js b/packages/error/test.js index 5905200d..af5c26a3 100644 --- a/packages/error/test.js +++ b/packages/error/test.js @@ -1,5 +1,4 @@ import XMPPError from "./index.js"; -// eslint-disable-next-line n/no-extraneous-import import parse from "@xmpp/xml/lib/parse.js"; test("fromElement", () => { diff --git a/packages/events/index.js b/packages/events/index.js index cbe287f7..56c64636 100644 --- a/packages/events/index.js +++ b/packages/events/index.js @@ -9,6 +9,12 @@ import procedure from "./lib/procedure.js"; import listeners from "./lib/listeners.js"; import onoff from "./lib/onoff.js"; +function tick() { + return new Promise((resolve) => { + process.nextTick(resolve); + }); +} + export { EventEmitter, timeout, @@ -19,4 +25,5 @@ export { procedure, listeners, onoff, + tick, }; diff --git a/packages/reconnect/test.js b/packages/reconnect/test.js index 6ace155d..76933e14 100644 --- a/packages/reconnect/test.js +++ b/packages/reconnect/test.js @@ -1,5 +1,4 @@ import _reconnect from "./index.js"; -// eslint-disable-next-line n/no-extraneous-import import Connection from "@xmpp/connection"; test("schedules a reconnect when disconnect is emitted", () => { diff --git a/packages/sasl2/index.js b/packages/sasl2/index.js index f6d2c86a..7cb81975 100644 --- a/packages/sasl2/index.js +++ b/packages/sasl2/index.js @@ -114,26 +114,20 @@ export default function sasl2({ streamFeatures, saslFactory }, onAuthenticate) { ); async function done(credentials, mechanism, userAgent) { - if (fast_available) { - const { token } = credentials; - // eslint-disable-next-line unicorn/no-negated-condition - if (!token) { - fast._requestToken(streamFeatures); - } else { - const success = await fast.auth({ - authenticate, - entity, - userAgent, - token, - streamFeatures, - features, - credentials, - }); - if (success) return; - // If fast authentication fails, continue and try with sasl - } - } + // Try fast + const success = await fast.auth({ + authenticate, + entity, + userAgent, + streamFeatures, + features, + credentials, + }); + if (success) return; + + // fast.auth may mutate streamFeatures to request a token + // If fast authentication fails, continue and try without await authenticate({ entity, userAgent, diff --git a/packages/starttls/starttls.test.js b/packages/starttls/starttls.test.js index 3cad7750..c2f19d1a 100644 --- a/packages/starttls/starttls.test.js +++ b/packages/starttls/starttls.test.js @@ -1,7 +1,6 @@ import tls from "tls"; import { canUpgrade } from "./starttls.js"; import net from "net"; -// eslint-disable-next-line n/no-extraneous-import import WebSocket from "@xmpp/websocket/lib/Socket.js"; test("canUpgrade", () => { diff --git a/packages/stream-management/stream-features.test.js b/packages/stream-management/stream-features.test.js index 4728a1a1..69f2fa64 100644 --- a/packages/stream-management/stream-features.test.js +++ b/packages/stream-management/stream-features.test.js @@ -1,10 +1,6 @@ import { mockClient } from "@xmpp/test"; -function tick() { - return new Promise((resolve) => { - process.nextTick(resolve); - }); -} +import { tick } from "@xmpp/events"; test("enable - enabled", async () => { const { entity } = mockClient(); diff --git a/packages/tls/test.js b/packages/tls/test.js index 82097918..bbfbba14 100644 --- a/packages/tls/test.js +++ b/packages/tls/test.js @@ -2,7 +2,6 @@ import ConnectionTLS from "./lib/Connection.js"; import tls from "tls"; import { promise } from "@xmpp/test"; -// eslint-disable-next-line n/no-extraneous-import import selfsigned from "selfsigned"; test("socketParameters()", () => { diff --git a/test/client.test.js b/test/client.test.js index 457f180f..6437b653 100644 --- a/test/client.test.js +++ b/test/client.test.js @@ -1,4 +1,3 @@ -// eslint-disable-next-line n/no-extraneous-import import { promise } from "@xmpp/events"; import { client, xml, jid } from "../packages/client/index.js"; import debug from "../packages/debug/index.js";