Skip to content

Commit 72f7c8a

Browse files
fix: πŸ› OIDC authentications not properly showing username (#2930)
* fix: πŸ› properly show username for oidc auth method in user menu * test: πŸ’ fix admin ui tests * test: πŸ’ fix desktop ui tests * refactor: πŸ’‘ admin to not store username in authenticated object * refactor: πŸ’‘ use getter in app controller * test: πŸ’ POC for how we could update our tests to use auth account * refactor: πŸ’‘ remove unused service * test: πŸ’ fix all tests in ui/admin * test: πŸ’ revert one change * refactor: πŸ’‘ update ui/desktop w/ feedback + fix tests * refactor: πŸ’‘ add in error handling * refactor: πŸ’‘ cleanup unused service * test: πŸ’ fix e2e test selector βœ… Closes: https://hashicorp.atlassian.net/browse/ICU-17231
1 parent 838bc37 commit 72f7c8a

File tree

195 files changed

+722
-1114
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

195 files changed

+722
-1114
lines changed

β€Žaddons/api/mirage/factories/scope.jsβ€Ž

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import { trait } from 'miragejs';
88
import permissions from '../helpers/permissions';
99
import generateId from '../helpers/id';
1010
import { faker } from '@faker-js/faker';
11+
import { TYPE_AUTH_METHOD_PASSWORD } from 'api/models/auth-method';
1112

1213
export default factory.extend({
1314
type: 'global',
@@ -108,4 +109,21 @@ export default factory.extend({
108109
}
109110
},
110111
}),
112+
113+
withGlobalAuth: trait({
114+
afterCreate(record, server) {
115+
if (record.type === 'global') {
116+
const authMethod = server.create('auth-method', {
117+
type: TYPE_AUTH_METHOD_PASSWORD,
118+
scope: record,
119+
});
120+
server.create('account', {
121+
type: TYPE_AUTH_METHOD_PASSWORD,
122+
full_name: 'admin',
123+
scope: record,
124+
authMethod,
125+
});
126+
}
127+
},
128+
}),
111129
});

β€Žaddons/api/mirage/route-handlers/auth.jsβ€Ž

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ const oidcRequiredAttempts = 3;
1313

1414
const commandHandlers = {
1515
password: {
16-
login: (payload, scopeAttrs) => {
16+
login: (payload, auth_method_id, account_id, scopeAttrs) => {
1717
if (payload.attributes.login_name === 'error') {
1818
return new Response(400);
1919
} else {
@@ -25,9 +25,9 @@ const commandHandlers = {
2525
scope: scopeAttrs,
2626
id: 'token123',
2727
token: 'thetokenstring',
28-
account_id: 'authenticatedaccount',
28+
account_id: account_id || 'authenticatedaccount',
2929
user_id: 'authenticateduser',
30-
auth_method_id: 'authmethod123',
30+
auth_method_id: auth_method_id || 'authmethod123',
3131
created_time: '',
3232
updated_time: '',
3333
last_used_time: '',
@@ -61,7 +61,7 @@ const commandHandlers = {
6161
},
6262
},
6363
),
64-
token: (_, scopeAttrs) => {
64+
token: (_, auth_method_id, account_id, scopeAttrs) => {
6565
oidcAttemptCounter++;
6666
if (oidcAttemptCounter < oidcRequiredAttempts) {
6767
return new Response(202);
@@ -74,9 +74,9 @@ const commandHandlers = {
7474
scope: scopeAttrs,
7575
id: 'token123',
7676
token: 'thetokenstring',
77-
account_id: '1',
77+
account_id: account_id || '1',
7878
user_id: 'authenticateduser',
79-
auth_method_id: 'authmethod123',
79+
auth_method_id: auth_method_id || 'authmethod123',
8080
created_time: '',
8181
updated_time: '',
8282
last_used_time: '',
@@ -90,7 +90,7 @@ const commandHandlers = {
9090
};
9191

9292
// Handles all custom methods on /auth-methods/:id route
93-
export function authHandler({ scopes, authMethods }, request) {
93+
export function authHandler({ scopes, authMethods, accounts }, request) {
9494
const [, id, method] = request.params.id_method.match(
9595
/(?<id>.[^:]*):(?<method>(.*))/,
9696
);
@@ -100,8 +100,14 @@ export function authHandler({ scopes, authMethods }, request) {
100100
const payload = JSON.parse(request.requestBody);
101101
const { command } = payload;
102102
const scope = scopes.find(authMethod.scopeId);
103+
const account = accounts.where({ authMethodId: authMethod.id })?.models[0];
103104
const scopeAttrs = this.serialize(scopes.find(scope.id));
104-
return commandHandlers[authMethod.type][command](payload, scopeAttrs);
105+
return commandHandlers[authMethod.type][command](
106+
payload,
107+
authMethod.id,
108+
account?.id,
109+
scopeAttrs,
110+
);
105111
}
106112

107113
// TODO: this handler doesn't really belong here, but we already route

β€Žaddons/auth/addon/authenticators/base.jsβ€Ž

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -101,15 +101,14 @@ export default class BaseAuthenticator extends SimpleAuthBaseAuthenticator {
101101
* @param {string} username
102102
* @return {object}
103103
*/
104-
normalizeData(data, username) {
104+
normalizeData(data) {
105105
// Pull fields up from `data.attributes` for easier access in JavaScript.
106106
// The `attributes` field exists on the Go side for its convenience but is
107107
// unnecessary here.
108108
Object.assign(data, data.attributes);
109109
// Add booleans indicated the scope type
110110
data.isGlobal = data?.scope?.type === 'global';
111111
data.isOrg = data?.scope?.type === 'org';
112-
if (username) data.username = username;
113112
return data;
114113
}
115114

β€Žaddons/auth/addon/authenticators/password.jsβ€Ž

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -72,8 +72,6 @@ export default class PasswordAuthenticator extends BaseAuthenticator {
7272
fetch(authEndpointURL, { method: 'post', body }),
7373
);
7474
const json = await response.json();
75-
return response.status < 400
76-
? resolve(this.normalizeData(json, login_name))
77-
: reject();
75+
return response.status < 400 ? resolve(this.normalizeData(json)) : reject();
7876
}
7977
}

β€Žaddons/core/translations/en-us.yamlβ€Ž

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ titles:
4949
project-actions: Project Actions
5050
user-menu: User Menu
5151
back-link: 'Back to {scope}'
52+
authenticated: Signed in as
5253
descriptions:
5354
empty-set: There are no items to display yet. You may be able to add items or try back later.
5455
cluster-url-initialization: To get started, please enter your cluster URL

β€Že2e-tests/admin/tests/auth-method-oidc-vault.spec.jsβ€Ž

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -133,9 +133,7 @@ test(
133133
).toBeVisible();
134134

135135
// Log back in as an admin
136-
await page.getByRole('button', { name: 'User Menu' }).click();
137-
await page.getByRole('button', { name: 'Sign Out' }).click();
138-
await expect(page.getByRole('button', { name: 'Sign In' })).toBeVisible();
136+
await loginPage.logout(email);
139137
await loginPage.login(adminLoginName, adminPassword);
140138
await expect(
141139
page.getByRole('navigation', { name: 'breadcrumbs' }).getByText('Orgs'),

β€Žui/admin/app/routes/application.jsβ€Ž

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,8 @@ export default class ApplicationRoute extends Route {
7575
if (userId && hostUrl) {
7676
await this.sqlite.setup(formatDbName(userId, hostUrl));
7777
}
78+
79+
await this.session.loadAuthenticatedAccount();
7880
}
7981
}
8082

β€Žui/admin/app/services/session.jsβ€Ž

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,15 @@
55

66
import BaseSessionService from 'ember-simple-auth/services/session';
77
import { service } from '@ember/service';
8+
import { tracked } from '@glimmer/tracking';
89
import { formatDbName } from 'api/services/sqlite';
910

1011
export default class SessionService extends BaseSessionService {
1112
@service sqlite;
1213
@service('browser/window') window;
14+
@service store;
15+
16+
@tracked username;
1317

1418
/**
1519
* Extend ember simple auth's handleAuthentication method
@@ -22,8 +26,31 @@ export default class SessionService extends BaseSessionService {
2226
await this.sqlite.setup(formatDbName(userId, hostUrl));
2327
}
2428

29+
await this.loadAuthenticatedAccount();
30+
2531
// We let ember-simple-auth handle transitioning back to the index after authentication.
2632
// This route can be configured in our environment config.
2733
super.handleAuthentication(...arguments);
2834
}
35+
36+
/**
37+
* Loads account used to authenticate so that it can be used to display
38+
* the authenticated username.
39+
*/
40+
async loadAuthenticatedAccount() {
41+
if (this.data?.authenticated?.account_id) {
42+
try {
43+
const account = await this.store.findRecord(
44+
'account',
45+
this.data.authenticated.account_id,
46+
);
47+
this.username = account.accountName;
48+
} catch (e) {
49+
// We are purposefully ignoring errors since loading the
50+
// authenticated account should not block a user because
51+
// account information is for populating a username.
52+
console.error(e);
53+
}
54+
}
55+
}
2956
}

β€Žui/admin/app/styles/app.scssβ€Ž

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1025,3 +1025,18 @@
10251025
.injected-app-credential-alert {
10261026
margin-bottom: 1rem;
10271027
}
1028+
1029+
// User menu dropdown in global side nav
1030+
.user-menu-dropdown {
1031+
max-width: 125px;
1032+
1033+
&__toggle-button {
1034+
width: 100%;
1035+
1036+
.hds-dropdown-toggle-button__text {
1037+
white-space: nowrap;
1038+
overflow: hidden;
1039+
text-overflow: ellipsis;
1040+
}
1041+
}
1042+
}

β€Žui/admin/app/templates/application.hbsβ€Ž

Lines changed: 63 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -71,41 +71,73 @@
7171
</Hds::Dropdown>
7272
{{/if}}
7373

74-
<Hds::Dropdown
75-
@enableCollisionDetection={{true}}
76-
data-test-side-nav-user-dropdown
77-
as |dd|
78-
>
79-
{{#if this.session.data.authenticated.username}}
74+
{{#if this.session.username}}
75+
<Hds::Dropdown
76+
@enableCollisionDetection={{true}}
77+
class='user-menu-dropdown'
78+
data-test-side-nav-user-dropdown
79+
as |dd|
80+
>
8081
<dd.ToggleButton
8182
@icon='user'
82-
@text={{this.session.data.authenticated.username}}
83+
@text={{this.session.username}}
84+
{{hds-tooltip this.session.username}}
85+
class='user-menu-dropdown__toggle-button'
8386
/>
84-
{{else}}
87+
<dd.Title @text={{t 'titles.authenticated'}} />
88+
<dd.Description @text={{this.session.username}} />
89+
<dd.Separator />
90+
91+
<dd.Interactive @route='account.change-password'>{{t
92+
'actions.change-password'
93+
}}</dd.Interactive>
94+
<dd.Separator />
95+
96+
<dd.Interactive {{on 'click' this.invalidateSession}}>{{t
97+
'actions.deauthenticate'
98+
}}</dd.Interactive>
99+
<dd.Separator />
100+
101+
<dd.Title @text={{t 'actions.toggle-color-theme'}} />
102+
{{#each this.themes as |theme|}}
103+
<dd.Radio
104+
@value={{theme.value}}
105+
checked={{eq this.session.data.theme theme.value}}
106+
{{on 'change' (fn this.toggleTheme theme.value)}}
107+
>
108+
{{t (concat 'themes.' theme.label)}}
109+
</dd.Radio>
110+
{{/each}}
111+
</Hds::Dropdown>
112+
{{else}}
113+
<Hds::Dropdown
114+
@enableCollisionDetection={{true}}
115+
data-test-side-nav-user-dropdown
116+
as |dd|
117+
>
85118
<dd.ToggleIcon @icon='user' @text={{t 'titles.user-menu'}} />
86-
{{/if}}
87-
88-
<dd.Interactive @route='account.change-password'>{{t
89-
'actions.change-password'
90-
}}</dd.Interactive>
91-
<dd.Separator />
92-
93-
<dd.Interactive {{on 'click' this.invalidateSession}}>{{t
94-
'actions.deauthenticate'
95-
}}</dd.Interactive>
96-
<dd.Separator />
97-
98-
<dd.Title @text={{t 'actions.toggle-color-theme'}} />
99-
{{#each this.themes as |theme|}}
100-
<dd.Radio
101-
@value={{theme.value}}
102-
checked={{eq this.session.data.theme theme.value}}
103-
{{on 'change' (fn this.toggleTheme theme.value)}}
104-
>
105-
{{t (concat 'themes.' theme.label)}}
106-
</dd.Radio>
107-
{{/each}}
108-
</Hds::Dropdown>
119+
<dd.Interactive @route='account.change-password'>{{t
120+
'actions.change-password'
121+
}}</dd.Interactive>
122+
<dd.Separator />
123+
124+
<dd.Interactive {{on 'click' this.invalidateSession}}>{{t
125+
'actions.deauthenticate'
126+
}}</dd.Interactive>
127+
<dd.Separator />
128+
129+
<dd.Title @text={{t 'actions.toggle-color-theme'}} />
130+
{{#each this.themes as |theme|}}
131+
<dd.Radio
132+
@value={{theme.value}}
133+
checked={{eq this.session.data.theme theme.value}}
134+
{{on 'change' (fn this.toggleTheme theme.value)}}
135+
>
136+
{{t (concat 'themes.' theme.label)}}
137+
</dd.Radio>
138+
{{/each}}
139+
</Hds::Dropdown>
140+
{{/if}}
109141
</:actions>
110142
</Hds::SideNav::Header>
111143
</:header>

0 commit comments

Comments
Β (0)