From 862301f5d6069998122469a3c0b3476d9a30067f Mon Sep 17 00:00:00 2001
From: Sebastian <sebastian@squidex.io>
Date: Wed, 31 May 2023 16:46:57 +0200
Subject: [PATCH] Improve login journey.

---
 .../Apps/Templates/TemplatesClient.cs         |  2 --
 .../src/Squidex.Web/ApiExceptionConverter.cs  | 10 +++++++++-
 backend/src/Squidex.Web/ErrorDto.cs           |  2 +-
 .../apps/pages/apps-page.component.ts         |  2 +-
 .../framework/angular/http/http-extensions.ts |  2 +-
 .../guards/must-be-authenticated.guard.ts     | 16 +++++++++-------
 .../must-be-not-authenticated.guard.spec.ts   | 19 ++++++++++++++++---
 .../guards/must-be-not-authenticated.guard.ts |  9 ++++-----
 .../src/app/shared/state/templates.state.ts   |  8 ++++----
 .../shell/pages/home/home-page.component.ts   |  9 +++++----
 .../pages/logout/logout-page.component.ts     | 14 +++++++++++---
 .../TestSuite.ApiTests/AppClientsTests.cs     |  1 -
 .../AppContributorsTests.cs                   |  1 -
 .../TestSuite.ApiTests/AppLanguagesTests.cs   |  2 --
 .../TestSuite.ApiTests/RuleRunnerTests.cs     |  1 -
 15 files changed, 61 insertions(+), 37 deletions(-)

diff --git a/backend/src/Squidex.Domain.Apps.Entities/Apps/Templates/TemplatesClient.cs b/backend/src/Squidex.Domain.Apps.Entities/Apps/Templates/TemplatesClient.cs
index b5d468ae4..7add382f8 100644
--- a/backend/src/Squidex.Domain.Apps.Entities/Apps/Templates/TemplatesClient.cs
+++ b/backend/src/Squidex.Domain.Apps.Entities/Apps/Templates/TemplatesClient.cs
@@ -28,8 +28,6 @@ public sealed class TemplatesClient
     {
         var httpClient = httpClientFactory.CreateClient();
 
-        var result = new List<Template>();
-
         foreach (var repository in options.Repositories.OrEmpty())
         {
             var url = $"{repository.ContentUrl}/README.md";
diff --git a/backend/src/Squidex.Web/ApiExceptionConverter.cs b/backend/src/Squidex.Web/ApiExceptionConverter.cs
index e02907c23..37584d9b5 100644
--- a/backend/src/Squidex.Web/ApiExceptionConverter.cs
+++ b/backend/src/Squidex.Web/ApiExceptionConverter.cs
@@ -142,7 +142,15 @@ public static class ApiExceptionConverter
 
     private static ErrorDto CreateError(int status, string? message = null, string? errorCode = null, IEnumerable<string>? details = null)
     {
-        var error = new ErrorDto { StatusCode = status, Message = message ?? string.Empty };
+        var error = new ErrorDto
+        {
+            StatusCode = status
+        };
+
+        if (!string.IsNullOrWhiteSpace(message))
+        {
+            error.Message = message;
+        }
 
         if (!string.IsNullOrWhiteSpace(errorCode))
         {
diff --git a/backend/src/Squidex.Web/ErrorDto.cs b/backend/src/Squidex.Web/ErrorDto.cs
index ccb5943bb..3cc74323f 100644
--- a/backend/src/Squidex.Web/ErrorDto.cs
+++ b/backend/src/Squidex.Web/ErrorDto.cs
@@ -14,7 +14,7 @@ public sealed class ErrorDto
 {
     [LocalizedRequired]
     [Display(Description = "Error message.")]
-    public string Message { get; set; }
+    public string Message { get; set; } = string.Empty;
 
     [Display(Description = "The error code.")]
     public string? ErrorCode { get; set; }
diff --git a/frontend/src/app/features/apps/pages/apps-page.component.ts b/frontend/src/app/features/apps/pages/apps-page.component.ts
index 8fde70032..9d60bb1db 100644
--- a/frontend/src/app/features/apps/pages/apps-page.component.ts
+++ b/frontend/src/app/features/apps/pages/apps-page.component.ts
@@ -98,7 +98,7 @@ export class AppsPageComponent implements OnInit {
                 }
             });
 
-        this.templatesState.load();
+        this.templatesState.load(false, true);
     }
 
     public createNewApp(template?: TemplateDto) {
diff --git a/frontend/src/app/framework/angular/http/http-extensions.ts b/frontend/src/app/framework/angular/http/http-extensions.ts
index efac5188e..c91fa3f97 100644
--- a/frontend/src/app/framework/angular/http/http-extensions.ts
+++ b/frontend/src/app/framework/angular/http/http-extensions.ts
@@ -125,7 +125,7 @@ export function parseError(response: HttpErrorResponse, fallback: string) {
         }
     }
 
-    if (parsed && Types.isString(parsed.message)) {
+    if (parsed && Types.isString(parsed.message) && parsed.message) {
         return new ErrorDto(status, parsed.message, parsed.errorCode, parsed.details, response);
     }
 
diff --git a/frontend/src/app/shared/guards/must-be-authenticated.guard.ts b/frontend/src/app/shared/guards/must-be-authenticated.guard.ts
index 73c3f20b4..cc88921b2 100644
--- a/frontend/src/app/shared/guards/must-be-authenticated.guard.ts
+++ b/frontend/src/app/shared/guards/must-be-authenticated.guard.ts
@@ -29,14 +29,16 @@ export class MustBeAuthenticatedGuard implements CanActivate {
         return this.authService.userChanges.pipe(
             take(1),
             tap(user => {
-                if (!user) {
-                    const redirectPath = this.location.path(true);
+                if (user) {
+                    return;
+                }
+
+                const redirectPath = this.location.path(true);
 
-                    if (redirect) {
-                        this.authService.loginRedirect(redirectPath);
-                    } else {
-                        this.router.navigate([''], { queryParams: { redirectPath } });
-                    }
+                if (redirect) {
+                    this.authService.loginRedirect(redirectPath);
+                } else {
+                    this.router.navigate([''], { queryParams: { redirectPath } });
                 }
             }),
             map(user => !!user));
diff --git a/frontend/src/app/shared/guards/must-be-not-authenticated.guard.spec.ts b/frontend/src/app/shared/guards/must-be-not-authenticated.guard.spec.ts
index cbc344aec..e8f78520e 100644
--- a/frontend/src/app/shared/guards/must-be-not-authenticated.guard.spec.ts
+++ b/frontend/src/app/shared/guards/must-be-not-authenticated.guard.spec.ts
@@ -37,7 +37,7 @@ describe('MustBeNotAuthenticatedGuard', () => {
         authService.setup(x => x.userChanges)
             .returns(() => of(<any>{}));
 
-        const result = await firstValueFrom(authGuard.canActivate());
+        const result = await firstValueFrom(authGuard.canActivate({} as any));
 
         expect(result!).toBeFalsy();
 
@@ -48,7 +48,7 @@ describe('MustBeNotAuthenticatedGuard', () => {
         authService.setup(x => x.userChanges)
             .returns(() => of(null));
 
-        const result = await firstValueFrom(authGuard.canActivate());
+        const result = await firstValueFrom(authGuard.canActivate({} as any));
 
         expect(result).toBeTruthy();
 
@@ -61,10 +61,23 @@ describe('MustBeNotAuthenticatedGuard', () => {
         authService.setup(x => x.userChanges)
             .returns(() => of(null));
 
-        const result = await firstValueFrom(authGuard.canActivate());
+        const result = await firstValueFrom(authGuard.canActivate({ queryParams: {} } as any));
 
         expect(result).toBeFalsy();
 
         authService.verify(x => x.loginRedirect('/my-path'), Times.once());
     });
+
+    it('should not redirect after logout', async () => {
+        uiOptions.value.redirectToLogin = true;
+
+        authService.setup(x => x.userChanges)
+            .returns(() => of(null));
+
+        const result = await firstValueFrom(authGuard.canActivate({ queryParams: { logout: true } } as any));
+
+        expect(result).toBeTruthy();
+
+        authService.verify(x => x.loginRedirect('/my-path'), Times.never());
+    });
 });
diff --git a/frontend/src/app/shared/guards/must-be-not-authenticated.guard.ts b/frontend/src/app/shared/guards/must-be-not-authenticated.guard.ts
index da647c97f..8dc18d2a1 100644
--- a/frontend/src/app/shared/guards/must-be-not-authenticated.guard.ts
+++ b/frontend/src/app/shared/guards/must-be-not-authenticated.guard.ts
@@ -7,7 +7,7 @@
 
 import { Location } from '@angular/common';
 import { Injectable } from '@angular/core';
-import { CanActivate, Router } from '@angular/router';
+import { ActivatedRouteSnapshot, CanActivate, Router } from '@angular/router';
 import { Observable } from 'rxjs';
 import { map, take, tap } from 'rxjs/operators';
 import { UIOptions } from '@app/framework';
@@ -15,7 +15,6 @@ import { AuthService } from './../services/auth.service';
 
 @Injectable()
 export class MustBeNotAuthenticatedGuard implements CanActivate {
-
     constructor(
         private readonly authService: AuthService,
         private readonly location: Location,
@@ -24,15 +23,15 @@ export class MustBeNotAuthenticatedGuard implements CanActivate {
     ) {
     }
 
-    public canActivate(): Observable<boolean> {
-        const redirect = this.uiOptions.get('redirectToLogin');
+    public canActivate(snapshot: ActivatedRouteSnapshot): Observable<boolean> {
+        const redirect = this.uiOptions.get('redirectToLogin') && !snapshot.queryParams.logout;
 
         return this.authService.userChanges.pipe(
             take(1),
             tap(user => {
                 const redirectPath = this.location.path(true);
 
-                if (redirect) {
+                if (!user && redirect) {
                     this.authService.loginRedirect(redirectPath);
                 } else if (user) {
                     this.router.navigate(['app'], { queryParams: { redirectPath } });
diff --git a/frontend/src/app/shared/state/templates.state.ts b/frontend/src/app/shared/state/templates.state.ts
index bc4a069f8..b03d5acd9 100644
--- a/frontend/src/app/shared/state/templates.state.ts
+++ b/frontend/src/app/shared/state/templates.state.ts
@@ -37,15 +37,15 @@ export class TemplatesState extends State<Snapshot> {
         super({ templates: [] }, 'Templates');
     }
 
-    public load(isReload = false): Observable<any> {
+    public load(isReload = false, silent = false): Observable<any> {
         if (isReload) {
             this.resetState('Loading Initial');
         }
 
-        return this.loadInternal(isReload);
+        return this.loadInternal(isReload, silent);
     }
 
-    private loadInternal(isReload: boolean): Observable<any> {
+    private loadInternal(isReload: boolean, silent: boolean): Observable<any> {
         this.next({ isLoading: true }, 'Loading Started');
 
         return this.templatesService.getTemplates().pipe(
@@ -63,6 +63,6 @@ export class TemplatesState extends State<Snapshot> {
             finalize(() => {
                 this.next({ isLoading: false }, 'Loading Done');
             }),
-            shareSubscribed(this.dialogs));
+            shareSubscribed(this.dialogs, { silent }));
     }
 }
diff --git a/frontend/src/app/shell/pages/home/home-page.component.ts b/frontend/src/app/shell/pages/home/home-page.component.ts
index 6fba02a47..b2e9d5002 100644
--- a/frontend/src/app/shell/pages/home/home-page.component.ts
+++ b/frontend/src/app/shell/pages/home/home-page.component.ts
@@ -8,7 +8,7 @@
 import { Location } from '@angular/common';
 import { Component } from '@angular/core';
 import { ActivatedRoute, Router } from '@angular/router';
-import { AuthService } from '@app/shared';
+import { AuthService, UIOptions } from '@app/shared';
 
 @Component({
     selector: 'sqx-home-page',
@@ -23,6 +23,7 @@ export class HomePageComponent {
         private readonly location: Location,
         private readonly route: ActivatedRoute,
         private readonly router: Router,
+        private readonly uiOptions: UIOptions,
     ) {
     }
 
@@ -31,15 +32,15 @@ export class HomePageComponent {
             this.route.snapshot.queryParams.redirectPath ||
             this.location.path();
 
-        if (this.isInternetExplorer()) {
+        if (this.isInternetExplorer() || this.uiOptions.get('redirectToLogin')) {
             this.authService.loginRedirect(redirectPath);
             return;
         }
 
         try {
-            const path = await this.authService.loginPopup(redirectPath);
+            const path = await this.authService.loginPopup(redirectPath) || '/app';
 
-            this.router.navigateByUrl(path || '/app', { replaceUrl: true });
+            this.router.navigateByUrl(path, { replaceUrl: true });
         } catch {
             this.router.navigate(['/'], { replaceUrl: true });
         }
diff --git a/frontend/src/app/shell/pages/logout/logout-page.component.ts b/frontend/src/app/shell/pages/logout/logout-page.component.ts
index 49f5777a0..906282acf 100644
--- a/frontend/src/app/shell/pages/logout/logout-page.component.ts
+++ b/frontend/src/app/shell/pages/logout/logout-page.component.ts
@@ -22,11 +22,19 @@ export class LogoutPageComponent implements OnInit {
 
     public async ngOnInit() {
         try {
-            const path = await this.authService.logoutRedirectComplete();
+            const path = await this.authService.logoutRedirectComplete() || '/';
 
-            this.router.navigateByUrl(path || '/app', { replaceUrl: true });
+            this.router.navigateByUrl(addQuery(path, 'logout', 'true'), { replaceUrl: true });
         } catch {
-            this.router.navigate(['/'], { replaceUrl: true });
+            this.router.navigate(['/'], { replaceUrl: true, queryParams: { logout: true } });
         }
     }
 }
+
+function addQuery(path: string, key: string, value: string) {
+    if (path.indexOf('?') >= 0) {
+        return `${path}&${key}=${value}`;
+    } else {
+        return `${path}?${key}=${value}`;
+    }
+}
diff --git a/tools/TestSuite/TestSuite.ApiTests/AppClientsTests.cs b/tools/TestSuite/TestSuite.ApiTests/AppClientsTests.cs
index c0418f677..e6c156af7 100644
--- a/tools/TestSuite/TestSuite.ApiTests/AppClientsTests.cs
+++ b/tools/TestSuite/TestSuite.ApiTests/AppClientsTests.cs
@@ -17,7 +17,6 @@ namespace TestSuite.ApiTests;
 [UsesVerify]
 public sealed class AppClientsTests : IClassFixture<ClientFixture>
 {
-    private readonly string appName = Guid.NewGuid().ToString();
     private readonly string id = Guid.NewGuid().ToString();
     private readonly string clientRole = "Editor";
     private readonly string clientName = "My Client";
diff --git a/tools/TestSuite/TestSuite.ApiTests/AppContributorsTests.cs b/tools/TestSuite/TestSuite.ApiTests/AppContributorsTests.cs
index 9227ba7ae..16607b8a0 100644
--- a/tools/TestSuite/TestSuite.ApiTests/AppContributorsTests.cs
+++ b/tools/TestSuite/TestSuite.ApiTests/AppContributorsTests.cs
@@ -17,7 +17,6 @@ namespace TestSuite.ApiTests;
 [UsesVerify]
 public sealed class AppContributorsTests : IClassFixture<ClientFixture>
 {
-    private readonly string appName = Guid.NewGuid().ToString();
     private readonly string email = $"{Guid.NewGuid()}@squidex.io";
 
     public ClientFixture _ { get; }
diff --git a/tools/TestSuite/TestSuite.ApiTests/AppLanguagesTests.cs b/tools/TestSuite/TestSuite.ApiTests/AppLanguagesTests.cs
index 83d26aac3..cdab49bd2 100644
--- a/tools/TestSuite/TestSuite.ApiTests/AppLanguagesTests.cs
+++ b/tools/TestSuite/TestSuite.ApiTests/AppLanguagesTests.cs
@@ -17,8 +17,6 @@ namespace TestSuite.ApiTests;
 [UsesVerify]
 public sealed class AppLanguagesTests : IClassFixture<ClientFixture>
 {
-    private readonly string appName = Guid.NewGuid().ToString();
-
     public ClientFixture _ { get; }
 
     public AppLanguagesTests(ClientFixture fixture)
diff --git a/tools/TestSuite/TestSuite.ApiTests/RuleRunnerTests.cs b/tools/TestSuite/TestSuite.ApiTests/RuleRunnerTests.cs
index bf9fab3f3..a701d993e 100644
--- a/tools/TestSuite/TestSuite.ApiTests/RuleRunnerTests.cs
+++ b/tools/TestSuite/TestSuite.ApiTests/RuleRunnerTests.cs
@@ -18,7 +18,6 @@ namespace TestSuite.ApiTests;
 public class RuleRunnerTests : IClassFixture<ClientFixture>, IClassFixture<WebhookCatcherFixture>
 {
     private readonly string secret = Guid.NewGuid().ToString();
-    private readonly string appName = Guid.NewGuid().ToString();
     private readonly string schemaName = $"schema-{Guid.NewGuid()}";
     private readonly string schemaNameRef = $"schema-{Guid.NewGuid()}-ref";
     private readonly string contentString = Guid.NewGuid().ToString();