PrivateCoffee/element-web
4
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 4

Fix OIDC bugs due to amnesiac stores forgetting OIDC issuer & other data (#12166)

Browse source 
* Fix OIDC bugs due to amnesiac stores forgetting OIDC issuer & other data

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Fix tests

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

---------

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
This commit is contained in:
Michael Telatynski 2024-01-23 13:34:10 +00:00 committed by GitHub
parent 11096b207a
commit 4e68b91515
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 33 additions and 51 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

22
src/utils/oidc/persistOidcSettings.ts
View file

@ -21,7 +21,7 @@ const tokenIssuerStorageKey = "mx_oidc_token_issuer";
const idTokenClaimsStorageKey = "mx_oidc_id_token_claims"; const idTokenClaimsStorageKey = "mx_oidc_id_token_claims";
/** /**
* Persists oidc clientId and issuer in session storage * Persists oidc clientId and issuer in local storage
* Only set after successful authentication * Only set after successful authentication
* @param clientId * @param clientId
* @param issuer * @param issuer
@ -31,27 +31,27 @@ export const persistOidcAuthenticatedSettings = (
issuer: string, issuer: string,
idTokenClaims: IdTokenClaims, idTokenClaims: IdTokenClaims,
): void => { ): void => {
sessionStorage.setItem(clientIdStorageKey, clientId); localStorage.setItem(clientIdStorageKey, clientId);
sessionStorage.setItem(tokenIssuerStorageKey, issuer); localStorage.setItem(tokenIssuerStorageKey, issuer);
sessionStorage.setItem(idTokenClaimsStorageKey, JSON.stringify(idTokenClaims)); localStorage.setItem(idTokenClaimsStorageKey, JSON.stringify(idTokenClaims));
}; };
/** /**
* Retrieve stored oidc issuer from session storage * Retrieve stored oidc issuer from local storage
* When user has token from OIDC issuer, this will be set * When user has token from OIDC issuer, this will be set
* @returns issuer or undefined * @returns issuer or undefined
*/ */
export const getStoredOidcTokenIssuer = (): string | undefined => { export const getStoredOidcTokenIssuer = (): string | undefined => {
return sessionStorage.getItem(tokenIssuerStorageKey) ?? undefined; return localStorage.getItem(tokenIssuerStorageKey) ?? undefined;
}; };
/** /**
* Retrieves stored oidc client id from session storage * Retrieves stored oidc client id from local storage
* @returns clientId * @returns clientId
* @throws when clientId is not found in session storage * @throws when clientId is not found in local storage
*/ */
export const getStoredOidcClientId = (): string => { export const getStoredOidcClientId = (): string => {
const clientId = sessionStorage.getItem(clientIdStorageKey); const clientId = localStorage.getItem(clientIdStorageKey);
if (!clientId) { if (!clientId) {
throw new Error("Oidc client id not found in storage"); throw new Error("Oidc client id not found in storage");
} }
@ -59,11 +59,11 @@ export const getStoredOidcClientId = (): string => {
}; };
/** /**
* Retrieve stored id token claims from session storage * Retrieve stored id token claims from local storage
* @returns idtokenclaims or undefined * @returns idtokenclaims or undefined
*/ */
export const getStoredOidcIdTokenClaims = (): IdTokenClaims | undefined => { export const getStoredOidcIdTokenClaims = (): IdTokenClaims | undefined => {
const idTokenClaims = sessionStorage.getItem(idTokenClaimsStorageKey); const idTokenClaims = localStorage.getItem(idTokenClaimsStorageKey);
if (!idTokenClaims) { if (!idTokenClaims) {
return; return;
} }

4
test/components/structures/MatrixChat-test.tsx
View file

@ -449,8 +449,8 @@ describe("<MatrixChat />", () => {
await flushPromises(); await flushPromises();
expect(sessionStorage.getItem("mx_oidc_client_id")).toEqual(clientId); expect(localStorage.getItem("mx_oidc_client_id")).toEqual(clientId);
expect(sessionStorage.getItem("mx_oidc_token_issuer")).toEqual(issuer); expect(localStorage.getItem("mx_oidc_token_issuer")).toEqual(issuer);
}); });
it("should set logged in and start MatrixClient", async () => { it("should set logged in and start MatrixClient", async () => {

30
test/stores/oidc/OidcClientStore-test.ts
View file

@ -34,19 +34,16 @@ describe("OidcClientStore", () => {
const clientId = "test-client-id"; const clientId = "test-client-id";
const metadata = mockOpenIdConfiguration(); const metadata = mockOpenIdConfiguration();
const account = metadata.issuer + "account"; const account = metadata.issuer + "account";
const mockSessionStorage: Record<string, string> = {
mx_oidc_client_id: clientId,
mx_oidc_token_issuer: metadata.issuer,
};
const mockClient = getMockClientWithEventEmitter({ const mockClient = getMockClientWithEventEmitter({
waitForClientWellKnown: jest.fn().mockResolvedValue({}), waitForClientWellKnown: jest.fn().mockResolvedValue({}),
}); });
beforeEach(() => { beforeEach(() => {
jest.spyOn(sessionStorage.__proto__, "getItem") localStorage.clear();
.mockClear() localStorage.setItem("mx_oidc_client_id", clientId);
.mockImplementation((key) => mockSessionStorage[key as string] ?? null); localStorage.setItem("mx_oidc_token_issuer", metadata.issuer);
mocked(discoverAndValidateAuthenticationConfig).mockClear().mockResolvedValue({ mocked(discoverAndValidateAuthenticationConfig).mockClear().mockResolvedValue({
metadata, metadata,
account, account,
@ -71,7 +68,7 @@ describe("OidcClientStore", () => {
}); });
it("should return false when no issuer is in session storage", () => { it("should return false when no issuer is in session storage", () => {
jest.spyOn(sessionStorage.__proto__, "getItem").mockReturnValue(null); localStorage.clear();
const store = new OidcClientStore(mockClient); const store = new OidcClientStore(mockClient);
expect(store.isUserAuthenticatedWithOidc).toEqual(false); expect(store.isUserAuthenticatedWithOidc).toEqual(false);
@ -98,14 +95,7 @@ describe("OidcClientStore", () => {
}); });
it("should log and return when no clientId is found in storage", async () => { it("should log and return when no clientId is found in storage", async () => {
const sessionStorageWithoutClientId: Record<string, string | null> = { localStorage.removeItem("mx_oidc_client_id");
...mockSessionStorage,
mx_oidc_client_id: null,
};
jest.spyOn(sessionStorage.__proto__, "getItem").mockImplementation(
(key) => sessionStorageWithoutClientId[key as string] ?? null,
);
const store = new OidcClientStore(mockClient); const store = new OidcClientStore(mockClient);
// no oidc client // no oidc client
@ -209,13 +199,7 @@ describe("OidcClientStore", () => {
it("should throw when oidcClient could not be initialised", async () => { it("should throw when oidcClient could not be initialised", async () => {
// make oidcClient initialisation fail // make oidcClient initialisation fail
mockClient.waitForClientWellKnown.mockResolvedValue(undefined as any); mockClient.waitForClientWellKnown.mockResolvedValue(undefined as any);
const sessionStorageWithoutIssuer: Record<string, string | null> = { localStorage.removeItem("mx_oidc_token_issuer");
...mockSessionStorage,
mx_oidc_token_issuer: null,
};
jest.spyOn(sessionStorage.__proto__, "getItem").mockImplementation(
(key) => sessionStorageWithoutIssuer[key as string] ?? null,
);
const store = new OidcClientStore(mockClient); const store = new OidcClientStore(mockClient);

28
test/utils/oidc/persistOidcSettings-test.ts
View file

@ -24,10 +24,11 @@ import {
} from "../../../src/utils/oidc/persistOidcSettings"; } from "../../../src/utils/oidc/persistOidcSettings";
describe("persist OIDC settings", () => { describe("persist OIDC settings", () => {
beforeEach(() => { jest.spyOn(Storage.prototype, "getItem");
jest.spyOn(sessionStorage.__proto__, "getItem").mockClear().mockReturnValue(null); jest.spyOn(Storage.prototype, "setItem");
jest.spyOn(sessionStorage.__proto__, "setItem").mockClear(); beforeEach(() => {
localStorage.clear();
}); });
const clientId = "test-client-id"; const clientId = "test-client-id";
@ -45,20 +46,17 @@ describe("persist OIDC settings", () => {
describe("persistOidcAuthenticatedSettings", () => { describe("persistOidcAuthenticatedSettings", () => {
it("should set clientId and issuer in session storage", () => { it("should set clientId and issuer in session storage", () => {
persistOidcAuthenticatedSettings(clientId, issuer, idTokenClaims); persistOidcAuthenticatedSettings(clientId, issuer, idTokenClaims);
expect(sessionStorage.setItem).toHaveBeenCalledWith("mx_oidc_client_id", clientId); expect(localStorage.setItem).toHaveBeenCalledWith("mx_oidc_client_id", clientId);
expect(sessionStorage.setItem).toHaveBeenCalledWith("mx_oidc_token_issuer", issuer); expect(localStorage.setItem).toHaveBeenCalledWith("mx_oidc_token_issuer", issuer);
expect(sessionStorage.setItem).toHaveBeenCalledWith( expect(localStorage.setItem).toHaveBeenCalledWith("mx_oidc_id_token_claims", JSON.stringify(idTokenClaims));
"mx_oidc_id_token_claims",
JSON.stringify(idTokenClaims),
);
}); });
}); });
describe("getStoredOidcTokenIssuer()", () => { describe("getStoredOidcTokenIssuer()", () => {
it("should return issuer from session storage", () => { it("should return issuer from session storage", () => {
jest.spyOn(sessionStorage.__proto__, "getItem").mockReturnValue(issuer); localStorage.setItem("mx_oidc_token_issuer", issuer);
expect(getStoredOidcTokenIssuer()).toEqual(issuer); expect(getStoredOidcTokenIssuer()).toEqual(issuer);
expect(sessionStorage.getItem).toHaveBeenCalledWith("mx_oidc_token_issuer"); expect(localStorage.getItem).toHaveBeenCalledWith("mx_oidc_token_issuer");
}); });
it("should return undefined when no issuer in session storage", () => { it("should return undefined when no issuer in session storage", () => {
@ -68,9 +66,9 @@ describe("persist OIDC settings", () => {
describe("getStoredOidcClientId()", () => { describe("getStoredOidcClientId()", () => {
it("should return clientId from session storage", () => { it("should return clientId from session storage", () => {
jest.spyOn(sessionStorage.__proto__, "getItem").mockReturnValue(clientId); localStorage.setItem("mx_oidc_client_id", clientId);
expect(getStoredOidcClientId()).toEqual(clientId); expect(getStoredOidcClientId()).toEqual(clientId);
expect(sessionStorage.getItem).toHaveBeenCalledWith("mx_oidc_client_id"); expect(localStorage.getItem).toHaveBeenCalledWith("mx_oidc_client_id");
}); });
it("should throw when no clientId in session storage", () => { it("should throw when no clientId in session storage", () => {
expect(() => getStoredOidcClientId()).toThrow("Oidc client id not found in storage"); expect(() => getStoredOidcClientId()).toThrow("Oidc client id not found in storage");
@ -79,9 +77,9 @@ describe("persist OIDC settings", () => {
describe("getStoredOidcIdTokenClaims()", () => { describe("getStoredOidcIdTokenClaims()", () => {
it("should return issuer from session storage", () => { it("should return issuer from session storage", () => {
jest.spyOn(sessionStorage.__proto__, "getItem").mockReturnValue(JSON.stringify(idTokenClaims)); localStorage.setItem("mx_oidc_id_token_claims", JSON.stringify(idTokenClaims));
expect(getStoredOidcIdTokenClaims()).toEqual(idTokenClaims); expect(getStoredOidcIdTokenClaims()).toEqual(idTokenClaims);
expect(sessionStorage.getItem).toHaveBeenCalledWith("mx_oidc_id_token_claims"); expect(localStorage.getItem).toHaveBeenCalledWith("mx_oidc_id_token_claims");
}); });
it("should return undefined when no issuer in session storage", () => { it("should return undefined when no issuer in session storage", () => {