From 5085a8729deb2fbce3fe8f1cfe3b9054c0dcb551 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 11 Jun 2020 19:58:09 +0200 Subject: [PATCH 1/8] Delete old RemoteRepository when syncing If a user delete a repository or if their permissions were removed from that repository, we delete our local RemoteRepository instance tied to that User. --- readthedocs/oauth/services/github.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/readthedocs/oauth/services/github.py b/readthedocs/oauth/services/github.py index 670becdf921..e71fa10b4e7 100644 --- a/readthedocs/oauth/services/github.py +++ b/readthedocs/oauth/services/github.py @@ -6,6 +6,8 @@ from allauth.socialaccount.models import SocialToken from allauth.socialaccount.providers.github.views import GitHubOAuth2Adapter + +from django.db.models import Q from django.conf import settings from django.urls import reverse from requests.exceptions import RequestException @@ -51,6 +53,13 @@ def sync_repositories(self): 'try reconnecting your account' ) + # Delete RemoteRepository where the user doesn't have access anymore + # (skip RemoteRepository tied to a Project on this user) + full_names = {repo.get('full_name') for repo in repos} + self.user.oauth_repositories.exclude( + Q(full_name__in=full_names) | Q(project__isnull=False) + ).delete() + def sync_organizations(self): """Sync organizations from GitHub API.""" try: From c001f07216c57c67d3f6914611c98d91d4d613e5 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 15 Jun 2020 16:25:19 +0200 Subject: [PATCH 2/8] Make SSO users member of organization that has SSO integration on --- readthedocs/organizations/models.py | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/readthedocs/organizations/models.py b/readthedocs/organizations/models.py index ffa102c66ce..107454ff21a 100644 --- a/readthedocs/organizations/models.py +++ b/readthedocs/organizations/models.py @@ -1,6 +1,7 @@ """Organizations models.""" from autoslug import AutoSlugField +from django.conf import settings from django.contrib.auth.models import User from django.db import models from django.db.models import Q @@ -93,7 +94,23 @@ def get_absolute_url(self): @property def users(self): - return (self.members.all() | self.owners.all().distinct()).distinct() + sso_users = User.objects.none() + # TODO: find another way to check if we are under corporate site + if settings.ALLOW_PRIVATE_REPOS: + from readthedocsinc.acl.sso.models import SSOIntegration + if SSOIntegration.objects.filter(organization=self).exists(): + # TODO: use RemoteRepository.remote_id instead of full_name + full_names = self.projects.values('remote_repository__full_name') + sso_users = User.objects.filter(oauth_repositories__full_name__in=full_names).distinct() + + return ( + # Members + self.members.all() | + # Owners + self.owners.all().distinct() | + # SSO Users + sso_users + ).distinct() @property def members(self): From 24d290e68436e760ae1270ae12b4a3cddbad97f0 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 22 Jun 2020 17:01:59 +0200 Subject: [PATCH 3/8] Lint --- readthedocs/organizations/models.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/readthedocs/organizations/models.py b/readthedocs/organizations/models.py index 107454ff21a..6365e3e0567 100644 --- a/readthedocs/organizations/models.py +++ b/readthedocs/organizations/models.py @@ -97,11 +97,13 @@ def users(self): sso_users = User.objects.none() # TODO: find another way to check if we are under corporate site if settings.ALLOW_PRIVATE_REPOS: - from readthedocsinc.acl.sso.models import SSOIntegration + from readthedocsinc.acl.sso.models import SSOIntegration # noqa if SSOIntegration.objects.filter(organization=self).exists(): # TODO: use RemoteRepository.remote_id instead of full_name full_names = self.projects.values('remote_repository__full_name') - sso_users = User.objects.filter(oauth_repositories__full_name__in=full_names).distinct() + sso_users = User.objects.filter( + oauth_repositories__full_name__in=full_names, + ).distinct() return ( # Members From 619ef518546d7568fbc926c1eb7924aa869ea4a2 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 23 Jun 2020 11:58:34 +0200 Subject: [PATCH 4/8] Delete old RemoteRepository when syncing GitHub service --- readthedocs/oauth/services/github.py | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/readthedocs/oauth/services/github.py b/readthedocs/oauth/services/github.py index e71fa10b4e7..64cd95994a8 100644 --- a/readthedocs/oauth/services/github.py +++ b/readthedocs/oauth/services/github.py @@ -37,8 +37,15 @@ class GitHubService(Service): def sync(self): """Sync repositories and organizations.""" - self.sync_repositories() - self.sync_organizations() + repos = self.sync_repositories() + organization_repos = self.sync_organizations() + + # Delete RemoteRepository where the user doesn't have access anymore + # (skip RemoteRepository tied to a Project on this user) + full_names = {repo.get('full_name') for repo in repos + organization_repos} + self.user.oauth_repositories.exclude( + Q(full_name__in=full_names) | Q(project__isnull=False) + ).delete() def sync_repositories(self): """Sync repositories from GitHub API.""" @@ -46,6 +53,7 @@ def sync_repositories(self): try: for repo in repos: self.create_repository(repo) + return repos except (TypeError, ValueError): log.warning('Error syncing GitHub repositories') raise SyncServiceError( @@ -53,13 +61,6 @@ def sync_repositories(self): 'try reconnecting your account' ) - # Delete RemoteRepository where the user doesn't have access anymore - # (skip RemoteRepository tied to a Project on this user) - full_names = {repo.get('full_name') for repo in repos} - self.user.oauth_repositories.exclude( - Q(full_name__in=full_names) | Q(project__isnull=False) - ).delete() - def sync_organizations(self): """Sync organizations from GitHub API.""" try: @@ -74,6 +75,7 @@ def sync_organizations(self): ) for repo in org_repos: self.create_repository(repo, organization=org_obj) + return org_repos except (TypeError, ValueError): log.warning('Error syncing GitHub organizations') raise SyncServiceError( From 2d9845d088a28a3b8210e0544e2345cb592a145e Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 23 Jun 2020 12:36:10 +0200 Subject: [PATCH 5/8] Remove Organization (.members and .users) to avoid confusion These methods came from corporate and we are not really using them in community. Now that corporate is implementing SSO, we need to make some modification on them and we can't include them here. We could re-add them once SSO is brought to community or we can implement them in a different class that we can override with our SettingsOverrideObject method. --- readthedocs/organizations/models.py | 29 ----------------------------- 1 file changed, 29 deletions(-) diff --git a/readthedocs/organizations/models.py b/readthedocs/organizations/models.py index 6365e3e0567..603227448ee 100644 --- a/readthedocs/organizations/models.py +++ b/readthedocs/organizations/models.py @@ -92,35 +92,6 @@ def __str__(self): def get_absolute_url(self): return reverse('organization_detail', args=(self.slug,)) - @property - def users(self): - sso_users = User.objects.none() - # TODO: find another way to check if we are under corporate site - if settings.ALLOW_PRIVATE_REPOS: - from readthedocsinc.acl.sso.models import SSOIntegration # noqa - if SSOIntegration.objects.filter(organization=self).exists(): - # TODO: use RemoteRepository.remote_id instead of full_name - full_names = self.projects.values('remote_repository__full_name') - sso_users = User.objects.filter( - oauth_repositories__full_name__in=full_names, - ).distinct() - - return ( - # Members - self.members.all() | - # Owners - self.owners.all().distinct() | - # SSO Users - sso_users - ).distinct() - - @property - def members(self): - """Return members as an aggregate over all organization teams.""" - return User.objects.filter( - Q(teams__organization=self) | Q(owner_organizations=self), - ).distinct() - def save(self, *args, **kwargs): # pylint: disable=arguments-differ if not self.slug: self.slug = slugify(self.name) From bab2b7f9a0eae5b84b783f892a7ba247bf649996 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 23 Jun 2020 12:43:32 +0200 Subject: [PATCH 6/8] Remove not used import --- readthedocs/organizations/models.py | 1 - 1 file changed, 1 deletion(-) diff --git a/readthedocs/organizations/models.py b/readthedocs/organizations/models.py index 603227448ee..9ce691bb403 100644 --- a/readthedocs/organizations/models.py +++ b/readthedocs/organizations/models.py @@ -1,7 +1,6 @@ """Organizations models.""" from autoslug import AutoSlugField -from django.conf import settings from django.contrib.auth.models import User from django.db import models from django.db.models import Q From 83c42f4b09716caea5319c398c120c7cdcef15d2 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 23 Jun 2020 13:14:31 +0200 Subject: [PATCH 7/8] Use AdminPermission class for Organization (.users and .members) With this pattern we can override them from Corporate using the same method we already have, without confusions. Note that .users and .members are exactly the same logic. --- readthedocs/core/permissions.py | 30 ++++++++++++++++++++++++++--- readthedocs/organizations/models.py | 9 +++++++++ 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/readthedocs/core/permissions.py b/readthedocs/core/permissions.py index a3bfa04235c..cfac043f23f 100644 --- a/readthedocs/core/permissions.py +++ b/readthedocs/core/permissions.py @@ -8,14 +8,38 @@ class AdminPermissionBase: @classmethod - def is_admin(cls, user, project): + def admins(cls, obj): + from readthedocs.projects.models import Project + from readthedocs.organizations.models import Organization + + if isinstance(obj, Project): + return project.users.all() + + if isinstance(obj, Organization): + return obj.owners.all() + + @classmethod + def members(cls, obj): + from readthedocs.projects.models import Project + from readthedocs.organizations.models import Organization + + if isinstance(obj, Project): + return project.users.all() + + if isinstance(obj, Organization): + return User.objects.filter( + Q(teams__organization=obj) | Q(owner_organizations=obj), + ).distinct() + + @classmethod + def is_admin(cls, user, obj): # This explicitly uses "user in project.users.all" so that # users on projects can be cached using prefetch_related or prefetch_related_objects - return user in project.users.all() or user.is_superuser + return user in cls.admins(obj) or user.is_superuser @classmethod def is_member(cls, user, obj): - return user in obj.users.all() or user.is_superuser + return user in cls.members(obj) or user.is_superuser class AdminPermission(SettingsOverrideObject): diff --git a/readthedocs/organizations/models.py b/readthedocs/organizations/models.py index 9ce691bb403..5a1c9c4567f 100644 --- a/readthedocs/organizations/models.py +++ b/readthedocs/organizations/models.py @@ -9,6 +9,7 @@ from django.utils.translation import ugettext_lazy as _ from readthedocs.core.utils import slugify +from readthedocs.core.permissions import AdminPermission from . import constants from .managers import TeamManager, TeamMemberManager @@ -91,6 +92,14 @@ def __str__(self): def get_absolute_url(self): return reverse('organization_detail', args=(self.slug,)) + @property + def users(self): + return AdminPermission.members(self) + + @property + def members(self): + return AdminPermission.members(self) + def save(self, *args, **kwargs): # pylint: disable=arguments-differ if not self.slug: self.slug = slugify(self.name) From 8c354f6b7ced3932fe24dd0bdc6c1f9733f56b6b Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 23 Jun 2020 16:33:21 +0200 Subject: [PATCH 8/8] Lint and small fixes --- readthedocs/core/permissions.py | 6 ++++-- readthedocs/organizations/models.py | 1 - 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/readthedocs/core/permissions.py b/readthedocs/core/permissions.py index cfac043f23f..0da4af85f84 100644 --- a/readthedocs/core/permissions.py +++ b/readthedocs/core/permissions.py @@ -1,6 +1,8 @@ # -*- coding: utf-8 -*- """Objects for User permission checks.""" +from django.contrib.auth.models import User +from django.db.models import Q from readthedocs.core.utils.extend import SettingsOverrideObject @@ -13,7 +15,7 @@ def admins(cls, obj): from readthedocs.organizations.models import Organization if isinstance(obj, Project): - return project.users.all() + return obj.users.all() if isinstance(obj, Organization): return obj.owners.all() @@ -24,7 +26,7 @@ def members(cls, obj): from readthedocs.organizations.models import Organization if isinstance(obj, Project): - return project.users.all() + return obj.users.all() if isinstance(obj, Organization): return User.objects.filter( diff --git a/readthedocs/organizations/models.py b/readthedocs/organizations/models.py index 5a1c9c4567f..7b5f909184f 100644 --- a/readthedocs/organizations/models.py +++ b/readthedocs/organizations/models.py @@ -3,7 +3,6 @@ from autoslug import AutoSlugField from django.contrib.auth.models import User from django.db import models -from django.db.models import Q from django.urls import reverse from django.utils.crypto import salted_hmac from django.utils.translation import ugettext_lazy as _