Skip to content

Commit

Permalink
feat(organizations)!: Restrict project usage access to admins and org…
Browse files Browse the repository at this point in the history
…anization owners (#5333)

### 📣 Summary
Restricted access to project usage data to admins and organization
owners only.


### 📖 Description
This PR introduces stricter access control for project usage data,
limiting visibility and management to admins and organization owners.
This change ensures that only authorized users can access and manage
usage metrics, protecting organizational data from unauthorized access.
  • Loading branch information
noliveleger authored Dec 9, 2024
1 parent 3f471dc commit 0bb717d
Show file tree
Hide file tree
Showing 11 changed files with 128 additions and 135 deletions.
34 changes: 22 additions & 12 deletions kobo/apps/organizations/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,21 +30,31 @@ def has_object_permission(self, request, view, obj):


class HasOrgRolePermission(IsOrgAdminPermission):

def has_object_permission(self, request, view, obj):
if super().has_object_permission(request, view, obj):
return True
return request.method in permissions.SAFE_METHODS


class OrganizationNestedHasOrgRolePermission(HasOrgRolePermission):
def has_permission(self, request, view):

if not super().has_permission(request, view):
return False

organization = Organization.objects.filter(
id=view.kwargs.get('organization_id')
).first()
if organization and not self.has_object_permission(
request, view, organization
):
return False
return True
try:
organization = Organization.objects.get(
id=view.kwargs.get('organization_id')
)
except Organization.DoesNotExist:
raise Http404

return super().has_object_permission(request, view, organization)

def has_object_permission(self, request, view, obj):
obj = obj if isinstance(obj, Organization) else obj.organization
if super().has_object_permission(request, view, obj):
return True
return request.method in permissions.SAFE_METHODS
"""
The object check is always performed on the parent (organization) and
is validated in `has_permission()`. Therefore, this method always returns True.
"""
return True
64 changes: 64 additions & 0 deletions kobo/apps/organizations/tests/test_organizations_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,70 @@ def test_api_response_includes_is_mmo_with_override_and_subscription(
self.assertEqual(response.data['is_mmo'], True)


@ddt
class OrganizationDetailAPITestCase(BaseTestCase):

fixtures = ['test_data']
URL_NAMESPACE = URL_NAMESPACE

def setUp(self):
self.someuser = User.objects.get(username='someuser')
self.organization = self.someuser.organization
self.organization.mmo_override = True
self.organization.save(update_fields=['mmo_override'])

# anotheruser is an admin of someuser's organization
self.anotheruser = User.objects.get(username='anotheruser')
self.organization.add_user(self.anotheruser, is_admin=True)

# alice is a regular member of someuser's organization
self.alice = User.objects.create_user(
username='alice', password='alice', email='[email protected]'
)
self.organization.add_user(self.alice, is_admin=False)

# bob is external to someuser's organization
self.bob = User.objects.create_user(
username='bob', password='bob', email='[email protected]'
)

@data(
('someuser', status.HTTP_200_OK),
('anotheruser', status.HTTP_200_OK),
('alice', status.HTTP_403_FORBIDDEN),
('bob', status.HTTP_404_NOT_FOUND),
)
@unpack
def test_asset_usage(self, username, expected_status_code):
user = User.objects.get(username=username)
self.client.force_login(user)

url = reverse(
self._get_endpoint('organizations-asset-usage'),
kwargs={'id': self.organization.id}
)
response = self.client.get(url)
assert response.status_code == expected_status_code

@data(
('someuser', status.HTTP_200_OK),
('anotheruser', status.HTTP_200_OK),
('alice', status.HTTP_200_OK),
('bob', status.HTTP_404_NOT_FOUND),
)
@unpack
def test_service_usage(self, username, expected_status_code):
user = User.objects.get(username=username)
self.client.force_login(user)

url = reverse(
self._get_endpoint('organizations-service-usage'),
kwargs={'id': self.organization.id}
)
response = self.client.get(url)
assert response.status_code == expected_status_code


class BaseOrganizationAssetApiTestCase(BaseAssetTestCase):
"""
This test suite (e.g. classes which inherit from this one) does not cover
Expand Down
58 changes: 18 additions & 40 deletions kobo/apps/organizations/views.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
from django.conf import settings
from django.contrib.postgres.aggregates import ArrayAgg
from django.db.models import (
QuerySet,
Case,
Expand All @@ -10,7 +8,7 @@
)
from django.db.models.expressions import Exists
from django.utils.http import http_date
from rest_framework import status, viewsets
from rest_framework import viewsets
from rest_framework.decorators import action
from rest_framework.request import Request
from rest_framework.response import Response
Expand All @@ -26,10 +24,13 @@
from kpi.utils.object_permission import get_database_user
from kpi.views.v2.asset import AssetViewSet
from .models import Organization, OrganizationOwner, OrganizationUser
from .permissions import HasOrgRolePermission, IsOrgAdminPermission
from .permissions import (
HasOrgRolePermission,
IsOrgAdminPermission,
OrganizationNestedHasOrgRolePermission,
)
from .serializers import OrganizationSerializer, OrganizationUserSerializer
from ..accounts.mfa.models import MfaMethod
from ..stripe.constants import ACTIVE_STRIPE_STATUSES


class OrganizationAssetViewSet(AssetViewSet):
Expand Down Expand Up @@ -74,7 +75,7 @@ class OrganizationViewSet(viewsets.ModelViewSet):
"""
Organizations are groups of users with assigned permissions and configurations
- Organization admins can manage the organization and it's membership
- Organization admins can manage the organization and its membership
- Connect to authentication mechanisms and enforce policy
- Create teams and projects under the organization
"""
Expand Down Expand Up @@ -153,16 +154,13 @@ def service_usage(self, request, pk=None, *args, **kwargs):
> }
### CURRENT ENDPOINT
"""

context = {
'organization_id': kwargs.get('id', None),
**self.get_serializer_context(),
}
self.get_object() # check permissions

serializer = ServiceUsageSerializer(
get_database_user(request.user),
context=context,
context=self.get_serializer_context(),
)

response = Response(
data=serializer.data,
headers={
Expand All @@ -172,7 +170,7 @@ def service_usage(self, request, pk=None, *args, **kwargs):

return response

@action(detail=True, methods=['get'])
@action(detail=True, methods=['get'], permission_classes=[IsOrgAdminPermission])
def asset_usage(self, request, pk=None, *args, **kwargs):
"""
## Organization Asset Usage Tracker
Expand Down Expand Up @@ -212,31 +210,11 @@ def asset_usage(self, request, pk=None, *args, **kwargs):
### CURRENT ENDPOINT
"""

org_id = kwargs.get('id', None)
# Check if the organization exists and if the user is the owner
try:
organization = Organization.objects.prefetch_related('organization_users__user').filter(
id=org_id, owner__organization_user__user_id=request.user.id,
).annotate(
user_ids=ArrayAgg('organization_users__user_id')
)[0]
except IndexError:
return Response(
{'error': 'There was a problem finding the organization.'},
status=status.HTTP_400_BAD_REQUEST,
)
organization = self.get_object()

# default to showing all users for this org
asset_users = organization.user_ids
# if Stripe is enabled, check that the org is on a plan that supports Organization features
if settings.STRIPE_ENABLED and not Organization.objects.filter(
id=org_id,
djstripe_customers__subscriptions__status__in=ACTIVE_STRIPE_STATUSES,
djstripe_customers__subscriptions__items__price__product__metadata__has_key='plan_type',
djstripe_customers__subscriptions__items__price__product__metadata__plan_type='enterprise',
).exists():
# No active subscription that supports multiple users, just get this user's data
asset_users = [request.user.id]
user_id = get_database_user(request.user).pk
if organization.is_mmo:
user_id = organization.owner_user_object.pk

assets = (
Asset.objects.only(
Expand All @@ -247,7 +225,7 @@ def asset_usage(self, request, pk=None, *args, **kwargs):
)
.select_related('owner')
.filter(
owner__in=asset_users,
owner_id=user_id,
asset_type=ASSET_TYPE_SURVEY,
)
)
Expand Down Expand Up @@ -418,7 +396,7 @@ class OrganizationMemberViewSet(viewsets.ModelViewSet):
in updates.
"""
serializer_class = OrganizationUserSerializer
permission_classes = [HasOrgRolePermission]
permission_classes = [OrganizationNestedHasOrgRolePermission]
http_method_names = ['get', 'patch', 'delete']
lookup_field = 'user__username'

Expand All @@ -437,7 +415,7 @@ def get_queryset(self):
organization_user=OuterRef('pk')
).values('pk')

# Annotate with role based on organization ownership and admin status
# Annotate with the role based on organization ownership and admin status
queryset = OrganizationUser.objects.filter(
organization_id=organization_id
).select_related('user__extra_details').annotate(
Expand Down
33 changes: 2 additions & 31 deletions kobo/apps/stripe/tests/test_organization_usage.py
Original file line number Diff line number Diff line change
Expand Up @@ -83,19 +83,6 @@ def setUp(self):
def tearDown(self):
cache.clear()

def test_usage_doesnt_include_org_users_without_subscription(self):
"""
Test that the endpoint *only* returns usage for the logged-in user
if they don't have a subscription that includes Organizations.
"""
response = self.client.get(self.detail_url)
# without a plan, the user should only see their usage
assert response.data['total_submission_count']['all_time'] == self.expected_submissions_single
assert response.data['total_submission_count']['current_month'] == self.expected_submissions_single
assert response.data['total_storage_bytes'] == (
self.expected_file_size() * self.expected_submissions_single
)

def test_usage_for_plans_with_org_access(self):
"""
Test that the endpoint aggregates usage for each user in the organization
Expand All @@ -112,22 +99,6 @@ def test_usage_for_plans_with_org_access(self):
self.expected_file_size() * self.expected_submissions_multi
)

def test_doesnt_include_org_users_with_invalid_plan(self):
"""
Test that the endpoint *doesn't* aggregates usage for the organization
when subscribed to a product that doesn't include org access
"""

generate_plan_subscription(self.organization)

response = self.client.get(self.detail_url)
# without the proper subscription, the user should only see their usage
assert response.data['total_submission_count']['current_month'] == self.expected_submissions_single
assert response.data['total_submission_count']['all_time'] == self.expected_submissions_single
assert response.data['total_storage_bytes'] == (
self.expected_file_size() * self.expected_submissions_single
)

@pytest.mark.performance
def test_endpoint_speed(self):
# get the average request time for 10 hits to the endpoint
Expand Down Expand Up @@ -425,12 +396,12 @@ def test_nonexistent_organization(self):
non_org_id = 'lkdjalkfewkl'
url_with_non_org_id = f'{self.url}{non_org_id}/asset_usage/'
response = self.client.get(url_with_non_org_id)
assert response.status_code == status.HTTP_400_BAD_REQUEST
assert response.status_code == status.HTTP_404_NOT_FOUND

def test_user_not_member_of_organization(self):
self.client.force_login(self.someuser)
response = self.client.get(self.detail_url)
assert response.status_code == status.HTTP_400_BAD_REQUEST
assert response.status_code == status.HTTP_404_NOT_FOUND

def test_successful_retrieval(self):
generate_mmo_subscription(self.organization)
Expand Down
3 changes: 2 additions & 1 deletion kobo/apps/trackers/tests/submission_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,11 @@ def _get_uid(count):
return uids

for idx, user in enumerate(users):
real_owner = user.organization.owner_user_object
assets = assets + baker.make(
Asset,
content=content_source_asset,
owner=user,
owner=real_owner,
asset_type='survey',
name='test',
uid=iter(_get_uid(assets_per_user)),
Expand Down
2 changes: 1 addition & 1 deletion kobo/apps/trackers/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ def get_organization_usage(organization: Organization, usage_type: UsageType) ->
Get the used amount for a given organization and usage type
"""
usage_calc = ServiceUsageCalculator(
organization.owner.organization_user.user, organization, disable_cache=True
organization.owner.organization_user.user, disable_cache=True
)
usage = usage_calc.get_nlp_usage_by_type(USAGE_LIMIT_MAP[usage_type])

Expand Down
1 change: 1 addition & 0 deletions kpi/filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,7 @@ class SearchFilter(filters.BaseFilterBackend):
"""

def filter_queryset(self, request, queryset, view):

try:
q = request.query_params['q']
except AttributeError:
Expand Down
10 changes: 1 addition & 9 deletions kpi/serializers/v2/service_usage.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
from rest_framework import serializers
from rest_framework.fields import empty

from kobo.apps.organizations.models import Organization
from kobo.apps.organizations.utils import (
get_monthly_billing_dates,
get_yearly_billing_dates,
Expand Down Expand Up @@ -111,14 +110,7 @@ class ServiceUsageSerializer(serializers.Serializer):

def __init__(self, instance=None, data=empty, **kwargs):
super().__init__(instance=instance, data=data, **kwargs)
organization = None
organization_id = self.context.get('organization_id', None)
if organization_id:
organization = Organization.objects.filter(
organization_users__user_id=instance.id,
id=organization_id,
).first()
self.calculator = ServiceUsageCalculator(instance, organization)
self.calculator = ServiceUsageCalculator(user=instance)

def get_current_month_end(self, user):
return self.calculator.current_month_end.isoformat()
Expand Down
Loading

0 comments on commit 0bb717d

Please sign in to comment.