Filter IC Permission Sets based on access #49936

tcsc · 2024-12-09T14:39:27Z

If a user is granted access to an IC Permission Set via an access request,
the UI treats the referenced account as if all Permission Sets are granted.
This hides the requestable Permission Sets from the user, making it
impossible for a user to requests more Permission Sets on that account.

This patch filters the Permission Sets delivered to the UI based on whether
the user already has access the Permission Sets and marks any requestable
Permission Sets on the account.

This provides the UI with enough information to decide how to display the
Permission Set list in order to get around the issue.

smallinsky · 2024-12-10T13:19:43Z

api/proto/teleport/legacy/types/types.proto

+
+  // RequiresRequest indicates that the user needs to raise an access request to
+  // be granted this permission set
+  bool RequiresRequest = 4 [(gogoproto.jsontag) = "assignment_name,omitempty"];


Why is the RequiresRequest attribute needed for the permission_set object?

The RequiresRequest attribute is only used by the Front End. For the same flow in Okta, when an app has multiple okta_groups or AWS app access with multiple IAM configurations, the RequiresRequest attribute was not necessary apart from the top level app property and Instead, the logic riels on the top-level app.RequiresRequest flag.

So why is it handled differently in this case? For the AWS IC flow, why is the RequiresRequest flag needed for each selected object, whereas for other flows (e.g., Okta with multiple okta_groups), the logic relies on the top-level app.RequiresRequest flag?

Also does the FE is aware of this change ?
cc: @flyinghermit

We could probably do this with the enriched resource, outside of the AppV3 and avoiding the protobuf change.

OK, so the reason this needs to be in the protobuf definition is that the app representing the IC Account goes over the wire (at least logically) at some point between the Unified Resource Cache listing and being rendered to json.

As far as I can tell, we need to calculate which Account Assignments are available to (or requestable by) a user during this listing, because once we have transmitted the resource to the URC client, we no longer have the tools available to evaluate the user's access.

If we want to expose this requestability info to the UI, then we need to expose it to the Unified Resource Cache client first, and for that it has to be part of the App's wire format.

lib/auth/auth_with_roles.go

api/proto/teleport/legacy/types/types.proto

If a user is granted access to an IC Permission Set via an access request, the UI treats the referenced account as if all Permission Sets are granted. This hides the requestable Permission Sets from the user, making it impossible for a user to requests more Permission Sets on that account. This patch filters the Permission Sets delivered to the UI based on whether the user already has access the Permission Sets and marks any requestable Permission Sets on the account. This provides the UI with enough information to decide how to display the Permission Set list in order to get around the issue.

lib/auth/auth_with_roles.go

smallinsky · 2024-12-11T16:43:54Z

api/types/app.go

@@ -472,7 +472,7 @@ func (a *AppV3) GetDisplayName() string {
 	if a.Spec.IdentityCenter == nil {
 		return ""
 	}
-	return a.GetName()
+	return a.Metadata.Description


This change is not related to "Filter IC Permission Sets based on access" right ?

Just want to make sure that this is not leftover from local stash.

I needed to change the name value of the "app" to be the account ID - for two reasons:

so I can pull it out to build the dummy Account Assignment that we use to query access

to avoid the uniqueness issues we discussed yesterday

The knock-on effect of this is that the friendly name is now pulled from the app description.

smallinsky · 2024-12-11T16:54:10Z

lib/auth/auth_with_roles.go

+			continue
+		}
+		output = append(output, ps)
+		if _, requestable := checker.requestableMap[ps.AssignmentID]; requestable {


Why the checker.requestableMap map is updated here? This filterICPermissionSets function is called after the checker.requestableMap map is used in

teleport/lib/auth/auth_with_roles.go

Line 1457 in 5b55287

paginatedResources, err := services.MakePaginatedResources(ctx, types.KindUnifiedResource, unifiedResources, resourceAccess.requestableMap)

The access check for the AccountAssignment on line 1530 modifies the requestableMap as a side effect, and that's how we pull out whether the assignment requires an Access Request.

Ah, Thanks. I had in mind the previous logic model but for this flow where the checker.checkAccess is reused that makes sens.

smallinsky · 2024-12-11T16:55:21Z

lib/auth/auth_with_roles.go

+
+	var output []*types.IdentityCenterPermissionSet
+	for _, ps := range pss {
+		assignment.Metadata.Name = ps.AssignmentID


Why the assignment.Metadata.Name needs to be updated. Does the checker.checkAccess RBAC check depends on the resource name ?

Yes - when it writes to the requestable map

Co-authored-by: Marek Smoliński <[email protected]>

public-teleport-github-review-bot · 2024-12-12T23:11:25Z

@tcsc See the table below for backport results.

Branch	Result
branch/v17	Failed

Backports #49936 If a user is granted access to an IC Permission Set via an access request, the UI treats the referenced account as if all Permission Sets are granted. This hides the requestable Permission Sets from the user, making it impossible for a user to requests more Permission Sets on that account. This patch filters the Permission Sets delivered to the UI based on whether the user already has access the Permission Sets and marks any requestable Permission Sets on the account. This provides the UI with enough information to decide how to display the Permission Set list in order to get around the issue. --------- Co-authored-by: Marek Smoliński <[email protected]>

* [v17] Filter IC Permission Sets based on access Backports #49936 If a user is granted access to an IC Permission Set via an access request, the UI treats the referenced account as if all Permission Sets are granted. This hides the requestable Permission Sets from the user, making it impossible for a user to requests more Permission Sets on that account. This patch filters the Permission Sets delivered to the UI based on whether the user already has access the Permission Sets and marks any requestable Permission Sets on the account. This provides the UI with enough information to decide how to display the Permission Set list in order to get around the issue. --------- Co-authored-by: Marek Smoliński <[email protected]> * fix bad cherrypick --------- Co-authored-by: Marek Smoliński <[email protected]>

tcsc added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v17 labels Dec 9, 2024

tcsc requested review from r0mant and smallinsky December 9, 2024 14:39

smallinsky reviewed Dec 10, 2024

View reviewed changes

rosstimothy reviewed Dec 10, 2024

View reviewed changes

api/proto/teleport/legacy/types/types.proto Outdated Show resolved Hide resolved

espadolini reviewed Dec 10, 2024

View reviewed changes

api/proto/teleport/legacy/types/types.proto Outdated Show resolved Hide resolved

tcsc force-pushed the tcsc/idc-filter-permission-sets branch from 155d548 to 5b55287 Compare December 11, 2024 15:01

tcsc marked this pull request as ready for review December 11, 2024 15:12

Merge branch 'master' into tcsc/idc-filter-permission-sets

fc4b0b3

github-actions bot requested review from gabrielcorado and zmb3 December 11, 2024 15:13

github-actions bot added the size/md label Dec 11, 2024

smallinsky reviewed Dec 11, 2024

View reviewed changes

tcsc and others added 3 commits December 12, 2024 12:46

Merge branch 'master' into tcsc/idc-filter-permission-sets

113135a

Update lib/auth/auth_with_roles.go

009d6aa

Co-authored-by: Marek Smoliński <[email protected]>

Update auth_with_roles.go

aef3a56

tcsc requested review from smallinsky, espadolini and rosstimothy December 12, 2024 02:35

smallinsky approved these changes Dec 12, 2024

View reviewed changes

tcsc added the aws-iam-identity-center label Dec 12, 2024

r0mant approved these changes Dec 12, 2024

View reviewed changes

public-teleport-github-review-bot bot removed request for gabrielcorado and zmb3 December 12, 2024 22:41

tcsc added this pull request to the merge queue Dec 12, 2024

Merged via the queue into master with commit b3fbbc2 Dec 12, 2024
42 checks passed

tcsc deleted the tcsc/idc-filter-permission-sets branch December 12, 2024 23:10

tcsc mentioned this pull request Dec 13, 2024

[v17] Filter IC Permission Sets based on access #50191

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter IC Permission Sets based on access #49936

Filter IC Permission Sets based on access #49936

tcsc commented Dec 9, 2024

smallinsky Dec 10, 2024

tcsc Dec 11, 2024

tcsc Dec 11, 2024

smallinsky Dec 11, 2024

tcsc Dec 12, 2024

smallinsky Dec 11, 2024 •

edited

Loading

tcsc Dec 12, 2024

smallinsky Dec 12, 2024

smallinsky Dec 11, 2024

tcsc Dec 12, 2024

public-teleport-github-review-bot bot commented Dec 12, 2024

Filter IC Permission Sets based on access #49936

Filter IC Permission Sets based on access #49936

Conversation

tcsc commented Dec 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smallinsky Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

public-teleport-github-review-bot bot commented Dec 12, 2024

smallinsky Dec 11, 2024 •

edited

Loading