From 43069907eb9273bbd1f2384155c9b2687fd89ad1 Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Wed, 5 Apr 2023 17:01:09 +0200 Subject: [PATCH] Fix query param handling in AuthService.signInCallback --- .../unreleased/bugfix-open-external-apps | 6 ++- packages/web-runtime/src/router/helpers.ts | 13 +---- .../src/services/auth/authService.ts | 18 ++----- .../tests/unit/router/index.spec.ts | 3 -- .../unit/services/auth/authService.spec.ts | 48 ++++++++++++------- 5 files changed, 39 insertions(+), 49 deletions(-) diff --git a/changelog/unreleased/bugfix-open-external-apps b/changelog/unreleased/bugfix-open-external-apps index cdbaca44628..8525d5c9e95 100644 --- 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 index e0a4706a78b..e69df63455e 100644 --- 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 index 73f62179761..0bffd74570f 100644 --- 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 index 0478851b4a2..c8e5a6de665 100644 --- 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 index cb1b603dc84..cef73fa23f1 100644 --- 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 + }) + } + ) }) })