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 all 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
128 changes: 93 additions & 35 deletions src/Lifecycle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,23 @@ import GenericToast from "./components/views/toasts/GenericToast";
const HOMESERVER_URL_KEY = "mx_hs_url";
const ID_SERVER_URL_KEY = "mx_is_url";

/*
* Keys used when storing the tokens in indexeddb or localstorage
*/
const ACCESS_TOKEN_STORAGE_KEY = "mx_access_token";
const REFRESH_TOKEN_STORAGE_KEY = "mx_refresh_token";
/*
* Used as initialization vector during encryption in persistTokenInStorage
* And decryption in restoreFromLocalStorage
*/
const ACCESS_TOKEN_IV = "access_token";
const REFRESH_TOKEN_IV = "refresh_token";
/*
* Keys for localstorage items which indicate whether we expect a token in indexeddb.
*/
const HAS_ACCESS_TOKEN_STORAGE_KEY = "mx_has_access_token";
const HAS_REFRESH_TOKEN_STORAGE_KEY = "mx_has_refresh_token";

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

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

const credentials = {
accessToken,
refreshToken,
homeserverUrl,
identityServerUrl,
deviceId,
Expand Down Expand Up @@ -503,25 +520,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(HAS_ACCESS_TOKEN_STORAGE_KEY) === "true" || !!accessToken;
const userId = localStorage.getItem("mx_user_id") ?? undefined;
const deviceId = localStorage.getItem("mx_device_id") ?? undefined;

Expand Down Expand Up @@ -607,7 +624,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_IV);
encrKey.fill(0);
}
} else {
Expand Down Expand Up @@ -846,28 +863,41 @@ 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 initializationVector Initialization vector for encrypting the token. Only used when `pickleKey` is present
* @param token the token to store, when undefined any existing token at the storageKey is removed from storage
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param token the token to store, when undefined any existing token at the storageKey is removed from storage
* @param token The token to store. When undefined, any existing token at the `storageKey` is removed from storage.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could still use clarification

* @param pickleKey optional pickle key used to encrypt token
* @param hasTokenStorageKey Localstorage key for an item which stores whether we expect to have a token in indexeddb, eg "mx_has_access_token".
*/
async function persistTokenInStorage(
storageKey: string,
initializationVector: string,
token: string | undefined,
pickleKey: string | undefined,
hasTokenStorageKey: string,
): Promise<void> {
// 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, initializationVector);
encrKey.fill(0);
} catch (e) {
logger.warn("Could not encrypt access token", e);
Expand All @@ -876,28 +906,56 @@ 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.
Dismissed
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.
Dismissed
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);
Dismissed Show dismissed Hide dismissed
if (credentials.identityServerUrl) {
localStorage.setItem(ID_SERVER_URL_KEY, credentials.identityServerUrl);
Dismissed Show dismissed Hide dismissed
}
localStorage.setItem("mx_user_id", credentials.userId);
Dismissed Show dismissed Hide dismissed
localStorage.setItem("mx_is_guest", JSON.stringify(credentials.guest));
Dismissed Show dismissed Hide dismissed

await persistTokenInStorage(
ACCESS_TOKEN_STORAGE_KEY,
ACCESS_TOKEN_IV,
credentials.accessToken,
credentials.pickleKey,
HAS_ACCESS_TOKEN_STORAGE_KEY,
);
await persistTokenInStorage(
REFRESH_TOKEN_STORAGE_KEY,
REFRESH_TOKEN_IV,
credentials.refreshToken,
credentials.pickleKey,
HAS_REFRESH_TOKEN_STORAGE_KEY,
);

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 @@ -1090,7 +1148,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 @@ -56,6 +56,7 @@ export interface IMatrixClientCreds {
userId: string;
deviceId?: string;
accessToken: string;
refreshToken?: string;
guest?: boolean;
pickleKey?: string;
freshLogin?: boolean;
Expand Down
33 changes: 19 additions & 14 deletions src/utils/oidc/authorize.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,31 +68,36 @@ const getCodeAndStateFromQueryParams = (queryParams: QueryDict): { code: string;
return { code, state };
};

/**
* Attempt to complete authorization code flow to get an access token
* @param queryParams the query-parameters extracted from the real query-string of the starting URI.
* @returns Promise that resolves with accessToken, identityServerUrl, and homeserverUrl when login was successful
* @throws When we failed to get a valid access token
*/
export const completeOidcLogin = async (
queryParams: QueryDict,
): Promise<{
type CompleteOidcLoginResponse = {
// url of the homeserver selected during login
homeserverUrl: string;
// identity server url as discovered during login
identityServerUrl?: string;
// accessToken gained from OIDC token issuer
accessToken: string;
// refreshToken gained from OIDC token issuer, when falsy token cannot be refreshed
refreshToken?: string;
// this client's id as registered with the OIDC issuer
clientId: string;
// issuer used during authentication
issuer: string;
}> => {
};
/**
* Attempt to complete authorization code flow to get an access token
* @param queryParams the query-parameters extracted from the real query-string of the starting URI.
* @returns Promise that resolves with a CompleteOidcLoginResponse when login was successful
* @throws When we failed to get a valid access token
*/
export const completeOidcLogin = async (queryParams: QueryDict): Promise<CompleteOidcLoginResponse> => {
const { code, state } = getCodeAndStateFromQueryParams(queryParams);
const { homeserverUrl, tokenResponse, identityServerUrl, oidcClientSettings } =
await completeAuthorizationCodeGrant(code, state);

// @TODO(kerrya) do something with the refresh token https://github.com/vector-im/element-web/issues/25444

return {
homeserverUrl: homeserverUrl,
identityServerUrl: identityServerUrl,
homeserverUrl,
identityServerUrl,
accessToken: tokenResponse.access_token,
refreshToken: tokenResponse.refresh_token,
richvdh marked this conversation as resolved.
Show resolved Hide resolved
clientId: oidcClientSettings.clientId,
issuer: oidcClientSettings.issuer,
};
Expand Down
14 changes: 14 additions & 0 deletions test/Lifecycle-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,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 @@ -410,6 +412,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/utils/oidc/authorize-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ describe("OIDC authorization", () => {

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