Fix broken nextUrl= parameter logic (#940)

* Fix next url issues Signed-off-by: Peter Nied <[email protected]> * Update request.url.query & request.url.path usages Signed-off-by: Peter Nied <[email protected]> * Add integration tests for the nextUrl scenarios Signed-off-by: Peter Nied <[email protected]> * Correct redirect for home test Signed-off-by: Peter Nied <[email protected]> * Fixes linter errors Signed-off-by: Darshit Chanpura <[email protected]> * Use headless browser for ITs Signed-off-by: Darshit Chanpura <[email protected]> * Modifies ITs to test nextUrl using anonymous auth enabled and disabled Signed-off-by: Darshit Chanpura <[email protected]> Co-authored-by: Darshit Chanpura <[email protected]>
opensearch-project · Apr 19, 2022 · 611ef5d · 611ef5d
1 parent 25263e7
commit 611ef5d
Show file tree

Hide file tree

Showing 9 changed files with 155 additions and 26 deletions.
diff --git a/.github/workflows/integration-test.yml b/.github/workflows/integration-test.yml
@@ -2,6 +2,10 @@ name: Integration Tests
 
 on: [push, pull_request]
 
+env:
+  TEST_BROWSER_HEADLESS: 1
+  CI: 1
+
 jobs:
   tests:
     name: Run integration tests

diff --git a/server/auth/types/basic/basic_auth.ts b/server/auth/types/basic/basic_auth.ts
@@ -29,7 +29,7 @@ import { SecuritySessionCookie } from '../../../session/security_cookie';
 import { BasicAuthRoutes } from './routes';
 import { AuthenticationType } from '../authentication_type';
 import { LOGIN_PAGE_URI } from '../../../../common';
-import { composeNextUrlQeuryParam } from '../../../utils/next_url';
+import { composeNextUrlQueryParam } from '../../../utils/next_url';
 
 export class BasicAuthentication extends AuthenticationType {
   private static readonly AUTH_HEADER_NAME: string = 'authorization';
@@ -107,7 +107,7 @@ export class BasicAuthentication extends AuthenticationType {
     toolkit: AuthToolkit
   ): OpenSearchDashboardsResponse {
     if (this.isPageRequest(request)) {
-      const nextUrlParam = composeNextUrlQeuryParam(
+      const nextUrlParam = composeNextUrlQueryParam(
         request,
         this.coreSetup.http.basePath.serverBasePath
       );

diff --git a/server/auth/types/basic/routes.ts b/server/auth/types/basic/routes.ts
@@ -24,7 +24,7 @@ import { User } from '../../user';
 import { SecurityClient } from '../../../backend/opensearch_security_client';
 import { API_AUTH_LOGIN, API_AUTH_LOGOUT, LOGIN_PAGE_URI } from '../../../../common';
 import { resolveTenant } from '../../../multitenancy/tenant_resolver';
-import { ParsedUrlQueryParams } from '../../../utils/next_url';
+import { encodeUriQuery } from '../../../../../../src/plugins/opensearch_dashboards_utils/common/url/encode_uri_query';
 
 export class BasicAuthRoutes {
   constructor(
@@ -165,14 +165,14 @@ export class BasicAuthRoutes {
       async (context, request, response) => {
         if (this.config.auth.anonymous_auth_enabled) {
           let user: User;
-          const path: string = `${request.url.path}`;
           // If the request contains no redirect path, simply redirect to basepath.
           let redirectUrl: string = this.coreSetup.http.basePath.serverBasePath
             ? this.coreSetup.http.basePath.serverBasePath
             : '/';
-          const requestQuery = request.url.query as ParsedUrlQueryParams;
-          if (requestQuery?.nextUrl !== undefined) {
-            redirectUrl = requestQuery.nextUrl;
+          const requestQuery = request.url.searchParams;
+          const nextUrl = requestQuery?.get('nextUrl');
+          if (nextUrl) {
+            redirectUrl = nextUrl;
           }
           context.security_plugin.logger.info('The Redirect Path is ' + redirectUrl);
           try {
@@ -183,7 +183,9 @@ export class BasicAuthRoutes {
             );
             return response.redirected({
               headers: {
-                location: `${this.coreSetup.http.basePath.serverBasePath}${LOGIN_PAGE_URI}`,
+                location: `${this.coreSetup.http.basePath.serverBasePath}${LOGIN_PAGE_URI}${
+                  nextUrl ? '?nextUrl=' + encodeUriQuery(redirectUrl) : ''
+                }`,
               },
             });
           }

diff --git a/server/auth/types/jwt/jwt_auth.ts b/server/auth/types/jwt/jwt_auth.ts
@@ -57,7 +57,7 @@ export class JwtAuthentication extends AuthenticationType {
   private getTokenFromUrlParam(request: OpenSearchDashboardsRequest): string | undefined {
     const urlParamName = this.config.jwt?.url_param;
     if (urlParamName) {
-      const token = (request.url.query as ParsedUrlQuery)[urlParamName];
+      const token = request.url.searchParams.get('urlParamName');
       return (token as string) || undefined;
     }
     return undefined;
@@ -81,7 +81,7 @@ export class JwtAuthentication extends AuthenticationType {
     }
 
     const urlParamName = this.config.jwt?.url_param;
-    if (urlParamName && (request.url.query as ParsedUrlQuery)[urlParamName]) {
+    if (urlParamName && request.url.searchParams.get('urlParamName')) {
       return true;
     }
 

diff --git a/server/auth/types/openid/openid_auth.ts b/server/auth/types/openid/openid_auth.ts
@@ -34,7 +34,7 @@ import { SecuritySessionCookie } from '../../../session/security_cookie';
 import { OpenIdAuthRoutes } from './routes';
 import { AuthenticationType } from '../authentication_type';
 import { callTokenEndpoint } from './helper';
-import { composeNextUrlQeuryParam } from '../../../utils/next_url';
+import { composeNextUrlQueryParam } from '../../../utils/next_url';
 
 export interface OpenIdAuthConfig {
   authorizationEndpoint?: string;
@@ -212,7 +212,7 @@ export class OpenIdAuthentication extends AuthenticationType {
   ): IOpenSearchDashboardsResponse {
     if (this.isPageRequest(request)) {
       // nextUrl is a key value pair
-      const nextUrl = composeNextUrlQeuryParam(
+      const nextUrl = composeNextUrlQueryParam(
         request,
         this.coreSetup.http.basePath.serverBasePath
       );

diff --git a/server/multitenancy/tenant_resolver.ts b/server/multitenancy/tenant_resolver.ts
@@ -42,13 +42,16 @@ export function resolveTenant(
   cookie: SecuritySessionCookie
 ): string | undefined {
   let selectedTenant: string | undefined;
-  const query: any = request.url.query as any;
-  if (query && (query.security_tenant || query.securitytenant)) {
-    selectedTenant = query.security_tenant ? query.security_tenant : query.securitytenant;
-  } else if (request.headers.securitytenant || request.headers.security_tenant) {
+  const securityTenant_ = request?.url?.searchParams?.get('securityTenant_');
+  const securitytenant = request?.url?.searchParams?.get('securitytenant');
+  if (securityTenant_) {
+    selectedTenant = securityTenant_;
+  } else if (securitytenant) {
+    selectedTenant = securitytenant;
+  } else if (request.headers.securitytenant || request.headers.securityTenant_) {
     selectedTenant = request.headers.securitytenant
       ? (request.headers.securitytenant as string)
-      : (request.headers.security_tenant as string);
+      : (request.headers.securityTenant_ as string);
   } else if (isValidTenant(cookie.tenant)) {
     selectedTenant = cookie.tenant;
   } else {

diff --git a/server/utils/next_url.test.ts b/server/utils/next_url.test.ts
@@ -13,7 +13,57 @@
  *   permissions and limitations under the License.
  */
 
-import { validateNextUrl, INVALID_NEXT_URL_PARAMETER_MESSAGE } from './next_url';
+import {
+  composeNextUrlQueryParam,
+  validateNextUrl,
+  INVALID_NEXT_URL_PARAMETER_MESSAGE,
+} from './next_url';
+
+describe('test composeNextUrlQueryParam', () => {
+  test('no base, no path', () => {
+    expect(
+      composeNextUrlQueryParam(
+        {
+          url: 'http://localhost:123',
+        },
+        ''
+      )
+    ).toEqual('');
+  });
+
+  test('no base, path', () => {
+    expect(
+      composeNextUrlQueryParam(
+        {
+          url: 'http://localhost:123/alpha/major/foxtrot',
+        },
+        ''
+      )
+    ).toEqual('nextUrl=%2Falpha%2Fmajor%2Ffoxtrot');
+  });
+
+  test('base, no path', () => {
+    expect(
+      composeNextUrlQueryParam(
+        {
+          url: 'http://localhost:123',
+        },
+        'xyz'
+      )
+    ).toEqual('');
+  });
+
+  test('base, path', () => {
+    expect(
+      composeNextUrlQueryParam(
+        {
+          url: 'http://localhost:123/alpha/major/foxtrot',
+        },
+        'xyz'
+      )
+    ).toEqual('nextUrl=xyz%2Falpha%2Fmajor%2Ffoxtrot');
+  });
+});
 
 /* eslint-disable no-script-url */
 describe('test validateNextUrl', () => {

diff --git a/server/utils/next_url.ts b/server/utils/next_url.ts
@@ -13,19 +13,27 @@
  *   permissions and limitations under the License.
  */
 
-import { cloneDeep } from 'lodash';
-import { format } from 'url';
-import { ParsedUrlQuery, stringify } from 'querystring';
+import { parse } from 'url';
+import { ParsedUrlQuery } from 'querystring';
 import { OpenSearchDashboardsRequest } from 'opensearch-dashboards/server';
+import { encodeUriQuery } from '../../../../src/plugins/opensearch_dashboards_utils/common/url/encode_uri_query';
 
-export function composeNextUrlQeuryParam(
+export function composeNextUrlQueryParam(
   request: OpenSearchDashboardsRequest,
   basePath: string
 ): string {
-  const url = cloneDeep(request.url);
-  url.pathname = `${basePath}${url.pathname}`;
-  const nextUrl = format(url);
-  return stringify({ nextUrl });
+  try {
+    const currentUrl = request.url.toString();
+    const parsedUrl = parse(currentUrl, true);
+    const nextUrl = parsedUrl?.path;
+
+    if (!!nextUrl && nextUrl !== '/') {
+      return `nextUrl=${encodeUriQuery(basePath + nextUrl)}`;
+    }
+  } catch (error) {
+    /* Ignore errors from parsing */
+  }
+  return '';
 }
 
 export interface ParsedUrlQueryParams extends ParsedUrlQuery {

diff --git a/test/jest_integration/basic_auth.test.ts b/test/jest_integration/basic_auth.test.ts
@@ -207,4 +207,66 @@ describe('start OpenSearch Dashboards server', () => {
 
     expect(response.status).toEqual(302);
   });
+
+  it('can access api/status route with admin credential', async () => {
+    const response = await osdTestServer.request
+      .get(root, '/api/status')
+      .set(AUTHORIZATION_HEADER_NAME, ADMIN_CREDENTIALS);
+    expect(response.status).toEqual(200);
+  });
+
+  it('redirect for home follows login for anonymous auth enabled', async () => {
+    const response = await osdTestServer.request
+      .get(root, '/app/home#/')
+      .unset(AUTHORIZATION_HEADER_NAME);
+
+    expect(response.status).toEqual(302);
+    expect(response.header.location).toEqual('/auth/anonymous?nextUrl=%2Fapp%2Fhome');
+
+    const response2 = await osdTestServer.request.get(root, response.header.location);
+
+    expect(response2.status).toEqual(302);
+    expect(response2.header.location).toEqual('/app/login?nextUrl=%2Fapp%2Fhome');
+
+    const response3 = await osdTestServer.request.get(root, response2.header.location);
+
+    expect(response3.status).toEqual(200);
+  });
+
+  it('redirect for home follows login for anonymous auth disabled', async () => {
+    const response = await osdTestServer.request
+      .get(anonymousDisabledRoot, '/app/home#/')
+      .unset(AUTHORIZATION_HEADER_NAME);
+
+    expect(response.status).toEqual(302);
+    expect(response.header.location).toEqual('/app/login?nextUrl=%2Fapp%2Fhome');
+
+    const response2 = await osdTestServer.request.get(
+      anonymousDisabledRoot,
+      response.header.location
+    );
+
+    // should hit login page and should not allow it to login becasue anonymouse auth is disabled
+    expect(response2.status).toEqual(200);
+  });
+
+  it('redirects to an object ignores after hash with anonymous auth enabled', async () => {
+    const startingPath = `/app/dashboards#/view/edf84fe0-e1a0-11e7-b6d5-4dc382ef7f5b`;
+    const expectedPath = `/app/login?nextUrl=%2Fapp%2Fdashboards`;
+
+    const response = await osdTestServer.request
+      .get(root, startingPath)
+      .unset(AUTHORIZATION_HEADER_NAME);
+
+    expect(response.status).toEqual(302);
+
+    const response2 = await osdTestServer.request.get(root, response.header.location);
+
+    expect(response2.status).toEqual(302);
+    expect(response2.header.location).toEqual(expectedPath);
+
+    const response3 = await osdTestServer.request.get(root, response2.header.location);
+
+    expect(response3.status).toEqual(200);
+  });
 });
-Original file line number
+Diff line change
@@ Expand Up / @@ -2,6 +2,10 @@ name: Integration Tests @@
     on: [push, pull_request]
+    env:
+      TEST_BROWSER_HEADLESS: 1
+      CI: 1
     jobs:
       tests:
         name: Run integration tests
@@ Expand Down @@