[ALS-0000] Fix bugs in session token lifecycle (#78)

hms-dbmi · Jun 27, 2024 · baaef46 · baaef46
1 parent b1a5fdd
commit baaef46
Show file tree

Hide file tree

Showing 7 changed files with 86 additions and 27 deletions.
diff --git a/src/lib/api.ts b/src/lib/api.ts
@@ -29,7 +29,7 @@ async function send({
   }
 
   if (browser) {
-    const token = sessionStorage.getItem('token');
+    const token = localStorage.getItem('token');
     if (token) {
       opts.headers['Authorization'] = `Token ${token}`;
     }

diff --git a/src/lib/components/UserToken.svelte b/src/lib/components/UserToken.svelte
@@ -2,7 +2,13 @@
   import { getModalStore, getToastStore, ProgressRadial } from '@skeletonlabs/skeleton';
   import CopyButton from '$lib/components/buttons/CopyButton.svelte';
   import ErrorAlert from '$lib/components/ErrorAlert.svelte';
-  import { user, getUser, refreshToken as refresh } from '$lib/stores/User';
+  import {
+    user,
+    getUser,
+    getTokenExpiration,
+    getTokenExpirationAsDate,
+    refreshLongTermToken as refresh,
+  } from '$lib/stores/User';
 
   const modalStore = getModalStore();
   const toastStore = getToastStore();
@@ -46,7 +52,7 @@
   function extractExperationDate(token: string) {
     if (!token) return 0;
     try {
-      return JSON.parse(atob(token.split('.')[1])).exp * 1000;
+      return getTokenExpiration(token);
     } catch (error) {
       console.error(error);
       toastStore.trigger({
@@ -91,7 +97,9 @@
         <label for="expires">Expires:</label>
         <div>
           <span id="expires" class="w-1/3 mr-2"
-            >{new Date(expires).toString().substring(0, 24)}</span
+            >{getTokenExpirationAsDate($user?.token || '')
+              ?.toString()
+              ?.substring(0, 24)}</span
           >
           {#if badge}
             <span id="expires-badge" class="badge {badge}" data-testid="expires-badge"

diff --git a/src/lib/stores/User.ts b/src/lib/stores/User.ts
@@ -7,11 +7,24 @@ import type { User } from '$lib/models/User';
 import { PicsurePrivileges } from '$lib/models/Privilege';
 import { routes, features } from '$lib/configuration';
 
-function restoreUserSession() {
+export const user: Writable<User> = writable(restoreUser());
+
+user.subscribe((user: User) => {
+  if (browser) {
+    clearSessionTokenIfExpired();
+    localStorage.setItem('user', JSON.stringify(user));
+  }
+});
+
+function restoreUser() {
   if (browser && localStorage.getItem('user')) {
+    clearSessionTokenIfExpired();
     try {
       const user = JSON.parse(localStorage.getItem('user') || '{}');
-      console.log('Restored user from local storage:  ', user);
+      if (!user || Object.keys(user).length === 0) {
+        return {};
+      }
+      console.log('Restored user from local storage: ', user);
       return user;
     } catch (error) {
       console.error('Error reading user from local storage:  ' + error);
@@ -21,13 +34,15 @@ function restoreUserSession() {
   return {};
 }
 
-export const user: Writable<User> = writable(restoreUserSession());
-
-user.subscribe((value) => {
+function clearSessionTokenIfExpired() {
   if (browser) {
-    localStorage.setItem('user', JSON.stringify(value));
+    const token = localStorage.getItem('token');
+    if (token && isTokenExpired(token)) {
+      console.log('Clearing expired token from local storage.');
+      localStorage.removeItem('token');
+    }
   }
-});
+}
 
 export const userRoutes: Readable<Route[]> = derived(user, ($user) => {
   const userPrivileges: string[] = $user?.privileges || [];
@@ -68,7 +83,7 @@ export async function getUser(force?: boolean) {
   }
 }
 
-export async function refreshToken() {
+export async function refreshLongTermToken() {
   const newLongTermToken = await api
     .get('psama/user/me/refresh_long_term_token')
     .then((response: { userLongTermToken: string }) => {
@@ -82,14 +97,43 @@ export async function refreshToken() {
 
 export async function login(token: string) {
   if (browser && token) {
-    sessionStorage.setItem('token', token);
+    localStorage.setItem('token', token);
     await getUser(true);
   }
 }
 
 export async function logout() {
   if (browser) {
-    sessionStorage.removeItem('token');
+    localStorage.removeItem('token');
   }
   user.set({});
 }
+
+export function isTokenExpired(token: string) {
+  try {
+    return getTokenExpiration(token) < new Date().getTime();
+  } catch (error) {
+    console.error('Error checking token expiration: ' + error);
+    return true;
+  }
+}
+
+export function getTokenExpiration(token: string) {
+  if (!token) {
+    throw new Error('No token provided.');
+  }
+  try {
+    return JSON.parse(atob(token.split('.')[1])).exp * 1000;
+  } catch (error) {
+    throw new Error('Error parsing token: ' + error);
+  }
+}
+
+export function getTokenExpirationAsDate(token: string) {
+  try {
+    return new Date(getTokenExpiration(token));
+  } catch (error) {
+    console.error('Error getting token expiration as date: ' + error);
+    return undefined;
+  }
+}
diff --git a/src/routes/(picsure)/(authorized)/+layout.ts b/src/routes/(picsure)/(authorized)/+layout.ts
@@ -1,10 +1,13 @@
 import type { LayoutLoad } from './$types';
 import { browser } from '$app/environment';
 import { redirect } from '@sveltejs/kit';
+import { isTokenExpired } from '$lib/stores/User';
 
 export const load: LayoutLoad = ({ url }) => {
-  console.log('layout load');
-  if (browser && !sessionStorage.getItem('token')) {
+  if (
+    browser &&
+    (!localStorage.getItem('token') || isTokenExpired(localStorage.getItem('token') || ''))
+  ) {
     throw redirect(303, `/login?redirectTo=${url.pathname}`);
   }
 };
diff --git a/tests/custom-context.ts b/tests/custom-context.ts
@@ -33,6 +33,7 @@ export const unauthedTest = base.extend({
   context: async ({ context }, use) => {
     await context.addInitScript(() => {
       sessionStorage.clear();
+      localStorage.clear();
     });
 
     use(context);
@@ -42,12 +43,11 @@ export const unauthedTest = base.extend({
 export const test = base.extend({
   context: async ({ context }, use) => {
     await mockApiSuccess(context, '*/**/psama/user/me?hasToken', picsureUser);
-    await context.addInitScript(() => {
-      sessionStorage.setItem(
+    await context.addInitScript((picsureUser: User) => {
+      localStorage.setItem('user', JSON.stringify(picsureUser));
+      localStorage.setItem(
         'token',
-        'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwiZW1haWwiOiJ0Z' +
-          'XN0QHBpYy1zdXJlLm9yZyIsImV4cCI6MTYxMjE2NDk4MiwiaWF0IjoxNjA5NTcyOTgyfQ.kzaW-ZkhCPlTgdGQQAz_CA1ZB80PpZ5aiRa2' +
-          'lj46hbw',
+        'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwiZW1haWwiOiJ0ZXN0QHBpYy1zdXJlLm9yZyIsImV4cCI6OTYwOTU3Mjk4MiwiaWF0IjoxNjA5NTcyOTgyfQ.M1W7a3jQNoHQxAUwfj3sDqyVtNH_DkRdzsIF3prIYQA',
       );
     });
 
@@ -58,13 +58,12 @@ export const test = base.extend({
 export function getUserTest(user: User = picsureUser) {
   return base.extend({
     context: async ({ context }, use) => {
-      await context.addInitScript(() => {
-        sessionStorage.setItem(
+      await context.addInitScript((user: User) => {
+        localStorage.setItem(
           'token',
-          'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwiZW1haWwiOiJ0Z' +
-            'XN0QHBpYy1zdXJlLm9yZyIsImV4cCI6MTYxMjE2NDk4MiwiaWF0IjoxNjA5NTcyOTgyfQ.kzaW-ZkhCPlTgdGQQAz_CA1ZB80PpZ5aiRa2' +
-            'lj46hbw',
+          'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwiZW1haWwiOiJ0ZXN0QHBpYy1zdXJlLm9yZyIsImV4cCI6OTYwOTU3Mjk4MiwiaWF0IjoxNjA5NTcyOTgyfQ.M1W7a3jQNoHQxAUwfj3sDqyVtNH_DkRdzsIF3prIYQA',
         );
+        localStorage.setItem('user', JSON.stringify(user));
       });
 
       await mockApiSuccess(context, '*/**/psama/authentication', user);

diff --git a/tests/mock-data.ts b/tests/mock-data.ts
@@ -3,6 +3,8 @@ import { PicsurePrivileges } from '../src/lib/models/Privilege';
 import { resources } from '../src/lib/configuration';
 
 export const mockToken =
+  'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwiZW1haWwiOiJ0ZXN0QHBpYy1zdXJlLm9yZyIsImV4cCI6OTYwOTU3Mjk4MiwiaWF0IjoxNjA5NTcyOTgyfQ.M1W7a3jQNoHQxAUwfj3sDqyVtNH_DkRdzsIF3prIYQA';
+export const mockExpiredToken =
   'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwiZW1haWwiOiJ0ZXN0QHBpYy1zdXJlLm9yZyIsImV4cCI6MTYxMjE2NDk4MiwiaWF0IjoxNjA5NTcyOTgyfQ.kzaW-ZkhCPlTgdGQQAz_CA1ZB80PpZ5aiRa2lj46hbw';
 export const mockLoginResponse =
   '/login/loading?redirectTo=/&provider=AUTH0#access_token=' +

diff --git a/tests/routes/api/test.ts b/tests/routes/api/test.ts
@@ -1,7 +1,7 @@
 import { expect } from '@playwright/test';
 import { test, mockApiFail, mockApiSuccess } from '../../custom-context';
 import { branding } from '../../../src/lib/configuration';
-import { picsureUser, roles as mockRoles } from '../../../tests/mock-data';
+import { picsureUser, roles as mockRoles, mockExpiredToken } from '../../../tests/mock-data';
 
 const placeHolderDots =
   '••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••';
@@ -43,6 +43,9 @@ test.describe('API page', () => {
   });
   test('Has expected badge and expiration', async ({ page }) => {
     // Given
+    const user = picsureUser;
+    user.token = mockExpiredToken;
+    await mockApiSuccess(page, '*/**/psama/user/me?hasToken', user);
     await page.goto('/api');
     // When
     const expires = page.locator('#expires');