Skip to content

Commit

Permalink
fix: master api key org api access (#3817)
Browse files Browse the repository at this point in the history
  • Loading branch information
gagantrivedi committed Apr 22, 2024
1 parent 0cff3a9 commit cae2eac
Show file tree
Hide file tree
Showing 4 changed files with 124 additions and 8 deletions.
22 changes: 21 additions & 1 deletion api/api_keys/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

from django.db.models import QuerySet

from organisations.models import Organisation
from organisations.models import Organisation, OrganisationRole
from permissions.permission_service import (
get_permitted_environments_for_master_api_key,
get_permitted_projects_for_master_api_key,
Expand Down Expand Up @@ -35,9 +35,29 @@ def pk(self) -> str:
def is_master_api_key_user(self) -> bool:
return True

@property
def organisations(self) -> QuerySet[Organisation]:
return Organisation.objects.filter(id=self.key.organisation_id)

def belongs_to(self, organisation_id: int) -> bool:
return self.key.organisation_id == organisation_id

def is_organisation_admin(
self, organisation: typing.Union["Organisation", int]
) -> bool:
org_id = organisation.id if hasattr(organisation, "id") else organisation
return self.key.is_admin and self.key.organisation_id == org_id

def get_organisation_role(self, organisation: Organisation) -> typing.Optional[str]:
if self.key.organisation_id != organisation.id:
return None

return (
OrganisationRole.ADMIN.value
if self.key.is_admin
else OrganisationRole.USER.value
)

def is_project_admin(self, project: "Project") -> bool:
return is_master_api_key_project_admin(self.key, project)

Expand Down
2 changes: 1 addition & 1 deletion api/organisations/permissions/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ def has_permission(self, request, view):

def has_object_permission(self, request, view, obj):
return request.user.is_organisation_admin(obj) or (
view.action == "my_permissions" and obj in request.user.organisations.all()
view.action == "my_permissions" and request.user.belongs_to(obj)
)


Expand Down
83 changes: 83 additions & 0 deletions api/tests/unit/api_keys/test_user.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import pytest
from pytest_lazyfixture import lazy_fixture

from api_keys.models import MasterAPIKey
from api_keys.user import APIKeyUser
from environments.permissions.models import EnvironmentPermissionModel
from organisations.models import Organisation, OrganisationRole
from organisations.permissions.models import OrganisationPermissionModel
from projects.models import ProjectPermissionModel

Expand Down Expand Up @@ -286,3 +288,84 @@ def test_get_permitted_environments(
else:
assert environments.count() == 1
assert environments.first() == expected_environment


def test_is_organisation_admin_for_admin_key(
admin_master_api_key_object: MasterAPIKey,
organisation: Organisation,
organisation_two: Organisation,
) -> None:
# Given
user = APIKeyUser(admin_master_api_key_object)

# Then
# Return True for the associated organisation
assert user.is_organisation_admin(organisation) is True
assert user.is_organisation_admin(organisation.id) is True

# Returns False for other organisation
assert user.is_organisation_admin(organisation_two) is False
assert user.is_organisation_admin(organisation_two.id) is False


def test_is_organisation_admin_for_non_admin_key(
master_api_key_object: MasterAPIKey,
organisation: Organisation,
organisation_two: Organisation,
) -> None:
# Given
user = APIKeyUser(master_api_key_object)

# Then
assert user.is_organisation_admin(organisation) is False
assert user.is_organisation_admin(organisation.id) is False

assert user.is_organisation_admin(organisation_two) is False
assert user.is_organisation_admin(organisation_two.id) is False


def test_organisation_property(
master_api_key_object: MasterAPIKey,
organisation: Organisation,
):
# Given
user = APIKeyUser(master_api_key_object)

# When
organisations = user.organisations

# Then
assert organisations.count() == 1
assert organisations.first().id == organisation.id


def test_get_organisation_role_for_admin_key(
admin_master_api_key_object: MasterAPIKey,
organisation: Organisation,
organisation_two: Organisation,
) -> None:
# Given
user = APIKeyUser(admin_master_api_key_object)

# When/Then
# Returns ADMIN for the associated organisation
assert user.get_organisation_role(organisation) == OrganisationRole.ADMIN.value

# Returns None for other organisation
assert user.get_organisation_role(organisation_two) is None


def test_get_organisation_role_for_non_admin_key(
master_api_key_object: MasterAPIKey,
organisation: Organisation,
organisation_two: Organisation,
) -> None:
# Given
user = APIKeyUser(master_api_key_object)

# When/Then
# Returns USER for the associated organisation
assert user.get_organisation_role(organisation) == OrganisationRole.USER.value

# Returns None for other organisation
assert user.get_organisation_role(organisation_two) is None
25 changes: 19 additions & 6 deletions api/tests/unit/organisations/test_unit_organisations_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
from django.utils import timezone
from freezegun import freeze_time
from pytest_django.fixtures import SettingsWrapper
from pytest_lazyfixture import lazy_fixture
from pytest_mock import MockerFixture
from pytz import UTC
from rest_framework import status
Expand Down Expand Up @@ -110,8 +111,12 @@ def test_create_new_orgnisation_returns_403_with_non_superuser(
)


@pytest.mark.parametrize(
"client",
[lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")],
)
def test_should_update_organisation_data(
admin_client: APIClient,
client: APIClient,
organisation: Organisation,
) -> None:
# Given
Expand All @@ -120,7 +125,7 @@ def test_should_update_organisation_data(
data = {"name": new_organisation_name, "restrict_project_create_to_admin": True}

# When
response = admin_client.put(url, data=data)
response = client.put(url, data=data)

# Then
organisation.refresh_from_db()
Expand Down Expand Up @@ -325,16 +330,20 @@ def test_can_invite_user_as_user(
assert Invite.objects.get(email=invited_email).role == OrganisationRole.USER.name


@pytest.mark.parametrize(
"client",
[lazy_fixture("admin_master_api_key_client"), lazy_fixture("staff_client")],
)
def test_user_can_get_projects_for_an_organisation(
organisation: Organisation,
staff_client: APIClient,
client: APIClient,
project: Project,
) -> None:
# Given
url = reverse("api-v1:organisations:organisation-projects", args=[organisation.pk])

# When
response = staff_client.get(url)
response = client.get(url)

# Then
assert response.status_code == status.HTTP_200_OK
Expand Down Expand Up @@ -423,9 +432,13 @@ def test_update_subscription_gets_subscription_data_from_chargebee(
assert organisation.subscription.customer_id == customer_id


@pytest.mark.parametrize(
"client",
[lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")],
)
def test_delete_organisation(
organisation: Organisation,
admin_client: APIClient,
client: APIClient,
project: Project,
environment: Environment,
feature: Feature,
Expand All @@ -435,7 +448,7 @@ def test_delete_organisation(
url = reverse("api-v1:organisations:organisation-detail", args=[organisation.id])

# When
response = admin_client.delete(url)
response = client.delete(url)

# Then
assert response.status_code == status.HTTP_204_NO_CONTENT
Expand Down

0 comments on commit cae2eac

Please sign in to comment.