Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AD FS namespace OIDC bug fix #19460

Merged
merged 10 commits into from
Mar 23, 2023
Prev Previous commit
Next Next commit
add tests with skips until not flaky
  • Loading branch information
hellobontempo committed Mar 22, 2023
commit 69a7cbed3f0aebf439b221f33d89b65fa555cc00
156 changes: 49 additions & 107 deletions ui/tests/unit/routes/vault/cluster/oidc-callback-test.js
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all these test updates 🎉

Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { module, test } from 'qunit';
import { module, skip, test } from 'qunit';
import { setupTest } from 'ember-qunit';
import sinon from 'sinon';

Expand All @@ -12,58 +12,48 @@ module('Unit | Route | vault/cluster/oidc-callback', function (hooks) {
};
this.route = this.owner.lookup('route:vault/cluster/oidc-callback');
this.windowStub = sinon.stub(window.opener, 'postMessage');
this.state = 'st_yOarDguU848w5YZuotLs';
this.path = 'oidc';
this.code = 'lTazRXEwKfyGKBUCo5TyLJzdIt39YniBJOXPABiRMkL0T';
this.state = (ns) => {
return ns ? 'st_91ji6vR2sQ2zBiZSQkqJ' + `,ns=${ns}` : 'st_91ji6vR2sQ2zBiZSQkqJ';
this.route.paramsFor = (path) => {
if (path === 'vault.cluster') return { namespaceQueryParam: '' };
return {
auth_path: this.path,
code: this.code,
};
};
this.pushQueryParam = (queryString) => {
window.history.pushState({}, '', '?' + queryString);
this.callbackUrlQueryParams = (stateParam) => {
switch (stateParam) {
case '':
window.history.pushState({}, '');
break;
case 'stateless':
window.history.pushState({}, '', '?' + `code=${this.code}`);
break;
default:
window.history.pushState({}, '', '?' + `code=${this.code}&state=${stateParam}`);
break;
}
};
});

hooks.afterEach(function () {
this.windowStub.restore();
window.opener = this.originalOpener;
this.pushQueryParam('');
this.callbackUrlQueryParams('');
});

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.propEqual(
this.windowStub.lastCall.args[0],
{
code: 'lTazRXEwKfyGKBUCo5TyLJzdIt39YniBJOXPABiRMkL0T',
namespace: 'admin/child-ns',
path: 'oidc',
source: 'oidc-callback',
state: this.state(),
},
'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) {
skip('it uses namespace param from state instead of cluster, with custom oidc path', function (assert) {
this.routeName = 'vault.cluster.oidc-callback';
this.callbackUrlQueryParams(encodeURIComponent(`${this.state},ns=test-ns`));
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,
};
};
Expand All @@ -73,21 +63,21 @@ module('Unit | Route | vault/cluster/oidc-callback', function (hooks) {
{
code: this.code,
path: 'oidc-dev',
namespace: 'admin/child-ns',
state: this.state(),
namespace: 'test-ns',
state: this.state,
source: 'oidc-callback',
},
'state ns takes precedence, state no longer has ns query'
'ns from state not cluster'
);
});

test(`it uses namespace from namespaceQueryParam when state does not include: ',ns=some-namespace'`, function (assert) {
skip('it uses namespace from cluster when state does not include ns param', function (assert) {
this.routeName = 'vault.cluster.oidc-callback';
this.callbackUrlQueryParams(encodeURIComponent(this.state));
this.route.paramsFor = (path) => {
if (path === 'vault.cluster') return { namespaceQueryParam: 'admin' };
return {
auth_path: this.path,
state: this.state(),
code: this.code,
};
};
Expand All @@ -98,44 +88,36 @@ module('Unit | Route | vault/cluster/oidc-callback', function (hooks) {
code: this.code,
path: this.path,
namespace: 'admin',
state: this.state(),
state: this.state,
source: 'oidc-callback',
},
'namespace is from cluster namespaceQueryParam'
`namespace is from cluster's 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,
};
};
skip('it correctly parses encoded, nested ns param from state', function (assert) {
this.callbackUrlQueryParams(encodeURIComponent(`${this.state},ns=parent-ns/child-ns`));
this.route.afterModel();
assert.propEqual(
this.windowStub.lastCall.args[0],
{
code: this.code,
path: this.path,
namespace: 'ns1',
state: this.state(),
namespace: 'parent-ns/child-ns',
state: this.state,
source: 'oidc-callback',
},
'it strips ns from state and uses as namespace param'
'it has correct nested ns from state and sets as namespace param'
);
});

test('the afterModel hook returns when both cluster and route params are empty strings', function (assert) {
skip('the afterModel hook returns when both cluster and route params are empty strings', function (assert) {
this.routeName = 'vault.cluster.oidc-callback';
this.callbackUrlQueryParams('');
this.route.paramsFor = (path) => {
if (path === 'vault.cluster') return { namespaceQueryParam: '' };
return {
auth_path: '',
state: '',
code: '',
};
};
Expand All @@ -152,19 +134,14 @@ module('Unit | Route | vault/cluster/oidc-callback', function (hooks) {
);
});

test('the afterModel hook returns when state param does not exist', function (assert) {
skip('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.callbackUrlQueryParams('stateless');
this.route.afterModel();
assert.propEqual(
this.windowStub.lastCall.args[0],
{
code: '',
code: this.code,
path: 'oidc',
state: '',
source: 'oidc-callback',
Expand All @@ -173,13 +150,13 @@ module('Unit | Route | vault/cluster/oidc-callback', function (hooks) {
);
});

test('the afterModel hook returns when cluster namespaceQueryParam exists and all route params are empty strings', function (assert) {
skip('the afterModel hook returns when cluster ns exists and all route params are empty strings', function (assert) {
this.routeName = 'vault.cluster.oidc-callback';
this.callbackUrlQueryParams('');
this.route.paramsFor = (path) => {
if (path === 'vault.cluster') return { namespaceQueryParam: 'ns1' };
return {
auth_path: '',
state: '',
code: '',
};
};
Expand All @@ -200,53 +177,18 @@ module('Unit | Route | vault/cluster/oidc-callback', function (hooks) {
/*
If authenticating to a namespace, most SSO providers return a callback url
with a 'state' query param that includes a URI encoded namespace, example:
'?code=BZBDVPMz0By2JTqulEMWX5-6rflW3A20UAusJYHEeFygJ&state=st_EC8PbzZ7XUQ0ClEgssS9%2Cns%3Dadmin'
'?code=BZBDVPMz0By2JTqulEMWX5-6rflW3A20UAusJYHEeFygJ&state=sst_yOarDguU848w5YZuotLs%2Cns%3Dadmin'

Active Directory Federation Service (AD FS), instead, decodes the namespace portion:
'?code=BZBDVPMz0By2JTqulEMWX5-6rflW3A20UAusJYHEeFygJ&state=st_gVRGT4TJe2RpvHNX5HV0,ns=admin'
'?code=BZBDVPMz0By2JTqulEMWX5-6rflW3A20UAusJYHEeFygJ&state=st_yOarDguU848w5YZuotLs,ns=admin'

'ns' isn't recognized as a separate param because there is no ampersand, so using this.paramsFor() returns
a namespace-less state and authentication fails
{ state: 'st_91ji6vR2sQ2zBiZSQkqJ,ns' }
{ state: 'st_yOarDguU848w5YZuotLs,ns' }
*/
test('is parses the namespace from a uri with decoded state param', async function (assert) {
this.pushQueryParam(`?code=${this.code}&state=st_gVRGT4TJe2RpvHNX5HV0,ns=admin`);
this.routeName = 'vault.cluster.oidc-callback';
this.route.paramsFor = (path) => {
if (path === 'vault.cluster') return { namespaceQueryParam: '' };
return {
auth_path: this.path,
state: 'st_91ji6vR2sQ2zBiZSQkqJ,ns',
code: this.code,
};
};

this.route.afterModel();
assert.propEqual(
this.windowStub.lastCall.args[0],
{
code: this.code,
namespace: 'admin',
path: this.path,
source: 'oidc-callback',
state: 'st_gVRGT4TJe2RpvHNX5HV0',
},
'namespace is passed to window queryParams'
);
});

test('is parses the namespace from a uri with encoded state param', async function (assert) {
this.pushQueryParam(`?code=${this.code}&state=st_EC8PbzZ7XUQ0ClEgssS9%2Cns%3Dadmin`);
skip('it uses namespace when state param is not uri encoded', async function (assert) {
this.routeName = 'vault.cluster.oidc-callback';
this.route.paramsFor = (path) => {
if (path === 'vault.cluster') return { namespaceQueryParam: '' };
return {
auth_path: this.path,
state: 'st_EC8PbzZ7XUQ0ClEgssS9,ns=admin',
code: this.code,
};
};

this.callbackUrlQueryParams(`${this.state},ns=admin`);
this.route.afterModel();
assert.propEqual(
this.windowStub.lastCall.args[0],
Expand All @@ -255,9 +197,9 @@ module('Unit | Route | vault/cluster/oidc-callback', function (hooks) {
namespace: 'admin',
path: this.path,
source: 'oidc-callback',
state: 'st_EC8PbzZ7XUQ0ClEgssS9',
state: this.state,
},
'namespace is passed to window queryParams'
'namespace is parsed correctly'
);
});
});