Merge pull request #2540 from getsentry/access-v-membership

Vary access and membership for teams

Author: dcramer

src/sentry/api/serializers/models/team.py

@@ -3,7 +3,6 @@
 import itertools
 
 from collections import defaultdict
-from django.conf import settings
 
 from sentry.app import env
 from sentry.api.serializers import Serializer, register, serialize
@@ -17,19 +16,7 @@
 class TeamSerializer(Serializer):
     def get_attrs(self, item_list, user):
         request = env.request
-        if (request.is_superuser() and request.user == user) or settings.SENTRY_PUBLIC:
-            inactive_memberships = frozenset(
-                OrganizationMemberTeam.objects.filter(
-                    team__in=item_list,
-                    organizationmember__user=user,
-                    is_active=False,
-                ).values_list('team', flat=True)
-            )
-            memberships = frozenset([
-                t.id for t in item_list
-                if t.id not in inactive_memberships
-            ])
-        elif user.is_authenticated():
+        if user.is_authenticated():
             memberships = frozenset(
                 OrganizationMemberTeam.objects.filter(
                     organizationmember__user=user,
@@ -50,11 +37,22 @@ def get_attrs(self, item_list, user):
         else:
             access_requests = frozenset()
 
+        is_superuser = request.is_superuser() and request.user == user
         result = {}
         for team in item_list:
+            is_member = team.id in memberships
+            if is_member:
+                has_access = True
+            elif is_superuser:
+                has_access = True
+            elif team.organization.flags.allow_joinleave:
+                has_access = True
+            else:
+                has_access = False
             result[team] = {
                 'pending_request': team.id in access_requests,
-                'is_member': team.id in memberships,
+                'is_member': is_member,
+                'has_access': has_access,
             }
         return result
 
@@ -65,6 +63,7 @@ def serialize(self, obj, attrs, user):
             'name': obj.name,
             'dateCreated': obj.date_added,
             'isMember': attrs['is_member'],
+            'hasAccess': attrs['has_access'],
             'isPending': attrs['pending_request'],
         }
 

src/sentry/auth/access.py

@@ -2,6 +2,8 @@
 
 __all__ = ['from_user', 'from_member', 'DEFAULT']
 
+import warnings
+
 from django.conf import settings
 
 from sentry.models import AuthIdentity, AuthProvider, OrganizationMember
@@ -10,7 +12,10 @@
 class BaseAccess(object):
     is_active = False
     sso_is_valid = False
+    # teams with valid access
     teams = ()
+    # teams with valid membership
+    memberships = ()
     scopes = frozenset()
 
     def has_scope(self, scope):
@@ -19,12 +24,22 @@ def has_scope(self, scope):
         return scope in self.scopes
 
     def has_team(self, team):
+        warnings.warn('has_team() is deprecated in favor of has_team_access',
+                      DeprecationWarning)
+        return self.has_team_access(team)
+
+    def has_team_access(self, team):
         if not self.is_active:
             return False
         return team in self.teams
 
+    def has_team_membership(self, team):
+        if not self.is_active:
+            return False
+        return team in self.memberships
+
     def has_team_scope(self, team, scope):
-        return self.has_team(team) and self.has_scope(scope)
+        return self.has_team_access(team) and self.has_scope(scope)
 
     def to_django_context(self):
         return {
@@ -37,8 +52,9 @@ class Access(BaseAccess):
     # TODO(dcramer): this is still a little gross, and ideally backend access
     # would be based on the same scopes as API access so theres clarity in
     # what things mean
-    def __init__(self, scopes, is_active, teams, sso_is_valid):
+    def __init__(self, scopes, is_active, teams, memberships, sso_is_valid):
         self.teams = teams
+        self.memberships = memberships
         self.scopes = scopes
 
         self.is_active = is_active
@@ -50,10 +66,12 @@ def from_request(request, organization):
         return DEFAULT
 
     if request.is_superuser():
+        team_list = list(organization.team_set.all())
         return Access(
             scopes=settings.SENTRY_SCOPES,
             is_active=True,
-            teams=organization.team_set.all(),
+            teams=team_list,
+            memberships=team_list,
             sso_is_valid=True,
         )
     return from_user(request.user, organization)
@@ -74,6 +92,9 @@ def from_user(user, organization):
     except OrganizationMember.DoesNotExist:
         return DEFAULT
 
+    # ensure cached relation
+    om.organization = organization
+
     return from_member(om)
 
 
@@ -100,11 +121,18 @@ def from_member(member):
             else:
                 sso_is_valid = auth_identity.is_valid(member)
 
+    team_memberships = member.get_teams()
+    if member.organization.flags.allow_joinleave:
+        team_access = list(member.organization.team_set.all())
+    else:
+        team_access = team_memberships
+
     return Access(
         is_active=True,
         sso_is_valid=sso_is_valid,
         scopes=member.get_scopes(),
-        teams=member.get_teams(),
+        memberships=team_memberships,
+        teams=team_access,
     )
 
 
@@ -121,6 +149,10 @@ def is_active(self):
     def teams(self):
         return ()
 
+    @property
+    def memberships(self):
+        return ()
+
     @property
     def scopes(self):
         return frozenset()

src/sentry/static/sentry/app/views/projectDetails.jsx

@@ -91,9 +91,9 @@ const ProjectDetails = React.createClass({
       return;
     }
     let [activeTeam, activeProject] = this.identifyProject();
-    let isMember = activeTeam && activeTeam.isMember;
+    let hasAccess = activeTeam && activeTeam.hasAccess;
 
-    if (activeProject && isMember) {
+    if (activeProject && hasAccess) {
       // TODO(dcramer): move member list to organization level
       this.api.request(this.getMemberListEndpoint(), {
         success: (data) => {
@@ -108,7 +108,7 @@ const ProjectDetails = React.createClass({
         error: false,
         errorType: null
       });
-    } else if (isMember === false) {
+    } else if (activeTeam && activeTeam.isMember) {
       this.setState({
         project: activeProject,
         team: activeTeam,

src/sentry/testutils/cases.py

@@ -276,7 +276,10 @@ class PermissionTestCase(TestCase):
     def setUp(self):
         super(PermissionTestCase, self).setUp()
         self.owner = self.create_user(is_superuser=False)
-        self.organization = self.create_organization(owner=self.owner)
+        self.organization = self.create_organization(
+            owner=self.owner,
+            flags=0,  # disable default allow_joinleave access
+        )
         self.team = self.create_team(organization=self.organization)
 
     def assert_can_access(self, user, path, method='GET'):

tests/sentry/api/endpoints/test_organization_access_request_details.py

@@ -112,10 +112,14 @@ def test_team_admin_can_approve(self):
 
         assert resp.status_code == 204
 
-    def test_teamless_admin_cannot_approve(self):
+    def test_teamless_admin_cannot_approve_with_closed_membership(self):
         self.login_as(user=self.user)
 
-        organization = self.create_organization(name='foo', owner=self.user)
+        organization = self.create_organization(
+            name='foo',
+            owner=self.user,
+            flags=0,  # kill allow_joinleave
+        )
         user = self.create_user('[email protected]')
         member = self.create_member(
             organization=organization,

tests/sentry/auth/test_access.py

@@ -4,7 +4,7 @@
 from mock import Mock
 
 from sentry.auth import access
-from sentry.models import AuthProvider
+from sentry.models import AuthProvider, Organization
 from sentry.testutils import TestCase
 
 
@@ -18,7 +18,8 @@ def test_no_access(self):
         assert not result.is_active
         assert result.sso_is_valid
         assert not result.scopes
-        assert not result.has_team(team)
+        assert not result.has_team_access(team)
+        assert not result.has_team_membership(team)
 
     def test_owner_all_teams(self):
         user = self.create_user()
@@ -33,11 +34,15 @@ def test_owner_all_teams(self):
         assert result.is_active
         assert result.sso_is_valid
         assert result.scopes == member.get_scopes()
-        assert result.has_team(team)
+        assert result.has_team_access(team)
+        assert result.has_team_membership(team)
 
-    def test_member_no_teams(self):
+    def test_member_no_teams_closed_membership(self):
         user = self.create_user()
-        organization = self.create_organization(owner=self.user)
+        organization = self.create_organization(
+            owner=self.user,
+            flags=0,  # disable default allow_joinleave
+        )
         member = self.create_member(
             organization=organization, user=user,
             role='member',
@@ -48,7 +53,27 @@ def test_member_no_teams(self):
         assert result.is_active
         assert result.sso_is_valid
         assert result.scopes == member.get_scopes()
-        assert not result.has_team(team)
+        assert not result.has_team_access(team)
+        assert not result.has_team_membership(team)
+
+    def test_member_no_teams_open_membership(self):
+        user = self.create_user()
+        organization = self.create_organization(
+            owner=self.user,
+            flags=Organization.flags.allow_joinleave,
+        )
+        member = self.create_member(
+            organization=organization, user=user,
+            role='member', teams=(),
+        )
+        team = self.create_team(organization=organization)
+
+        result = access.from_user(user, organization)
+        assert result.is_active
+        assert result.sso_is_valid
+        assert result.scopes == member.get_scopes()
+        assert result.has_team_access(team)
+        assert not result.has_team_membership(team)
 
     def test_team_restricted_org_member_access(self):
         user = self.create_user()
@@ -64,7 +89,8 @@ def test_team_restricted_org_member_access(self):
         assert result.is_active
         assert result.sso_is_valid
         assert result.scopes == member.get_scopes()
-        assert result.has_team(team)
+        assert result.has_team_access(team)
+        assert result.has_team_membership(team)
 
     def test_unlinked_sso(self):
         user = self.create_user()
@@ -106,4 +132,5 @@ def test_no_access(self):
         assert not result.is_active
         assert result.sso_is_valid
         assert not result.scopes
-        assert not result.has_team(Mock())
+        assert not result.has_team_access(Mock())
+        assert not result.has_team_membership(Mock())

tests/sentry/web/frontend/test_organization_members.py

@@ -58,7 +58,11 @@ def test_renders_with_context(self):
         ]
 
     def test_shows_access_requests_for_team_admin(self):
-        organization = self.create_organization(name='foo', owner=self.user)
+        organization = self.create_organization(
+            name='foo',
+            owner=self.user,
+            flags=0,  # kill default allow_joinleave
+        )
         team_1 = self.create_team(name='foo', organization=organization)
         team_2 = self.create_team(name='bar', organization=organization)