feat(project-tree): rbac #30933

mariusandra · 2025-04-08T12:56:17Z

Problem

The file system doesn't support role based access controls.

Changes

Add AccessControl filtering to file system list
Also add it to all other file system endpoints
Remove returning results for the "Unfiled" endpoint (skips filtering and we're not using them anyway)

Small caveat

Caveat: I understand currently not all types are implemented for access control, which includes hog functions... However if/when implemented, the hog_function resource types won't work out of the box. The file system stores the type as "hog_function/destination", whereas I assume access controls will just store "hog_function". We might want to limit access per hog function type... or we might want to refactor the file system to add a "subtype" field. I'm not sure, but I don't see anything really blocking here...

How did you test this code?

Just with unit tests until now.

posthog/rbac/user_access_control.py

zlwaterfield · 2025-04-08T13:30:09Z

posthog/rbac/user_access_control.py

+
+        # 3) Annotate the queryset with 'effective_access_level'.
+        queryset = queryset.annotate(
+            effective_access_level=Coalesce(


In the new access control system we aren't using effective_access_level, any uses of it are legacy. In this case, I see why you did it because it's not the exact resource but a pointer to it. Just wanted to call that out.

zlwaterfield · 2025-04-08T13:33:16Z

posthog/rbac/user_access_control.py

+
+        # 4) Exclude items that have 'none' (0) unless user is the creator
+        #    i.e. skip items where effective_access_level=0 AND created_by != user
+        queryset = queryset.exclude(Q(effective_access_level=0) & ~Q(created_by=user))


I see why you're mapping these to integers for easy filtering, but we tried to avoid using numbers because adding new access levels would make this hard to change.

We have the notion of ordered_access_levels, which was meant to solve this.

Could we instead annotate the access control enum and exclude where it's "none" to be more explicit?

posthog/rbac/user_access_control.py

greptile-apps

PR Summary

This PR integrates RBAC into the file system API by filtering querysets based on user access control and updating outcomes for endpoints like "unfiled".

Modified /posthog/api/file_system.py to apply RBAC filtering and prevent users from accessing unauthorized file system objects.
Introduced and utilized filter_and_annotate_file_system_queryset in /posthog/rbac/user_access_control.py.
Updated tests in /posthog/api/test/test_file_system.py to assert RBAC restrictions and change the "unfiled" endpoint to return only a count.
Extended RBAC tests in /posthog/rbac/test/test_user_access_control.py to cover new file system scenarios.

_{4 file(s) reviewed, 2 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

posthog/api/test/test_file_system.py

posthog/rbac/test/test_user_access_control.py

zlwaterfield

Just comments, no blockers

zlwaterfield · 2025-04-09T13:48:20Z

posthog/api/file_system.py

-            FileSystem.objects.filter(team=self.team, path__startswith=f"{instance.path}/").delete()
+            qs = FileSystem.objects.filter(team=self.team, path__startswith=f"{instance.path}/")
+            if self.user_access_control:
+                qs = self.user_access_control.filter_and_annotate_file_system_queryset(qs)


Interesting - what happens to things in it where they don't have access? This is just deleting the file system not the actual resource, right?

Yes, currently deletes are only at the filesystem level. It'll probably remain like this for a bit (as I'll be off for a week now), though the plan is to move it to the actual object level. If you wouldn't have access, those files would just linger on. Someone else with access will then see the folder with whatever is left.

zlwaterfield · 2025-04-09T13:53:14Z

posthog/rbac/user_access_control.py

@@ -444,6 +446,74 @@ def filter_queryset_by_access_level(self, queryset: QuerySet, include_all_if_adm

        return queryset

+    def filter_and_annotate_file_system_queryset(self, queryset: QuerySet["FileSystem"]) -> QuerySet["FileSystem"]:


You're creative 😉

This looks good to me

zlwaterfield · 2025-04-09T13:54:56Z

posthog/api/file_system.py

@@ -138,12 +139,18 @@ def safely_get_queryset(self, queryset: QuerySet) -> QuerySet:
        if self.action == "list":
            queryset = queryset.order_by("path")

+        if self.user_access_control:
+            queryset = self.user_access_control.filter_and_annotate_file_system_queryset(queryset)


These changes in the viewset look good but I do wonder if the access control filter should be moved into the model manager so you don't have to think about it. Something to ponder. I just worry if it needs to be used in every location then it could get forgotten.

I do wonder the same. I just added a count_by_path endpoint, and had to now merge with master and add the access control check manually.

I did think about making this check automatic, but decided against it because I wasn't 100% sure of the Django API here and of all the places to add it. I was afraid of accidentally missing something, and causing leakage tha way. I guess this is good enough for now?

Yeah good for now but worth relooking at.

Bascially just add a custom manager to the FileSystem model that overwrites the queryset. The tricky part would be making sure the manager has access to the user access control which it might not be possible.

Quick psuedo code:

class FileSystemManager(RootTeamManager): def get_queryset(self): qs = super().get_queryset() qs = self.user_access_control.filter_and_annotate_file_system_queryset(qs) return qs class FileSystem(models.Model): objects = FileSystemManager() ...

mariusandra

Thanks! 🙌

mariusandra · 2025-04-09T14:14:36Z

posthog/api/file_system.py

-            FileSystem.objects.filter(team=self.team, path__startswith=f"{instance.path}/").delete()
+            qs = FileSystem.objects.filter(team=self.team, path__startswith=f"{instance.path}/")
+            if self.user_access_control:
+                qs = self.user_access_control.filter_and_annotate_file_system_queryset(qs)


Yes, currently deletes are only at the filesystem level. It'll probably remain like this for a bit (as I'll be off for a week now), though the plan is to move it to the actual object level. If you wouldn't have access, those files would just linger on. Someone else with access will then see the folder with whatever is left.

mariusandra · 2025-04-09T14:24:15Z

posthog/api/file_system.py

@@ -138,12 +139,18 @@ def safely_get_queryset(self, queryset: QuerySet) -> QuerySet:
        if self.action == "list":
            queryset = queryset.order_by("path")

+        if self.user_access_control:
+            queryset = self.user_access_control.filter_and_annotate_file_system_queryset(queryset)


I do wonder the same. I just added a count_by_path endpoint, and had to now merge with master and add the access control check manually.

I did think about making this check automatic, but decided against it because I wasn't 100% sure of the Django API here and of all the places to add it. I was afraid of accidentally missing something, and causing leakage tha way. I guess this is good enough for now?

mariusandra and others added 3 commits April 8, 2025 14:45

feat(project-tree): rbac

4c3b8a3

skip import

2eb944e

Update query snapshots

721e328

zlwaterfield reviewed Apr 8, 2025

View reviewed changes

mariusandra added 2 commits April 8, 2025 18:17

add more checks

d9ef1c9

add admin test

888a2ab

mariusandra requested a review from zlwaterfield April 8, 2025 19:38

tests for other endpoints

0a20676

mariusandra marked this pull request as ready for review April 8, 2025 21:33

greptile-apps bot reviewed Apr 8, 2025

View reviewed changes

posthog/api/test/test_file_system.py Show resolved Hide resolved

posthog/rbac/test/test_user_access_control.py Show resolved Hide resolved

zlwaterfield approved these changes Apr 9, 2025

View reviewed changes

mariusandra added 3 commits April 9, 2025 16:03

Merge branch 'master' into project-tree-rbac

e44876f

add access control to one more endpoint

290a56f

remove todo line

d64f068

mariusandra commented Apr 9, 2025

View reviewed changes

mariusandra enabled auto-merge (squash) April 9, 2025 14:24

()

2044a39

mariusandra mentioned this pull request Apr 9, 2025

feat(project-tree): file-system api flag access #31001

Merged

mariusandra merged commit f617252 into master Apr 9, 2025
93 checks passed

mariusandra deleted the project-tree-rbac branch April 9, 2025 15:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(project-tree): rbac #30933

feat(project-tree): rbac #30933

mariusandra commented Apr 8, 2025 •

edited

Loading

zlwaterfield Apr 8, 2025

zlwaterfield Apr 8, 2025

mariusandra Apr 8, 2025

greptile-apps bot left a comment

zlwaterfield left a comment

zlwaterfield Apr 9, 2025

mariusandra Apr 9, 2025

zlwaterfield Apr 9, 2025

zlwaterfield Apr 9, 2025

mariusandra Apr 9, 2025

zlwaterfield Apr 9, 2025

mariusandra left a comment

mariusandra Apr 9, 2025

mariusandra Apr 9, 2025

		@@ -444,6 +446,74 @@ def filter_queryset_by_access_level(self, queryset: QuerySet, include_all_if_adm

		return queryset

		def filter_and_annotate_file_system_queryset(self, queryset: QuerySet["FileSystem"]) -> QuerySet["FileSystem"]:

feat(project-tree): rbac #30933

feat(project-tree): rbac #30933

Conversation

mariusandra commented Apr 8, 2025 • edited Loading

Problem

Changes

Small caveat

How did you test this code?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

zlwaterfield left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariusandra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariusandra commented Apr 8, 2025 •

edited

Loading