Skip to content

Include requiresRequest flag for searchAsRoles requests#40328

Merged
avatus merged 1 commit intomasterfrom
avatus/search_as_roles
Apr 29, 2024
Merged

Include requiresRequest flag for searchAsRoles requests#40328
avatus merged 1 commit intomasterfrom
avatus/search_as_roles

Conversation

@avatus
Copy link
Copy Markdown
Contributor

@avatus avatus commented Apr 8, 2024

This PR is the start of a series that will allow users to make access requests directly from the resources page in the web UI.

The purpose of this PR is to add a flag to differentiate between resources being returned due to static access, and resources returned during an "extended" auth context (for a searchAsRoles) request.

The second part of this series will be appending this RequiresRequest value to all of the makeResource methods for the resources returned to the web UI. I decided to leave that out of this PR so the focus could be on the method I'm proposing to differentiate between the two types of access.

changelog: Audit events for AccessRequestResourceSearch are no longer emitted.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 8, 2024

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

Comment thread lib/auth/auth_with_roles.go
@avatus avatus requested review from nklaassen and rosstimothy and removed request for EdwardDowling April 8, 2024 17:41
@avatus avatus added the no-changelog Indicates that a PR does not require a changelog entry label Apr 8, 2024
Comment thread api/proto/teleport/legacy/client/proto/authservice.proto Outdated
Comment thread lib/auth/auth_with_roles.go Outdated
Comment thread lib/auth/auth_with_roles_test.go
@avatus avatus removed the no-changelog Indicates that a PR does not require a changelog entry label Apr 8, 2024
Comment thread lib/auth/auth_with_roles.go Outdated
Comment thread lib/auth/auth_with_roles.go Outdated
Comment thread api/client/client.go Outdated
Comment thread lib/auth/auth_with_roles.go Outdated
Comment thread lib/services/unified_resource.go Outdated
@avatus avatus force-pushed the avatus/search_as_roles branch 2 times, most recently from 2190190 to ec99805 Compare April 10, 2024 17:32
@avatus
Copy link
Copy Markdown
Contributor Author

avatus commented Apr 23, 2024

After fiddling with this a bit, I think instead of checking if searchAsRoles exists in the request, I'll have to add a new field like includeRequestable. This will help with backward compatibility and also allow us to do something like "get searchAsRoles, get only have access, or both".

If a new web client calls an old proxy with searchAsRoles, it will receive all the resources (including requestable) but without the distinction between which is which, the new web client will display all as "has access". However, if we add the new field, then the old proxy will essentially ignore it and will only receive has access resources. I believe for backward compatible in this case, NOT seeing requestable resources is better than seeing resources that you think you have access to (during the upgrade).

Also, if an old web client hits a new proxy, it won't send the new param anyway and everything will be as expected.

I will update the PR with this new field and then address the current feedback (that doesn't care about the param name)

@avatus avatus requested review from nklaassen and rosstimothy April 27, 2024 02:11
Copy link
Copy Markdown
Contributor

@nklaassen nklaassen left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me, I would approve but I don't see any tests for this, will you be adding some?

Comment thread api/proto/teleport/legacy/client/proto/authservice.proto Outdated
Comment thread api/proto/teleport/legacy/client/proto/authservice.proto Outdated
Comment thread lib/auth/auth_with_roles_test.go
Comment thread lib/services/unified_resource.go Outdated
Comment thread lib/services/unified_resource.go Outdated
Comment thread lib/auth/auth_with_roles.go Outdated
@avatus
Copy link
Copy Markdown
Contributor Author

avatus commented Apr 29, 2024

will you be adding some?

Yup, just added. thanks for the detailed feedback!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Only populated ..... ?

Comment thread lib/auth/auth_with_roles.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

add a comment to this field too?

@avatus avatus force-pushed the avatus/search_as_roles branch from f085fdc to 282bf78 Compare April 29, 2024 17:05
@avatus avatus enabled auto-merge April 29, 2024 17:05
@avatus avatus requested a review from nklaassen April 29, 2024 17:07
@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from espadolini April 29, 2024 17:10
@avatus avatus added this pull request to the merge queue Apr 29, 2024
Merged via the queue into master with commit d129b1d Apr 29, 2024
@avatus avatus deleted the avatus/search_as_roles branch April 29, 2024 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants