Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

OIDC: persist refresh token #11249

Merged
merged 24 commits into from
Sep 19, 2023
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
2607997
test persistCredentials without a pickle key
Jul 11, 2023
3506c06
Merge branch 'develop' into kerry/25708/test-persist-credentials
Jul 12, 2023
609f790
test setLoggedIn with pickle key
Jul 12, 2023
f3092c7
lint
Jul 12, 2023
fad7f33
type error
Jul 12, 2023
32d5fb0
extract token persisting code into function, persist refresh token
Jul 12, 2023
e6529f1
store has_refresh_token too
Jul 12, 2023
66d57e5
pass refreshToken from oidcAuthGrant into credentials
Jul 12, 2023
b33e347
rest restore session with pickle key
Jul 13, 2023
823ba2e
Merge branch 'kerry/25708/test-persist-credentials' into kerry/25708/…
Jul 13, 2023
b7e0603
Merge branch 'develop' into kerry/25708/test-persist-credentials
Jul 13, 2023
b8b0c86
Merge branch 'kerry/25708/test-persist-credentials' into kerry/25708/…
Jul 13, 2023
f059642
Merge branch 'develop' into kerry/25708/test-persist-credentials
Jul 16, 2023
9272110
Merge branch 'kerry/25708/test-persist-credentials' into kerry/25708/…
Jul 17, 2023
d24fbd0
Merge branch 'develop' into kerry/25708/save-refresh-token
Jul 19, 2023
880c258
Merge branch 'kerry/25708/save-refresh-token' of https://github.com/m…
Jul 19, 2023
65c0734
Merge branch 'develop' into kerry/25708/save-refresh-token
Jul 19, 2023
70ddb4a
comments
Jul 20, 2023
56441dc
Merge branch 'develop' into kerry/25708/save-refresh-token
Jul 20, 2023
af481b2
prettier
Jul 20, 2023
e2d2d33
Update src/Lifecycle.ts
Sep 17, 2023
678815d
Merge branch 'develop' into kerry/25708/save-refresh-token
Sep 17, 2023
1f903c9
comments
Sep 18, 2023
777dbba
Merge branch 'develop' into kerry/25708/save-refresh-token
Sep 18, 2023
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
113 changes: 80 additions & 33 deletions src/Lifecycle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,14 @@ import { completeOidcLogin } from "./utils/oidc/authorize";
const HOMESERVER_URL_KEY = "mx_hs_url";
const ID_SERVER_URL_KEY = "mx_is_url";

const ACCESS_TOKEN_STORAGE_KEY = "mx_access_token";
const REFRESH_TOKEN_STORAGE_KEY = "mx_refresh_token";
/**
* Used during encryption/decryption of token
*/
const ACCESS_TOKEN_NAME = "access_token";
const REFRESH_TOKEN_NAME = "refresh_token";
richvdh marked this conversation as resolved.
Show resolved Hide resolved

dis.register((payload) => {
if (payload.action === Action.TriggerLogout) {
// noinspection JSIgnoredPromiseFromCall - we don't care if it fails
Expand Down Expand Up @@ -215,7 +223,7 @@ export async function attemptDelegatedAuthLogin(
*/
async function attemptOidcNativeLogin(queryParams: QueryDict): Promise<boolean> {
try {
const { accessToken, homeserverUrl, identityServerUrl } = await completeOidcLogin(queryParams);
const { accessToken, refreshToken, homeserverUrl, identityServerUrl } = await completeOidcLogin(queryParams);

const {
user_id: userId,
Expand All @@ -225,6 +233,7 @@ async function attemptOidcNativeLogin(queryParams: QueryDict): Promise<boolean>

const credentials = {
accessToken,
refreshToken,
homeserverUrl,
identityServerUrl,
deviceId,
Expand Down Expand Up @@ -452,25 +461,25 @@ export async function getStoredSessionVars(): Promise<Partial<IStoredSession>> {
const isUrl = localStorage.getItem(ID_SERVER_URL_KEY) ?? undefined;
let accessToken: string | undefined;
try {
accessToken = await StorageManager.idbLoad("account", "mx_access_token");
accessToken = await StorageManager.idbLoad("account", ACCESS_TOKEN_STORAGE_KEY);
} catch (e) {
logger.error("StorageManager.idbLoad failed for account:mx_access_token", e);
}
if (!accessToken) {
accessToken = localStorage.getItem("mx_access_token") ?? undefined;
accessToken = localStorage.getItem(ACCESS_TOKEN_STORAGE_KEY) ?? undefined;
if (accessToken) {
try {
// try to migrate access token to IndexedDB if we can
await StorageManager.idbSave("account", "mx_access_token", accessToken);
localStorage.removeItem("mx_access_token");
await StorageManager.idbSave("account", ACCESS_TOKEN_STORAGE_KEY, accessToken);
localStorage.removeItem(ACCESS_TOKEN_STORAGE_KEY);
} catch (e) {
logger.error("migration of access token to IndexedDB failed", e);
}
}
}
// if we pre-date storing "mx_has_access_token", but we retrieved an access
// token, then we should say we have an access token
const hasAccessToken = localStorage.getItem("mx_has_access_token") === "true" || !!accessToken;
const hasAccessToken = localStorage.getItem(`mx_has_${ACCESS_TOKEN_NAME}`) === "true" || !!accessToken;
richvdh marked this conversation as resolved.
Show resolved Hide resolved
const userId = localStorage.getItem("mx_user_id") ?? undefined;
const deviceId = localStorage.getItem("mx_device_id") ?? undefined;

Expand Down Expand Up @@ -556,7 +565,7 @@ export async function restoreFromLocalStorage(opts?: { ignoreGuest?: boolean }):
logger.log("Got pickle key");
if (typeof accessToken !== "string") {
const encrKey = await pickleKeyToAesKey(pickleKey);
decryptedAccessToken = await decryptAES(accessToken, encrKey, "access_token");
decryptedAccessToken = await decryptAES(accessToken, encrKey, ACCESS_TOKEN_NAME);
encrKey.fill(0);
}
} else {
Expand Down Expand Up @@ -761,28 +770,40 @@ async function showStorageEvictedDialog(): Promise<boolean> {
// `instanceof`. Babel 7 supports this natively in their class handling.
class AbortLoginAndRebuildStorage extends Error {}

async function persistCredentials(credentials: IMatrixClientCreds): Promise<void> {
localStorage.setItem(HOMESERVER_URL_KEY, credentials.homeserverUrl);
if (credentials.identityServerUrl) {
localStorage.setItem(ID_SERVER_URL_KEY, credentials.identityServerUrl);
}
localStorage.setItem("mx_user_id", credentials.userId);
localStorage.setItem("mx_is_guest", JSON.stringify(credentials.guest));

// store whether we expect to find an access token, to detect the case
/**
* Persist a token in storage
* When pickle key is present, will attempt to encrypt the token
* Stores in idb, falling back to localStorage
*
* @param storageKey key used to store the token
* @param name eg "access_token" used as initialization vector during encryption
richvdh marked this conversation as resolved.
Show resolved Hide resolved
* @param token
richvdh marked this conversation as resolved.
Show resolved Hide resolved
* @param pickleKey optional pickle key used to encrypt token
*/
async function persistTokenInStorage(
storageKey: string,
name: string,
richvdh marked this conversation as resolved.
Show resolved Hide resolved
token: string | undefined,
pickleKey: IMatrixClientCreds["pickleKey"],
kerryarchibald marked this conversation as resolved.
Show resolved Hide resolved
): Promise<void> {
const hasTokenStorageKey = `mx_has_${name}`;
richvdh marked this conversation as resolved.
Show resolved Hide resolved
// store whether we expect to find a token, to detect the case
// where IndexedDB is blown away
if (credentials.accessToken) {
localStorage.setItem("mx_has_access_token", "true");
if (token) {
localStorage.setItem(hasTokenStorageKey, "true");
} else {
localStorage.removeItem("mx_has_access_token");
localStorage.removeItem(hasTokenStorageKey);
}

if (credentials.pickleKey) {
let encryptedAccessToken: IEncryptedPayload | undefined;
if (pickleKey) {
let encryptedToken: IEncryptedPayload | undefined;
try {
if (!token) {
throw new Error("No token: not attempting encryption");
}
// try to encrypt the access token using the pickle key
const encrKey = await pickleKeyToAesKey(credentials.pickleKey);
encryptedAccessToken = await encryptAES(credentials.accessToken, encrKey, "access_token");
const encrKey = await pickleKeyToAesKey(pickleKey);
encryptedToken = await encryptAES(token, encrKey, name);
encrKey.fill(0);
} catch (e) {
logger.warn("Could not encrypt access token", e);
Expand All @@ -791,28 +812,54 @@ async function persistCredentials(credentials: IMatrixClientCreds): Promise<void
// save either the encrypted access token, or the plain access
// token if we were unable to encrypt (e.g. if the browser doesn't
// have WebCrypto).
await StorageManager.idbSave("account", "mx_access_token", encryptedAccessToken || credentials.accessToken);
await StorageManager.idbSave("account", storageKey, encryptedToken || token);
} catch (e) {
// if we couldn't save to indexedDB, fall back to localStorage. We
// store the access token unencrypted since localStorage only saves
// strings.
if (!!credentials.accessToken) {
localStorage.setItem("mx_access_token", credentials.accessToken);
if (!!token) {
localStorage.setItem(storageKey, token);
richvdh marked this conversation as resolved.
Show resolved Hide resolved
} else {
localStorage.removeItem("mx_access_token");
localStorage.removeItem(storageKey);
}
}
localStorage.setItem("mx_has_pickle_key", String(true));
} else {
try {
await StorageManager.idbSave("account", "mx_access_token", credentials.accessToken);
await StorageManager.idbSave("account", storageKey, token);
} catch (e) {
if (!!credentials.accessToken) {
localStorage.setItem("mx_access_token", credentials.accessToken);
if (!!token) {
localStorage.setItem(storageKey, token);
richvdh marked this conversation as resolved.
Show resolved Hide resolved
} else {
localStorage.removeItem("mx_access_token");
localStorage.removeItem(storageKey);
}
}
}
}

async function persistCredentials(credentials: IMatrixClientCreds): Promise<void> {
localStorage.setItem(HOMESERVER_URL_KEY, credentials.homeserverUrl);

Check failure

Code scanning / CodeQL

Clear text storage of sensitive information

This stores sensitive data returned by [a call to sendLoginRequest](1) as clear text.
if (credentials.identityServerUrl) {
localStorage.setItem(ID_SERVER_URL_KEY, credentials.identityServerUrl);

Check failure

Code scanning / CodeQL

Clear text storage of sensitive information

This stores sensitive data returned by [a call to sendLoginRequest](1) as clear text.
}
localStorage.setItem("mx_user_id", credentials.userId);

Check failure

Code scanning / CodeQL

Clear text storage of sensitive information

This stores sensitive data returned by [a call to sendLoginRequest](1) as clear text.
localStorage.setItem("mx_is_guest", JSON.stringify(credentials.guest));

Check failure

Code scanning / CodeQL

Clear text storage of sensitive information

This stores sensitive data returned by [a call to sendLoginRequest](1) as clear text.

await persistTokenInStorage(
ACCESS_TOKEN_STORAGE_KEY,
ACCESS_TOKEN_NAME,
credentials.accessToken,
credentials.pickleKey,
);
await persistTokenInStorage(
REFRESH_TOKEN_STORAGE_KEY,
REFRESH_TOKEN_NAME,
credentials.refreshToken,
credentials.pickleKey,
);

if (credentials.pickleKey) {
localStorage.setItem("mx_has_pickle_key", String(true));
} else {
if (localStorage.getItem("mx_has_pickle_key") === "true") {
logger.error("Expected a pickle key, but none provided. Encryption may not work.");
}
Expand Down Expand Up @@ -1003,7 +1050,7 @@ async function clearStorage(opts?: { deleteEverything?: boolean }): Promise<void
AbstractLocalStorageSettingsHandler.clear();

try {
await StorageManager.idbDelete("account", "mx_access_token");
await StorageManager.idbDelete("account", ACCESS_TOKEN_STORAGE_KEY);
} catch (e) {
logger.error("idbDelete failed for account:mx_access_token", e);
}
Expand Down
1 change: 1 addition & 0 deletions src/MatrixClientPeg.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ export interface IMatrixClientCreds {
userId: string;
deviceId?: string;
accessToken: string;
refreshToken?: string;
guest?: boolean;
pickleKey?: string;
freshLogin?: boolean;
Expand Down
2 changes: 2 additions & 0 deletions src/utils/oidc/authorize.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ export const completeOidcLogin = async (
homeserverUrl: string;
identityServerUrl?: string;
accessToken: string;
refreshToken?: string;
richvdh marked this conversation as resolved.
Show resolved Hide resolved
}> => {
const { code, state } = getCodeAndStateFromQueryParams(queryParams);
const { homeserverUrl, tokenResponse, identityServerUrl } = await completeAuthorizationCodeGrant(code, state);
Expand All @@ -91,5 +92,6 @@ export const completeOidcLogin = async (
homeserverUrl: homeserverUrl,
identityServerUrl: identityServerUrl,
accessToken: tokenResponse.access_token,
refreshToken: tokenResponse.refresh_token,
richvdh marked this conversation as resolved.
Show resolved Hide resolved
};
};
14 changes: 14 additions & 0 deletions test/Lifecycle-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,8 @@ describe("Lifecycle", () => {
jest.spyOn(mockPlatform, "createPickleKey");
});

const refreshToken = "test-refresh-token";

it("should remove fresh login flag from session storage", async () => {
await setLoggedIn(credentials);

Expand Down Expand Up @@ -392,6 +394,18 @@ describe("Lifecycle", () => {
expect(localStorage.setItem).not.toHaveBeenCalledWith("mx_access_token", accessToken);
});

it("should persist a refreshToken when present", async () => {
await setLoggedIn({
...credentials,
refreshToken,
});

expect(StorageManager.idbSave).toHaveBeenCalledWith("account", "mx_access_token", accessToken);
expect(StorageManager.idbSave).toHaveBeenCalledWith("account", "mx_refresh_token", refreshToken);
// dont put accessToken in localstorage when we have idb
expect(localStorage.setItem).not.toHaveBeenCalledWith("mx_access_token", accessToken);
});

it("should remove any access token from storage when there is none in credentials and idb save fails", async () => {
jest.spyOn(StorageManager, "idbSave").mockRejectedValue("oups");
await setLoggedIn({
Expand Down
1 change: 1 addition & 0 deletions test/components/structures/MatrixChat-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -908,6 +908,7 @@ describe("<MatrixChat />", () => {
expect(localStorageSetSpy).toHaveBeenCalledWith("mx_hs_url", homeserverUrl);
expect(localStorageSetSpy).toHaveBeenCalledWith("mx_user_id", userId);
expect(localStorageSetSpy).toHaveBeenCalledWith("mx_has_access_token", "true");
expect(localStorageSetSpy).toHaveBeenCalledWith("mx_has_refresh_token", "true");
expect(localStorageSetSpy).toHaveBeenCalledWith("mx_device_id", deviceId);
});

Expand Down
1 change: 1 addition & 0 deletions test/utils/oidc/authorize-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ describe("OIDC authorization", () => {

expect(result).toEqual({
accessToken: tokenResponse.access_token,
refreshToken: tokenResponse.refresh_token,
homeserverUrl,
identityServerUrl,
});
Expand Down