Skip to content

Add integration access rule to web user context #24256

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.

Sign up for GitHub

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

Merged
kimlisa merged 5 commits into from
Apr 11, 2023
Merged

Add integration access rule to web user context #24256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
11e1ba4
Add integration access rule to web user context
kimlisa Apr 5, 2023
c54fc0a
Merge branch 'master' into lisa/discover/add-integration-to-user-context
kimlisa Apr 6, 2023
d8a3da6
Merge branch 'master' into lisa/discover/add-integration-to-user-context
kimlisa Apr 11, 2023
665d16a
Merge branch 'master' into lisa/discover/add-integration-to-user-context
kimlisa Apr 11, 2023
9f034e4
Merge branch 'master' into lisa/discover/add-integration-to-user-context
kimlisa Apr 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/web/ui/usercontext.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,8 @@ type userACL struct {
License access `json:"license"`
// Plugins defines whether the user has access to manage hosted plugin instances
Plugins access `json:"plugins"`
// Integrations defines whether the user has access to manage integrations.
Integrations access `json:"integrations"`
Copy link
Copy Markdown
Contributor Author

@kimlisa kimlisa Apr 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@marcoandredinis i just realized that the frontend will need to check for both integration.create and integration.use before starting the aws integration b/c at the list rds dbs screen we will need to use the integration?

Copy link
Copy Markdown
Contributor

@marcoandredinis marcoandredinis Apr 11, 2023

Choose a reason for hiding this comment

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

Yes 👍
We'll need the use verb in order to call Integration's APIs

Copy link
Copy Markdown
Collaborator

@zmb3 zmb3 Apr 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Do we really need a new verb for this?

I'm concerned that we just add random verbs like this that are only used for one particular feature and then never document them, and after doing this for years we have a confusing RBAC system that no one can understand.

For example, I can totally see a support ticket coming in that says "my role has the use verb for desktops but I can't connect to the desktop, why?"

Is there a way to use the existing RBAC system that we have today rather than extend it just for thi feature?

Copy link
Copy Markdown
Contributor

@marcoandredinis marcoandredinis Apr 11, 2023

Choose a reason for hiding this comment

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

Well, we considered other options but ended up with the use verb
Do you think one of the following would be better?

  • another role option: integration_labels
  • re-use another "standard" verb to check when calling a 3rd party API: integration.create?
  • change the verb to execute and hopefully it can become a standard verb from now on to indicate that the role allows execution of some kind in the resource (ssh into a node, kube exec in a pod, ...)

cc @r0mant

Copy link
Copy Markdown
Collaborator

@r0mant r0mant Apr 11, 2023

Choose a reason for hiding this comment

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

I don't think we have other existing verbs we can reuse here tbh. Other verbs we have are either for CRUD actions, or were also introduced specifically for individual features. For example, we have VerbRotate, VerbCreateEnrolleToken, VerbEnroll: https://github.com/gravitational/teleport/blob/master/api/types/constants.go.

Compared to these, VerbUse is actually less "random" concept IMO, for example Kubernetes uses it (no pun intended) in its RBAC too.

// DeviceTrust defines access to device trust.
DeviceTrust access `json:"deviceTrust"`
}
Expand Down Expand Up @@ -207,6 +209,7 @@ func NewUserContext(user types.User, userRoles services.RoleSet, features proto.
download := newAccess(userRoles, ctx, types.KindDownload)
license := newAccess(userRoles, ctx, types.KindLicense)
deviceTrust := newAccess(userRoles, ctx, types.KindDevice)
integrationsAccess := newAccess(userRoles, ctx, types.KindIntegration)

acl := userACL{
AccessRequests: requestAccess,
Expand All @@ -232,6 +235,7 @@ func NewUserContext(user types.User, userRoles services.RoleSet, features proto.
Download: download,
License: license,
Plugins: pluginsAccess,
Integrations: integrationsAccess,
DeviceTrust: deviceTrust,
}

Expand Down
5 changes: 5 additions & 0 deletions lib/web/ui/usercontext_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ func TestNewUserContext(t *testing.T) {
Resources: []string{types.KindWindowsDesktop},
Verbs: services.RW(),
},
{
Resources: []string{types.KindIntegration},
Verbs: services.RW(),
},
})

// not setting the rule, or explicitly denying, both denies access
Expand Down Expand Up @@ -83,6 +87,7 @@ func TestNewUserContext(t *testing.T) {
require.Equal(t, userContext.Name, "root")
require.Empty(t, cmp.Diff(userContext.ACL.AuthConnectors, allowed))
require.Empty(t, cmp.Diff(userContext.ACL.TrustedClusters, allowed))
require.Empty(t, cmp.Diff(userContext.ACL.Integrations, allowed))
require.Empty(t, cmp.Diff(userContext.ACL.AppServers, denied))
require.Empty(t, cmp.Diff(userContext.ACL.DBServers, denied))
require.Empty(t, cmp.Diff(userContext.ACL.KubeServers, denied))
Expand Down