Skip to content

Commit

Permalink
chore(staff): Let staff access user details endpoint (#64631)
Browse files Browse the repository at this point in the history
This endpoint is open to people viewing themselves, superuser, and staff

There is logic inside PUT and delete that changes depending on which
mode you are. Right now:
1. PUT - Superuser can change users to be superuser or staff if they
have `users.admin`
2. DELETE - Superusers can hard delete users if they have `users.admin`

We want to prevent superusers in the future from being able to do this,
but allow staff because these actions are only performed through the
_admin portal

When checking for `users.admin`, once the feature flag is removed we'll
include an explicit check for `is_active_staff` to achieve this.
  • Loading branch information
schew2381 authored Feb 14, 2024
1 parent cc97408 commit 60a336c
Show file tree
Hide file tree
Showing 4 changed files with 265 additions and 71 deletions.
30 changes: 21 additions & 9 deletions src/sentry/api/endpoints/user_details.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,13 @@
from sentry import roles
from sentry.api.api_publish_status import ApiPublishStatus
from sentry.api.base import control_silo_endpoint
from sentry.api.bases.user import UserEndpoint
from sentry.api.bases.user import UserAndStaffPermission, UserEndpoint
from sentry.api.decorators import sudo_required
from sentry.api.endpoints.organization_details import post_org_pending_deletion
from sentry.api.serializers import serialize
from sentry.api.serializers.models.user import DetailedSelfUserSerializer
from sentry.api.serializers.rest_framework import CamelSnakeModelSerializer
from sentry.auth.superuser import is_active_superuser
from sentry.auth.elevated_mode import has_elevated_mode
from sentry.constants import LANGUAGES
from sentry.models.options.user_option import UserOption
from sentry.models.organization import OrganizationStatus
Expand Down Expand Up @@ -154,6 +154,8 @@ class UserDetailsEndpoint(UserEndpoint):
"PUT": ApiPublishStatus.UNKNOWN,
}

permission_classes = (UserAndStaffPermission,)

def get(self, request: Request, user) -> Response:
"""
Retrieve User Details
Expand Down Expand Up @@ -182,7 +184,13 @@ def put(self, request: Request, user) -> Response:
:param string default_issue_event: Event displayed by default, "recommended", "latest" or "oldest"
:auth: required
"""
if not request.access.has_permission("users.admin"):
# We want to prevent superusers from setting users to superuser or staff
# because this is only done through _admin. This will always be enforced
# once the feature flag is removed.
can_elevate_user = has_elevated_mode(request) and request.access.has_permission(
"users.admin"
)
if not can_elevate_user:
if not user.is_superuser and request.data.get("isSuperuser"):
return Response(
{"detail": "Missing required permission to add superuser."},
Expand All @@ -194,11 +202,13 @@ def put(self, request: Request, user) -> Response:
status=status.HTTP_403_FORBIDDEN,
)

if request.access.has_permission("users.admin"):
if can_elevate_user:
serializer_cls = PrivilegedUserSerializer
# with superuser read write separation, superuser read cannot hit this endpoint
# so we can keep this as is_active_superuser
elif is_active_superuser(request):
# With superuser read/write separation, superuser read cannot hit this endpoint
# so we can keep this as is_active_superuser. Once the feature flag is
# removed and we only check is_active_staff, we can remove this comment.
elif has_elevated_mode(request):
# TODO(schew2381): Rename to staff serializer
serializer_cls = SuperuserUserSerializer
else:
serializer_cls = UserSerializer
Expand Down Expand Up @@ -257,7 +267,6 @@ def delete(self, request: Request, user) -> Response:
:param list organizations: List of organization ids to remove
:auth required:
"""

serializer = DeleteUserSerializer(data=request.data)

if not serializer.is_valid():
Expand Down Expand Up @@ -328,9 +337,12 @@ def delete(self, request: Request, user) -> Response:
}

hard_delete = serializer.validated_data.get("hardDelete", False)
can_delete = has_elevated_mode(request) and request.access.has_permission("users.admin")

# Only active superusers can hard delete accounts
if hard_delete and not request.access.has_permission("users.admin"):
# This will be changed to only active staff can delete accounts once the
# staff feature flag is removed.
if hard_delete and not can_delete:
return Response(
{"detail": "Missing required permission to hard delete account."},
status=status.HTTP_403_FORBIDDEN,
Expand Down
18 changes: 11 additions & 7 deletions src/sentry/api/serializers/models/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
from sentry.api.serializers import Serializer, register
from sentry.api.serializers.types import SerializedAvatarFields
from sentry.app import env
from sentry.auth.superuser import is_active_superuser
from sentry.auth.elevated_mode import has_elevated_mode
from sentry.models.authenticator import Authenticator
from sentry.models.authidentity import AuthIdentity
from sentry.models.avatars.user_avatar import UserAvatar
Expand Down Expand Up @@ -122,7 +122,8 @@ def _user_is_requester(self, obj: User, requester: User | AnonymousUser | RpcUse
def _get_identities(
self, item_list: Sequence[User], user: User
) -> dict[int, list[AuthIdentity]]:
if not (env.request and is_active_superuser(env.request)):

if not (env.request and has_elevated_mode(env.request)):
item_list = [x for x in item_list if x.id == user.id]

queryset = AuthIdentity.objects.filter(
Expand Down Expand Up @@ -290,8 +291,10 @@ def serialize(
) -> DetailedUserSerializerResponse:
d = cast(DetailedUserSerializerResponse, super().serialize(obj, attrs, user))

# XXX(dcramer): we don't use is_active_superuser here as we simply
# want to tell the UI that we're an authenticated superuser, and
# TODO(schew2381): Remove mention of superuser below once the staff feature flag is removed

# XXX(dcramer): we don't check for active superuser/staff here as we simply
# want to tell the UI that we're an authenticated superuser/staff, and
# for requests that require an *active* session, they should prompt
# on-demand. This ensures things like links to the Sentry admin can
# still easily be rendered.
Expand Down Expand Up @@ -357,9 +360,10 @@ def serialize(
d = cast(DetailedSelfUserSerializerResponse, super().serialize(obj, attrs, user))

# safety check to never return this information if the acting user is not 1) this user, 2) an admin
if user.id == obj.id or user.is_superuser:
# XXX(dcramer): we don't use is_active_superuser here as we simply
# want to tell the UI that we're an authenticated superuser, and
# TODO(schew2381): Remove user.is_superuser once the staff feature flag is removed
if user.id == obj.id or user.is_superuser or user.is_staff:
# XXX(dcramer): we don't check for active superuser/staff here as we simply
# want to tell the UI that we're an authenticated superuser/staff, and
# for requests that require an *active* session, they should prompt
# on-demand. This ensures things like links to the Sentry admin can
# still easily be rendered.
Expand Down
Loading

0 comments on commit 60a336c

Please sign in to comment.