QuerySets: filter permissions by organizations #8298

stsewd · 2021-06-29T22:17:07Z

With the aim to bring logic from .com to .org more closer
I'm moving the organization's filters to .org.
They are going to be used if RTD_ALLOW_ORGANIZATIONS is True,
but they eventually will be merged with the current querysets
when organizations and normal projects are supported in .org.

In .com we are relying on user.projects having the
the projects where the user is member of and organization owner
(we are syncing this with signals).
We don't rely on that hack anymore and always check from the
organization models. The signals won't be removed for now,
but shouldn't be needed anymore.

To re-use more code, I have brought the SSO concept here,
but isn't implemented yet, we can bring the SSO models and logic later
easily.

This could be seen as more queries,
but the main ones were already being executed in .com (Project/VersionQueryset).
The other ones are executed only when using the API V2,
so I don't think that would be a problem.

I started this refactor using a mixin,
but then we would need to override every single
queryset in .com, using composition only requires
one override (already used),
and doesn't bring other methods into the class.

With the aim to bring logic from .com to .org more closer I'm moving the organization's filters to .org. They are going to be used if `RTD_ALLOW_ORGANIZATIONS` is True, but they eventually will be merged with the current querysets when organizations and normal projects are supported in .org. In .com we are relying on user.projects having the the projects where the user is member of and organization owner (we are syncing this with signals). We don't rely on that hack anymore and always check from the organization models. The signals won't be removed for now, but shouldn't be needed anymore. To re-use more code I have brought the SSO concept here, but isn't implemented yet, we can bring the SSO models and logic later easily. This could be seen as more queries, but the main ones were already being executed in .com. The other ones are executed only when using the API V2, so I don't think that would be a problem. I started this refactor using a mixin, but then we would need to override every single queryset in .com, using composition only requires one override (already used), and doesn't bring other methods into the class.

readthedocs/builds/querysets.py

readthedocs/core/permissions.py

stsewd · 2021-06-29T22:20:20Z

readthedocs/settings/base.py

@@ -610,6 +610,7 @@ def DOCKER_LIMITS(self):
    DEFAULT_VERSION_PRIVACY_LEVEL = 'public'
    GROK_API_HOST = 'https://api.grokthedocs.com'
    ALLOW_ADMIN = True
+    RTD_ALLOW_ORGANIZATIONS = False


This setting will change to RTD_ALLOW_ONLY_ORGANIZATONS at some point when we support both single projects and organizations in .org :)

stsewd · 2021-06-29T22:27:50Z

readthedocs/core/permissions.py

+    def _has_sso_enabled(cls, organization):
+        return False
+
+    @classmethod
+    def _get_projects_for_sso_user(cls, user, admin=False):
+        from readthedocs.projects.models import Project
+        return Project.objects.none()


don't really like having these as placeholders, but was this or repeat the whole thing in .com or block this till we move the vcs integration.

Why don't bring CorporateAdminPermission._get_projects_for_sso_user here and use the setting that you just added RTD_ALLOW_ORGANIZATIONS to decide what to do?

In any case, if we are already planning to change the content of this method, I'd like to see a TODO: list explaining the plan here and what's the code we need to bring here.

Why don't bring CorporateAdminPermission._get_projects_for_sso_user here and use the setting that you just added RTD_ALLOW_ORGANIZATIONS to decide what to do?

It has references to models that we don't have here.

I'd like to see a TODO: list explaining the plan here and what's the code we need to bring here.

This is already overriden in .com.

Now that we have the models in community, we can move the code from corporate here and use the RTD_ALLOW_ORGANIZATIONS as suggested, right? That way we will have all the code in the same place and once we enable organization in community it will just work.

IMO, the less overrides, the better.

humitos

I like this change. Using more AdminPermission is good. Hopefully, we end having all the permission login in one place someday 😄

I'd like to see the changes I'm asking for in the comments before merging this. It will be hard to re-review this code again once we have SSO working on .org. I prefer to call the methods/functions as we expect to be called in the future instead of having to come back here to add an attribute (e.g. provider= in has_sso_enabled)

Finally, seems weird to read "This code was copied from ..." but then find there are changes in the code. It doesn't feel trustable.

readthedocs/core/permissions.py

humitos · 2021-06-30T09:52:17Z

readthedocs/core/permissions.py

@@ -46,4 +114,3 @@ def is_member(cls, user, obj):

 class AdminPermission(SettingsOverrideObject):


nit: would be good to rename this to Permission instead of AdminPermission at some point since it's not related to admin anymore.

readthedocs/core/permissions.py

humitos · 2021-06-30T10:17:05Z

readthedocs/core/permissions.py

+    def _has_sso_enabled(cls, organization):
+        return False
+
+    @classmethod
+    def _get_projects_for_sso_user(cls, user, admin=False):
+        from readthedocs.projects.models import Project
+        return Project.objects.none()


Why don't bring CorporateAdminPermission._get_projects_for_sso_user here and use the setting that you just added RTD_ALLOW_ORGANIZATIONS to decide what to do?

humitos · 2021-06-30T10:17:49Z

readthedocs/core/permissions.py

+    def _has_sso_enabled(cls, organization):
+        return False
+
+    @classmethod
+    def _get_projects_for_sso_user(cls, user, admin=False):
+        from readthedocs.projects.models import Project
+        return Project.objects.none()


In any case, if we are already planning to change the content of this method, I'd like to see a TODO: list explaining the plan here and what's the code we need to bring here.

stsewd · 2021-07-27T01:01:43Z

readthedocs/core/permissions.py

+    @classmethod
+    def has_sso_enabled(cls, obj, provider=None):
+        return False
+
+    @classmethod
+    def _get_projects_for_sso_user(cls, user, admin=False):
+        from readthedocs.projects.models import Project
+        return Project.objects.none()


These two depend on the SSO implementation, they are overridden on .com.

humitos · 2021-08-18T10:02:48Z

This is looking good. I ask a question at #8298 (comment) because I think it makes sense to bring that corporate code here at this point before merging.

stsewd · 2021-08-19T21:44:51Z

@humitos didn't move those because it includes very specific code related to the implementation, and it also includes checks for google, which we won't have in .org I think.

ericholscher · 2021-08-31T22:30:07Z

Final review waiting on @humitos 👍

humitos

I'm fine merging this, but I still think is worth removing the override from corporate on those methods as I mentioned in my previous comment.

These are the methods that are overwritten from corporate: https://github.com/readthedocs/readthedocs-corporate/pull/1232/files#diff-d040f1c8c0a21fef1c927a0070abb1f47b46df75075141fba8af6bd0acd7feb0R37

and I think we can add an if settings.RTD_ALLOW_ORGANIZATIONS in community to execute one or the other. That way, we are removing an override and reducing the complexity while sharing more code between instances. I don't clearly see why we can't do this. Besides, once we enable GH SSO on community, it will just work.

Google provider check won't be a problem since there won't be organizations with it in community, so I will be just skipped.

stsewd commented Jun 29, 2021

View reviewed changes

readthedocs/builds/querysets.py Show resolved Hide resolved

stsewd commented Jun 29, 2021

View reviewed changes

readthedocs/core/permissions.py Show resolved Hide resolved

stsewd commented Jun 29, 2021

View reviewed changes

stsewd requested a review from a team June 29, 2021 22:26

stsewd commented Jun 29, 2021

View reviewed changes

stsewd mentioned this pull request Jun 29, 2021

QuerySets: remove unused overrides #8299

Merged

humitos requested changes Jun 30, 2021

View reviewed changes

Update docstring

9caa36d

stsewd requested a review from humitos June 30, 2021 14:57

stsewd added 4 commits July 26, 2021 17:56

Merge branch 'master' into take-orgs-into-consideration

323164e

Use SSO models

ae9a1fe

Merge branch 'master' into take-orgs-into-consideration

f717967

Fix circular import

0dcf741

stsewd force-pushed the take-orgs-into-consideration branch from c790d98 to 0dcf741 Compare July 27, 2021 00:20

Fix merge

07a30c4

stsewd commented Jul 27, 2021

View reviewed changes

Merge branch 'master' into take-orgs-into-consideration

3d49de1

Merge branch 'master' into take-orgs-into-consideration

f16251a

stsewd requested a review from a team August 30, 2021 14:58

humitos approved these changes Sep 1, 2021

View reviewed changes

Merge branch 'master' into take-orgs-into-consideration

b8ca408

stsewd merged commit 7f4215f into master Sep 1, 2021

stsewd deleted the take-orgs-into-consideration branch September 1, 2021 21:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QuerySets: filter permissions by organizations #8298

QuerySets: filter permissions by organizations #8298

stsewd commented Jun 29, 2021

stsewd Jun 29, 2021

stsewd Jun 29, 2021

humitos Jun 30, 2021

humitos Jun 30, 2021

stsewd Jun 30, 2021

humitos Aug 18, 2021 •

edited

humitos left a comment

humitos Jun 30, 2021

humitos Jun 30, 2021

humitos Jun 30, 2021

stsewd Jul 27, 2021

humitos commented Aug 18, 2021

stsewd commented Aug 19, 2021

ericholscher commented Aug 31, 2021

humitos left a comment

		@@ -46,4 +114,3 @@ def is_member(cls, user, obj):

		class AdminPermission(SettingsOverrideObject):

QuerySets: filter permissions by organizations #8298

QuerySets: filter permissions by organizations #8298

Conversation

stsewd commented Jun 29, 2021

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

humitos Aug 18, 2021 • edited

Choose a reason for hiding this comment

humitos 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

humitos commented Aug 18, 2021

stsewd commented Aug 19, 2021

ericholscher commented Aug 31, 2021

humitos left a comment

Choose a reason for hiding this comment

humitos Aug 18, 2021 •

edited