From c97ec8f1119616fb28a0274b1fbdf55b1f563d24 Mon Sep 17 00:00:00 2001
From: Aofei Sheng <aofei@aofeisheng.com>
Date: Fri, 18 Oct 2024 09:30:32 +0800
Subject: [PATCH] refactor(auth): remove refresh token expiration handling

- Remove handling of refresh token expiration as Casdoor never returns
`refresh_expires_in`[^1].
- Treat refresh tokens as always valid as they are renewed with each
access token refresh[^2].
- Return `401 Unauthorized` instead of `500 Internal Server Error` when
JWT parsing fails.

[^1]: https://github.com/casdoor/casdoor/blob/6f1f93725e77c8288aa0ac1c0d996feff906ddcf/object/token_oauth.go#L383-L390
[^2]: https://github.com/casdoor/casdoor/blob/6f1f93725e77c8288aa0ac1c0d996feff906ddcf/object/token_oauth.go#L351-L376

Signed-off-by: Aofei Sheng <aofei@aofeisheng.com>
---
 spx-backend/internal/controller/user.go       |  2 +-
 spx-backend/internal/controller/user_test.go  |  2 +-
 .../community/user/FollowButton.vue           |  2 +-
 .../src/components/navbar/NavbarProfile.vue   | 13 ++--
 .../components/project/ProjectCreateModal.vue |  7 ++-
 spx-gui/src/pages/community/home.vue          | 16 ++---
 spx-gui/src/pages/editor/index.vue            | 18 +++---
 spx-gui/src/stores/user.ts                    | 63 +++++++------------
 8 files changed, 56 insertions(+), 67 deletions(-)

diff --git a/spx-backend/internal/controller/user.go b/spx-backend/internal/controller/user.go
index 75ce3626..2c598b42 100644
--- a/spx-backend/internal/controller/user.go
+++ b/spx-backend/internal/controller/user.go
@@ -69,7 +69,7 @@ func ensureUser(ctx context.Context, expectedUserID int64) (*model.User, error)
 func (ctrl *Controller) UserFromToken(ctx context.Context, token string) (*model.User, error) {
 	claims, err := ctrl.casdoorClient.ParseJwtToken(token)
 	if err != nil {
-		return nil, fmt.Errorf("ctrl.casdoorClient.ParseJwtToken failed: %w", err)
+		return nil, fmt.Errorf("ctrl.casdoorClient.ParseJwtToken failed: %w: %w", ErrUnauthorized, err)
 	}
 	mUser, err := model.FirstOrCreateUser(ctx, ctrl.db, claims.Name)
 	if err != nil {
diff --git a/spx-backend/internal/controller/user_test.go b/spx-backend/internal/controller/user_test.go
index 43798a14..9ea5fc45 100644
--- a/spx-backend/internal/controller/user_test.go
+++ b/spx-backend/internal/controller/user_test.go
@@ -133,7 +133,7 @@ func TestControllerUserFromToken(t *testing.T) {
 
 		_, err := ctrl.UserFromToken(context.Background(), "invalid-token")
 		require.Error(t, err)
-		assert.EqualError(t, err, "ctrl.casdoorClient.ParseJwtToken failed: token contains an invalid number of segments")
+		assert.EqualError(t, err, "ctrl.casdoorClient.ParseJwtToken failed: unauthorized: token contains an invalid number of segments")
 	})
 }
 
diff --git a/spx-gui/src/components/community/user/FollowButton.vue b/spx-gui/src/components/community/user/FollowButton.vue
index aff43d6d..d7a81fd7 100644
--- a/spx-gui/src/components/community/user/FollowButton.vue
+++ b/spx-gui/src/components/community/user/FollowButton.vue
@@ -10,7 +10,7 @@ const props = defineProps<{
   name: string
 }>()
 
-const followable = computed(() => props.name !== useUserStore().userInfo?.name)
+const followable = computed(() => props.name !== useUserStore().userInfo()?.name)
 const following = ref<boolean | null>(null)
 
 watch(
diff --git a/spx-gui/src/components/navbar/NavbarProfile.vue b/spx-gui/src/components/navbar/NavbarProfile.vue
index ac6def26..9f7f8881 100644
--- a/spx-gui/src/components/navbar/NavbarProfile.vue
+++ b/spx-gui/src/components/navbar/NavbarProfile.vue
@@ -1,5 +1,5 @@
 <template>
-  <div v-if="!userStore.userInfo" class="sign-in">
+  <div v-if="!userInfo" class="sign-in">
     <UIButton type="secondary" :disabled="!isOnline" @click="userStore.initiateSignIn()">{{
       $t({ en: 'Sign in', zh: '登录' })
     }}</UIButton>
@@ -7,13 +7,13 @@
   <UIDropdown v-else placement="bottom-end" :offset="{ x: -4, y: 8 }">
     <template #trigger>
       <div class="avatar">
-        <img class="avatar-img" :src="userStore.userInfo.avatar" />
+        <img class="avatar-img" :src="userInfo.avatar" />
       </div>
     </template>
     <UIMenu class="user-menu">
       <UIMenuGroup>
         <UIMenuItem :interactive="false">
-          {{ userStore.userInfo.displayName || userStore.userInfo.name }}
+          {{ userInfo.displayName || userInfo.name }}
         </UIMenuItem>
       </UIMenuGroup>
       <UIMenuGroup>
@@ -39,17 +39,20 @@ import { useNetwork } from '@/utils/network'
 import { getUserPageRoute } from '@/router'
 import { useUserStore } from '@/stores'
 import { UIButton, UIDropdown, UIMenu, UIMenuGroup, UIMenuItem } from '@/components/ui'
+import { computed } from 'vue'
 
 const userStore = useUserStore()
 const { isOnline } = useNetwork()
 const router = useRouter()
 
+const userInfo = computed(() => userStore.userInfo())
+
 function handleUserPage() {
-  router.push(getUserPageRoute(userStore.userInfo!.name))
+  router.push(getUserPageRoute(userInfo.value!.name))
 }
 
 function handleProjects() {
-  router.push(getUserPageRoute(userStore.userInfo!.name, 'projects'))
+  router.push(getUserPageRoute(userInfo.value!.name, 'projects'))
 }
 </script>
 
diff --git a/spx-gui/src/components/project/ProjectCreateModal.vue b/spx-gui/src/components/project/ProjectCreateModal.vue
index 1cb90aff..bb346c27 100644
--- a/spx-gui/src/components/project/ProjectCreateModal.vue
+++ b/spx-gui/src/components/project/ProjectCreateModal.vue
@@ -58,6 +58,7 @@ import defaultSpritePng from '@/assets/default-sprite.png'
 import defaultBackdropImg from '@/assets/default-backdrop.png'
 import { Backdrop } from '@/models/backdrop'
 import { Project } from '@/models/project'
+import { computed } from 'vue'
 
 const props = defineProps<{
   visible: boolean
@@ -71,6 +72,8 @@ const emit = defineEmits<{
 const { t } = useI18n()
 const userStore = useUserStore()
 
+const userInfo = computed(() => userStore.userInfo())
+
 const form = useForm({
   name: ['', validateName]
 })
@@ -134,8 +137,8 @@ async function validateName(name: string): Promise<FormValidationResult> {
     })
 
   // check naming conflict
-  if (userStore.userInfo == null) throw new Error('login required')
-  const username = userStore.userInfo.name
+  if (userInfo.value == null) throw new Error('login required')
+  const username = userInfo.value.name
   const existedProject = await getProject(username, name).catch((e) => {
     if (e instanceof ApiException && e.code === ApiExceptionCode.errorNotFound) return null
     throw e
diff --git a/spx-gui/src/pages/community/home.vue b/spx-gui/src/pages/community/home.vue
index 36560421..43263993 100644
--- a/spx-gui/src/pages/community/home.vue
+++ b/spx-gui/src/pages/community/home.vue
@@ -1,7 +1,7 @@
 <template>
   <CenteredWrapper class="main">
     <ProjectsSection
-      v-if="userStore.isSignedIn"
+      v-if="userStore.isSignedIn()"
       context="home"
       :link-to="myProjectsRoute"
       :query-ret="myProjects"
@@ -84,7 +84,7 @@
       />
     </ProjectsSection>
     <ProjectsSection
-      v-if="userStore.isSignedIn"
+      v-if="userStore.isSignedIn()"
       context="home"
       :link-to="followingCreatedRoute"
       :query-ret="followingCreatedProjects"
@@ -128,14 +128,16 @@ const numInRow = 5 // at most 5 projects in a row, depending on the screen width
 
 const userStore = useUserStore()
 
+const userInfo = computed(() => userStore.userInfo())
+
 const myProjectsRoute = computed(() => {
-  if (userStore.userInfo == null) return ''
-  return getUserPageRoute(userStore.userInfo.name, 'projects')
+  if (userInfo.value == null) return ''
+  return getUserPageRoute(userInfo.value.name, 'projects')
 })
 
 const myProjects = useQuery(
   async () => {
-    if (userStore.userInfo == null) return []
+    if (userInfo.value == null) return []
     const { data: projects } = await listProject({
       pageIndex: 1,
       pageSize: numInRow,
@@ -162,13 +164,13 @@ const communityRemixingProjects = useQuery(
 )
 
 const followingCreatedRoute = computed(() => {
-  if (userStore.userInfo == null) return ''
+  if (userInfo.value == null) return ''
   return getExploreRoute(ExploreOrder.FollowingCreated)
 })
 
 const followingCreatedProjects = useQuery(
   async () => {
-    if (userStore.userInfo == null) return []
+    if (userInfo.value == null) return []
     return exploreProjects({ order: ExploreOrder.FollowingCreated, count: numInRow })
   },
   { en: 'Failed to load projects', zh: '加载失败' }
diff --git a/spx-gui/src/pages/editor/index.vue b/spx-gui/src/pages/editor/index.vue
index fdc745e0..07a1cf60 100644
--- a/spx-gui/src/pages/editor/index.vue
+++ b/spx-gui/src/pages/editor/index.vue
@@ -3,16 +3,12 @@
     <header class="editor-header">
       <EditorNavbar :project="project" />
     </header>
-    <main v-if="userStore.userInfo" class="editor-main">
+    <main v-if="userInfo" class="editor-main">
       <UILoading v-if="isLoading" />
       <UIError v-else-if="error != null" :retry="refetch">
         {{ $t(error.userMessage) }}
       </UIError>
-      <EditorContextProvider
-        v-else-if="project != null"
-        :project="project"
-        :user-info="userStore.userInfo"
-      >
+      <EditorContextProvider v-else-if="project != null" :project="project" :user-info="userInfo">
         <ProjectEditor />
       </EditorContextProvider>
     </main>
@@ -20,7 +16,7 @@
 </template>
 
 <script setup lang="ts">
-import { watchEffect, watch, onMounted, onUnmounted } from 'vue'
+import { watchEffect, watch, onMounted, onUnmounted, computed } from 'vue'
 import { useRouter } from 'vue-router'
 import { useUserStore } from '@/stores'
 import { AutoSaveMode, Project } from '@/models/project'
@@ -45,11 +41,13 @@ const userStore = useUserStore()
 watchEffect(() => {
   // This will be called on mount and whenever userStore changes,
   // which are the cases when userStore.signOut() is called
-  if (!userStore.isSignedIn) {
+  if (!userStore.isSignedIn()) {
     userStore.initiateSignIn()
   }
 })
 
+const userInfo = computed(() => userStore.userInfo())
+
 const router = useRouter()
 
 const withConfirm = useConfirmDialog()
@@ -113,9 +111,9 @@ const {
   refetch
 } = useQuery(
   () => {
-    // We need to read `userStore.userInfo.name` & `projectName.value` synchronously,
+    // We need to read `userInfo.value?.name` & `projectName.value` synchronously,
     // so their change will drive `useQuery` to re-fetch
-    return loadProject(userStore.userInfo?.name, props.projectName)
+    return loadProject(userInfo.value?.name, props.projectName)
   },
   { en: 'Failed to load project', zh: '加载项目失败' }
 )
diff --git a/spx-gui/src/stores/user.ts b/spx-gui/src/stores/user.ts
index c6ad6fdb..a54a6f04 100644
--- a/spx-gui/src/stores/user.ts
+++ b/spx-gui/src/stores/user.ts
@@ -12,9 +12,8 @@ export interface UserInfo {
 
 interface TokenResponse {
   access_token: string
-  refresh_token: string
   expires_in: number
-  refresh_expires_in: number
+  refresh_token: string
 }
 
 const casdoorAuthRedirectPath = '/callback'
@@ -28,35 +27,9 @@ const tokenExpiryDelta = 60 * 1000 // 1 minute in milliseconds
 export const useUserStore = defineStore('spx-user', {
   state: () => ({
     accessToken: null as string | null,
-    refreshToken: null as string | null,
-
-    // timestamp in milliseconds, null if never expires
-    accessTokenExpiresAt: null as number | null,
-    refreshTokenExpiresAt: null as number | null
+    accessTokenExpiresAt: null as number | null, // timestamp in milliseconds, null if never expires
+    refreshToken: null as string | null
   }),
-  getters: {
-    isAccessTokenValid(): boolean {
-      return !!(
-        this.accessToken &&
-        (this.accessTokenExpiresAt === null ||
-          this.accessTokenExpiresAt - tokenExpiryDelta > Date.now())
-      )
-    },
-    isRefreshTokenValid(): boolean {
-      return !!(
-        this.refreshToken &&
-        (this.refreshTokenExpiresAt === null ||
-          this.refreshTokenExpiresAt - tokenExpiryDelta > Date.now())
-      )
-    },
-    isSignedIn(): boolean {
-      return this.isAccessTokenValid || this.isRefreshTokenValid
-    },
-    userInfo(): UserInfo | null {
-      if (!this.isSignedIn) return null
-      return jwtDecode<UserInfo>(this.accessToken!)
-    }
-  },
   actions: {
     initiateSignIn(
       returnTo: string = window.location.pathname + window.location.search + window.location.hash
@@ -74,16 +47,15 @@ export const useUserStore = defineStore('spx-user', {
     },
     signOut() {
       this.accessToken = null
-      this.refreshToken = null
       this.accessTokenExpiresAt = null
-      this.refreshTokenExpiresAt = null
+      this.refreshToken = null
     },
     async ensureAccessToken(): Promise<string | null> {
-      if (this.isAccessTokenValid) return this.accessToken
+      if (this.isAccessTokenValid()) return this.accessToken
 
-      if (this.isRefreshTokenValid) {
+      if (this.refreshToken != null) {
         try {
-          const resp = await casdoorSdk.refreshAccessToken(this.refreshToken!)
+          const resp = await casdoorSdk.refreshAccessToken(this.refreshToken)
           this.handleTokenResponse(resp)
         } catch (e) {
           console.error('failed to refresh access token', e)
@@ -93,7 +65,7 @@ export const useUserStore = defineStore('spx-user', {
         // Due to casdoor-js-sdk's lack of error handling, we must check if the access token is valid after calling
         // `casdoorSdk.refreshAccessToken`. The token might still be invalid if, e.g., the server has already revoked
         // the refresh token.
-        if (this.isAccessTokenValid) return this.accessToken
+        if (this.isAccessTokenValid()) return this.accessToken
       }
 
       this.signOut()
@@ -101,11 +73,22 @@ export const useUserStore = defineStore('spx-user', {
     },
     handleTokenResponse(resp: TokenResponse) {
       this.accessToken = resp.access_token
-      this.refreshToken = resp.refresh_token
       this.accessTokenExpiresAt = resp.expires_in ? Date.now() + resp.expires_in * 1000 : null
-      this.refreshTokenExpiresAt = resp.refresh_expires_in
-        ? Date.now() + resp.refresh_expires_in * 1000
-        : null
+      this.refreshToken = resp.refresh_token
+    },
+    isAccessTokenValid(): boolean {
+      return !!(
+        this.accessToken &&
+        (this.accessTokenExpiresAt === null ||
+          this.accessTokenExpiresAt - tokenExpiryDelta > Date.now())
+      )
+    },
+    isSignedIn(): boolean {
+      return this.isAccessTokenValid()
+    },
+    userInfo(): UserInfo | null {
+      if (!this.isSignedIn()) return null
+      return jwtDecode<UserInfo>(this.accessToken!)
     }
   },
   persist: true