-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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: Ensure the partition is passed through to the request for the SSO auth URL #11979
Conversation
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.
Notes:
@@ -154,7 +154,7 @@ as |TabState IgnoredGuard IgnoredAction tabDispatch tabState|> | |||
@nspace={{or this.value.Namespace @nspace}} | |||
@partition={{or this.value.Partition @partition}} | |||
@type={{if this.value.Name 'oidc' 'secret'}} | |||
@value={{if this.value.Name this.value.Name this.value}} |
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.
If its an OIDC flow then pass the entire ODIC object through instead of just the name (the object contains the partition name received from the backend, so we are sure thats correct)
use the Consul or SSO depending on the shape of the `@value` argument. All in | ||
all this means we can remove the `@type` argument making a slimmer component | ||
API. | ||
|
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.
Just noting an improvement I'd like to do thats not important here so I'd like to not do it in this PR, but at some point in the probably distant future.
partition=@partition | ||
nspace=@nspace | ||
partition=(or @value.Partition @partition) | ||
nspace=(or @value.Namespace @nspace) |
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.
As mentioned above, now we pass the full OIDC provider object through we can access the Partition (and probably best to do the same with Namespace here so I added that also)
@@ -30,7 +30,7 @@ as |State Guard Action dispatch state|> | |||
<DataSource | |||
@src={{uri (concat path '/oidc/provider/${value}') | |||
(hash | |||
value=@value | |||
value=@value.Name |
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.
Again, now we are passing the object through, access the Name/ID here for he HTTP request for the AuthURL
@@ -22,6 +22,7 @@ export default class OidcSerializer extends Serializer { | |||
cb(headers, { | |||
Name: query.id, | |||
Namespace: query.ns, | |||
Partition: query.partition, |
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.
This is just to keep ember-data happy with unique IDs, very unlikely to ever get a clash, but this is more correct.
@@ -43,6 +43,9 @@ export default function(type, value, doc = document) { | |||
case 'authMethod': | |||
key = 'CONSUL_AUTH_METHOD_COUNT'; | |||
break; | |||
case 'oidcProvider': | |||
key = 'CONSUL_OIDC_PROVIDER_COUNT'; | |||
break; |
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.
This allows us to mock numbers of OIDC providers during testing
@@ -40,6 +40,9 @@ export default function(type) { | |||
case 'authMethod': | |||
requests = ['/v1/acl/auth-methods', '/v1/acl/auth-method/']; | |||
break; | |||
case 'oidcProvider': | |||
requests = ['/v1/internal/ui/oidc-auth-methods']; |
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.
This allows us to replace mock values for OIDC providers during testing
provider.popup.open = async function() { | ||
return response; | ||
}; | ||
}; |
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.
This bit I thought would be more difficult than it actually was, I remember I'd half done this ages ago but it never went in for some reason. It allows us to mock an OIDC provider with no popup.
I first tried with popups based on work a previous PR (#11248) which was supposed to be the road to be able to acceptance test SSO as e2e as possible, but the popup blocker gets in the way and I'm not even sure you can then interact with the popups content with Ember anyway. So I was left with this being the closest place I could mock things.
@@ -12,6 +12,9 @@ export default function(scenario, respondWith, set) { | |||
} | |||
respondWith(url, data); | |||
}) | |||
.given(['the "$provider" oidcProvider responds with from yaml\n$yaml'], function(name, data) { | |||
oidc(name, data); | |||
}) |
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.
Wiring up the oidcProvider mocking function above with the step in the feature tests.
.given(['partitions are enabled'], function() { | ||
doc.cookie = `CONSUL_PARTITIONS_ENABLE=1`; | ||
set('CONSUL_PARTITIONS_ENABLE', 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.
Allows us to enable both SSO and partitions during testing.
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.
👍
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/547306. |
🍒✅ Cherry pick of commit 78e9c0d onto |
… auth URL (#11979) * Make sure the mocks reflect the requested partition/namespace * Ensure partition is passed through to the HTTP adapter * Pass AuthMethod object through to TokenSource in order to use Partition * Change up docs and add potential improvements for future * Pass the query partition back onto the response * Make sure the OIDC callback mock returns a Partition * Enable OIDC provider mock overwriting during acceptance testing * Make sure we can enable partitions and SSO post bootup only required ...for now * Wire up oidc provider mocking * Add SSO full auth flow acceptance tests
There were a couple of places we'd missed adding the
partition
word to make sure that the partition is passed through to certain HTTP requests. This PR fixes those up.Most of the code here is centered around testing this out in as close to e2e as possible, but I also took it for a spin with a Google OIDC provider and manually tested during our development environment with our fake OIDC provider. In all tests the correct partition is being appended to the HTTP API query params as expected.