Merge pull request #8782 from owncloud/authservice-signincallback-que…

…ry-params fix: query param handling in AuthService.signInCallback
owncloud · Apr 6, 2023 · 772abbc · 772abbc
2 parents 84b9539 + 4306990
commit 772abbc
Show file tree

Hide file tree

Showing 5 changed files with 39 additions and 49 deletions.
diff --git a/changelog/unreleased/bugfix-open-external-apps b/changelog/unreleased/bugfix-open-external-apps
@@ -1,6 +1,8 @@
 Bugfix: Open files in external app
 
-We've fixed a bug that caused office documents not to be opened in app provider editors anymore.
+We've fixed bugs that caused office documents not to be opened in app provider editors anymore.
 
-https://github.com/owncloud/web/pull/8705
 https://github.com/owncloud/web/issues/8695
+https://github.com/owncloud/web/pull/8705
+https://github.com/owncloud/web/issues/8773
+https://github.com/owncloud/web/pull/8782
diff --git a/packages/web-runtime/src/router/helpers.ts b/packages/web-runtime/src/router/helpers.ts
@@ -1,6 +1,5 @@
 import { base, router } from './index'
 import { RouteLocation, RouteParams, Router, RouteRecordNormalized } from 'vue-router'
-import pick from 'lodash-es/pick'
 import {
   AuthContext,
   authContextValues,
@@ -11,17 +10,7 @@ import {
 export const buildUrl = (pathname) => {
   const isHistoryMode = !!base
   const baseUrl = new URL(window.location.href.split('#')[0])
-
-  // 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
+  baseUrl.search = ''
 
   if (isHistoryMode) {
     // in history mode we can't determine the base path, it must be provided by the document

diff --git a/packages/web-runtime/src/services/auth/authService.ts b/packages/web-runtime/src/services/auth/authService.ts
@@ -13,7 +13,6 @@ import {
 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
@@ -176,23 +175,12 @@ export class AuthService {
    * Sign in callback gets called from the IDP after initial login.
    */
   public async signInCallback() {
-    const currentQuery = unref(this.router.currentRoute).query
-
     try {
       await this.userManager.signinRedirectCallback(this.buildSignInCallbackUrl())
-
-      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'
-      ])
-
+      const redirectRoute = this.router.resolve(this.userManager.getAndClearPostLoginRedirectUrl())
       return this.router.replace({
-        path: redirectUrl,
-        query: transportQuery
+        path: redirectRoute.path,
+        ...(redirectRoute.query && { query: redirectRoute.query })
       })
     } catch (e) {
       console.warn('error during authentication:', e)

diff --git a/packages/web-runtime/tests/unit/router/index.spec.ts b/packages/web-runtime/tests/unit/router/index.spec.ts
@@ -15,9 +15,6 @@ 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

diff --git a/packages/web-runtime/tests/unit/services/auth/authService.spec.ts b/packages/web-runtime/tests/unit/services/auth/authService.spec.ts
@@ -1,27 +1,41 @@
 import { AuthService } from 'web-runtime/src/services/auth/authService'
+import { createRouter } from 'web-test-helpers/src'
 
 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()
+      ['/', '/', {}],
+      ['/?details=sharing', '/', { details: 'sharing' }],
+      [
+        '/external?contextRouteName=files-spaces-personal&fileId=0f897576',
+        '/external',
+        {
+          contextRouteName: 'files-spaces-personal',
+          fileId: '0f897576'
+        }
+      ]
+    ])(
+      'parses query params and passes them explicitly to router.replace: %s => %s %s',
+      async (url, path, query: any) => {
+        const authService = new AuthService()
 
-      jest.replaceProperty(authService as any, 'userManager', {
-        signinRedirectCallback: jest.fn(),
-        getAndClearPostLoginRedirectUrl: jest.fn()
-      })
+        jest.replaceProperty(authService as any, 'userManager', {
+          signinRedirectCallback: jest.fn(),
+          getAndClearPostLoginRedirectUrl: () => url
+        })
 
-      jest.replaceProperty(authService as any, 'router', {
-        currentRoute: { query },
-        replace
-      })
-      await authService.signInCallback()
+        const router = createRouter()
+        const replaceSpy = jest.spyOn(router, 'replace')
 
-      expect(replace.mock.calls[0][0].query).toEqual(expected)
-    })
+        authService.initialize(null, null, null, router, null, null)
+
+        await authService.signInCallback()
+
+        expect(replaceSpy).toHaveBeenCalledWith({
+          path,
+          query
+        })
+      }
+    )
   })
 })