From 8261aae6bfe98cab9f8df75b4aa8396cd41305f4 Mon Sep 17 00:00:00 2001 From: "clairebontempo@gmail.com" Date: Thu, 25 Aug 2022 08:46:00 -0700 Subject: [PATCH 1/8] revert to using paramsFor but add check for state having ns= --- ui/app/routes/vault/cluster/oidc-callback.js | 24 +++++++------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/ui/app/routes/vault/cluster/oidc-callback.js b/ui/app/routes/vault/cluster/oidc-callback.js index 27a2f3b5838..5c1fcd1c24f 100644 --- a/ui/app/routes/vault/cluster/oidc-callback.js +++ b/ui/app/routes/vault/cluster/oidc-callback.js @@ -5,29 +5,21 @@ export default Route.extend({ model() { // left blank so we render the template immediately }, + // 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 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 { auth_path: path, code, state } = this.paramsFor(this.routeName); + let { namespaceQueryParam: namespace } = this.paramsFor('vault.cluster'); + console.log(namespace, 'ANAMESPCE'); + if (namespace === '' && state?.includes(',ns=')) { let arrayParams = state.split(',ns='); state = arrayParams[0]; - ns = arrayParams[1]; + namespace = arrayParams[1]; } - let { auth_path: path } = this.paramsFor(this.routeName); - let { namespaceQueryParam: namespace } = this.paramsFor('vault.cluster'); 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) { From ebb4e5b5a540d74466c9ebc6d885078ca47e55bd Mon Sep 17 00:00:00 2001 From: "clairebontempo@gmail.com" Date: Thu, 25 Aug 2022 08:46:18 -0700 Subject: [PATCH 2/8] revert to using paramsFor but add check for state having ns= --- ui/app/routes/vault/cluster/oidc-callback.js | 1 - 1 file changed, 1 deletion(-) diff --git a/ui/app/routes/vault/cluster/oidc-callback.js b/ui/app/routes/vault/cluster/oidc-callback.js index 5c1fcd1c24f..2784c501b4e 100644 --- a/ui/app/routes/vault/cluster/oidc-callback.js +++ b/ui/app/routes/vault/cluster/oidc-callback.js @@ -11,7 +11,6 @@ export default Route.extend({ afterModel() { let { auth_path: path, code, state } = this.paramsFor(this.routeName); let { namespaceQueryParam: namespace } = this.paramsFor('vault.cluster'); - console.log(namespace, 'ANAMESPCE'); if (namespace === '' && state?.includes(',ns=')) { let arrayParams = state.split(',ns='); state = arrayParams[0]; From c6eea49343da6b129fc1387814a8d4112a9d0912 Mon Sep 17 00:00:00 2001 From: "clairebontempo@gmail.com" Date: Thu, 25 Aug 2022 12:30:03 -0700 Subject: [PATCH 3/8] cleanup hook --- ui/app/routes/vault/cluster/oidc-callback.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/ui/app/routes/vault/cluster/oidc-callback.js b/ui/app/routes/vault/cluster/oidc-callback.js index 2784c501b4e..cf92cea1178 100644 --- a/ui/app/routes/vault/cluster/oidc-callback.js +++ b/ui/app/routes/vault/cluster/oidc-callback.js @@ -5,13 +5,11 @@ export default Route.extend({ model() { // left blank so we render the template immediately }, - // 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 afterModel() { let { auth_path: path, code, state } = this.paramsFor(this.routeName); let { namespaceQueryParam: namespace } = this.paramsFor('vault.cluster'); - if (namespace === '' && state?.includes(',ns=')) { + // only replace namespace param from cluster if state has a namespace + if (state?.includes(',ns=')) { let arrayParams = state.split(',ns='); state = arrayParams[0]; namespace = arrayParams[1]; From 1ce89c7e95d8faa53332ddaa07bcfe88d0f7155e Mon Sep 17 00:00:00 2001 From: "clairebontempo@gmail.com" Date: Thu, 25 Aug 2022 14:58:10 -0700 Subject: [PATCH 4/8] add tests --- .../vault/cluster/oidc-callback-test.js | 111 ++++++++++++++++++ 1 file changed, 111 insertions(+) create mode 100644 ui/tests/unit/routes/vault/cluster/oidc-callback-test.js 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 00000000000..470be4ea9eb --- /dev/null +++ b/ui/tests/unit/routes/vault/cluster/oidc-callback-test.js @@ -0,0 +1,111 @@ +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); + const parentNs = 'admin'; + const childNs = 'admin/child-ns'; + const path = 'oidc'; + const customPath = 'oidc-dev'; + const code = 'lTazRXEwKfyGKBUCo5TyLJzdIt39YniBJOXPABiRMkL0T'; + const state = (ns) => { + ns ? 'st_91ji6vR2sQ2zBiZSQkqJ' + `,ns=${ns}` : 'st_91ji6vR2sQ2zBiZSQkqJ'; + }; + + hooks.beforeEach(function () { + this.router = this.owner.lookup('service:router'); + this.route = this.owner.lookup('route:vault/cluster/oidc-callback'); + this.windowStub = sinon.stub(window.opener, 'postMessage'); + }); + + 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: parentNs }; + return { + auth_path: path, + state: state(childNs), + code, + }; + }; + assert.ok(this.windowStub.calledWith, 'test'); + assert.propContains( + this.route.afterModel(), + { + path, + namespace: childNs, + state: state(), + }, + 'state and namespace queryParams are correct' + ); + }); + + 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: parentNs }; + return { + auth_path: customPath, + state: state(childNs), + code, + }; + }; + assert.propContains( + this.route.afterModel(), + { + path: customPath, + namespace: childNs, + state: state(), + }, + 'state ns takes precedence, state no longer has ns query' + ); + }); + + test('it uses namespace from namespaceQueryParam when no ns param from state', function (assert) { + this.routeName = 'vault.cluster.oidc-callback'; + this.route.paramsFor = (path) => { + if (path === 'vault.cluster') return { namespaceQueryParam: parentNs }; + return { + auth_path: path, + state: state(), + code, + }; + }; + assert.propContains( + this.route.afterModel(), + { + path, + namespace: parentNs, + state: 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: path, + state: state('ns1'), + code, + }; + }; + assert.propContains( + this.route.afterModel(), + { + path, + namespace: 'ns1', + state: state(), + }, + 'it strips ns from state and uses as namespace param' + ); + }); +}); From cd131a736b9e9c973d59bb4c27ac23463a942af4 Mon Sep 17 00:00:00 2001 From: "clairebontempo@gmail.com" Date: Thu, 25 Aug 2022 15:03:28 -0700 Subject: [PATCH 5/8] add changelog --- changelog/16886.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/16886.txt diff --git a/changelog/16886.txt b/changelog/16886.txt new file mode 100644 index 00000000000..73b9b418b36 --- /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 From 3ddcb461e9141a0ef653e19d907ede96c102178a Mon Sep 17 00:00:00 2001 From: hashishaw Date: Fri, 26 Aug 2022 10:34:06 -0500 Subject: [PATCH 6/8] Test troubleshooting --- .../vault/cluster/oidc-callback-test.js | 69 ++++++++++++------- 1 file changed, 44 insertions(+), 25 deletions(-) diff --git a/ui/tests/unit/routes/vault/cluster/oidc-callback-test.js b/ui/tests/unit/routes/vault/cluster/oidc-callback-test.js index 470be4ea9eb..8aa58e9be95 100644 --- a/ui/tests/unit/routes/vault/cluster/oidc-callback-test.js +++ b/ui/tests/unit/routes/vault/cluster/oidc-callback-test.js @@ -2,21 +2,37 @@ import { module, test } from 'qunit'; import { setupTest } from 'ember-qunit'; import sinon from 'sinon'; +// window.opener = { +// postMessage: () => {}, +// origin: 'http://localhost:4200', +// }; +const origin = 'http://localhost:4200'; + module('Unit | Route | vault/cluster/oidc-callback', function (hooks) { setupTest(hooks); - const parentNs = 'admin'; - const childNs = 'admin/child-ns'; - const path = 'oidc'; - const customPath = 'oidc-dev'; - const code = 'lTazRXEwKfyGKBUCo5TyLJzdIt39YniBJOXPABiRMkL0T'; - const state = (ns) => { - ns ? 'st_91ji6vR2sQ2zBiZSQkqJ' + `,ns=${ns}` : 'st_91ji6vR2sQ2zBiZSQkqJ'; - }; hooks.beforeEach(function () { + this.originalOpener = window.opener; + window.opener = { + postMessage: () => {}, + origin, // todo need? + }; this.router = this.owner.lookup('service:router'); this.route = this.owner.lookup('route:vault/cluster/oidc-callback'); this.windowStub = sinon.stub(window.opener, 'postMessage'); + this.parentNs = 'admin'; + this.childNs = 'admin/child-ns'; + this.path = 'oidc'; + this.customPath = 'oidc-dev'; + this.code = 'lTazRXEwKfyGKBUCo5TyLJzdIt39YniBJOXPABiRMkL0T'; + this.state = (ns) => { + ns ? 'st_91ji6vR2sQ2zBiZSQkqJ' + `,ns=${ns}` : 'st_91ji6vR2sQ2zBiZSQkqJ'; + }; + }); + + hooks.afterEach(function () { + this.windowStub.restore(); + window.opener = this.originalOpener; }); test('it calls route', function (assert) { @@ -25,24 +41,27 @@ module('Unit | Route | vault/cluster/oidc-callback', function (hooks) { 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: parentNs }; + if (path === 'vault.cluster') return { namespaceQueryParam: this.parentNs }; return { - auth_path: path, - state: state(childNs), - code, + auth_path: this.path, + state: this.state(this.childNs), + code: this.code, }; }; - assert.ok(this.windowStub.calledWith, 'test'); + this.route.afterModel(); + + assert.ok(this.windowStub.calledOnce, 'it is called'); assert.propContains( - this.route.afterModel(), + this.windowStub.getCall(0).args[0], { - path, - namespace: childNs, - state: state(), + code: 'lTazRXEwKfyGKBUCo5TyLJzdIt39YniBJOXPABiRMkL0T', + namespace: 'admin', + path: 'oidc', + source: 'oidc-callback', }, - 'state and namespace queryParams are correct' + + 'calls correct' ); }); @@ -51,17 +70,17 @@ module('Unit | Route | vault/cluster/oidc-callback', function (hooks) { this.route.paramsFor = (path) => { if (path === 'vault.cluster') return { namespaceQueryParam: parentNs }; return { - auth_path: customPath, - state: state(childNs), - code, + auth_path: this.customPath, + state: this.state(this.childNs), + code: this.code, }; }; assert.propContains( this.route.afterModel(), { - path: customPath, - namespace: childNs, - state: state(), + path: this.customPath, + namespace: this.childNs, + state: this.state(), }, 'state ns takes precedence, state no longer has ns query' ); From 3d5221a817e9217290f5bc5c349735ec49397e27 Mon Sep 17 00:00:00 2001 From: "clairebontempo@gmail.com" Date: Fri, 26 Aug 2022 09:29:12 -0700 Subject: [PATCH 7/8] cleanup tests, use window stub correctly! --- ui/app/routes/vault/cluster/oidc-callback.js | 4 +- .../vault/cluster/oidc-callback-test.js | 114 ++++++++++++------ 2 files changed, 75 insertions(+), 43 deletions(-) diff --git a/ui/app/routes/vault/cluster/oidc-callback.js b/ui/app/routes/vault/cluster/oidc-callback.js index cf92cea1178..7cd43bdb823 100644 --- a/ui/app/routes/vault/cluster/oidc-callback.js +++ b/ui/app/routes/vault/cluster/oidc-callback.js @@ -10,9 +10,7 @@ export default Route.extend({ let { namespaceQueryParam: namespace } = this.paramsFor('vault.cluster'); // only replace namespace param from cluster if state has a namespace if (state?.includes(',ns=')) { - let arrayParams = state.split(',ns='); - state = arrayParams[0]; - namespace = arrayParams[1]; + [state, namespace] = state.split(',ns='); } path = window.decodeURIComponent(path); const source = 'oidc-callback'; // required by event listener in auth-jwt component diff --git a/ui/tests/unit/routes/vault/cluster/oidc-callback-test.js b/ui/tests/unit/routes/vault/cluster/oidc-callback-test.js index 8aa58e9be95..a278125a548 100644 --- a/ui/tests/unit/routes/vault/cluster/oidc-callback-test.js +++ b/ui/tests/unit/routes/vault/cluster/oidc-callback-test.js @@ -2,12 +2,6 @@ import { module, test } from 'qunit'; import { setupTest } from 'ember-qunit'; import sinon from 'sinon'; -// window.opener = { -// postMessage: () => {}, -// origin: 'http://localhost:4200', -// }; -const origin = 'http://localhost:4200'; - module('Unit | Route | vault/cluster/oidc-callback', function (hooks) { setupTest(hooks); @@ -15,18 +9,13 @@ module('Unit | Route | vault/cluster/oidc-callback', function (hooks) { this.originalOpener = window.opener; window.opener = { postMessage: () => {}, - origin, // todo need? }; - this.router = this.owner.lookup('service:router'); this.route = this.owner.lookup('route:vault/cluster/oidc-callback'); this.windowStub = sinon.stub(window.opener, 'postMessage'); - this.parentNs = 'admin'; - this.childNs = 'admin/child-ns'; this.path = 'oidc'; - this.customPath = 'oidc-dev'; this.code = 'lTazRXEwKfyGKBUCo5TyLJzdIt39YniBJOXPABiRMkL0T'; this.state = (ns) => { - ns ? 'st_91ji6vR2sQ2zBiZSQkqJ' + `,ns=${ns}` : 'st_91ji6vR2sQ2zBiZSQkqJ'; + return ns ? 'st_91ji6vR2sQ2zBiZSQkqJ' + `,ns=${ns}` : 'st_91ji6vR2sQ2zBiZSQkqJ'; }; }); @@ -42,10 +31,10 @@ module('Unit | Route | vault/cluster/oidc-callback', function (hooks) { 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: this.parentNs }; + if (path === 'vault.cluster') return { namespaceQueryParam: 'admin' }; return { auth_path: this.path, - state: this.state(this.childNs), + state: this.state('admin/child-ns'), code: this.code, }; }; @@ -53,55 +42,55 @@ module('Unit | Route | vault/cluster/oidc-callback', function (hooks) { assert.ok(this.windowStub.calledOnce, 'it is called'); assert.propContains( - this.windowStub.getCall(0).args[0], + this.windowStub.lastCall.args[0], { code: 'lTazRXEwKfyGKBUCo5TyLJzdIt39YniBJOXPABiRMkL0T', - namespace: 'admin', + namespace: 'admin/child-ns', path: 'oidc', - source: 'oidc-callback', }, - - 'calls correct' + '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: parentNs }; + if (path === 'vault.cluster') return { namespaceQueryParam: 'admin' }; return { - auth_path: this.customPath, - state: this.state(this.childNs), + auth_path: 'oidc-dev', + state: this.state('admin/child-ns'), code: this.code, }; }; + this.route.afterModel(); assert.propContains( - this.route.afterModel(), + this.windowStub.lastCall.args[0], { - path: this.customPath, - namespace: this.childNs, + 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 no ns param from state', function (assert) { + 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: parentNs }; + if (path === 'vault.cluster') return { namespaceQueryParam: 'admin' }; return { - auth_path: path, - state: state(), - code, + auth_path: this.path, + state: this.state(), + code: this.code, }; }; + this.route.afterModel(); assert.propContains( - this.route.afterModel(), + this.windowStub.lastCall.args[0], { - path, - namespace: parentNs, - state: state(), + path: this.path, + namespace: 'admin', + state: this.state(), }, 'namespace is from cluster namespaceQueryParam' ); @@ -112,19 +101,64 @@ module('Unit | Route | vault/cluster/oidc-callback', function (hooks) { this.route.paramsFor = (path) => { if (path === 'vault.cluster') return { namespaceQueryParam: '' }; return { - auth_path: path, - state: state('ns1'), - code, + auth_path: this.path, + state: this.state('ns1'), + code: this.code, }; }; + this.route.afterModel(); assert.propContains( - this.route.afterModel(), + this.windowStub.lastCall.args[0], { - path, + path: this.path, namespace: 'ns1', - state: state(), + 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', 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 cluster namespaceQueryParam exists and all route params are empty', 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' + ); + }); }); From e31ef6582a3e4becc9aa87b0bd968944945f858b Mon Sep 17 00:00:00 2001 From: "clairebontempo@gmail.com" Date: Fri, 26 Aug 2022 09:34:17 -0700 Subject: [PATCH 8/8] add test for state param not existing at all --- .../vault/cluster/oidc-callback-test.js | 24 +++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/ui/tests/unit/routes/vault/cluster/oidc-callback-test.js b/ui/tests/unit/routes/vault/cluster/oidc-callback-test.js index a278125a548..5e4e680c074 100644 --- a/ui/tests/unit/routes/vault/cluster/oidc-callback-test.js +++ b/ui/tests/unit/routes/vault/cluster/oidc-callback-test.js @@ -118,7 +118,7 @@ module('Unit | Route | vault/cluster/oidc-callback', function (hooks) { ); }); - test('the afterModel hook returns when both cluster and route params are empty', function (assert) { + 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: '' }; @@ -140,7 +140,27 @@ module('Unit | Route | vault/cluster/oidc-callback', function (hooks) { ); }); - test('the afterModel hook returns when cluster namespaceQueryParam exists and all route params are empty', function (assert) { + 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' };