-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 - jwt auth #6188
UI - jwt auth #6188
Conversation
@@ -0,0 +1,9 @@ | |||
<div class="is-flex-v-centered auth-button--auth0" > | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✂️
top: -6px; | ||
left: -0.75rem; | ||
} | ||
.auth-button--auth0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we not want to combine .auth-button--gitlab
and .auth-button--auth0
since they have the same styles?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yep, I think they didn't start out that way which is why they were separate - I'll combine them.
@@ -0,0 +1,6 @@ | |||
<div class="is-flex-v-centered auth-button--gitlab" > | |||
<svg class="auth-button-tile" id="logo_art" data-name="logo art" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 586 559"><defs><style>.cls-1{fill:#fc6d26;}.cls-2{fill:#e24329;}.cls-3{fill:#fca326;}</style></defs><title>gitlab-icon-rgb</title><g id="g44"><path id="path46" class="cls-1" d="M461.17,301.83l-18.91-58.12L404.84,128.43a6.47,6.47,0,0,0-12.27,0L355.15,243.64H230.82L193.4,128.43a6.46,6.46,0,0,0-12.26,0L143.78,243.64l-18.91,58.19a12.88,12.88,0,0,0,4.66,14.39L293,435,456.44,316.22a12.9,12.9,0,0,0,4.73-14.39"/></g><g id="g48"><path id="path50" class="cls-2" d="M293,434.91h0l62.16-191.28H230.87L293,434.91Z"/></g><g id="g56"><path id="path58" class="cls-1" d="M293,434.91,230.82,243.63h-87L293,434.91Z"/></g><g id="g64"><path id="path66" class="cls-3" d="M143.75,243.69h0l-18.91,58.12a12.88,12.88,0,0,0,4.66,14.39L293,435,143.75,243.69Z"/></g><g id="g72"><path id="path74" class="cls-2" d="M143.78,243.69h87.11L193.4,128.49a6.47,6.47,0,0,0-12.27,0l-37.35,115.2Z"/></g><g id="g76"><path id="path78" class="cls-1" d="M293,434.91l62.16-191.28H442.3L293,434.91Z"/></g><g id="g80"><path id="path82" class="cls-3" d="M442.24,243.69h0l18.91,58.12a12.85,12.85,0,0,1-4.66,14.39L293,434.91l149.2-191.22Z"/></g><g id="g84"><path id="path86" class="cls-2" d="M442.28,243.69h-87.1l37.42-115.2a6.46,6.46,0,0,1,12.26,0l37.42,115.2Z"/></g></svg> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to keep these id
and data-name
properties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh ha, that was from the original SVG, will remove them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Matthew!
I left a few comments on initial stuff I noticed whilst giving it an initial scan. I think my other thing I'd mention is something like an option of a auth-button type="google"
component instead of a component per provider. No biggie again though, just same thing really just different approach.
I'm guessing this is relatively more difficult to setup to play with than usual? I did see you demoing this on last weeks recording, looked really nice!
Ah the other thing I was going to ask was, on the sign in page, you keep the input Role field visible, do you ever need that? I know google at least will give you its own UI if you have no accounts associated, maybe others don't do this?
John
'The provider window was closed before authentication was complete. Please click Sign In to try again.'; | ||
const ERROR_MISSING_PARAMS = | ||
'The callback from the provider did not supply all of the required parameters. Please click Sign In to try again. If the problem persists, you may want to contact your administrator.'; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way to get this copy into the template?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, not without another level of indirection or two and then it would still need to pass a string which feels like it wouldn't gain anything over this. Currently the error message component takes an error or a model and will display when that value isn't falsey (and if isError is true in the case that it's a model). Most of the time the error comes from the server so it's dynamic. The other tricky part here is that the component itself doesn't render the error, it passes the error into an action and the parent component (auth-form.js
) renders the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aaah cool np, don't worry then, I didn't look into it properly, so I had no idea what other impact that would have - I think this just comes from me trying to get all my copy into templates over in Consul-land.
ui/app/components/auth-jwt.js
Outdated
if (oldSelectedAuthPath !== selectedAuthPath) { | ||
this.set('role', null); | ||
this.onRoleName(null); | ||
this.fetchRole.perform(null, { nodebounce: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hehe, I just read this as 'Node Bounce', I was like, that sounds like fun whats that?! 🤦♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol oh no, I'll add some casing in there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is nodedebounce
a custom property then? I assumed it was something you couldn't touch. Ah just seen where you are using it.
How about {debounce: false}
? Maybe another personal thing, but I generally try to avoid negative boolean property/method names so you don't get double negatives.
// those that are JWT type will 400 when trying to fetch the role | ||
isOIDC: computed('role', function() { | ||
return this.role && this.role.authUrl; | ||
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this computed properties first argument be role.authUrl
? Not sure if you have instances where things on role change but the role doesn't? I might be misunderstanding computed properties, so partly for my info also here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably also add in role.authUrl
for correctness, but the dependent keys tell ember when to dirty the cache so that the value will be recomputed on the next get
. In practice, we just want to check the authUrl when the role changes, which is why I left it out, but I'll add it in because authUrl
is part of the returned value.
}), | ||
|
||
getWindow() { | ||
return this.window || window; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 @ configurable window
, nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heh, though mocking it properly in testing was rough (but works!)
ui/app/components/auth-jwt.js
Outdated
let win = this.getWindow(); | ||
|
||
let left = win.screen.width / 2 - 250; | ||
let top = win.screen.height / 2 - 300; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe comment in what these values are or CONSTANT them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah just seen its half the popup width/height, maybe hoist the popup width/height out of the string below (but probably keep it in this method), and calculate these from that? No biggie, splitting hairs really.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yep, good idea!
} | ||
[class*='auth-button--'] .text { | ||
padding-left: $spacing-m; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this BEM looking style used elsewhere in the app?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, mostly no (though I'd like us to) - better to not muddy the waters, I'll change the prefix up.
// debounce | ||
yield timeout(WAIT_TIME); | ||
} | ||
let path = this.selectedAuthPath || 'jwt'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how does oidc
alias affect the default here?
…communication because of IE11
…new auth jwt tests
Hey @johncowen thanks for the review! I've addressed all of your inline comments (sorry a rebase means comments don't line up well any longer :( ) - will answer the other review stuff here:
These are mostly the brand SVG which is a bit gnarly so I think it's nice to have in a separate file, and this way there's also no need for a corresponding JS file (though with type attr, I don't think you'd have to either, but it would be nice to document the attributes).
I think now that this is rebased on the beta branch, it may include the relevant backend updates, but I haven't checked.
So when you configure the JWT/OIDC auth method, you can configure a default role, and you can have multiple roles per provider (each with their own claims). In the case that you don't have a default role or have multiple roles configured, we need to show the role input which is why it's always there. We tried to be more clever about if we had to show it or if we could put it in a "more options" drop down, but it made things more confusing. |
this.onError(err); | ||
}, | ||
|
||
prepareForOIDC: task(function*(oidcWindow) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, the use of ember concurrency tasks here is really slick and easy to follow. nice! 💅
|
||
actions: { | ||
async startOIDCAuth(data, e) { | ||
this.onError(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does calling onError
here do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onError
is an action passed from the outside here: https://github.com/hashicorp/vault/pull/6188/files/7456c6c0bf7f46f65d2fdcc47b1b3765ffc99d53#diff-5f637ddcdf2b0444777d3addb777746cR59 - the mut this.error
is a shorthand syntax for setting that property so anytime this.onError
is called in auth-jwt, the argument we pass will be set as the value of error
on auth-form
. Here, passing null
just clears out any error when a user clicks the button to start the flow.
export default Base.extend({ | ||
model: alias('params.firstObject'), | ||
key: computed('params', function() { | ||
return this.get('params').objectAt(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i thought i remembered us talking in a previous PR moving away from using this.get
and instead accessing the property directly with this.params
. is that the case? i don't fully understand the tradeoffs between the two, so any context you have would be great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, yep - I copied this code from last June when I erroneously deleted it and that was before the ember upgrade - here's a link to the RFC that describes the change: https://github.com/emberjs/rfcs/blob/master/text/0281-es5-getters.md I'll update this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hurray! thanks for the link -- very helpful!
Thanks @meirish ! Some good background info there. BTW I noticed how you got around changing the BEM looking |
This adds the JWT auth method and the new OIDC capabilities as a supported authentication method for the UI.
Authenticating with Auth0 via the new OIDC method:
Additionally this changes the look of authentication screen in the UI so that you will see only the dropdown to select an auth method if you have not tuned any auth methods to display when you're authenticated.
note: this requires changes in https://github.com/hashicorp/vault-plugin-auth-jwt/tree/oidc-dev to fully function