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

UI: Fix MFA + SSO login workflow #28873

Merged
merged 17 commits into from
Dec 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions changelog/28873.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
```release-note:bug
ui: Fixes login to web UI when MFA is enabled for OIDC (i.e. azure, auth0) and Okta auth methods
```
```release-note:bug
ui (enterprise): Fixes login to web UI when MFA is enabled for SAML auth methods
```
1 change: 1 addition & 0 deletions ui/app/components/auth-form.js
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,7 @@ export default Component.extend(DEFAULTS, {
data.path = 'okta';
}
}
// calls performAuth in login-form.js which initiates the authenticate @task
return this.performAuth(backend.type, data);
},
handleError(e) {
Expand Down
4 changes: 3 additions & 1 deletion ui/app/components/auth-jwt.js
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,9 @@ export default Component.extend({
// and show the error on the login screen
return this.cancelLogin(oidcWindow, e);
}
yield this.onSubmit(null, null, resp.auth.client_token);
const { mfa_requirement, client_token } = resp.auth;
// onSubmit calls doSubmit in auth-form.js
yield this.onSubmit({ mfa_requirement }, null, client_token);
}),

async startOIDCAuth() {
Expand Down
6 changes: 4 additions & 2 deletions ui/app/components/auth-saml.js
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,10 @@ export default class AuthSaml extends Component {
if (!resp?.auth) {
continue;
}
// We've obtained the Vault token for the authentication flow, now log in.
yield this.args.onSubmit(null, null, resp.auth.client_token);
// We've obtained the Vault token for the authentication flow now log in or pass MFA data
const { mfa_requirement, client_token } = resp.auth;
// onSubmit calls doSubmit in auth-form.js
yield this.args.onSubmit({ mfa_requirement }, null, client_token);
this.closeWindow(samlWindow);
return;
} catch (e) {
Expand Down
19 changes: 19 additions & 0 deletions ui/app/components/auth/login-form.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,13 +60,32 @@ export default class AuthLoginFormComponent extends Component {
} else {
this.delayAuthMessageReminder.perform();
}

/*
Checking for an mfa_requirement happens in two places.
Copy link
Contributor

Choose a reason for hiding this comment

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

great comments!

Login methods submitted using a child of <AuthForm> have custom auth logic where mfa_requirements are collected, if any.
This mfa data is passed to their respective onSubmit callback functions, then intercepted here and passed up to
the parent <Auth::Page> component (which renders the Mfa::MfaForm).

If doSubmit in <AuthForm> is called directly (by the "default" <form> component)
mfa is handled directly by the parent <Auth::Page> component.
*/
if (data?.mfa_requirement) {
const parsedMfaAuthResponse = this.auth._parseMfaResponse(data.mfa_requirement);
// calls onAuthResponse in parent auth/page.js component
this.args.onSuccess(parsedMfaAuthResponse, backendType, data);
// return here because mfa-form.js will finish login/authentication flow after mfa validation
return;
}

const authResponse = yield this.auth.authenticate({
clusterId,
backend: backendType,
data,
selectedAuth,
});

// calls onAuthResponse in auth/page.js
this.args.onSuccess(authResponse, backendType, data);
} catch (e) {
if (!this.auth.mfaError) {
Expand Down
11 changes: 10 additions & 1 deletion ui/app/components/auth/page.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,16 +51,25 @@ export default class AuthPage extends Component {
@action
onAuthResponse(authResponse, backend, data) {
const { mfa_requirement } = authResponse;
// if an mfa requirement exists further action is required
/*
Checking for an mfa_requirement happens in two places.
If doSubmit in <AuthForm> is called directly (by the <form> component) mfa is just handled here.
Login methods submitted using a child form component of <AuthForm> are first checked for mfa
in the Auth::LoginForm "authenticate" task, and then that data eventually bubbles up here.
*/
if (mfa_requirement) {
// if an mfa requirement exists further action is required
this.mfaAuthData = { mfa_requirement, backend, data };
} else {
// calls authSuccess in auth.js controller
this.args.onAuthSuccess(authResponse);
}
}

@action
onMfaSuccess(authResponse) {
// calls authSuccess in auth.js controller
this.args.onAuthSuccess(authResponse);
}

Expand Down
1 change: 1 addition & 0 deletions ui/app/components/mfa/mfa-form.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ export default class MfaForm extends Component {
clusterId: this.args.clusterId,
...this.args.authData,
});
// calls onMfaSuccess in auth/page.js
this.args.onSuccess(response);
} catch (error) {
const errors = error.errors || [];
Expand Down
49 changes: 38 additions & 11 deletions ui/app/services/auth.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ export default Service.extend({
permissions: service(),
currentCluster: service(),
router: service(),
store: service(),
namespaceService: service('namespace'),

IDLE_TIMEOUT: 3 * 60e3,
Expand Down Expand Up @@ -210,6 +211,12 @@ export default Service.extend({
return this.ajax(url, 'POST', { namespace });
},

async lookupSelf(token) {
return this.store
.adapterFor('application')
.ajax('/v1/auth/token/lookup-self', 'GET', { headers: { 'X-Vault-Token': token } });
},

revokeCurrentToken() {
const namespace = this.authData.userRootNamespace;
const url = '/v1/auth/token/revoke-self';
Expand Down Expand Up @@ -255,7 +262,7 @@ export default Service.extend({
return userRootNamespace;
},

persistAuthData() {
async persistAuthData() {
const [firstArg, resp] = arguments;
const currentNamespace = this.namespaceService.path || '';
// dropdown vs tab format
Expand All @@ -275,18 +282,12 @@ export default Service.extend({
mountPath,
...BACKENDS.find((b) => b.type === backend),
};
let displayName;
if (isArray(currentBackend.displayNamePath)) {
displayName = currentBackend.displayNamePath.map((name) => get(resp, name)).join('/');
} else {
displayName = get(resp, currentBackend.displayNamePath);
}

const { entity_id, policies, renewable, namespace_path } = resp;
const userRootNamespace = this.calculateRootNamespace(currentNamespace, namespace_path, backend);
const data = {
userRootNamespace,
displayName,
displayName: null, // set below
backend: currentBackend,
token: resp.client_token || get(resp, currentBackend.tokenPath),
policies,
Expand All @@ -311,9 +312,7 @@ export default Service.extend({
// this is intentionally not included in setExpirationSettings so we can unit test that method
if (Ember.testing) this.set('allowExpiration', false);

if (!data.displayName) {
data.displayName = (this.getTokenData(tokenName) || {}).displayName;
}
data.displayName = await this.setDisplayName(resp, currentBackend.displayNamePath, tokenName);

this.set('tokens', addToArray(this.tokens, tokenName));
this.setTokenData(tokenName, data);
Expand All @@ -325,6 +324,34 @@ export default Service.extend({
});
},

async setDisplayName(resp, displayNamePath, tokenName) {
let displayName;

// first check if auth response includes a display name
displayName = isArray(displayNamePath)
? displayNamePath.map((name) => get(resp, name)).join('/')
: get(resp, displayNamePath);

// if not, check stored token data
if (!displayName) {
displayName = (this.getTokenData(tokenName) || {}).displayName;
}

// this is a workaround for OIDC/SAML methods WITH mfa configured. at this time mfa/validate endpoint does not
// return display_name (or metadata that includes it) for this auth combination.
// this if block can be removed if/when the API returns display_name on the mfa/validate response.
if (!displayName) {
// if still nothing, request token data as a last resort
try {
const { data } = await this.lookupSelf(resp.client_token);
displayName = data.display_name;
} catch {
// silently fail since we're just trying to set a display name
}
}
return displayName;
},

setTokenData(token, data) {
this.storage(token).setItem(token, data);
},
Expand Down
17 changes: 17 additions & 0 deletions ui/tests/acceptance/oidc-auth-method-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { fakeWindow, buildMessage } from '../helpers/oidc-window-stub';
import sinon from 'sinon';
import { later, _cancelTimers as cancelTimers } from '@ember/runloop';
import { Response } from 'miragejs';
import { setupTotpMfaResponse } from 'vault/tests/helpers/auth/mfa-helpers';

module('Acceptance | oidc auth method', function (hooks) {
setupApplicationTest(hooks);
Expand Down Expand Up @@ -155,4 +156,20 @@ module('Acceptance | oidc auth method', function (hooks) {
await click('[data-test-auth-submit]');
assert.dom('[data-test-message-error-description]').hasText('Error fetching role: permission denied');
});

test('it prompts mfa if configured', async function (assert) {
assert.expect(1);

this.setupMocks(assert);
this.server.get('/auth/foo/oidc/callback', () => setupTotpMfaResponse('foo'));
await this.selectMethod('oidc');
later(() => {
window.postMessage(buildMessage().data, window.origin);
cancelTimers();
}, 50);

await click('[data-test-auth-submit]');
await waitUntil(() => find('[data-test-mfa-form]'));
assert.dom('[data-test-mfa-form]').exists('it renders TOTP MFA form');
});
});
12 changes: 12 additions & 0 deletions ui/tests/acceptance/saml-auth-method-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { setupMirage } from 'ember-cli-mirage/test-support';
import { Response } from 'miragejs';
import authPage from 'vault/tests/pages/auth';
import { fakeWindow } from 'vault/tests/helpers/oidc-window-stub';
import { setupTotpMfaResponse } from 'vault/tests/helpers/auth/mfa-helpers';

module('Acceptance | enterprise saml auth method', function (hooks) {
setupApplicationTest(hooks);
Expand Down Expand Up @@ -158,4 +159,15 @@ module('Acceptance | enterprise saml auth method', function (hooks) {
.dom('[data-test-select="auth-method"]')
.hasValue('saml', 'Previous auth method selected on logout');
});

test('it prompts mfa if configured', async function (assert) {
assert.expect(1);
this.server.put('/auth/saml/token', () => setupTotpMfaResponse('saml'));

await waitUntil(() => find('[data-test-select="auth-method"]'));
await fillIn('[data-test-select="auth-method"]', 'saml');
await click('[data-test-auth-submit]');
await waitUntil(() => find('[data-test-mfa-form]'));
assert.dom('[data-test-mfa-form]').exists('it renders TOTP MFA form');
});
});
24 changes: 24 additions & 0 deletions ui/tests/helpers/auth/mfa-helpers.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/**
* Copyright (c) HashiCorp, Inc.
* SPDX-License-Identifier: BUSL-1.1
*/

export const setupTotpMfaResponse = (mountPath) => ({
auth: {
mfa_requirement: {
mfa_request_id: '0edf0945-da02-1300-9a0a-cb052cd94eb4',
mfa_constraints: {
[mountPath]: {
any: [
{
type: 'totp',
id: '7028db82-7de3-01d7-26b5-84b147c80966',
uses_passcode: true,
},
],
},
},
},
num_uses: 0,
},
});
8 changes: 7 additions & 1 deletion ui/tests/integration/components/auth-jwt-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,13 @@ module('Integration | Component | auth jwt', function (hooks) {
});
this.window.trigger('message', buildMessage());
await settled();
assert.ok(this.handler.withArgs(null, null, 'token').calledOnce, 'calls the onSubmit handler with token');
const [callbackData, , token] = this.handler.lastCall.args;
assert.propEqual(
callbackData,
{ mfa_requirement: undefined },
'mfa_requirement is undefined if not returned by response'
);
assert.strictEqual(token, 'token', 'calls the onSubmit handler with token');
});

test('oidc: fails silently when event origin does not match window origin', async function (assert) {
Expand Down
Loading