Skip to content
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: LDAP Hierarchical roles #28824

Merged
merged 17 commits into from
Nov 6, 2024
Merged

UI: LDAP Hierarchical roles #28824

merged 17 commits into from
Nov 6, 2024

Conversation

hellobontempo
Copy link
Contributor

@hellobontempo hellobontempo commented Nov 2, 2024

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.

@hellobontempo hellobontempo mentioned this pull request Nov 2, 2024
6 tasks
@github-actions github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Nov 2, 2024
@@ -3,46 +3,92 @@
* SPDX-License-Identifier: BUSL-1.1
*/

import NamedPathAdapter from 'vault/adapters/named-path';
import ApplicationAdapter from 'vault/adapters/application';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can't use name as the primaryKey because static and dynamic roles can technically have the same name

import { encodePath } from 'vault/utils/path-encoding-helpers';
import { service } from '@ember/service';
import AdapterError from '@ember-data/adapter/error';
import { addManyToArray } from 'vault/helpers/add-to-array';
import sortObjects from 'vault/utils/sort-objects';

export default class LdapRoleAdapter extends NamedPathAdapter {
export const ldapRoleID = (type, name) => `type:${type}::name:${name}`;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is because ember data likes IDs and Vault doesn't return them. Pulled this into a helper here so it can be reused in tests and mirage so this didn't have to be updated in multiple places

urlForUpdateRecord(name, modelName, snapshot) {
const { backend, type } = snapshot.record;
return this.getURL(backend, this.pathForRoleType(type), name);
createOrUpdate(store, modelSchema, snapshot) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we're no longer extending the NamedPathAdapter, had to add these methods to manually return ID info

Copy link

github-actions bot commented Nov 2, 2024

CI Results:
All Go tests succeeded! ✅

return { id: ldapRoleID(type, name), backend, name, type };
}

_getURL(backend, path, name) {
Copy link
Contributor Author

@hellobontempo hellobontempo Nov 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added _ to denote private methods that are only used within this adapter to clearly distinguish them from builtin adapter methods

@hellobontempo hellobontempo marked this pull request as ready for review November 4, 2024 18:14
@hellobontempo hellobontempo requested a review from a team as a code owner November 4, 2024 18:14
Copy link

github-actions bot commented Nov 4, 2024

Build Results:
All builds succeeded! ✅

const options = this.args.roles
// hierarchical roles are not selectable
.filter((r: LdapRoleModel) => !r.name.endsWith('/'))
// *hack alert* - type is set as id so it renders beside name in search select
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@Monkeychip Monkeychip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice work. That PR description is what dreams are made of. One question, nothing blocking.

@hasChevron={{false}}
data-test-popup-menu-trigger="{{role.type}} {{role.name}}"
/>
{{#if (this.isHierarchical role.name)}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 Does this need fn wrapped around, so it's clear you're passing role.name into this.isHierarchical()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually a template helper so it has slightly different syntax 😄 Docs

Screenshot 2024-11-05 at 4 19 15 PM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL, thank you!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 🧼 :)

...ldapBreadcrumbs(roleAncestry.path_to_role, roleAncestry.type, backendModel.id, true),
];

// must call 'set' so breadcrumbs update as we navigate through directories
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call out.

import type Transition from '@ember/routing/transition';
import type LdapRoleModel from 'vault/models/ldap/role';
import type SecretEngineModel from 'vault/models/secret-engine';
import type Controller from '@ember/controller';
import type { Breadcrumb } from 'vault/vault/app-types';

interface LdapRolesRouteModel {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these are locally type definitions, I find simpler names to be easier to read and follow. WhenTheyGetReallyLong it's hard to map them back to their definition in my opinion 😅

@hellobontempo hellobontempo added backport/1.18.x backport/ent/1.17.x+ent Changes are backported to 1.17.x+ent labels Nov 6, 2024
@hellobontempo hellobontempo enabled auto-merge (squash) November 6, 2024 00:49
@hellobontempo hellobontempo merged commit 30d4e21 into main Nov 6, 2024
42 checks passed
@hellobontempo hellobontempo deleted the ui/ldap-hierarchical-roles branch November 6, 2024 00:52
@hellobontempo
Copy link
Contributor Author

Forgot to post but ✅ enterprise passed
Screenshot 2024-11-05 at 5 30 09 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/ent/1.17.x+ent Changes are backported to 1.17.x+ent backport/1.18.x 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