ZCU-PUB/fix: reload redirect loop after Shibboleth login (absolute reload URL, no SSR redirect) (#1384)

* ZCU-PUB/test: reload redirect must be absolute and must not fire during SSR

Replicates the redirect loop after a Shibboleth login: the relative
'reload/<timestamp>' URL is resolved against the current URL
(e.g. /bitstreams/<uuid>/download), producing growing
/bitstreams/<uuid>/reload/reload/... URLs, because during SSR the redirect
repeats on every request (the dsRedirectUrl cookie cannot be cleared
server-side - ServerCookieService.remove is a no-op).

Also stops static-page.component.spec from mutating the shared environment
object (it replaced environment.ui, breaking later specs reading
environment.ui.nameSpace).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* ZCU-PUB/fix: absolute reload URL and no post-login hard redirect during SSR

Fixes the redirect loop after a Shibboleth login:

- navigateToRedirectUrl() and refreshAfterChangeLanguage() build an absolute,
  ui.nameSpace-aware /reload/<ts> URL. A relative 'reload/<ts>' URL in the
  Location header is resolved against the request URL (e.g.
  /bitstreams/<uuid>/download after an external login), producing nested
  /bitstreams/<uuid>/reload/... URLs which never match the top-level
  'reload/:rnd' route. navigateToRedirectUrl() also skips the redirect when
  the current path already is the reload page.
- authenticatedSuccess$ dispatches RedirectAfterLoginSuccess only in the
  browser. The server cannot clear the dsRedirectUrl cookie
  (ServerCookieService.set/remove are no-ops), so a hard redirect during SSR
  repeats on every request - one 'reload/' segment per 302 hop - until the
  browser aborts at its redirect limit.
- ServerHardRedirectService.redirect() never emits a relative Location header.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

Author: milanmajchrak

src/app/core/auth/auth.effects.spec.ts

@@ -1,5 +1,7 @@
 import { fakeAsync, TestBed, tick } from '@angular/core/testing';
+import { NgZone } from '@angular/core';
 
+import { Actions } from '@ngrx/effects';
 import { provideMockActions } from '@ngrx/effects/testing';
 import { Store, StoreModule } from '@ngrx/store';
 import { MockStore, provideMockStore } from '@ngrx/store/testing';
@@ -17,6 +19,7 @@ import {
   CheckAuthenticationTokenCookieAction,
   LogOutErrorAction,
   LogOutSuccessAction,
+  RedirectAfterLoginSuccessAction,
   RefreshTokenErrorAction,
   RefreshTokenSuccessAction,
   RetrieveAuthenticatedEpersonAction,
@@ -176,6 +179,52 @@ describe('AuthEffects', () => {
       done();
     });
 
+    describe('when a redirect url is set', () => {
+      const redirectUrl = '/bitstreams/eb0ca4e9-3e00-4c1f-8b19-a2c9f4f01234/download';
+
+      beforeEach(() => {
+        spyOn((authEffects as any).authService, 'storeToken');
+        authServiceStub.setRedirectUrl(redirectUrl);
+        actions = hot('--a-', {
+          a: {
+            type: AuthActionTypes.AUTHENTICATED_SUCCESS, payload: {
+              authenticated: true,
+              authToken: token,
+              userHref: EPersonMock._links.self.href
+            }
+          }
+        });
+      });
+
+      afterEach(() => {
+        authServiceStub.setRedirectUrl(undefined);
+      });
+
+      it('should return a REDIRECT_AFTER_LOGIN_SUCCESS action in the browser', () => {
+        const expected = cold('--b-', { b: new RedirectAfterLoginSuccessAction(redirectUrl) });
+
+        expect(authEffects.authenticatedSuccess$).toBeObservable(expected);
+      });
+
+      it('should return a RETRIEVE_AUTHENTICATED_EPERSON action during server-side rendering', () => {
+        // The server cannot clear the redirect cookie (ServerCookieService.set/remove are no-ops),
+        // so a hard redirect during SSR would repeat on every request and create a redirect loop.
+        // The redirect must be left to the browser.
+        const serverAuthEffects: AuthEffects = new (AuthEffects as any)(
+          TestBed.inject(Actions),
+          TestBed.inject(NgZone),
+          authorizationService,
+          authServiceStub as any,
+          TestBed.inject(Store) as any,
+          'server'
+        );
+
+        const expected = cold('--b-', { b: new RetrieveAuthenticatedEpersonAction(EPersonMock._links.self.href) });
+
+        expect(serverAuthEffects.authenticatedSuccess$).toBeObservable(expected);
+      });
+    });
+
   });
 
   describe('checkToken$', () => {

src/app/core/auth/auth.effects.ts

@@ -1,4 +1,5 @@
-import { Injectable, NgZone } from '@angular/core';
+import { Inject, Injectable, NgZone, PLATFORM_ID } from '@angular/core';
+import { isPlatformBrowser } from '@angular/common';
 
 import {
   asyncScheduler,
@@ -100,7 +101,11 @@ export class AuthEffects {
       map((redirectUrl: string) => [action, redirectUrl])
     )),
     map(([action, redirectUrl]: [AuthenticatedSuccessAction, string]) => {
-      if (hasValue(redirectUrl)) {
+      // Perform the redirect only in the browser: the server cannot clear the redirect cookie
+      // (ServerCookieService.set/remove are no-ops), so a hard redirect during SSR would repeat
+      // on every request and create a redirect loop.
+      // The browser re-runs this effect after hydration and performs the redirect itself.
+      if (hasValue(redirectUrl) && isPlatformBrowser(this.platformId)) {
         return new RedirectAfterLoginSuccessAction(redirectUrl);
       } else {
         return new RetrieveAuthenticatedEpersonAction(action.payload.userHref);
@@ -287,6 +292,7 @@ export class AuthEffects {
               private zone: NgZone,
               private authorizationsService: AuthorizationDataService,
               private authService: AuthService,
-              private store: Store<AppState>) {
+              private store: Store<AppState>,
+              @Inject(PLATFORM_ID) private platformId: any) {
   }
 }

src/app/core/auth/auth.service.spec.ts

@@ -34,6 +34,7 @@ import { NotificationsServiceStub } from '../../shared/testing/notifications-ser
 import { SetUserAsIdleAction, UnsetUserAsIdleAction } from './auth.actions';
 import { SpecialGroupDataMock, SpecialGroupDataMock$ } from '../../shared/testing/special-group.mock';
 import { cold } from 'jasmine-marbles';
+import { environment } from '../../../environments/environment';
 
 describe('AuthService test', () => {
 
@@ -103,7 +104,7 @@ describe('AuthService test', () => {
     linkService = {
       resolveLinks: {}
     };
-    hardRedirectService = jasmine.createSpyObj('hardRedirectService', ['redirect']);
+    hardRedirectService = jasmine.createSpyObj('hardRedirectService', ['redirect', 'getCurrentRoute']);
     spyOn(linkService, 'resolveLinks').and.returnValue({ authenticated: true, eperson: observableOf({ payload: {} }) });
 
   }
@@ -374,28 +375,39 @@ describe('AuthService test', () => {
       expect(storage.remove).toHaveBeenCalled();
     });
 
-    it('should redirect to reload with redirect url', () => {
+    // The reload URL must be absolute (nameSpace-aware): a relative 'reload/...' URL is resolved
+    // against the current URL - e.g. against /bitstreams/<uuid>/download after an external
+    // (Shibboleth) login - producing invalid nested URLs like /bitstreams/<uuid>/reload/reload/...
+    const reloadPrefix = environment.ui.nameSpace.replace(/\/$/, '') + '/reload/';
+
+    it('should redirect to the absolute reload URL with redirect url', () => {
       authService.navigateToRedirectUrl('/collection/123');
       // Reload with redirect URL set to /collection/123
-      expect(hardRedirectService.redirect).toHaveBeenCalledWith(jasmine.stringMatching(new RegExp('reload/[0-9]*\\?redirect=' + encodeURIComponent('/collection/123'))));
+      expect(hardRedirectService.redirect).toHaveBeenCalledWith(jasmine.stringMatching(new RegExp('^' + reloadPrefix + '[0-9]+\\?redirect=' + encodeURIComponent('/collection/123') + '$')));
     });
 
-    it('should redirect to reload with /home', () => {
+    it('should redirect to the absolute reload URL with /home', () => {
       authService.navigateToRedirectUrl('/home');
       // Reload with redirect URL set to /home
-      expect(hardRedirectService.redirect).toHaveBeenCalledWith(jasmine.stringMatching(new RegExp('reload/[0-9]*\\?redirect=' + encodeURIComponent('/home'))));
+      expect(hardRedirectService.redirect).toHaveBeenCalledWith(jasmine.stringMatching(new RegExp('^' + reloadPrefix + '[0-9]+\\?redirect=' + encodeURIComponent('/home') + '$')));
     });
 
-    it('should redirect to regular reload and not to /login', () => {
+    it('should redirect to the absolute reload URL and not to /login', () => {
       authService.navigateToRedirectUrl('/login');
       // Reload without a redirect URL
-      expect(hardRedirectService.redirect).toHaveBeenCalledWith(jasmine.stringMatching(new RegExp('reload/[0-9]*(?!\\?)$')));
+      expect(hardRedirectService.redirect).toHaveBeenCalledWith(jasmine.stringMatching(new RegExp('^' + reloadPrefix + '[0-9]+$')));
     });
 
-    it('should redirect to regular reload when no redirect url is found', () => {
+    it('should redirect to the absolute reload URL when no redirect url is found', () => {
       authService.navigateToRedirectUrl(undefined);
       // Reload without a redirect URL
-      expect(hardRedirectService.redirect).toHaveBeenCalledWith(jasmine.stringMatching(new RegExp('reload/[0-9]*(?!\\?)$')));
+      expect(hardRedirectService.redirect).toHaveBeenCalledWith(jasmine.stringMatching(new RegExp('^' + reloadPrefix + '[0-9]+$')));
+    });
+
+    it('should not redirect again when the current route is already the reload page', () => {
+      hardRedirectService.getCurrentRoute.and.returnValue(reloadPrefix + '123456789?redirect=' + encodeURIComponent('/home'));
+      authService.navigateToRedirectUrl('/collection/123');
+      expect(hardRedirectService.redirect).not.toHaveBeenCalled();
     });
 
     describe('impersonate', () => {

src/app/core/auth/auth.service.ts

@@ -487,9 +487,21 @@ export class AuthService {
    * @param redirectUrl
    */
   public navigateToRedirectUrl(redirectUrl: string) {
+    // Don't do redirect if the current page already is the reload page,
+    // otherwise the reload could be repeated indefinitely
+    // (only the path is checked - a query string could legitimately contain '/reload/')
+    const currentRoute = this.hardRedirectService.getCurrentRoute();
+    if (hasValue(currentRoute) && currentRoute.split('?')[0].includes('/reload/')) {
+      return;
+    }
     // Don't do redirect if already on reload url
     if (!hasValue(redirectUrl) || !redirectUrl.includes('reload/')) {
-      let url = `reload/${new Date().getTime()}`;
+      // The reload URL must be absolute (nameSpace-aware). A relative 'reload/...' URL is resolved
+      // against the current URL - e.g. against /bitstreams/<uuid>/download after an external
+      // (Shibboleth) login - and produces invalid nested URLs like /bitstreams/<uuid>/reload/...
+      // which never match the 'reload/:rnd' route.
+      const nameSpace = (environment.ui.nameSpace || '').replace(/\/$/, '');
+      let url = `${nameSpace}/reload/${new Date().getTime()}`;
       if (isNotEmpty(redirectUrl) && !redirectUrl.startsWith(LOGIN_ROUTE)) {
         url += `?redirect=${encodeURIComponent(redirectUrl)}`;
       }

src/app/core/locale/locale.service.spec.ts

@@ -1,6 +1,9 @@
-import { TestBed, waitForAsync } from '@angular/core/testing';
+import { fakeAsync, TestBed, tick, waitForAsync } from '@angular/core/testing';
 
 import { TranslateLoader, TranslateModule, TranslateService } from '@ngx-translate/core';
+import { of } from 'rxjs';
+
+import { environment } from '../../../environments/environment';
 
 import { CookieService } from '../services/cookie.service';
 import { CookieServiceMock } from '../../shared/mocks/cookie.service.mock';
@@ -147,4 +150,37 @@ describe('LocaleService test suite', () => {
       expect(service.getLanguageCodeList).toHaveBeenCalled();
     });
   });
+
+  describe('refreshAfterChangeLanguage', () => {
+    let originalUi;
+    let originalGetCurrentUrl;
+
+    beforeEach(() => {
+      // Pin the tested nameSpace value: environment is a shared mutable object and some specs
+      // replace environment.ui entirely, so this test must not rely on the suite order.
+      originalUi = (environment as any).ui;
+      (environment as any).ui = Object.assign({}, originalUi, { nameSpace: '/angular-dspace' });
+      originalGetCurrentUrl = (routeService as any).getCurrentUrl;
+    });
+
+    afterEach(() => {
+      (environment as any).ui = originalUi;
+      // routeServiceStub is a shared module-level singleton - restore it
+      (routeService as any).getCurrentUrl = originalGetCurrentUrl;
+    });
+
+    it('should hard redirect to the absolute (nameSpace-aware) reload URL', fakeAsync(() => {
+      // A relative 'reload/...' URL would be resolved against the current URL
+      // (e.g. /items/<uuid>) and produce an invalid nested URL like /items/<uuid>/reload/...
+      const currentUrl = '/items/1234';
+      const fakeLocation = { href: '' };
+      serviceAsAny._window = { nativeWindow: { location: fakeLocation } };
+      (routeService as any).getCurrentUrl = jasmine.createSpy('getCurrentUrl').and.returnValue(of(currentUrl));
+
+      service.refreshAfterChangeLanguage();
+      tick();
+
+      expect(fakeLocation.href).toMatch(new RegExp('^/angular-dspace/reload/[0-9]+\\?redirect=' + encodeURIComponent(currentUrl) + '$'));
+    }));
+  });
 });

src/app/core/locale/locale.service.ts

@@ -191,8 +191,13 @@ export class LocaleService {
   public refreshAfterChangeLanguage() {
     this.routeService.getCurrentUrl().pipe(take(1)).subscribe((currentURL) => {
       // Hard redirect to the reload page with a unique number behind it
-      // so that all state is definitely lost
-      this._window.nativeWindow.location.href = `reload/${new Date().getTime()}?redirect=` + encodeURIComponent(currentURL);
+      // so that all state is definitely lost.
+      // The reload URL must be absolute (nameSpace-aware). A relative 'reload/...' URL is resolved
+      // against the current URL and produces invalid nested URLs like /items/<uuid>/reload/...
+      // which never match the 'reload/:rnd' route.
+      const nameSpace = (environment.ui.nameSpace || '').replace(/\/$/, '');
+      this._window.nativeWindow.location.href =
+        `${nameSpace}/reload/${new Date().getTime()}?redirect=` + encodeURIComponent(currentURL);
     });
 
   }

src/app/core/services/server-hard-redirect.service.spec.ts

@@ -23,7 +23,7 @@ describe('ServerHardRedirectService', () => {
   });
 
   describe('when performing a default redirect', () => {
-    const redirect = 'test redirect';
+    const redirect = '/test/redirect';
 
     beforeEach(() => {
       service.redirect(redirect);
@@ -36,7 +36,7 @@ describe('ServerHardRedirectService', () => {
   });
 
   describe('when performing a 301 redirect', () => {
-    const redirect = 'test 301 redirect';
+    const redirect = '/test/301/redirect';
     const redirectStatusCode = 301;
 
     beforeEach(() => {
@@ -49,6 +49,35 @@ describe('ServerHardRedirectService', () => {
     });
   });
 
+  describe('when performing a redirect to a relative url', () => {
+    // A relative URL in the Location header is resolved by the browser against the request URL,
+    // e.g. 'reload/123' requested at /bitstreams/<uuid>/download resolves
+    // to /bitstreams/<uuid>/reload/123 - such a redirect must never be emitted
+    const redirect = 'reload/123456789';
+
+    beforeEach(() => {
+      service.redirect(redirect);
+    });
+
+    it('should redirect to the URL prefixed with a slash', () => {
+      expect(mockResponse.redirect).toHaveBeenCalledWith(302, '/' + redirect);
+      expect(mockResponse.end).toHaveBeenCalled();
+    });
+  });
+
+  describe('when performing a redirect to an external url', () => {
+    const redirect = 'https://external-host.com/path';
+
+    beforeEach(() => {
+      service.redirect(redirect);
+    });
+
+    it('should redirect to the unchanged external URL', () => {
+      expect(mockResponse.redirect).toHaveBeenCalledWith(302, redirect);
+      expect(mockResponse.end).toHaveBeenCalled();
+    });
+  });
+
   describe('when requesting the current route', () => {
 
     beforeEach(() => {

src/app/core/services/server-hard-redirect.service.ts

@@ -26,6 +26,13 @@ export class ServerHardRedirectService extends HardRedirectService {
    */
   redirect(url: string, statusCode?: number) {
 
+    // A relative URL in the Location header is resolved by the browser against the request URL
+    // (e.g. 'reload/123' requested at /bitstreams/<uuid>/download resolves to
+    // /bitstreams/<uuid>/reload/123) - make sure only absolute URLs are emitted
+    if (!url.startsWith('/') && !/^https?:\/\//i.test(url)) {
+      url = '/' + url;
+    }
+
     if (url === this.req.url) {
       return;
     }

src/app/static-page/static-page.component.spec.ts

@@ -27,7 +27,9 @@ describe('StaticPageComponent', () => {
       getCurrentLanguageCode: jasmine.createSpy('getCurrentLanguageCode'),
     });
 
-    appConfig = Object.assign(environment, {
+    // Do not mutate the shared `environment` object - replacing `environment.ui` would
+    // break any later spec that reads e.g. environment.ui.nameSpace
+    appConfig = Object.assign({}, environment, {
       ui: {
         namespace: 'testNamespace'
       }