Skip to content

chore: Drop resource_id support in rbac system #3426

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

Jump to bottom
Merged
merged 7 commits into from
Aug 9, 2022
Merged

chore: Drop resource_id support in rbac system #3426

merged 7 commits into from
Aug 9, 2022

Conversation

Emyrk
Copy link
Member

@Emyrk Emyrk commented Aug 9, 2022 •
edited
Loading

What this does

This drops resource_id from the RBAC permissions. This is a loss of a feature from RBAC. The goal is to reduce complexity of the rego policy, and reduce the number of permissions on actors.

At the present, no resource_id permissions are used. So this removes excess functionality that has no real planned use in the current roadmap.

Why did we have resource_id?

Intended to support:

  • Workspace agent tokens
    • This is handled outside the RBAC permission system
  • Roles as a resource (solved)
    • Solution needs to be determined. This intended solution was building a graph using permission nodes. Although "clean" from an additional code perspective, visualizing and displaying the output of this was going to be a challenge. It is likely easier to just build static lists of approved assign/remove roles for each role.
    • We can still implement this making each role a unique resource type.
  • Workspace sharing (ACL lists on object?)
    • Solution was to assign permission nodes to other users to share my workspace?? This solution was technically supported, but a bit convoluted as it requires assigning permissions to another user. It would also be hard to list all users that this workspace is shared with.

What do we gain?

By removing resource_id, there is only 3 properties of an object we need to know Authorize?: object_type, owner_id, and org_owner. This reduction only removes 1 property, but drastically reduces the number of unique objects in the system. This can drastically speedup Authorize() calls on large sets and enables future optimizations.

Can we revert this?

Yes. We can revert this, however I don't recommend we do Phase 2.

I think it is better to have a need arise and implement this back, then have this feature be available and possibly misused because it is "just there".

What this does not do

This does not reduce RBAC authorization benchmarks.

Emyrk added 4 commits August 9, 2022 08:29
@Emyrk
chore: Drop resource_id support in rbac system
44423b3 
resource_id was implemented to support:
 - Workspace agent tokens
 - Roles as a resource
 - Workspace sharing

The additional complexity is not required and will ease support
of rbac moving forward.
@Emyrk
Handle assigning/removing roles
1f7d5d3
@Emyrk
Update assign user roles test
0376027
@Emyrk
Return after forbidden
ea41968
@Emyrk Emyrk marked this pull request as ready for review August 9, 2022 16:58
kylecarbs
kylecarbs approved these changes Aug 9, 2022
View reviewed changes
Copy link
Member

@kylecarbs kylecarbs left a comment

Choose a reason for hiding this comment

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

I like the simplification here. Could we add a benchmark so we can see how much this improves the performance?

@Emyrk
Copy link
Member Author

Emyrk commented Aug 9, 2022

I like the simplification here. Could we add a benchmark so we can see how much this improves the performance?

This won't improve performance at all, but it allows me to use my new rego query that supports partials. I just need to drop this requirement first. I can add benchmarks for the sake of benchmarks though 👍

@kylecarbs
Copy link
Member

Ahh, I see. Should we just add that Rego query as part of this then?

@Emyrk
Copy link
Member Author

Emyrk commented Aug 9, 2022

Ahh, I see. Should we just add that Rego query as part of this then?

I have it like 90% written. I figured I'd drop this functionality, get the tests passing, then I can swap the rego without a sweat because if the tests pass, the implementation behavior is identical.

Feels "cleaner" to make that a different PR. If I add benchmarks to this, it also gives us a better comparison and we can go back in time in our git history a bit cleaner.

@kylecarbs
Copy link
Member

Makes sense to me! I approve.

Let's link the PRs in your future one, just so the timeline makes sense.

@Emyrk
Copy link
Member Author

Emyrk commented Aug 9, 2022 •
edited
Loading

Benchmarks:

$ go test -bench BenchmarkRBACFilter -benchmem -memprofile memprofile.out -cpuprofile profile.out -count=3 
goos: linux
goarch: amd64
pkg: github.com/coder/coder/coderd/rbac
cpu: Intel(R) Core(TM) i7-6770HQ CPU @ 2.60GHz
BenchmarkRBACFilter/NoRoles-8              13080             92679 ns/op       37322 B/op            684 allocs/op
BenchmarkRBACFilter/Admin-8                 3624            316964 ns/op      106771 B/op           2157 allocs/op
BenchmarkRBACFilter/OrgAdmin-8              3650            275078 ns/op       90953 B/op           1827 allocs/op
BenchmarkRBACFilter/OrgMember-8             3897            294138 ns/op      101517 B/op           2072 allocs/op
BenchmarkRBACFilter/ManyRoles-8             2319            503633 ns/op      161066 B/op           3245 allocs/op

@Emyrk Emyrk enabled auto-merge (squash) August 9, 2022 18:15
@Emyrk Emyrk merged commit db665e7 into main Aug 9, 2022
@Emyrk Emyrk deleted the stevenmasley/rbac_no_ids branch August 9, 2022 18:16
@Emyrk Emyrk mentioned this pull request Aug 10, 2022
Merged
johnstcn
johnstcn reviewed Aug 16, 2022
View reviewed changes
Copy link
Member

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johnstcn johnstcn johnstcn left review comments

@kylecarbs kylecarbs kylecarbs approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Emyrk @kylecarbs @johnstcn