Skip to content

Backport of UI: LDAP Hierarchical roles into release/1.18.x #28841

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

Conversation

hc-github-team-secure-vault-core
Copy link
Collaborator

Backport

This PR is auto-generated from #28824 to be assessed for backporting due to the inclusion of the label backport/1.18.x.

🚨

Warning automatic cherry-pick of commits failed. If the first commit failed,
you will see a blank no-op commit below. If at least one commit succeeded, you
will see the cherry-picked commits up to, not including, the commit where
the merge conflict occurred.

The person who merged in the original PR is:
@hellobontempo
This person should manually cherry-pick the original PR into a new backport PR,
and close this one when the manual backport PR is merged in.

merge conflict error: POST https://api.github.com/repos/hashicorp/vault/merges: 409 Merge conflict []

The below text is copied from the body of the original PR.


Description

This PR adds hierarchical role navigation to the UI for LDAP roles which was added to the CLI in 1.17 (Hierarchical Paths). These changes were nontrivial due to the API and GUI design, which I'll outline below for consideration during future list views and navigation with path style resources.

Screen.Recording.2024-11-04.at.12.10.31.PM.mov

Listing hierarchical roles in LDAP returns only top-level items. For example, this ldap engine has the dynamic role paths: admin/role1, admin/nested/role2, my-role. A LIST request has to be made at each level to glean this. Roles are also dependent on :type and so the same must be done for static roles with static-role in the path instead of role.

⚡ ⇒ vault list ldap/role 
Keys
----
admin/ 
my-role

⚡ ⇒ vault list ldap/role/admin
Keys
----
role1
nested/

⚡ ⇒ vault list ldap/role/admin/nested 
Keys
----
role2

Some API endpoints, like this namespaces one, list all of the paths Note: this is a sys/internal/ui endpoint

⇒ curl \
   --header "X-Vault-Token: <token>" \
   --request GET \
http://127.0.0.1:8200/v1/sys/internal/ui/namespaces | jq
--------
{
 "data": {
   "keys": [
     "admin/",
     "admin/child/"
   ]
 }
}

Considerations

Existing list patterns

KV v2 has a similar pattern because secret paths can be nested (i.e. nested/path/to/secret). The route file defines separate routes, list is for listing secrets in the engine and list-directory is for secret paths that end in a forward slash. The list route inherits from list-directory.js because much of the routing logic is the same. In LDAP, I chose to create a base route model class to share the lazyPaginatedQuery method but maintain separate model hooks.

The major difference here is in LDAP navigating through hierarchical roles does not update the filter input and pageFilter maps one to one to the filter input. I opted for this to separate concerns: url paths and navigation is unrelated to filtering the list results which is the responsibility of query params.

On the top, navigating into the admin/ subdirectory changes the URL and breadcrumbs, but the filter is not updated unless a user wants to filter within that directory. In KV v2, clicking into a subdirectory adds that path to the filter, even though the URL is not updated with the corresponding pageFilter

Note: locally I changed the config to only list 2 items to test pagination (instead of the default of 100)

Screenshot 2024-11-04 at 12 32 31 PM

In both engines, filtering updates the query params, but in KV v2 only the suffix is included (which makes sense because we're inside that secret path, however it's confusing UX because the filter input does not match the URL query params)
Screenshot 2024-11-04 at 12 35 50 PM

Aside - client-side pagination in KV v2 appears to be broken when pageFilter exists, which seems to be an additional argument for separate route models

Pagination

The original implementation of LDAP roles uses the same model ldap/role for both dynamic and static roles. Typically, we want model schemas to map closely to the API params (see Models docs for our expected ember data model patterns). Following this pattern, ldap/role should have been a base class with any shared params (like name) and for defining adapter/serializer methods as those are the same. Static and dynamic role models would inherit this class and then define type-specific fields and params (see #28829 for what this would look like).
Abandoned because the list view displays both static and dynamic roles and our client-side pagination service relies on these being the same model.

💡 Future improvement/suggestion

We should remove our reliance on the ember-data store for client-side pagination and instead manage that request and caching as a javascript class. This way we can request whatever data we want and store it, instead of relying on 1 ember data model per pagination view.

SearchSelect

The API structure of LDAP roles makes components like SearchSelect unusable out of the box as it requires a LIST request is made at each level. For the first iteration of the hierarchical roles implementation, we decided to filter out pathed roles, so only top-level roles are available for quickly generating credentials here:
Screenshot 2024-11-04 at 10 27 04 AM

💡 Future imrpovement/suggestion

When designing "quick action" cards for path-supported items we could build a component similar to KvSuggestionInput (code) which makes a request at each level. Or outfit the existing component to be more general and accept any resource to list and query.

TODO only if you're a HashiCorp employee

  • Backport Labels: If this PR is in the ENT repo and needs to be backported, backport
    to N, N-1, and N-2, using the backport/ent/x.x.x+ent labels. If this PR is in the CE repo, you should only backport to N, using the backport/x.x.x label, not the enterprise labels.
    • If this fixes a critical security vulnerability or severity 1 bug, it will also need to be backported to the current LTS versions of Vault. To ensure this, use all available enterprise labels.
  • ENT Breakage: If this PR either 1) removes a public function OR 2) changes the signature
    of a public function, even if that change is in a CE file, double check that
    applying the patch for this PR to the ENT repo and running tests doesn't
    break any tests. Sometimes ENT only tests rely on public functions in CE
    files.
  • Jira: If this change has an associated Jira, it's referenced either
    in the PR description, commit message, or branch name.
  • RFC: If this change has an associated RFC, please link it in the description.
  • ENT PR: If this change has an associated ENT PR, please link it in the
    description. Also, make sure the changelog is in this PR, not in your ENT PR.

Overview of commits

Copy link

CLA assistant check

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes


temp seems not to be a GitHub user.
You need a GitHub account to be able to sign the CLA.
If you have already a GitHub account, please add the email address used for this commit to your account.

Have you signed the CLA already but the status is still pending? Recheck it.

1 similar comment
Copy link

CLA assistant check

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes


temp seems not to be a GitHub user.
You need a GitHub account to be able to sign the CLA.
If you have already a GitHub account, please add the email address used for this commit to your account.

Have you signed the CLA already but the status is still pending? Recheck it.

@github-actions github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Nov 6, 2024
Copy link

github-actions bot commented Nov 6, 2024

CI Results:
All Go tests succeeded! ✅

@hellobontempo
Copy link
Contributor

closing for manual backport #28842

@hellobontempo hellobontempo deleted the backport/ui/ldap-hierarchical-roles/nearly-devoted-jay branch November 6, 2024 01:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants