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

Fix the UI to correctly determine if a user has access to a resource #9473

Merged
merged 3 commits into from
Dec 21, 2021

Conversation

codingllama
Copy link
Contributor

@codingllama codingllama commented Dec 17, 2021

Introduce the GuessIfAccessIsPossible method, so callers may verify if a user has access to a category of resources, instead of access to a specific resource.

Fixes a bug where the "Session Recordings" menu doesn't show for users with where-based roles.

@codingllama
Copy link
Contributor Author

Issue #5430.

FYI @russjones, this fixes the bug we talked about yesterday.

@codingllama
Copy link
Contributor Author

Friendly ping @jimbishopp @timothyb89? We want to get this out for 8.1, so I wanted to merge before I'm out for recess, if possible.

@codingllama codingllama force-pushed the codingllama/session-ui-view branch from e944b86 to 030bfb1 Compare December 20, 2021 17:26
@fspmarshall
Copy link
Contributor

I see what this is going for, but it definitely feels a bit brittle and easy to misuse...

IIUC the problem we are trying to solve here is that not all variables are in-scope at this level to evaluate the where clause correctly, yeah? I'm wondering if it would be possible to handle that specific case, rather than evaluate all of these expressions to true/false. I.e. is it possible to distinguish between a where clause that actually doesn't match, and a where clause for which a match is not decidable? I haven't looked at the internals of the where parser in a long time, so maybe that isn't a reasonable ask. Definitely worth looking into it though.

If it is possible to differentiate between where clauses that can't be evaluated versus those that fail, then I'd prefer that we implement a function which returns an enum (e.g. Allowed | Indeterminate | Denied). Something which clearly communicates this in-between state where we aren't sure if the where clause passes.

Regardless of whether or not the above is possible/practical, lets rename this method to something that really screams "not a valid RBAC check" (e.g. GuessIfAccessIsPossible). We don't want someone ever looking at this method and thinking they can make a real access-control decision based on its results.

@codingllama
Copy link
Contributor Author

Thanks for the feedback, @fspmarshall.

Let's start with the easy part: I like the rename and it seems a good way forward, so I'll go ahead and apply that.

I'm not looking forward to adding a 3rd possible response to CheckAccessToRule, the knock-on effect in Teleport seems worse than the new method, plus our Parser and Predicate concepts aren't really built for 3-state responses.

WDYT? Could we move forward with the new naming? (tagging @russjones)

@codingllama codingllama force-pushed the codingllama/session-ui-view branch from 030bfb1 to 1bbfe27 Compare December 21, 2021 14:09
@codingllama
Copy link
Contributor Author

Friendly ping to reviewers?

Copy link
Contributor

@timothyb89 timothyb89 left a comment

Choose a reason for hiding this comment

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

I don't mind this, especially with the name change.

@codingllama
Copy link
Contributor Author

Thanks everyone.

@codingllama codingllama force-pushed the codingllama/session-ui-view branch from 1bbfe27 to f759440 Compare December 21, 2021 18:26
@codingllama codingllama enabled auto-merge (squash) December 21, 2021 18:39
@codingllama codingllama force-pushed the codingllama/session-ui-view branch from f759440 to 2f8b327 Compare December 21, 2021 19:22
@codingllama codingllama merged commit f16e9f5 into master Dec 21, 2021
@codingllama codingllama deleted the codingllama/session-ui-view branch December 21, 2021 21:54
codingllama added a commit that referenced this pull request Dec 22, 2021
…9473)

Introduce the GuessIfAccessIsPossible method, so callers may verify if a user
has access to a category of resources, instead of access to a specific resource.

Fixes a bug where the "Session Recordings" menu doesn't show for users with
where-based roles.
codingllama added a commit that referenced this pull request Dec 22, 2021
…9473) (#9525)

Introduce the GuessIfAccessIsPossible method, so callers may verify if a user
has access to a category of resources, instead of access to a specific resource.

Fixes a bug where the "Session Recordings" menu doesn't show for users with
where-based roles.
@webvictim webvictim mentioned this pull request Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants