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.

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

Better describe AccessCapabilities response when role filtering is applied #49062

Closed
wants to merge 1 commit into from
Closed

Better describe AccessCapabilities response when role filtering is applied #49062

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 10 additions & 2 deletions api/proto/teleport/legacy/types/types.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2641,11 +2641,19 @@ message AccessRequestFilter {
// is granted via their dynamic access privileges which may not be
// calculable by directly examining the user's own static roles.
message AccessCapabilities {
// RequestableRoles is a list of existent roles which the user is allowed to request.
// RequestableRoles is a list of existent roles which the user is allowed to request. It's only
// set when [[AccessCapabilitiesRequest]].RequestableRoles flag is set to true.
//
// If [[AccessCapabilitiesRequest]].RequestIDs is not empty and
// [[AccessCapabilitiesRequest]].FilterRequestableRolesByResource is set to true,
// ApplicableRolesForResources will be empty and this list will be trimmed down to roles which
// allow access to the provided resources.
repeated string RequestableRoles = 1 [(gogoproto.jsontag) = "requestable_roles,omitempty"];
// SuggestedReviewers is a list of all reviewers which are suggested by the user's roles.
repeated string SuggestedReviewers = 2 [(gogoproto.jsontag) = "suggested_reviewers,omitempty"];
// ApplicableRolesForResources is a list of the roles applicable for access to a given set of resources.
// ApplicableRolesForResources is a list of the roles applicable for access to a given set of
// resources. It's calculated only when the [[AccessCapabilitiesRequest]].RequestIDs is not empty
// [[AccessCapabilitiesRequest]].FilterRequestableRolesByResource is false.
repeated string ApplicableRolesForResources = 3 [(gogoproto.jsontag) = "applicable_roles,omitempty"];
// RequestPrompt is an optional message which tells users what they aught to request.
string RequestPrompt = 4 [(gogoproto.jsontag) = "request_prompt,omitempty"];
Expand Down
12 changes: 10 additions & 2 deletions api/types/types.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 5 additions & 6 deletions lib/auth/auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5129,20 +5129,19 @@ func (a *Server) CreateAccessRequestV2(ctx context.Context, req types.AccessRequ

req.SetCreationTime(now)

// Always perform variable expansion on creation only; this ensures the
// access request that is reviewed is the same that is approved.
// Access request has to be expanded during creation. See [[services.ExpandVars]] godoc to
// see why.
expandOpts := services.ExpandVars(true)
if err := services.ValidateAccessRequestForUser(ctx, a.clock, a, req, identity, expandOpts); err != nil {
return nil, trace.Wrap(err)
}

// Look for user groups and associated applications to the request.
requestedResourceIDs := req.GetRequestedResourceIDs()
var additionalResources []types.ResourceID

var userGroups []types.ResourceID
existingApps := map[string]struct{}{}
for _, resource := range requestedResourceIDs {
for _, resource := range req.GetRequestedResourceIDs() {
switch resource.Kind {
case types.KindApp:
existingApps[resource.Name] = struct{}{}
Expand Down Expand Up @@ -5175,8 +5174,8 @@ func (a *Server) CreateAccessRequestV2(ctx context.Context, req types.AccessRequ
}

if len(additionalResources) > 0 {
requestedResourceIDs = append(requestedResourceIDs, additionalResources...)
req.SetRequestedResourceIDs(requestedResourceIDs)
ids := append(req.GetRequestedResourceIDs(), additionalResources...)
req.SetRequestedResourceIDs(ids)
}

if req.GetDryRun() {
Expand Down
85 changes: 52 additions & 33 deletions lib/services/access_request_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1512,7 +1512,8 @@ func TestPruneRequestRoles(t *testing.T) {
},
},
loginHint: "responder",
// With "responder" login hint, only request node-access.
// With "responder" login hint, only request node-access (it has fewer
// Logins).
expectRoles: []string{"node-access"},
},
{
Expand Down Expand Up @@ -1775,30 +1776,34 @@ func TestGetRequestableRoles(t *testing.T) {
user := g.user(t)

tests := []struct {
name string
userRole string
requestedResources []types.ResourceID
disableFilter bool
allowedResourceIDs []types.ResourceID
expectedRoles []string
name string
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 change is intended PR desc says:

Better describe AccessCapabilities response when role filtering is applied

However, while the changes look good overall, they reorder tests (like filter by resources <-->"filtering disabled, rename some test cases, and add additional expectedApplicableRolesForResources and additional test case. From a review perspective, this makes it a bit messy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test changes:

  • rename can view resource but not assume role → can request resource but not assume role
  • add confirm can request resource when filtering disabled
  • don’t know how I managed to reorder filtering disabled with filter by resources - sorry for the unnecessary nosie

Adding expectedApplicableRolesForResources made it easier for me to comprehend what's going on here and how those two thing relate, but it may be subjective. I won't disagree with that.

Naming PR Better ... may be controversial. It was easier to understand it for me. I hoped it may be the same for others. If the general consensus is it makes less understandable then I'm happy to close it.

userRole string
requestedResources []types.ResourceID
disableFilter bool
allowedResourceIDs []types.ResourceID
expectedRoles []string
expectedApplicableRolesForResources []string
}{
{
name: "no resources to filter by",
userRole: "full-search",
expectedRoles: []string{"partial-access", "full-access"},
name: "no resources to filter by",
userRole: "full-search",
expectedRoles: []string{"partial-access", "full-access"},
expectedApplicableRolesForResources: []string{},
},
{
name: "filtering disabled",
userRole: "full-search",
requestedResources: []types.ResourceID{getResourceID(9)},
disableFilter: true,
expectedRoles: []string{"partial-access", "full-access"},
name: "filter by resources",
userRole: "full-search",
requestedResources: []types.ResourceID{getResourceID(9)},
expectedRoles: []string{"full-access"},
expectedApplicableRolesForResources: []string{},
},
{
name: "filter by resources",
userRole: "full-search",
requestedResources: []types.ResourceID{getResourceID(9)},
expectedRoles: []string{"full-access"},
name: "filtering disabled",
userRole: "full-search",
requestedResources: []types.ResourceID{getResourceID(9)},
disableFilter: true,
expectedRoles: []string{"partial-access", "full-access"},
expectedApplicableRolesForResources: []string{"full-access"},
},
{
name: "resource in another cluster",
Expand All @@ -1811,25 +1816,38 @@ func TestGetRequestableRoles(t *testing.T) {
Name: "node-9",
},
},
expectedRoles: []string{"partial-access", "full-access"},
expectedRoles: []string{"partial-access", "full-access"},
expectedApplicableRolesForResources: []string{},
},
{
name: "resource user shouldn't know about",
userRole: "partial-search",
requestedResources: []types.ResourceID{getResourceID(9)},
expectedRoles: []string{"partial-access", "full-access"},
expectedApplicableRolesForResources: []string{},
},
{
name: "resource user shouldn't know about",
userRole: "partial-search",
requestedResources: []types.ResourceID{getResourceID(9)},
expectedRoles: []string{"partial-access", "full-access"},
name: "can request resource but not assume role",
userRole: "partial-roles",
requestedResources: []types.ResourceID{getResourceID(9)},
expectedRoles: []string{},
expectedApplicableRolesForResources: []string{},
},
{
name: "can view resource but not assume role",
userRole: "partial-roles",
requestedResources: []types.ResourceID{getResourceID(9)},
name: "confirm can request resource when filtering disabled",
userRole: "partial-roles",
requestedResources: []types.ResourceID{getResourceID(9)},
disableFilter: true,
expectedRoles: []string{"partial-access"},
expectedApplicableRolesForResources: []string{"full-access"},
},
{
name: "prevent transitive access",
userRole: "partial-access",
requestedResources: []types.ResourceID{getResourceID(9)},
allowedResourceIDs: []types.ResourceID{getResourceID(0), getResourceID(1), getResourceID(2), getResourceID(3), getResourceID(4)},
expectedRoles: []string{"full-access", "full-search"},
name: "prevent transitive access",
userRole: "partial-access",
requestedResources: []types.ResourceID{getResourceID(9)},
allowedResourceIDs: []types.ResourceID{getResourceID(0), getResourceID(1), getResourceID(2), getResourceID(3), getResourceID(4)},
expectedRoles: []string{"full-access", "full-search"},
expectedApplicableRolesForResources: []string{},
},
}

Expand All @@ -1847,7 +1865,8 @@ func TestGetRequestableRoles(t *testing.T) {
FilterRequestableRolesByResource: !tc.disableFilter,
})
require.NoError(t, err)
require.ElementsMatch(t, tc.expectedRoles, accessCaps.RequestableRoles)
require.ElementsMatch(t, tc.expectedRoles, accessCaps.RequestableRoles, "RequestableRoles")
require.ElementsMatch(t, tc.expectedApplicableRolesForResources, accessCaps.ApplicableRolesForResources, "ApplicableRolesForResources")
})
}
}
Expand Down
Loading