fix: 🐛 OIDC authentications not properly showing username #2930
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
lisbet-alvarez
force-pushed
the
ICU-17231-bug-fix-oidc-authentications-not-properly-showing-username
branch
from
1eeece9 to
6e20c6c
Compare
…roperly-showing-username
There was a problem hiding this comment.
I think the fact that this all lives on the a specific user model means that we don't need to copy over and modify to the data.authenticated hash, we can probably derive it more easily from a getter that does a peekRecord, and rely on the certain points in the application flow to refresh the model as needed
ui/admin/app/routes/application.js
Outdated
| account.full_name; | ||
| this.session.set('data.authenticated.username', username); | ||
| } catch (e) { | ||
| // no op |
There was a problem hiding this comment.
Is there a specific error that could happen here that should be ignored? Catch-all error handlers can hide issues, I think generally it's better to let them bubble and have a global error handler that can log them and/or show a toast
There was a problem hiding this comment.
Not in particular. I was thinking that for whatever reason if there was an error retrieving the account, then it shouldn't cause ui to error out. But ya thats a good point (this will be refactored either way)
There was a problem hiding this comment.
I chatted about this with @lisbet-alvarez but I actually think this is a scenario where it might make sense to explicitly squelch this. We don't really have logging on admin UI and I'm not sure how useful a toast is. If it's a 403 we shouldn't display anything but even if it's a 5xx, what would a user even do with the info?
…wing-username' of https://github.com/hashicorp/boundary-ui into ICU-17231-bug-fix-oidc-authentications-not-properly-showing-username
ui/admin/tests/helpers/index.js
Outdated
| await authenticateSession(); | ||
| this.server.create('scope', { id: 'global' }, 'withGlobalAuth'); | ||
| const account_id = this.server.schema.accounts.first().id; | ||
| await authenticateSession({ account_id }); |
There was a problem hiding this comment.
Thoughts on refactoring our test cases to use this helper in order to provide an account_id to the authenticateSession? I only pushed up changes for tests/acceptance/accounts as a POC.
Edit: We discussed this during techtime and agreed to go with this approach.
| as |dd| | ||
| > | ||
| {{#if this.session.data.authenticated.username}} | ||
| {{#if this.session.username}} |
There was a problem hiding this comment.
Non blocking: was this if statement and having 2 dropdowns strictly necessary? Is it not possible to have the class and content be conditional based on this.session.username and use one dropdown?
There was a problem hiding this comment.
Great question! This second drop down is rendered with a ToggleIcon instead of a ToggleButton. We can't do the this.session.username check inside the dropdown since on a re-render the event listeners seem to not get properly re-attached causing the dropdown to not expand (unless page is fully refreshed).
Yup that is correct, the only tests that would fail without the changes would be tests validating the dropdown has correct username. |
4 participants
✅ Closes: https://hashicorp.atlassian.net/browse/ICU-17231
Description
Use account_id (retrieved from authenticated data) to query authenticated account in order to display correct username for any authentication method. As a result, OIDC authentications should show proper username.
Screenshots (if appropriate)
OIDC + Password Auth in Admin UI:
Screen.Recording.2025-09-24.at.4.25.39.PM.mov
OIDC + Password Auth in Desktop UI:
Screen.Recording.2025-09-24.at.4.23.31.PM.mov
How to Test
Checklist
PCI review checklist
Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.