From 5fefab81f55a1389c7f4f478a4036535d3188595 Mon Sep 17 00:00:00 2001 From: claire bontempo <68122737+hellobontempo@users.noreply.github.com> Date: Fri, 26 Aug 2022 10:04:01 -0700 Subject: [PATCH] UI/OIDC auth bug for hcp namespace flag (#16886) * revert to using paramsFor but add check for state having ns= * revert to using paramsFor but add check for state having ns= * cleanup hook * add tests * add changelog * Test troubleshooting * cleanup tests, use window stub correctly! * add test for state param not existing at all Co-authored-by: hashishaw --- changelog/16886.txt | 3 + ui/app/routes/vault/cluster/oidc-callback.js | 23 +-- .../vault/cluster/oidc-callback-test.js | 184 ++++++++++++++++++ 3 files changed, 192 insertions(+), 18 deletions(-) create mode 100644 changelog/16886.txt create mode 100644 ui/tests/unit/routes/vault/cluster/oidc-callback-test.js diff --git a/changelog/16886.txt b/changelog/16886.txt new file mode 100644 index 000000000..73b9b418b --- /dev/null +++ b/changelog/16886.txt @@ -0,0 +1,3 @@ +```release-note:bug +ui: Fix OIDC callback to accept namespace flag in different formats +``` \ No newline at end of file diff --git a/ui/app/routes/vault/cluster/oidc-callback.js b/ui/app/routes/vault/cluster/oidc-callback.js index 27a2f3b58..7cd43bdb8 100644 --- a/ui/app/routes/vault/cluster/oidc-callback.js +++ b/ui/app/routes/vault/cluster/oidc-callback.js @@ -6,28 +6,15 @@ export default Route.extend({ // left blank so we render the template immediately }, afterModel() { - const queryString = decodeURIComponent(window.location.search); - // Since state param can also contain namespace, fetch the values using native url api. - // For instance, state params value can be state=st_123456,ns=d4fq - // Ember paramsFor used to strip out the value after the "=" sign. In short ns value was not being passed along. - let urlParams = new URLSearchParams(queryString); - let state = urlParams.get('state'), - code = urlParams.get('code'), - ns; - if (state.includes(',ns=')) { - let arrayParams = state.split(',ns='); - state = arrayParams[0]; - ns = arrayParams[1]; - } - let { auth_path: path } = this.paramsFor(this.routeName); + let { auth_path: path, code, state } = this.paramsFor(this.routeName); let { namespaceQueryParam: namespace } = this.paramsFor('vault.cluster'); + // only replace namespace param from cluster if state has a namespace + if (state?.includes(',ns=')) { + [state, namespace] = state.split(',ns='); + } path = window.decodeURIComponent(path); const source = 'oidc-callback'; // required by event listener in auth-jwt component let queryParams = { source, namespace, path, code, state }; - // If state had ns value, send it as part of namespace param - if (ns) { - queryParams.namespace = ns; - } window.opener.postMessage(queryParams, window.origin); }, setupController(controller) { diff --git a/ui/tests/unit/routes/vault/cluster/oidc-callback-test.js b/ui/tests/unit/routes/vault/cluster/oidc-callback-test.js new file mode 100644 index 000000000..5e4e680c0 --- /dev/null +++ b/ui/tests/unit/routes/vault/cluster/oidc-callback-test.js @@ -0,0 +1,184 @@ +import { module, test } from 'qunit'; +import { setupTest } from 'ember-qunit'; +import sinon from 'sinon'; + +module('Unit | Route | vault/cluster/oidc-callback', function (hooks) { + setupTest(hooks); + + hooks.beforeEach(function () { + this.originalOpener = window.opener; + window.opener = { + postMessage: () => {}, + }; + this.route = this.owner.lookup('route:vault/cluster/oidc-callback'); + this.windowStub = sinon.stub(window.opener, 'postMessage'); + this.path = 'oidc'; + this.code = 'lTazRXEwKfyGKBUCo5TyLJzdIt39YniBJOXPABiRMkL0T'; + this.state = (ns) => { + return ns ? 'st_91ji6vR2sQ2zBiZSQkqJ' + `,ns=${ns}` : 'st_91ji6vR2sQ2zBiZSQkqJ'; + }; + }); + + hooks.afterEach(function () { + this.windowStub.restore(); + window.opener = this.originalOpener; + }); + + test('it calls route', function (assert) { + assert.ok(this.route); + }); + + test('it uses namespace param from state not namespaceQueryParam from cluster with default path', function (assert) { + this.routeName = 'vault.cluster.oidc-callback'; + this.route.paramsFor = (path) => { + if (path === 'vault.cluster') return { namespaceQueryParam: 'admin' }; + return { + auth_path: this.path, + state: this.state('admin/child-ns'), + code: this.code, + }; + }; + this.route.afterModel(); + + assert.ok(this.windowStub.calledOnce, 'it is called'); + assert.propContains( + this.windowStub.lastCall.args[0], + { + code: 'lTazRXEwKfyGKBUCo5TyLJzdIt39YniBJOXPABiRMkL0T', + namespace: 'admin/child-ns', + path: 'oidc', + }, + 'namespace param is from state, ns=admin/child-ns' + ); + }); + + test('it uses namespace param from state not namespaceQueryParam from cluster with custom path', function (assert) { + this.routeName = 'vault.cluster.oidc-callback'; + this.route.paramsFor = (path) => { + if (path === 'vault.cluster') return { namespaceQueryParam: 'admin' }; + return { + auth_path: 'oidc-dev', + state: this.state('admin/child-ns'), + code: this.code, + }; + }; + this.route.afterModel(); + assert.propContains( + this.windowStub.lastCall.args[0], + { + path: 'oidc-dev', + namespace: 'admin/child-ns', + state: this.state(), + }, + 'state ns takes precedence, state no longer has ns query' + ); + }); + + test(`it uses namespace from namespaceQueryParam when state does not include: ',ns=some-namespace'`, function (assert) { + this.routeName = 'vault.cluster.oidc-callback'; + this.route.paramsFor = (path) => { + if (path === 'vault.cluster') return { namespaceQueryParam: 'admin' }; + return { + auth_path: this.path, + state: this.state(), + code: this.code, + }; + }; + this.route.afterModel(); + assert.propContains( + this.windowStub.lastCall.args[0], + { + path: this.path, + namespace: 'admin', + state: this.state(), + }, + 'namespace is from cluster namespaceQueryParam' + ); + }); + + test('it uses ns param from state when no namespaceQueryParam from cluster', function (assert) { + this.routeName = 'vault.cluster.oidc-callback'; + this.route.paramsFor = (path) => { + if (path === 'vault.cluster') return { namespaceQueryParam: '' }; + return { + auth_path: this.path, + state: this.state('ns1'), + code: this.code, + }; + }; + this.route.afterModel(); + assert.propContains( + this.windowStub.lastCall.args[0], + { + path: this.path, + namespace: 'ns1', + state: this.state(), + }, + 'it strips ns from state and uses as namespace param' + ); + }); + + test('the afterModel hook returns when both cluster and route params are empty strings', function (assert) { + this.routeName = 'vault.cluster.oidc-callback'; + this.route.paramsFor = (path) => { + if (path === 'vault.cluster') return { namespaceQueryParam: '' }; + return { + auth_path: '', + state: '', + code: '', + }; + }; + this.route.afterModel(); + assert.propContains( + this.windowStub.lastCall.args[0], + { + path: '', + state: '', + code: '', + }, + 'model hook returns with empty params' + ); + }); + + test('the afterModel hook returns when state param does not exist', function (assert) { + this.routeName = 'vault.cluster.oidc-callback'; + this.route.paramsFor = (path) => { + if (path === 'vault.cluster') return { namespaceQueryParam: '' }; + return { + auth_path: this.path, + }; + }; + this.route.afterModel(); + assert.propContains( + this.windowStub.lastCall.args[0], + { + code: undefined, + path: 'oidc', + state: undefined, + }, + 'model hook returns non-existent state param' + ); + }); + + test('the afterModel hook returns when cluster namespaceQueryParam exists and all route params are empty strings', function (assert) { + this.routeName = 'vault.cluster.oidc-callback'; + this.route.paramsFor = (path) => { + if (path === 'vault.cluster') return { namespaceQueryParam: 'ns1' }; + return { + auth_path: '', + state: '', + code: '', + }; + }; + this.route.afterModel(); + assert.propContains( + this.windowStub.lastCall.args[0], + { + path: '', + state: '', + code: '', + }, + 'model hook returns with empty parameters' + ); + }); +});