UI/update auth form to fetchRoles after a namespace is inputted, prior to OIDC auth #19541

hellobontempo · 2023-03-14T21:37:56Z

So this was tricky to reproduce and due to the whack-a-mole nature of these bugs, here is a very thorough breakdown:

The auth form component updates as the namespace changes to show the correct associated login methods (and corresponding button styles, if applicable). This means the form often reverts to the dropdown menu because by default auth methods are not listed as tabs when unauthenticated (this is a separate config option)

Notice in the gif below, there is an oidc tab because in /Root oidc has been configured so listing_visibility='unauth' (docs). However, as another namespace is typed, the default form changes because testns has no unuath'd methods.

the bug 🐛

For both OIDC and JWT, the role is updated when the method is selected and the AuthJwt component fires off its didReceiveAttrs lifecycle hook which runs the task fetchRole with whatever namespace has been set by the url params.

If a user very quickly types in a namespace (admin) that has the same method (oidc), mounted at the same path (also oidc) but only one of them have a default role configured (stick with me!) then the UI remains on the oidc tab, but the task to fetch for a role is not re-fired with the new namespace value. This is because the fetchRole only fires if the auth path has changed. You can see in the gif when we go from root to the admin namespace, we get the Invalid role error because root does not have a default role. However, after refreshing, the namespace stays in the url params and thus the role is set properly.

fix 🔧

Because the component is often torn down when the namespace changes it doesn't make sense to check if the namespace has changed (like we do for the auth path by this.oldSelectedAuthPath !== this.selectedAuthPath).

Instead, opted to reorder the methods in startOIDCAuth and tested with the following situations to insure the bug fixes introduced by #17661 were preserved.

oidc method mounted at incorrect-uris-oidc/ with missing URI data
mounted at missing-role-oidc with no role configured
mounted at proper-config-oidc with a default role setup, properly

Monkeychip · 2023-03-15T15:18:05Z

Thanks for going after this. I know we're waiting for an AD FS environment, but a reminder to add the backport labels.

hellobontempo · 2023-03-15T18:15:05Z

Thanks for going after this. I know we're waiting for an AD FS environment, but a reminder to add the backport labels.

This is a different bug actually, so no need to wait for AD FS to reproduce. There aren't release milestone tags made for the next patch version yet - so holding off on adding PR tags until they've been created. Should have mentioned this!

zofskeez

🚀

zofskeez · 2023-03-16T22:44:44Z

ui/app/components/auth-jwt.js

+      try {
+        await this.fetchRole.perform(this.roleName, { debounce: false });
+      } catch (error) {
+        // this task could be cancelled if the instances in didReceiveAttrs resolve after this was started
+        if (error?.name !== 'TaskCancelation') {
+          throw error;
+        }
+      }


I can't see an issue with moving this above the conditional. If anything, since a role is required it should have come before in the first place to ensure we have the necessary data.

That's my understanding? I'm guessing it wasn't above before because firing it off in didReceiveAttrs seems like it'd be sufficient. Unfortunately the conditionals that wrap fetchRole in that function don't account for the namespace updating

zofskeez · 2023-03-16T22:45:51Z

ui/app/templates/components/auth-form.hbs

@@ -32,18 +32,16 @@
              </li>
            {{/let}}
          {{/each}}
-          {{#if this.hasMethodsWithPath}}


This condition is already wrapped higher up correct? It looks like removing it is the only change here.

Yeah - meant to comment. It looked like this was redundant? So removed to clean it up - but want to make sure I'm not misreading...

zofskeez · 2023-03-16T22:49:14Z

ui/tests/acceptance/enterprise-namespaces-test.js

@@ -88,4 +89,90 @@ module('Acceptance | Enterprise | namespaces', function (hooks) {
      'Does not prepend root to namespace'
    );
  });
+
+  module('auth form', function (hooks) {


I don't think I've seen modules nested in other modules before. Neat idea!

I copied @hashishaw 's idea from the pki tests, thought it was a neat way to organize. (And make the setup reusable if/when we add more namespace tests?) : https://github.com/hashicorp/vault/blob/main/ui/tests/acceptance/pki/pki-engine-workflow-test.js#L154

zofskeez · 2023-03-16T22:49:50Z

ui/tests/acceptance/enterprise-namespaces-test.js

hashishaw

A couple smol comments. Great test coverage and comments!

hashishaw · 2023-03-20T19:49:39Z

ui/tests/acceptance/enterprise-namespaces-test.js

+
+  module('auth form', function (hooks) {
+    setupMirage(hooks);
+    const SELECTORS = {


Usually these live outside of the module, not sure if it's necessary but this here caught me off guard

Yeah I didn't know what to do with the module in a module situation? Should I move them just outside this module? or outside the "parent" module?

hashishaw · 2023-03-20T19:53:21Z

ui/tests/acceptance/enterprise-namespaces-test.js

+      });
+      await visit('/vault/auth?with=oidc%2F');
+      assert.dom(SELECTORS.authTab(this.rootOidc)).exists('renders oidc method tab for root');
+      await authPage.namespaceInput(this.namespace);


Is it worth using the typeIn method here rather than the page object's fillable (which I think uses fillin under the hood)?

I actually used fillIn on purpose because that's what caused the bug in the first place

hashishaw

Played around with this locally too, I think this is a reasonable approach for the fix! But I hope the next time we touch this file it's to rewrite it 🙃 Thank you for the very thorough bug description/breakdown!

…r to OIDC auth (#19541) * re-fetch roles if there is a namespace * remove redundant conditional * reorder oidc auth operations * add test * test cleanup * add changelog

…is inputted, prior to OIDC auth #19541 (#19661) * UI/update auth form to fetchRoles after a namespace is inputted, prior to OIDC auth (#19541) * re-fetch roles if there is a namespace * remove redundant conditional * reorder oidc auth operations * add test * test cleanup * add changelog * UI: fix enterprise test failures (#19671) * move oidc tests into new file * remove module from namespace test * remove entered line * add logout to afterEach hook * remove ns test * move test setup to within test * use logout.visit() instead * updates oidc auth namespaces test * reverts to authPage logout --------- Co-authored-by: Jordan Reimer <zofskeez@gmail.com> --------- Co-authored-by: Jordan Reimer <zofskeez@gmail.com>

hellobontempo added 3 commits March 14, 2023 15:33

re-fetch roles if there is a namespace

5afc510

remove redundant conditional

482d893

reorder oidc auth operations

1e50ff4

hellobontempo added ui bug Used to indicate a potential bug labels Mar 15, 2023

hellobontempo marked this pull request as ready for review March 15, 2023 01:33

hellobontempo requested a review from austingebauer as a code owner March 15, 2023 01:33

hellobontempo added this to the 1.11.9 milestone Mar 15, 2023

hellobontempo added backport/1.11.x labels Mar 15, 2023

hellobontempo added 3 commits March 15, 2023 17:12

add test

1ed2a00

test cleanup

e0cbc01

add changelog

d9c5335

hellobontempo changed the title ~~UI/re-fetch roles if there is a namespace~~ UI/fix auth form updating with namespace and fetchRoles before performing OIDC authentication Mar 16, 2023

hellobontempo changed the title ~~UI/fix auth form updating with namespace and fetchRoles before performing OIDC authentication~~ UI/update auth form so fetchRoles happens after a namespace is inputted, prior to OIDC auth Mar 16, 2023

hellobontempo changed the title ~~UI/update auth form so fetchRoles happens after a namespace is inputted, prior to OIDC auth~~ UI/update auth form to fetchRoles after a namespace is inputted, prior to OIDC auth Mar 16, 2023

zofskeez approved these changes Mar 16, 2023

View reviewed changes

hashishaw reviewed Mar 20, 2023

View reviewed changes

hashishaw approved these changes Mar 20, 2023

View reviewed changes

hellobontempo merged commit df6a4a8 into main Mar 21, 2023

This was referenced Mar 21, 2023

Backport 1.11.x: UI/update auth form to fetchRoles after a namespace is inputted, prior to OIDC auth #19541 #19659

Closed

Backport 1.12.x: UI/update auth form to fetchRoles after a namespace is inputted, prior to OIDC auth #19541 #19660

Closed

hellobontempo mentioned this pull request Mar 21, 2023

Backport 1.13.x: UI/update auth form to fetchRoles after a namespace is inputted, prior to OIDC auth #19541 #19661

Merged

hellobontempo removed backport/1.11.x labels Mar 21, 2023

hellobontempo removed this from the 1.11.9 milestone Mar 21, 2023

hellobontempo added this to the 1.13.1 milestone Mar 21, 2023

hellobontempo deleted the ui/VAULT-12868/update-auth-method-with-direct-nav branch March 23, 2023 18:55

jameshartig mentioned this pull request Apr 6, 2023

Allow code flow callback to be direct to Vault hashicorp/vault-plugin-auth-jwt#130

Open

fopina-ci mentioned this pull request Apr 27, 2023

Upstream tag v1.13.2 (revision b9b773f16) fopina/vault#47

Closed

fopina-ci mentioned this pull request Jun 8, 2023

Upstream tag v1.13.3 (revision 3bedf816c) fopina/vault#51

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI/update auth form to fetchRoles after a namespace is inputted, prior to OIDC auth #19541

UI/update auth form to fetchRoles after a namespace is inputted, prior to OIDC auth #19541

hellobontempo commented Mar 14, 2023 •

edited

Loading

Monkeychip commented Mar 15, 2023

hellobontempo commented Mar 15, 2023

zofskeez left a comment

zofskeez Mar 16, 2023

hellobontempo Mar 16, 2023

zofskeez Mar 16, 2023

hellobontempo Mar 16, 2023

zofskeez Mar 16, 2023

hellobontempo Mar 16, 2023

zofskeez Mar 16, 2023

hashishaw left a comment

hashishaw Mar 20, 2023

hellobontempo Mar 20, 2023

hashishaw Mar 20, 2023

hellobontempo Mar 20, 2023 •

edited

Loading

hashishaw left a comment

UI/update auth form to fetchRoles after a namespace is inputted, prior to OIDC auth #19541

UI/update auth form to fetchRoles after a namespace is inputted, prior to OIDC auth #19541

Conversation

hellobontempo commented Mar 14, 2023 • edited Loading

the bug 🐛

fix 🔧

Monkeychip commented Mar 15, 2023

hellobontempo commented Mar 15, 2023

zofskeez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hashishaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellobontempo Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

hashishaw left a comment

Choose a reason for hiding this comment

hellobontempo commented Mar 14, 2023 •

edited

Loading

hellobontempo Mar 20, 2023 •

edited

Loading