From 6a04d3ed0b7a3d24ced4b20993a8891f5357b443 Mon Sep 17 00:00:00 2001 From: Sean Engelhardt Date: Tue, 23 Apr 2019 13:23:18 +0200 Subject: [PATCH] Fix AuthGuard race conditions and false redirects Fixes an error where the auth guard had race conditions with the OpenSlides bootup routine (kinda hacky tbh) Fixes false redirects in the Auth.Service Also adjustes the file uploader to be a little more usable. --- .../core/core-services/auth-guard.service.ts | 13 +++++- .../app/core/core-services/auth.service.ts | 4 +- .../core/core-services/openslides.service.ts | 4 +- .../core/core-services/operator.service.ts | 8 ++++ client/src/app/core/deferred.ts | 42 +++++++++++++++++++ .../media-upload-content.component.html | 14 +++++-- 6 files changed, 78 insertions(+), 7 deletions(-) create mode 100644 client/src/app/core/deferred.ts diff --git a/client/src/app/core/core-services/auth-guard.service.ts b/client/src/app/core/core-services/auth-guard.service.ts index 6eec99ac2..10eba1a4d 100644 --- a/client/src/app/core/core-services/auth-guard.service.ts +++ b/client/src/app/core/core-services/auth-guard.service.ts @@ -2,6 +2,7 @@ import { Injectable } from '@angular/core'; import { CanActivate, ActivatedRouteSnapshot, CanActivateChild, Router } from '@angular/router'; import { OperatorService } from './operator.service'; +import { OpenSlidesService } from './openslides.service'; /** * Classical Auth-Guard. Checks if the user has to correct permissions to enter a page, and forwards to login if not. @@ -15,8 +16,13 @@ export class AuthGuard implements CanActivate, CanActivateChild { * * @param router To navigate to a target URL * @param operator Asking for the required permission + * @param openSlidesService Handle OpenSlides functions */ - public constructor(private router: Router, private operator: OperatorService) {} + public constructor( + private router: Router, + private operator: OperatorService, + private openSlidesService: OpenSlidesService + ) {} /** * Checks of the operator has the required permission to see the state. @@ -44,10 +50,13 @@ export class AuthGuard implements CanActivate, CanActivateChild { * * @param route the route the user wants to navigate to */ - public canActivateChild(route: ActivatedRouteSnapshot): boolean { + public async canActivateChild(route: ActivatedRouteSnapshot): Promise { + await this.operator.loaded; + if (this.canActivate(route)) { return true; } else { + this.openSlidesService.redirectUrl = location.pathname; this.router.navigate(['/error'], { queryParams: { error: 'Authentication Error', diff --git a/client/src/app/core/core-services/auth.service.ts b/client/src/app/core/core-services/auth.service.ts index 070e0ece6..3fa26e962 100644 --- a/client/src/app/core/core-services/auth.service.ts +++ b/client/src/app/core/core-services/auth.service.ts @@ -62,7 +62,9 @@ export class AuthService { } let redirect = this.OpenSlides.redirectUrl ? this.OpenSlides.redirectUrl : '/'; - if (redirect.includes('login')) { + + const excludedUrls = ['login']; + if (excludedUrls.some(url => redirect.includes(url))) { redirect = '/'; } this.router.navigate([redirect]); diff --git a/client/src/app/core/core-services/openslides.service.ts b/client/src/app/core/core-services/openslides.service.ts index b74960596..8d8bd7c8f 100644 --- a/client/src/app/core/core-services/openslides.service.ts +++ b/client/src/app/core/core-services/openslides.service.ts @@ -65,7 +65,9 @@ export class OpenSlidesService { // start autoupdate if the user is logged in: const response = await this.operator.whoAmIFromStorage(); if (!response.user && !response.guest_enabled) { - this.redirectUrl = location.pathname; + if (!location.pathname.includes('error')) { + this.redirectUrl = location.pathname; + } this.redirectToLoginIfNotSubpage(); this.checkOperator(false); } else { diff --git a/client/src/app/core/core-services/operator.service.ts b/client/src/app/core/core-services/operator.service.ts index b106e04ed..c3e20a83a 100644 --- a/client/src/app/core/core-services/operator.service.ts +++ b/client/src/app/core/core-services/operator.service.ts @@ -6,6 +6,7 @@ import { Group } from 'app/shared/models/users/group'; import { User } from '../../shared/models/users/user'; import { environment } from 'environments/environment'; import { DataStoreService } from './data-store.service'; +import { Deferred } from '../deferred'; import { OfflineService } from './offline.service'; import { OpenSlidesStatusService } from './openslides-status.service'; import { ViewUser } from 'app/site/users/models/view-user'; @@ -116,6 +117,12 @@ export class OperatorService implements OnAfterAppsLoaded { */ private currentWhoAmI: WhoAmI | null; + private readonly _loaded: Deferred = new Deferred(); + + public get loaded(): Promise { + return this._loaded.promise; + } + /** * Sets up an observer for watching changes in the DS. If the operator user or groups are changed, * the operator's permissions are updated. @@ -177,6 +184,7 @@ export class OperatorService implements OnAfterAppsLoaded { response = this.getDefaultWhoAmIResponse(); } await this.updateCurrentWhoAmI(response); + this._loaded.resolve(); return this.currentWhoAmI; } diff --git a/client/src/app/core/deferred.ts b/client/src/app/core/deferred.ts new file mode 100644 index 000000000..1d96b0694 --- /dev/null +++ b/client/src/app/core/deferred.ts @@ -0,0 +1,42 @@ +/** + * Helper class to asynchronously wait until certain promises are resolved + * + * @example + * ```ts + * // myService + * private loaded: Deferred = new Deferred(); + * // after something was done + * this.loaded.resolve(); + * + * // myOtherService or myComponent + * await this.myService.loaded; + * // + * ``` + */ +export class Deferred { + /** + * The promise to wait for + */ + public readonly promise: Promise; + + /** + * custom resolve function + */ + private _resolve: () => void; + + /** + * Creates the promise and overloads the resolve function + */ + public constructor() { + this.promise = new Promise(resolve => { + this.resolve = resolve; + }); + } + + /** + * Entry point for the resolve function + */ + public resolve(): void { + this._resolve(); + } +} diff --git a/client/src/app/shared/components/media-upload-content/media-upload-content.component.html b/client/src/app/shared/components/media-upload-content/media-upload-content.component.html index f90f4e59c..7c003850a 100644 --- a/client/src/app/shared/components/media-upload-content/media-upload-content.component.html +++ b/client/src/app/shared/components/media-upload-content/media-upload-content.component.html @@ -3,7 +3,7 @@
- Drop files into this area OR select files + Drop files into this area OR click here to select files
@@ -68,10 +68,18 @@
- - +