fix OIDC login redirect flow

Author: jesielviana

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

@@ -51,6 +51,7 @@ import {
 } from '../testing/special-group.mock';
 import { getMockTranslateService } from '../testing/translate.service.mock';
 import { createSuccessfulRemoteDataObject$ } from '../utilities/remote-data.utils';
+import { AuthRequestService } from './auth-request.service';
 import {
   SetUserAsIdleAction,
   UnsetUserAsIdleAction,
@@ -60,10 +61,9 @@ import {
   AuthService,
   IMPERSONATING_COOKIE,
 } from './auth.service';
-import { AuthRequestService } from './auth-request.service';
-import { AuthMethod } from './models/auth.method';
 import { AuthStatus } from './models/auth-status.model';
 import { AuthTokenInfo } from './models/auth-token-info.model';
+import { AuthMethod } from './models/auth.method';
 import {
   getAuthenticationToken,
   isAuthenticated,
@@ -447,6 +447,45 @@ describe('AuthService test', () => {
       expect(hardRedirectService.redirect).toHaveBeenCalledWith(jasmine.stringMatching(new RegExp('reload/[0-9]*\\?redirect=' + encodeURIComponent('/home'))));
     });
 
+    it('should replace a top-level external redirectUrl', () => {
+      const externalServerUrl = authService.getExternalServerRedirectUrl(
+        'https://dspace.test',
+        '/itemtemplate',
+        '/api/authn/shibboleth?redirectUrl=https://dspace.test/home',
+      );
+
+      expect(externalServerUrl).toBe('/api/authn/shibboleth?redirectUrl=https%3A%2F%2Fdspace.test%2Fitemtemplate');
+    });
+
+    it('should inject redirectUrl into a nested redirect_uri for OIDC providers', () => {
+      const externalServerUrl = authService.getExternalServerRedirectUrl(
+        'https://dspace.test',
+        '/itemtemplate',
+        'https://provider.example/authorize?client_id=test-client&response_type=code&scope=openid%20email&redirect_uri=' +
+        encodeURIComponent('https://rest.test/api/authn/oidc'),
+      );
+
+      const providerUrl = new URL(externalServerUrl);
+      const redirectUri = new URL(providerUrl.searchParams.get('redirect_uri'));
+
+      expect(redirectUri.toString()).toBe('https://rest.test/api/authn/oidc?redirectUrl=https%3A%2F%2Fdspace.test%2Fitemtemplate');
+    });
+
+    it('should preserve existing redirect_uri query params when injecting redirectUrl', () => {
+      const externalServerUrl = authService.getExternalServerRedirectUrl(
+        'https://dspace.test',
+        '/itemtemplate',
+        'https://provider.example/authorize?client_id=test-client&response_type=code&scope=openid%20email&redirect_uri=' +
+        encodeURIComponent('https://rest.test/api/authn/orcid?foo=bar'),
+      );
+
+      const providerUrl = new URL(externalServerUrl);
+      const redirectUri = new URL(providerUrl.searchParams.get('redirect_uri'));
+
+      expect(redirectUri.searchParams.get('foo')).toBe('bar');
+      expect(redirectUri.searchParams.get('redirectUrl')).toBe('https://dspace.test/itemtemplate');
+    });
+
     it('should redirect to regular reload and not to /login', () => {
       authService.navigateToRedirectUrl('/login');
       // Reload without a redirect URL

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

@@ -23,7 +23,6 @@ import {
   Store,
 } from '@ngrx/store';
 import { TranslateService } from '@ngx-translate/core';
-import Cookies from 'js-cookie';
 import {
   Observable,
   of,
@@ -63,6 +62,7 @@ import {
 import { PageInfo } from '../shared/page-info.model';
 import { URLCombiner } from '../url-combiner/url-combiner';
 import { createSuccessfulRemoteDataObject$ } from '../utilities/remote-data.utils';
+import { AuthRequestService } from './auth-request.service';
 import {
   CheckAuthenticationTokenAction,
   RefreshTokenAction,
@@ -72,13 +72,12 @@ import {
   SetUserAsIdleAction,
   UnsetUserAsIdleAction,
 } from './auth.actions';
-import { AuthRequestService } from './auth-request.service';
-import { AuthMethod } from './models/auth.method';
 import { AuthStatus } from './models/auth-status.model';
 import {
   AuthTokenInfo,
   TOKENITEM,
 } from './models/auth-token-info.model';
+import { AuthMethod } from './models/auth.method';
 import {
   getAuthenticatedUserId,
   getAuthenticationToken,
@@ -613,20 +612,23 @@ export class AuthService {
    */
   getExternalServerRedirectUrl(origin: string, redirectRoute: string, location: string): string {
     const correctRedirectUrl = new URLCombiner(origin, redirectRoute).toString();
+    const externalServerUrl = new URL(location, origin);
+
+    if (externalServerUrl.searchParams.has('redirectUrl')) {
+      externalServerUrl.searchParams.set('redirectUrl', correctRedirectUrl);
+    } else if (externalServerUrl.searchParams.has('redirect_uri')) {
+      const redirectUri = new URL(externalServerUrl.searchParams.get('redirect_uri'), origin);
+      redirectUri.searchParams.set('redirectUrl', correctRedirectUrl);
+      externalServerUrl.searchParams.set('redirect_uri', redirectUri.toString());
+    } else {
+      externalServerUrl.searchParams.set('redirectUrl', correctRedirectUrl);
+    }
 
-    let externalServerUrl = location;
-    const myRegexp = /\?redirectUrl=(.*)/g;
-    const match = myRegexp.exec(location);
-    const redirectUrlFromServer = (match && match[1]) ? match[1] : null;
-
-    // Check whether the current page is different from the redirect url received from rest
-    if (isNotNull(redirectUrlFromServer) && redirectUrlFromServer !== correctRedirectUrl) {
-      // change the redirect url with the current page url
-      const newRedirectUrl = `?redirectUrl=${correctRedirectUrl}`;
-      externalServerUrl = location.replace(/\?redirectUrl=(.*)/g, newRedirectUrl);
+    if (isNotNull(location.match(/^https?:\/\//))) {
+      return externalServerUrl.toString();
     }
 
-    return externalServerUrl;
+    return `${externalServerUrl.pathname}${externalServerUrl.search}${externalServerUrl.hash}`;
   }
 
   /**