UI: Align auth method ttl with tune value

[hellobontempo]

UI follow-on work for backend PR: #23914

---

If a ttl has not been configured for a mount then the mount uses system defaults which is conveyed by a ttl: 0. It's unclear what the actual default value is unless you run vault read sys/auth/token/tune:

⇒ vault read sys/auth/token/tune
Key                  Value
---                  -----
default_lease_ttl    768h
description          token based credentials
force_no_cache       false
max_lease_ttl        768h
token_type           default-service

The backend PR #23914 updates the sys/internal/ui/mounts endpoint to return the default TTL value if there is not a mount-specific TTL configured.

To leverage this update in the UI for auth methods, we have to change the query for those records from sys/auth/:path to also use sys/internal/ui/mounts. By updating this request we can now see the system default TTL value.

before, with 0 as default value

---

after

[github-actions]

CI Results:
All Go tests succeeded! ✅

[github-actions]

Build Results:
All builds succeeded! ✅

[hellobontempo]

Originally I updated the findAll method in the adapter to use sys/internal/ui/mounts, but that caused lots of test failures. It felt brittle to update all the places where .findAll is used because of how often the endpoint is used throughout the app.

Instead, opted to add a new query() method and just update where the methods are requested to decrease the likelihood of introducing a regression.

[noelledaley]

shucks! what exactly did you try while updating the findAll? the contents of the query method and everything inside the if (isUnauthenticated) inside the findAll look identical aside from the error handling, so it seems odd to need to re-implement it. i'm also thinking that the adapter's query method is meant for handling custom queries whereas this implementation of it makes the same request each time, without handling options that are passed in. to me that's a bit misleading, but i definitely understand the hesitation of causing test failures. could you share more about what your approach was and what the test failures were?

[hellobontempo]

Great questions! I agree it seems a bit odd. This approach is definitely a way to shim in this request to avoid disrupting existing logic.

My initial approach can be seen in this commit and subsequent failures can be seen here (there are so many failures you can't review the Summary but have to expand the test-ui and then search for not ok

Friday brain felt overwhelmed by the number of failures, but upon returning with fresh eyes 👀 I realized all I needed to update was mirage endpoints for sys/auth to be sys/internal/ui/mounts and the failures resolved 😅 . Though, seeing all of the failures while I worked out those updates made me nervous to make such a high level change for such a small configuration detail update. I'm not terribly confident in our test coverage for this part of the codebase 😞

I'm happy revert and reuse the existing findAll method if that feels more intuitive. Another idea is to pass an additional adapter option that explicitly says to use the sys/internal/ui/mounts endpoint in this one place. So something like:

  findAll(store, type, sinceToken, snapshotRecordArray) {
    const isUnauthenticated = snapshotRecordArray?.adapterOptions?.unauthenticated;
    const useMountsEndpoint = adapterOptions?.useMountsEndpoint
    if (isUnauthenticated || useMountsEndpoint) {
      const url = `/${this.urlPrefix()}/internal/ui/mounts`;
      return this.ajax(url, 'GET', {
        unauthenticated: isUnauthenticated,
      })
      ...
    });

[noelledaley]

nice! a fresh set of eyes always helps, and great that the test failures were easily addressable.

i think passing an adapter option like you've shared in your code snippet above is a great compromise between getting the necessary changes in today while minimizing risks of side effects. ultimately, we should work on improving our test coverage so we can more confidently make changes like this, but we can address that over time.

great work here!

[hellobontempo]

✅ enterprise tests

[hellobontempo]

Passed after recent commit

[noelledaley]

the code changes here look solid to me. you may want another set of eyes from someone who's more familiar with the auth methods code, but on my end i think this looks good! 👍

[hashishaw]

👏 great test coverage, and elegant solution. Just added a couple suggestions for cleanup that you can take or leave

[hashishaw]

cleanup suggestion: this might read a little cleaner as async/await

[hellobontempo]

I prefer that syntax as well, but I think I'll leave it is to minimize changes in this PR. Which reminds me that I wanted to ask Finn if this should be backported.