Bugfix: Broken re-login after logout fails

changelog/unreleased/bugfix-oidc-logout-query-param-replication

@@ -0,0 +1,6 @@
+Bugfix: Broken re-login after logout
+
+After a user logged out, it was no longer possible to login without reloading the ocis root domain,
+this has now been fixed and only allowed query-params are taken into account.
+
+https://github.com/owncloud/web/pull/8694

packages/web-runtime/src/router/helpers.ts

@@ -1,5 +1,6 @@
 import { base, router } from './index'
 import { RouteLocation, RouteParams, Router, RouteRecordNormalized } from 'vue-router'
+import pick from 'lodash-es/pick'
 import {
   AuthContext,
   authContextValues,
@@ -10,7 +11,18 @@ import {
 export const buildUrl = (pathname) => {
   const isHistoryMode = !!base
   const baseUrl = new URL(window.location.href.split('#')[0])
-  baseUrl.search = window.location.search
+
+  // searchParams defines a set of search parameters which should be part of new url.
+  // The resulting querystring only contains the properties listed here.
+  const searchParams = new URLSearchParams(
+    pick(Object.fromEntries(new URLSearchParams(window.location.search).entries()), [
+      // needed for private links
+      'details'
+    ])
+  ).toString()
+
+  baseUrl.search = searchParams
+
   if (isHistoryMode) {
     // in history mode we can't determine the base path, it must be provided by the document
     baseUrl.pathname = new URL(base.href).pathname

packages/web-runtime/src/services/auth/authService.ts

@@ -8,6 +8,7 @@ import { extractPublicLinkToken, isPublicLinkContext, isUserContext } from '../.
 import { unref } from 'vue'
 import { Ability } from 'web-pkg/src/utils'
 import { Language } from 'vue3-gettext'
+import pick from 'lodash-es/pick'
 
 export class AuthService {
   private clientService: ClientService
@@ -164,20 +165,25 @@ export class AuthService {
    * Sign in callback gets called from the IDP after initial login.
    */
   public async signInCallback() {
-    // craft a url that the parser in oidc-client-ts can handle… this is required for oauth2 logins
-    const url =
-      '/?' +
-      new URLSearchParams(
-        unref(this.router.currentRoute).query as Record<string, string>
-      ).toString()
+    const currentQuery = unref(this.router.currentRoute).query
+    // craft an url that the parser in oidc-client-ts can handle… this is required for oauth2 logins
+    const url = '/?' + new URLSearchParams(currentQuery as Record<string, string>).toString()
 
     try {
       await this.userManager.signinRedirectCallback(url)
 
       const redirectUrl = this.userManager.getAndClearPostLoginRedirectUrl()
+
+      // transportQuery defines a set of query parameters which should be part of router route replace.
+      // The resulting object only contains the properties listed here.
+      const transportQuery = pick(currentQuery, [
+        // needed for private links
+        'details'
+      ])
+
       return this.router.replace({
         path: redirectUrl,
-        query: unref(this.router.currentRoute).query
+        query: transportQuery
       })
     } catch (e) {
       console.warn('error during authentication:', e)

packages/web-runtime/tests/unit/router/index.spec.ts

@@ -15,6 +15,9 @@ describe('buildUrl', () => {
     ${'https://localhost:9200/files/list/all'}                                        | ${'/foo'}                    | ${'/login/foo'} | ${'https://localhost:9200/foo/login/foo'}
     ${'https://localhost:9200/files/list/all'}                                        | ${'/'}                       | ${'/bar.html'}  | ${'https://localhost:9200/bar.html'}
     ${'https://localhost:9200/files/list/all'}                                        | ${'/foo'}                    | ${'/bar.html'}  | ${'https://localhost:9200/foo/bar.html'}
+    ${'https://localhost:9200/files/list/all?details=91953740'}                       | ${'/foo'}                    | ${'/bar.html'}  | ${'https://localhost:9200/foo/bar.html?details=91953740'}
+    ${'https://localhost:9200/files/list/all?details=91953740&unknown=87987987'}      | ${'/foo'}                    | ${'/bar.html'}  | ${'https://localhost:9200/foo/bar.html?details=91953740'}
+    ${'https://localhost:9200/files/list/all?unknown=87987987'}                       | ${'/foo'}                    | ${'/bar.html'}  | ${'https://localhost:9200/foo/bar.html'}
   `('$path -> $expected', async ({ location, base, path, expected }) => {
     delete window.location
     window.location = new URL(location) as any

packages/web-runtime/tests/unit/services/auth/authService.spec.ts

@@ -0,0 +1,27 @@
+import { AuthService } from 'web-runtime/src/services/auth/authService'
+
+describe('AuthService', () => {
+  describe('signInCallback', () => {
+    it.each([
+      [{ details: 'details' }, { details: 'details' }],
+      [{ details: 'details', unknown: 'unknown' }, { details: 'details' }],
+      [{ unknown: 'unknown' }, {}]
+    ])('forwards only whitelisted query parameters: %s = %s', async (query, expected: any) => {
+      const authService = new AuthService()
+      const replace = jest.fn()
+
+      jest.replaceProperty(authService as any, 'userManager', {
+        signinRedirectCallback: jest.fn(),
+        getAndClearPostLoginRedirectUrl: jest.fn()
+      })
+
+      jest.replaceProperty(authService as any, 'router', {
+        currentRoute: { query },
+        replace
+      })
+      await authService.signInCallback()
+
+      expect(replace.mock.calls[0][0].query).toEqual(expected)
+    })
+  })
+})