Improve expired token workflow

Don't redirect the user on the login page (that could be confusing): display a sticky notification and an error page instead
Chocobozzz · Oct 24, 2024 · d11da8d · d11da8d
1 parent 05b5483
commit d11da8d
Show file tree

Hide file tree

Showing 8 changed files with 100 additions and 80 deletions.
diff --git a/client/src/app/+error-page/error-page.component.html b/client/src/app/+error-page/error-page.component.html
@@ -1,58 +1,74 @@
 <div class="root">
-  <div *ngIf="status !== 401 && status !== 403 && status !== 418" class="box">
-    <strong>{{ status }}.</strong>
-    <span class="ms-1 muted" i18n>That's an error.</span>
+  @if (status === 401) {
+    <div class="box">
+      <strong>{{ status }}.</strong>
+      <span class="ms-1 muted" i18n>You are not authorized here.</span>
 
-    <div class="text mt-4">
-      <ng-container *ngIf="type === 'video'" i18n>We couldn't find any video tied to the URL {{ pathname }} you were looking for.</ng-container>
-      <ng-container *ngIf="type !== 'video'" i18n>We couldn't find any resource tied to the URL {{ pathname }} you were looking for.</ng-container>
-    </div>
+      <div class="text mt-4">
+        @if (type === 'video') {
+          <ng-container i18n>You might need to login to see the video.</ng-container>
+        } @else {
+          <ng-container i18n>You might need to login to see the resource.</ng-container>
+        }
+      </div>
 
-    <div class="muted mt-4">
-      <span i18n="Possible reasons preceding a list of reasons a `Not Found` error page may occur">Possible reasons:</span>
-
-      <ul>
-        <li i18n>You may have used an outdated or broken link</li>
-        <li>
-          <ng-container *ngIf="type === 'video'" i18n>The video may have been moved or deleted</ng-container>
-          <ng-container *ngIf="type !== 'video'" i18n>The resource may have been moved or deleted</ng-container>
-        </li>
-        <li i18n>You may have typed the address or URL incorrectly</li>
-      </ul>
+      <my-login-link className="peertube-button-big-link orange-button mt-5"></my-login-link>
     </div>
-  </div>
+  } @else if (status === 403) {
+    <div class="box">
+      <strong>{{ status }}.</strong>
+      <span class="ms-1 muted" i18n>You are not authorized here.</span>
 
-  <div *ngIf="status === 401" class="box">
-    <strong>{{ status }}.</strong>
-    <span class="ms-1 muted" i18n>You are not authorized here.</span>
+      <div class="text mt-4">
+        @if (type === 'video') {
+          <ng-container i18n>You might need to check your account is allowed by the video or instance owner.</ng-container>
+        } @else {
+          <ng-container i18n>You might need to check your account is allowed by the resource or instance owner.</ng-container>
+        }
+      </div>
+    </div>
+  } @else if (status === 418) {
+    <div class="box">
+      <strong>{{ status }}.</strong>
+      <span class="ms-1 muted">I'm a teapot.</span>
 
-    <div class="text mt-4">
-      <ng-container *ngIf="type === 'video'" i18n>You might need to login to see the video.</ng-container>
-      <ng-container *ngIf="type !== 'video'" i18n>You might need to login to see the resource.</ng-container>
+      <div class="text mt-4" i18n="Description of a tea flavour, keeping the 'requested entity body' as a technical expression referring to a web request">
+        The requested entity body blends sweet bits with a mellow earthiness.
+      </div>
+      <div class="muted" i18n="This is about Sepia's tea">Sepia seems to like it.</div>
     </div>
+  } @else {
+    <div class="box">
+      <strong>{{ status }}.</strong>
+      <span class="ms-1 muted" i18n>That's an error.</span>
 
-    <my-login-link className="peertube-button-big-link orange-button mt-5"></my-login-link>
-  </div>
+      <div class="text mt-4">
+        @if (type === 'video') {
+          <ng-container i18n>We couldn't find any video tied to the URL {{ pathname }} you were looking for.</ng-container>
+        } @else {
+          <ng-container i18n>We couldn't find any resource tied to the URL {{ pathname }} you were looking for.</ng-container>
+        }
+      </div>
 
-  <div *ngIf="status === 403" class="box">
-    <strong>{{ status }}.</strong>
-    <span class="ms-1 muted" i18n>You are not authorized here.</span>
+      <div class="muted mt-4">
+        <span i18n="Possible reasons preceding a list of reasons a `Not Found` error page may occur">Possible reasons:</span>
 
-    <div class="text mt-4">
-      <ng-container *ngIf="type === 'video'" i18n>You might need to check your account is allowed by the video or instance owner.</ng-container>
-      <ng-container *ngIf="type !== 'video'" i18n>You might need to check your account is allowed by the resource or instance owner.</ng-container>
-    </div>
-  </div>
+        <ul>
+          <li i18n>You may have used an outdated or broken link</li>
 
-  <div *ngIf="status === 418" class="box">
-    <strong>{{ status }}.</strong>
-    <span class="ms-1 muted">I'm a teapot.</span>
+          <li>
+            @if (type === 'video') {
+              <ng-container i18n>The video may have been moved or deleted</ng-container>
+            } @else {
+              <ng-container i18n>The resource may have been moved or deleted</ng-container>
+            }
+          </li>
 
-    <div class="text mt-4" i18n="Description of a tea flavour, keeping the 'requested entity body' as a technical expression referring to a web request">
-      The requested entity body blends sweet bits with a mellow earthiness.
+          <li i18n>You may have typed the address or URL incorrectly</li>
+        </ul>
+      </div>
     </div>
-    <div class="muted" i18n="This is about Sepia's tea">Sepia seems to like it.</div>
-  </div>
+  }
 
   <img src='/client/assets/images/mascot/{{ getMascotName() }}.svg' alt='{{ status }} mascot' class="mb-4 h-auto">
 </div>
diff --git a/client/src/app/app.component.html b/client/src/app/app.component.html
@@ -60,12 +60,12 @@
 <p-toast position="bottom-right">
   <ng-template let-message pTemplate="message">
     <div class="notification-block">
+      <my-global-icon [iconName]="getNotificationIcon(message)"></my-global-icon>
+
       <div class="message">
         <h3>{{ message.summary }}</h3>
         <p>{{ message.detail }}</p>
       </div>
-
-      <my-global-icon [iconName]="getNotificationIcon(message)"></my-global-icon>
     </div>
   </ng-template>
 </p-toast>

diff --git a/client/src/app/app.component.ts b/client/src/app/app.component.ts
@@ -294,12 +294,12 @@ export class AppComponent implements OnInit, AfterViewInit {
     // Admin modal
     userSub.pipe(
       filter(user => user.role.id === UserRole.ADMINISTRATOR)
-    ).subscribe(user => this.openAdminModalsIfNeeded(user))
+    ).subscribe(user => setTimeout(() => this.openAdminModalsIfNeeded(user))) // Wait deferred modal creation in the view
 
     // Account modal
     userSub.pipe(
       filter(user => user.role.id !== UserRole.ADMINISTRATOR)
-    ).subscribe(user => this.openAccountModalsIfNeeded(user))
+    ).subscribe(user => setTimeout(() => this.openAccountModalsIfNeeded(user))) // Wait deferred modal creation in the view
   }
 
   private openAdminModalsIfNeeded (user: User) {

diff --git a/client/src/app/core/auth/auth.service.ts b/client/src/app/core/auth/auth.service.ts
@@ -1,15 +1,14 @@
-import { Hotkey, HotkeysService } from '@app/core'
-import { Observable, ReplaySubject, Subject, throwError as observableThrowError } from 'rxjs'
-import { catchError, map, mergeMap, share, tap } from 'rxjs/operators'
 import { HttpClient, HttpErrorResponse, HttpHeaders, HttpParams } from '@angular/common/http'
 import { Injectable } from '@angular/core'
 import { Router } from '@angular/router'
+import { Hotkey, HotkeysService } from '@app/core'
 import { Notifier } from '@app/core/notification/notifier.service'
+import { HttpStatusCode, OAuthClientLocal, User, UserLogin, UserRefreshToken, MyUser as UserServerModel } from '@peertube/peertube-models'
 import { logger, OAuthUserTokens, objectToUrlEncoded, peertubeLocalStorage } from '@root-helpers/index'
-import { HttpStatusCode, MyUser as UserServerModel, OAuthClientLocal, User, UserLogin, UserRefreshToken } from '@peertube/peertube-models'
+import { Observable, of, ReplaySubject, Subject, throwError } from 'rxjs'
+import { catchError, map, mergeMap, share, tap } from 'rxjs/operators'
 import { environment } from '../../../environments/environment'
 import { RestExtractor } from '../rest/rest-extractor.service'
-import { RedirectService } from '../routing'
 import { AuthStatus } from './auth-status.model'
 import { AuthUser } from './auth-user.model'
 
@@ -42,10 +41,9 @@ export class AuthService {
   private clientSecret: string = peertubeLocalStorage.getItem(AuthService.LOCAL_STORAGE_OAUTH_CLIENT_KEYS.CLIENT_SECRET)
   private loginChanged: Subject<AuthStatus>
   private user: AuthUser = null
-  private refreshingTokenObservable: Observable<any>
+  private refreshingTokenObservable: Observable<void>
 
   constructor (
-    private redirectService: RedirectService,
     private http: HttpClient,
     private notifier: Notifier,
     private hotkeysService: HotkeysService,
@@ -180,18 +178,20 @@ Ensure you have correctly configured PeerTube (config/ directory), in particular
 
   logout () {
     const authHeaderValue = this.getRequestHeaderValue()
-    const headers = new HttpHeaders().set('Authorization', authHeaderValue)
 
-    this.http.post<{ redirectUrl?: string }>(AuthService.BASE_REVOKE_TOKEN_URL, {}, { headers })
-      .subscribe({
-        next: res => {
-          if (res.redirectUrl) {
-            window.location.href = res.redirectUrl
-          }
-        },
+    const obs: Observable<{ redirectUrl?: string }> = authHeaderValue
+      ? this.http.post(AuthService.BASE_REVOKE_TOKEN_URL, {}, { headers: new HttpHeaders().set('Authorization', authHeaderValue) })
+      : of({})
 
-        error: err => logger.error(err)
-      })
+    obs.subscribe({
+      next: res => {
+        if (res.redirectUrl) {
+          window.location.href = res.redirectUrl
+        }
+      },
+
+      error: err => logger.error(err)
+    })
 
     this.user = null
 
@@ -200,6 +200,7 @@ Ensure you have correctly configured PeerTube (config/ directory), in particular
 
   refreshAccessToken () {
     if (this.refreshingTokenObservable) return this.refreshingTokenObservable
+    if (!this.getAccessToken()) return throwError(() => new Error($localize`You need to reconnect`))
 
     logger.info('Refreshing token...')
 
@@ -221,17 +222,13 @@ Ensure you have correctly configured PeerTube (config/ directory), in particular
           this.refreshingTokenObservable = null
         }),
         catchError(err => {
-          this.refreshingTokenObservable = null
-
-          logger.error(err)
-          logger.info('Cannot refresh token -> logout...')
+          logger.clientError(err)
           this.logout()
 
-          this.redirectService.redirectToLogin()
+          this.notifier.info($localize`Your authentication has expired, you need to reconnect.`, undefined, undefined, true)
+          this.refreshingTokenObservable = null
 
-          return observableThrowError(() => ({
-            error: $localize`You need to reconnect.`
-          }))
+          return throwError(() => new Error($localize`You need to reconnect`))
         }),
         share()
       )

diff --git a/client/src/app/core/routing/login-guard.service.ts b/client/src/app/core/routing/login-guard.service.ts
@@ -14,7 +14,10 @@ export class LoginGuard {
   canActivate (route: ActivatedRouteSnapshot, state: RouterStateSnapshot) {
     if (this.auth.isLoggedIn() === true) return true
 
-    this.redirectService.redirectToLogin()
+    const err = new Error('') as any
+    err.status = 401
+
+    this.redirectService.replaceBy401(err)
     return false
   }
 

diff --git a/client/src/app/core/routing/redirect.service.ts b/client/src/app/core/routing/redirect.service.ts
@@ -112,6 +112,10 @@ export class RedirectService {
     else this.router.navigate([ '/login' ])
   }
 
+  replaceBy401 (err: Error) {
+    this.router.navigate([ '/401' ], { state: { obj: err }, skipLocationChange: true })
+  }
+
   private doRedirect (redirectUrl: string, fallbackRoute?: string) {
     debugLogger('Redirecting on %s', redirectUrl)
 

diff --git a/client/src/app/core/routing/user-right-guard.service.ts b/client/src/app/core/routing/user-right-guard.service.ts
@@ -19,7 +19,11 @@ export class UserRightGuard {
       if (user.hasRight(neededUserRight)) return true
     }
 
-    this.redirectService.redirectToLogin()
+    const err = new Error('') as any
+    err.status = 403
+
+    this.redirectService.replaceBy401(err)
+
     return false
   }
 

diff --git a/client/src/sass/primeng-custom.scss b/client/src/sass/primeng-custom.scss
@@ -1088,7 +1088,7 @@ p-toast {
     min-width: 200px;
 
     .p-toast-icon-close {
-      opacity: 0;
+      color: pvar(--greyForegroundColor);
       position: absolute;
       right: 5px;
       top: 5px;
@@ -1097,10 +1097,6 @@ p-toast {
       background: url('../assets/images/feather/x.svg') no-repeat;
       background-size: contain;
     }
-
-    &:hover .p-toast-icon-close {
-      opacity: .3;
-    }
   }
 
   .p-toast-message {
@@ -1117,10 +1113,10 @@ p-toast {
       display: flex;
       align-items: center;
       width: 100%;
-      padding: 10px 20px;
+      padding: 10px;
 
       .message {
-        @include margin-right(20px);
+        @include margin-left(10px);
 
         flex-grow: 1;