[sso] OIDC Updates for the UI #15804
Conversation
Ember Asset Size actionAs of edea38f Files that got Bigger 🚨:
Files that stayed the same size 🤷:
|
philrenaud
force-pushed
the
ui-oidc-updates
branch
from
0298738 to
6f136c0
Compare
philrenaud
force-pushed
the
ui-oidc-updates
branch
from
6f136c0 to
ff1c4a1
Compare
| } else { | ||
| redirectURL = new URL(window.location.toString()); | ||
| redirectURL.search = ''; | ||
| redirectURL = redirectURL.href; |
There was a problem hiding this comment.
nit: This line nullifies the work you've just done on line 97 and 98, no?
There was a problem hiding this comment.
This isn't pure (I'm reusing the variable name), but it isn't nullifying the things above it. redirectURL.href is modified by setting its search prop to "", for example.
There was a problem hiding this comment.
What's hard for me to grok here is that, I'm reading this as:
variable `redirectURL` is assigned to a new object in memory using URL constructor
`redirectURL.search` is assigned a value of empty string
`redirectURL` is assigned to the value of `redirectURL.href` -- this would override what just happened in the previous step, no?
There was a problem hiding this comment.
If the URL constructor generated a POJO or something that would be true, but it differs in a few ways.
Consider if I was running this from a hypothetical site.com/page?foo=bar
redirectURL = { href:"site.com/page?foo=bar", search="foo=bar", ... }
redirectURL.search = "" // since URL.href is dependent upon search and other props, it is modified to become "site.com/page"
redirectURL = "site.com/page"
There was a problem hiding this comment.
Aha! Couldn't we have just take redirectURL.host or redirectURL.origin instead? But either way, that's clear now.
| }), | ||
| } | ||
| ); | ||
|
|
||
| if (res.ok) { | ||
| const data = await res.json(); | ||
| this.token.set('secret', data.ACLToken); | ||
| this.verifyToken(); | ||
| this.clearTokenProperties(); |
There was a problem hiding this comment.
nit: You're unloading the entire store here which is going to dispatch a large number of transactions. Maybe we should coordinate that this action is complete before moving to the next line of code via the await keyword.
There was a problem hiding this comment.
Noting that this is an existing established pattern from verifyToken() (sign in without SSO); the events being being triggered are not asynchronous (store.unloadAll() might be?) but all subsequent actions here are healthy ones to continue down regardless of what's happening int he background: we're clearing out state/code (meant for a single immediate transaction, not meant for persistence) and moving forward with a logged in state.
| this.state = null; | ||
| this.code = null; | ||
|
|
||
| this.resetStore(); |
There was a problem hiding this comment.
So we're unloading the store on line 155, then editing a token (from an empty store), and then unloading the store, again here. And then we fetch the token on line 163.
Maybe we might be doing too much work here. There's a small chance that once the store has a large number of items that we'll run into challenges with asynchronocity.
Should we get an integration test on this?
There was a problem hiding this comment.
You may be correct here! This looks like it could be a redundant unload of the store.
FWIW, we do run this in a regular acceptance test here with a non-trivial amount of store load.
There was a problem hiding this comment.
Interesting! There's a forEach being called when you inspect InstanceCache which is called on unloadAll. But, I'm not complaining! Less Work!!!
There was a problem hiding this comment.
My gut says there might be a lurking bug in here with how we're clearing out the store in the validateSSO logic. But I'm imagining you've already done a fair amount of smoke testing here anyhow. Did we test what happens on a slow network or having a large amount of items in the store?
This changes some properties to match expected endpoint props (AuthMethod -> AuthMethodName), and adds redirect URIs to requests.
Additionally, makes a change to the
list auth methodsAPI endpoint to not check for authentication (as we want to provide auth methods for users who are signed out, have invalid or expired tokens, etc.)