diff --git a/src/sentry/auth/access.py b/src/sentry/auth/access.py index 23a7aaceb474..5cced4b497de 100644 --- a/src/sentry/auth/access.py +++ b/src/sentry/auth/access.py @@ -15,7 +15,7 @@ from sentry import features, roles from sentry.api.exceptions import DataSecrecyError from sentry.auth.deprecated_scope_translation import translate_deprecated_scopes -from sentry.auth.principal import acting_user_id, has_user +from sentry.auth.principal import acting_user_id, has_user, require_user from sentry.auth.scope_declaration import check_scope_declaration, check_scope_declarations from sentry.auth.services.access.service import access_service from sentry.auth.services.auth import AuthenticatedToken, RpcAuthState, RpcMemberSsoState @@ -51,6 +51,7 @@ from sentry.sentry_apps.models.sentry_app import SentryApp from sentry.service_accounts.auth import is_service_account_auth from sentry.service_accounts.models import ServiceAccountProjectReplica, ServiceAccountReplica +from sentry.service_accounts.principal import ServiceAccountPrincipal from sentry.silo.base import SiloMode from sentry.types.request import _HttpRequestWithUser from sentry.users.models.user import User @@ -1233,7 +1234,9 @@ def organizationless_access( ) -def normalize_valid_user(user: User | RpcUser | AnonymousUser | None) -> User | RpcUser | None: +def normalize_valid_user( + user: User | RpcUser | AnonymousUser | ServiceAccountPrincipal | None, +) -> User | RpcUser | None: if not user or not has_user(user) or not user.is_active: return None return user @@ -1289,7 +1292,7 @@ def from_request( raise DataSecrecyError() if request.user.is_authenticated and request.user.is_sentry_app: - return _from_sentry_app(request.user, organization=organization) + return _from_sentry_app(require_user(request), organization=organization) if is_active_superuser(request): member: OrganizationMember | None = None @@ -1375,7 +1378,7 @@ def _from_rpc_sentry_app(context: RpcUserOrganizationContext | None = None) -> A def from_user( - user: User | RpcUser | AnonymousUser | None, + user: User | RpcUser | AnonymousUser | ServiceAccountPrincipal | None, organization: Organization | None = None, scopes: Iterable[str] | None = None, is_superuser: bool = False, diff --git a/src/sentry/auth/providers/saml2/provider.py b/src/sentry/auth/providers/saml2/provider.py index 46e287543615..156bd114e3a0 100644 --- a/src/sentry/auth/providers/saml2/provider.py +++ b/src/sentry/auth/providers/saml2/provider.py @@ -22,6 +22,7 @@ from sentry import features, options from sentry.auth.exceptions import IdentityNotValid from sentry.auth.helper import AuthHelper +from sentry.auth.principal import acting_user_id from sentry.auth.provider import Provider from sentry.auth.store import FLOW_LOGIN from sentry.auth.view import AuthView @@ -482,7 +483,10 @@ def handle_saml_single_logout(request: Request) -> OneLogin_Saml2_Auth: """ # Do not handle SLO if a user is in more than 1 organization # Propagating it to multiple IdPs results in confusion for the user - organizations = user_service.get_organizations(user_id=request.user.id) + user_id = acting_user_id(request.user) + if user_id is None: + return + organizations = user_service.get_organizations(user_id=user_id) if not len(organizations) == 1: return diff --git a/src/sentry/auth_v2/endpoints/auth_merge_user_accounts.py b/src/sentry/auth_v2/endpoints/auth_merge_user_accounts.py index ba873b2020c3..3f29769e955b 100644 --- a/src/sentry/auth_v2/endpoints/auth_merge_user_accounts.py +++ b/src/sentry/auth_v2/endpoints/auth_merge_user_accounts.py @@ -11,6 +11,7 @@ from sentry.api.paginator import OffsetPaginator from sentry.api.serializers import serialize from sentry.api.serializers.rest_framework.base import CamelSnakeSerializer +from sentry.auth.principal import require_user from sentry.auth_v2.endpoints.base import AuthV2Endpoint from sentry.users.api.serializers.user import UserSerializerWithOrgMemberships from sentry.users.models.user import User @@ -35,10 +36,7 @@ class AuthMergeUserAccountsEndpoint(AuthV2Endpoint): """ def get(self, request: Request) -> Response: - user = request.user - if not user.is_authenticated: - return Response(status=401) - + user = require_user(request) shared_email = user.email if not shared_email: return Response( @@ -54,15 +52,14 @@ def get(self, request: Request) -> Response: ) def post(self, request: Request) -> Response: - if not request.user.is_authenticated: - return Response(status=401) + user = require_user(request) validator = AuthMergeUserAccountsValidator(data=request.data) if not validator.is_valid(): return Response(validator.errors, status=400) result = validator.validated_data - primary_user = User.objects.get(id=request.user.id) + primary_user = User.objects.get(id=user.id) verification_code = UserMergeVerificationCode.objects.filter( user_id=primary_user.id ).first() diff --git a/src/sentry/auth_v2/endpoints/auth_user_merge_verification_code.py b/src/sentry/auth_v2/endpoints/auth_user_merge_verification_code.py index 8f7a16f0171e..06349c08f038 100644 --- a/src/sentry/auth_v2/endpoints/auth_user_merge_verification_code.py +++ b/src/sentry/auth_v2/endpoints/auth_user_merge_verification_code.py @@ -6,6 +6,7 @@ from sentry.api.api_publish_status import ApiPublishStatus from sentry.api.base import Endpoint, control_silo_endpoint from sentry.api.permissions import DisallowAgentToken, SentryIsAuthenticated +from sentry.auth.principal import require_user from sentry.users.models.user_merge_verification_code import UserMergeVerificationCode @@ -21,11 +22,7 @@ class AuthUserMergeVerificationCodeEndpoint(Endpoint): """ def post(self, request: Request) -> Response: - user = request.user - if user.id is None: - return Response( - status=403, data={"error": "Must be authenticated to use this endpoint."} - ) + user = require_user(request) try: # regenerate the code if it exists code = UserMergeVerificationCode.objects.get(user_id=user.id) diff --git a/src/sentry/auth_v2/utils/session.py b/src/sentry/auth_v2/utils/session.py index 1e68c18b5c22..b9cb95ea4c28 100644 --- a/src/sentry/auth_v2/utils/session.py +++ b/src/sentry/auth_v2/utils/session.py @@ -6,6 +6,8 @@ from rest_framework.request import Request from sentry.api.serializers import Serializer +from sentry.auth.principal import has_user +from sentry.service_accounts.principal import ServiceAccountPrincipal from sentry.users.models.user import User from sentry.users.services.user.model import RpcUser @@ -35,7 +37,7 @@ def serialize( self, obj: Request, attrs: Mapping[Any, Any], - user: User | RpcUser | AnonymousUser, + user: User | RpcUser | AnonymousUser | ServiceAccountPrincipal, **kwargs: Any, ) -> SessionSerializerResponse: """ @@ -76,9 +78,9 @@ def initialize_auth_flags(self) -> None: the user needs to complete on the frontend. Flags are only set if they don't already exist in the session. """ - user = self.request.user - if not user or user.is_anonymous: + if not has_user(self.request.user): return + user = self.request.user session = self.request.session diff --git a/src/sentry/relocation/api/endpoints/abort.py b/src/sentry/relocation/api/endpoints/abort.py index 3cde7be432cc..e9384b1c8ffd 100644 --- a/src/sentry/relocation/api/endpoints/abort.py +++ b/src/sentry/relocation/api/endpoints/abort.py @@ -9,6 +9,7 @@ from sentry.api.exceptions import ResourceDoesNotExist from sentry.api.permissions import SuperuserOrStaffFeatureFlaggedPermission from sentry.api.serializers import serialize +from sentry.auth.principal import acting_user_id from sentry.relocation.models.relocation import Relocation ERR_NOT_ABORTABLE_STATUS = ( @@ -43,7 +44,7 @@ def put(self, request: Request, relocation_uuid: str) -> Response: :auth: required """ - logger.info("relocations.abort.put.start", extra={"caller": request.user.id}) + logger.info("relocations.abort.put.start", extra={"caller": acting_user_id(request.user)}) try: relocation: Relocation = Relocation.objects.get(uuid=relocation_uuid) diff --git a/src/sentry/relocation/api/endpoints/artifacts/details.py b/src/sentry/relocation/api/endpoints/artifacts/details.py index affbaa6230b4..69fc3dbac107 100644 --- a/src/sentry/relocation/api/endpoints/artifacts/details.py +++ b/src/sentry/relocation/api/endpoints/artifacts/details.py @@ -13,6 +13,7 @@ from sentry.api.exceptions import ResourceDoesNotExist, StaffRequired, SuperuserRequired from sentry.api.permissions import SuperuserOrStaffFeatureFlaggedPermission from sentry.auth.elevated_mode import has_elevated_mode +from sentry.auth.principal import acting_user_id from sentry.auth.staff import has_staff_option from sentry.backup.crypto import ( CryptoKeyVersion, @@ -59,7 +60,9 @@ def get( :auth: required """ - logger.info("relocations.artifact.details.get.start", extra={"caller": request.user.id}) + logger.info( + "relocations.artifact.details.get.start", extra={"caller": acting_user_id(request.user)} + ) # TODO(schew2381): Remove the superuser reference below after feature flag is removed. # Must be superuser/staff AND have a `UserPermission` of `relocation.admin` to see access! diff --git a/src/sentry/relocation/api/endpoints/artifacts/index.py b/src/sentry/relocation/api/endpoints/artifacts/index.py index 86e578b9aad7..cdf50f87e836 100644 --- a/src/sentry/relocation/api/endpoints/artifacts/index.py +++ b/src/sentry/relocation/api/endpoints/artifacts/index.py @@ -10,6 +10,7 @@ from sentry.api.exceptions import ResourceDoesNotExist, StaffRequired, SuperuserRequired from sentry.api.permissions import SuperuserOrStaffFeatureFlaggedPermission from sentry.auth.elevated_mode import has_elevated_mode +from sentry.auth.principal import acting_user_id from sentry.auth.staff import has_staff_option from sentry.models.files.utils import get_relocation_storage from sentry.relocation.models.relocation import Relocation @@ -40,7 +41,9 @@ def get(self, request: Request, relocation_uuid: str) -> Response: :auth: required """ - logger.info("relocations.artifact.index.get.start", extra={"caller": request.user.id}) + logger.info( + "relocations.artifact.index.get.start", extra={"caller": acting_user_id(request.user)} + ) # TODO(schew2381): Remove the superuser reference below after feature flag is removed. # Must be superuser/staff AND have a `UserPermission` of `relocation.admin` to see access! diff --git a/src/sentry/relocation/api/endpoints/cancel.py b/src/sentry/relocation/api/endpoints/cancel.py index 05453f8ba4be..4db727e840a6 100644 --- a/src/sentry/relocation/api/endpoints/cancel.py +++ b/src/sentry/relocation/api/endpoints/cancel.py @@ -12,6 +12,7 @@ from sentry.api.exceptions import ResourceDoesNotExist from sentry.api.permissions import SuperuserOrStaffFeatureFlaggedPermission from sentry.api.serializers import serialize +from sentry.auth.principal import acting_user_id from sentry.relocation.api.endpoints import ERR_UNKNOWN_RELOCATION_STEP from sentry.relocation.models.relocation import Relocation @@ -57,7 +58,7 @@ def put(self, request: Request, relocation_uuid: str) -> Response: :auth: required """ - logger.info("relocations.cancel.put.start", extra={"caller": request.user.id}) + logger.info("relocations.cancel.put.start", extra={"caller": acting_user_id(request.user)}) try: relocation: Relocation = Relocation.objects.get(uuid=relocation_uuid) diff --git a/src/sentry/relocation/api/endpoints/details.py b/src/sentry/relocation/api/endpoints/details.py index 8886a2423cbf..e61c451e7264 100644 --- a/src/sentry/relocation/api/endpoints/details.py +++ b/src/sentry/relocation/api/endpoints/details.py @@ -9,6 +9,7 @@ from sentry.api.exceptions import ResourceDoesNotExist from sentry.api.permissions import SuperuserOrStaffFeatureFlaggedPermission from sentry.api.serializers import serialize +from sentry.auth.principal import acting_user_id from sentry.relocation.models.relocation import Relocation logger = logging.getLogger("sentry.relocation") @@ -33,7 +34,7 @@ def get(self, request: Request, relocation_uuid: str) -> Response: :auth: required """ - logger.info("relocations.details.get.start", extra={"caller": request.user.id}) + logger.info("relocations.details.get.start", extra={"caller": acting_user_id(request.user)}) try: return self.respond(serialize(Relocation.objects.get(uuid=relocation_uuid))) diff --git a/src/sentry/relocation/api/endpoints/index.py b/src/sentry/relocation/api/endpoints/index.py index 1dbd2c44959c..0a210c0c4d04 100644 --- a/src/sentry/relocation/api/endpoints/index.py +++ b/src/sentry/relocation/api/endpoints/index.py @@ -22,6 +22,7 @@ from sentry.api.permissions import DisallowAgentToken, SentryIsAuthenticated from sentry.api.serializers import serialize from sentry.auth.elevated_mode import has_elevated_mode +from sentry.auth.principal import acting_user_id, is_owned_by_acting_user, require_user from sentry.models.files.file import File from sentry.relocation.api.endpoints import ERR_FEATURE_DISABLED from sentry.relocation.api.serializers.relocation import RelocationSerializer @@ -105,7 +106,7 @@ def validate_new_relocation_request( return Response(status=status.HTTP_400_BAD_REQUEST) # Only superuser/staff can start relocations for other users. - creator = user_service.get_user(user_id=request.user.id) + creator = user_service.get_user(user_id=require_user(request).id) if creator is None: raise RuntimeError("Could not ascertain request user's username") if not elevated_user and creator.username != owner_username: @@ -179,7 +180,7 @@ def get(self, request: Request) -> Response: :auth: required """ - logger.info("relocations.index.get.start", extra={"caller": request.user.id}) + logger.info("relocations.index.get.start", extra={"caller": acting_user_id(request.user)}) if not request.user.is_authenticated: return Response(status=status.HTTP_400_BAD_REQUEST) @@ -189,7 +190,7 @@ def get(self, request: Request) -> Response: # TODO(schew2381): Remove the superuser reference below after feature flag is removed. # Non-superuser/staff can only see their own relocations. if not has_elevated_mode(request): - queryset = queryset.filter(owner_id=request.user.id) + queryset = queryset.filter(is_owned_by_acting_user(request.user, "owner_id")) query = request.GET.get("query") if query: @@ -243,7 +244,7 @@ def post(self, request: Request) -> Response: :auth: required """ - logger.info("relocations.index.post.start", extra={"caller": request.user.id}) + logger.info("relocations.index.post.start", extra={"caller": acting_user_id(request.user)}) if not request.user.is_authenticated: return Response(status=status.HTTP_400_BAD_REQUEST) @@ -277,12 +278,13 @@ def post(self, request: Request) -> Response: file = File.objects.create(name="raw-relocation-data.tar", type=RELOCATION_FILE_TYPE) file.putfile(fileobj, blob_size=RELOCATION_BLOB_SIZE, logger=logger) + creator = require_user(request) with atomic_transaction( using=(router.db_for_write(Relocation), router.db_for_write(RelocationFile)) ): provenance = Relocation.Provenance.SELF_HOSTED relocation: Relocation = Relocation.objects.create( - creator_id=request.user.id, + creator_id=creator.id, owner_id=owner.id, want_org_slugs=org_slugs, step=Relocation.Step.UPLOADING.value, @@ -301,7 +303,7 @@ def post(self, request: Request) -> Response: try: analytics.record( RelocationCreatedEvent( - creator_id=request.user.id, + creator_id=creator.id, owner_id=owner.id, uuid=str(relocation.uuid), ) diff --git a/src/sentry/relocation/api/endpoints/pause.py b/src/sentry/relocation/api/endpoints/pause.py index 0a64cf84aa93..35d3d086ff4c 100644 --- a/src/sentry/relocation/api/endpoints/pause.py +++ b/src/sentry/relocation/api/endpoints/pause.py @@ -12,6 +12,7 @@ from sentry.api.exceptions import ResourceDoesNotExist from sentry.api.permissions import SuperuserOrStaffFeatureFlaggedPermission from sentry.api.serializers import serialize +from sentry.auth.principal import acting_user_id from sentry.relocation.api.endpoints import ( ERR_COULD_NOT_PAUSE_RELOCATION_AT_STEP, ERR_UNKNOWN_RELOCATION_STEP, @@ -55,7 +56,7 @@ def put(self, request: Request, relocation_uuid: str) -> Response: :auth: required """ - logger.info("relocations.pause.put.start", extra={"caller": request.user.id}) + logger.info("relocations.pause.put.start", extra={"caller": acting_user_id(request.user)}) try: relocation: Relocation = Relocation.objects.get(uuid=relocation_uuid) diff --git a/src/sentry/relocation/api/endpoints/public_key.py b/src/sentry/relocation/api/endpoints/public_key.py index 66fac93df80d..44a9c8be9487 100644 --- a/src/sentry/relocation/api/endpoints/public_key.py +++ b/src/sentry/relocation/api/endpoints/public_key.py @@ -9,6 +9,7 @@ from sentry.api.base import Endpoint, cell_silo_endpoint from sentry.api.permissions import SentryIsAuthenticated from sentry.auth.elevated_mode import has_elevated_mode +from sentry.auth.principal import acting_user_id from sentry.backup.crypto import GCPKMSEncryptor, get_default_crypto_key_version from sentry.relocation.api.endpoints import ERR_FEATURE_DISABLED from sentry.utils.env import log_gcp_credentials_details @@ -35,7 +36,9 @@ def get(self, request: Request) -> Response: :auth: required """ - logger.info("publickeys.relocations.get.start", extra={"caller": request.user.id}) + logger.info( + "publickeys.relocations.get.start", extra={"caller": acting_user_id(request.user)} + ) # We only honor the `relocation.enabled` flag for non-superusers/staff. elevated_user = has_elevated_mode(request) diff --git a/src/sentry/relocation/api/endpoints/recover.py b/src/sentry/relocation/api/endpoints/recover.py index a72f2015dcad..45fd3d07de96 100644 --- a/src/sentry/relocation/api/endpoints/recover.py +++ b/src/sentry/relocation/api/endpoints/recover.py @@ -11,6 +11,7 @@ from sentry.api.exceptions import ResourceDoesNotExist from sentry.api.permissions import SuperuserOrStaffFeatureFlaggedPermission from sentry.api.serializers import serialize +from sentry.auth.principal import acting_user_id from sentry.relocation.api.endpoints import ( ERR_COULD_NOT_PAUSE_RELOCATION_AT_STEP, ERR_UNKNOWN_RELOCATION_STEP, @@ -123,7 +124,7 @@ def put(self, request: Request, relocation_uuid: str) -> Response: :auth: required """ - logger.info("relocations.recover.put.start", extra={"caller": request.user.id}) + logger.info("relocations.recover.put.start", extra={"caller": acting_user_id(request.user)}) # Use a `select_for_update` transaction to prevent duplicate tasks from being started by # racing recover calls. diff --git a/src/sentry/relocation/api/endpoints/retry.py b/src/sentry/relocation/api/endpoints/retry.py index de13b97f09ea..d68fbc5f2772 100644 --- a/src/sentry/relocation/api/endpoints/retry.py +++ b/src/sentry/relocation/api/endpoints/retry.py @@ -15,6 +15,7 @@ from sentry.api.exceptions import ResourceDoesNotExist from sentry.api.permissions import DisallowAgentToken, SentryIsAuthenticated from sentry.api.serializers import serialize +from sentry.auth.principal import acting_user_id, require_user from sentry.models.files.file import File from sentry.relocation.api.endpoints.index import ( get_autopause_value, @@ -57,7 +58,7 @@ def post(self, request: Request, relocation_uuid: str) -> Response: :auth: required """ - logger.info("relocations.retry.post.start", extra={"caller": request.user.id}) + logger.info("relocations.retry.post.start", extra={"caller": acting_user_id(request.user)}) if not request.user.is_authenticated: return Response(status=status.HTTP_400_BAD_REQUEST) @@ -107,11 +108,12 @@ def post(self, request: Request, relocation_uuid: str) -> Response: if err is not None: return err + creator = require_user(request) with atomic_transaction( using=(router.db_for_write(Relocation), router.db_for_write(RelocationFile)) ): new_relocation = Relocation.objects.create( - creator_id=request.user.id, + creator_id=creator.id, owner_id=relocation.owner_id, want_org_slugs=relocation.want_org_slugs, step=Relocation.Step.UPLOADING.value, @@ -136,7 +138,7 @@ def post(self, request: Request, relocation_uuid: str) -> Response: try: analytics.record( RelocationCreatedEvent( - creator_id=request.user.id, + creator_id=creator.id, owner_id=owner.id, uuid=str(new_relocation.uuid), ) diff --git a/src/sentry/relocation/api/endpoints/unpause.py b/src/sentry/relocation/api/endpoints/unpause.py index 3f102e2dfbc3..4721433e6a13 100644 --- a/src/sentry/relocation/api/endpoints/unpause.py +++ b/src/sentry/relocation/api/endpoints/unpause.py @@ -11,6 +11,7 @@ from sentry.api.exceptions import ResourceDoesNotExist from sentry.api.permissions import SuperuserOrStaffFeatureFlaggedPermission from sentry.api.serializers import serialize +from sentry.auth.principal import acting_user_id from sentry.relocation.api.endpoints import ( ERR_COULD_NOT_PAUSE_RELOCATION_AT_STEP, ERR_UNKNOWN_RELOCATION_STEP, @@ -132,7 +133,7 @@ def put(self, request: Request, relocation_uuid: str) -> Response: :auth: required """ - logger.info("relocations.unpause.put.start", extra={"caller": request.user.id}) + logger.info("relocations.unpause.put.start", extra={"caller": acting_user_id(request.user)}) # Use a `select_for_update` transaction to prevent duplicate tasks from being started by # racing unpause calls. diff --git a/src/sentry/users/api/bases/user.py b/src/sentry/users/api/bases/user.py index e6590b053e4f..07cfde2885fb 100644 --- a/src/sentry/users/api/bases/user.py +++ b/src/sentry/users/api/bases/user.py @@ -10,7 +10,7 @@ from sentry.api.exceptions import ResourceDoesNotExist from sentry.api.permissions import DemoSafePermission, StaffPermissionMixin from sentry.api.utils import to_valid_int_id -from sentry.auth.principal import acting_user_id, has_user +from sentry.auth.principal import acting_user_id, has_user, is_acting_user from sentry.auth.services.access.service import access_service from sentry.auth.superuser import is_active_superuser, superuser_has_permission from sentry.auth.system import is_system_auth @@ -34,7 +34,7 @@ def has_permission(self, request: Request, view: APIView) -> bool: def has_object_permission( self, request: Request, view: APIView, user: User | RpcUser | None ) -> bool: - if user is None or request.user.id == user.id: + if user is None or is_acting_user(request.user, user.id): return True if is_system_auth(request.auth): return True @@ -109,7 +109,7 @@ def has_object_permission( return request.auth.user_id == user.id # Deliberately not `super()`: `UserPermission` lets an active superuser act on # another account, which for personal display preferences has no operator use. - return request.user.id == user.id + return is_acting_user(request.user, user.id) class OrganizationUserPermission(UserAndStaffPermission): @@ -125,7 +125,7 @@ def has_org_permission(self, request: Request, user: User | RpcUser | None) -> b if organization_id is None: return False organization = organization_service.get_organization_by_id( - id=organization_id, user_id=request.user.id + id=organization_id, user_id=acting_user_id(request.user) ) if not organization: return False diff --git a/src/sentry/users/api/endpoints/authenticator_index.py b/src/sentry/users/api/endpoints/authenticator_index.py index dbd736f53706..633300150d14 100644 --- a/src/sentry/users/api/endpoints/authenticator_index.py +++ b/src/sentry/users/api/endpoints/authenticator_index.py @@ -8,6 +8,7 @@ from sentry.api.api_publish_status import ApiPublishStatus from sentry.api.base import Endpoint, control_silo_endpoint from sentry.auth.authenticators.base import ActivationChallengeResult +from sentry.auth.principal import require_user from sentry.users.models.authenticator import Authenticator @@ -27,7 +28,7 @@ def get(self, request: Request) -> Response: # Currently just expose u2f challenge, not sure if it's necessary to list all # authenticator interfaces that are enabled try: - interface = Authenticator.objects.get_interface(request.user, "u2f") + interface = Authenticator.objects.get_interface(require_user(request), "u2f") if not interface.is_enrolled(): raise LookupError() except LookupError: diff --git a/src/sentry/users/api/endpoints/user_authenticator_details.py b/src/sentry/users/api/endpoints/user_authenticator_details.py index a9cc2868efd6..7341ca9d2f4d 100644 --- a/src/sentry/users/api/endpoints/user_authenticator_details.py +++ b/src/sentry/users/api/endpoints/user_authenticator_details.py @@ -13,6 +13,7 @@ from sentry.api.serializers import serialize from sentry.auth.authenticators.recovery_code import RecoveryCodeInterface from sentry.auth.authenticators.u2f import U2fInterface, decode_credential_id +from sentry.auth.principal import require_user from sentry.auth.staff import has_staff_option, is_active_staff from sentry.auth.superuser import is_active_superuser from sentry.security.utils import capture_security_activity @@ -68,7 +69,7 @@ def _regenerate_recovery_code( capture_security_activity( account=user, type="recovery-codes-regenerated", - actor=request.user, + actor=require_user(request), ip_address=request.META["REMOTE_ADDR"], context={"authenticator": authenticator}, send_email=True, @@ -232,7 +233,7 @@ def delete( # have been sent to db for iface in backup_interfaces: capture_security_activity( - account=request.user, + account=user, type="mfa-removed", actor=request.user, ip_address=request.META["REMOTE_ADDR"], diff --git a/src/sentry/users/api/endpoints/user_details.py b/src/sentry/users/api/endpoints/user_details.py index 0596030bc2b8..e3814eedd41b 100644 --- a/src/sentry/users/api/endpoints/user_details.py +++ b/src/sentry/users/api/endpoints/user_details.py @@ -394,6 +394,7 @@ def delete(self, request: Request, user: User) -> Response: :param list organizations: List of organization ids to remove :auth required: """ + actor = require_user(request) serializer = DeleteUserSerializer(data=request.data) if not serializer.is_valid(): @@ -463,8 +464,8 @@ def delete(self, request: Request, user: User) -> Response: organization_id=member_mapping.organization_id, date_added=django_timezone.now(), event_id=audit_log.get_event_id("MEMBER_REMOVE"), - actor_user_id=require_user(request).id, - actor_label=request.user.username, + actor_user_id=actor.id, + actor_label=actor.username, ip_address=request.META["REMOTE_ADDR"], target_object_id=member_mapping.organizationmember_id, target_user_id=user.id, @@ -473,7 +474,7 @@ def delete(self, request: Request, user: User) -> Response: ) logging_data = { - "actor_id": require_user(request).id, + "actor_id": actor.id, "ip_address": request.META["REMOTE_ADDR"], "user_id": user.id, } @@ -490,7 +491,7 @@ def delete(self, request: Request, user: User) -> Response: status=status.HTTP_403_FORBIDDEN, ) - is_current_user = require_user(request).id == user.id + is_current_user = actor.id == user.id if hard_delete: user_id = user.id @@ -500,7 +501,7 @@ def delete(self, request: Request, user: User) -> Response: record_hard_user_deletion( user_id=user_id, user_email=user_email, - actor=request.user, + actor=actor, ip_address=request.META["REMOTE_ADDR"], ) else: @@ -510,7 +511,7 @@ def delete(self, request: Request, user: User) -> Response: delete_logger.info("user.deactivate", extra=logging_data) record_user_deactivation( user=user, - actor=request.user, + actor=actor, ip_address=request.META["REMOTE_ADDR"], ) diff --git a/src/sentry/users/api/endpoints/user_index.py b/src/sentry/users/api/endpoints/user_index.py index 0193238f32d6..5697516cf969 100644 --- a/src/sentry/users/api/endpoints/user_index.py +++ b/src/sentry/users/api/endpoints/user_index.py @@ -8,6 +8,7 @@ from sentry.api.paginator import DateTimePaginator from sentry.api.permissions import SuperuserOrStaffFeatureFlaggedPermission from sentry.api.serializers import serialize +from sentry.auth.principal import acting_user_id from sentry.db.models.query import in_iexact from sentry.search.utils import tokenize_query from sentry.users.api.serializers.user import UserSerializer @@ -40,7 +41,9 @@ def get(self, request: Request) -> Response: valid_ids = [] for v in value: if v == "me": - valid_ids.append(request.user.id) + user_id = acting_user_id(request.user) + if user_id is not None: + valid_ids.append(user_id) else: try: valid_ids.append(int(v)) diff --git a/src/sentry/users/api/endpoints/user_password.py b/src/sentry/users/api/endpoints/user_password.py index 541861042fc5..0a5e843851a4 100644 --- a/src/sentry/users/api/endpoints/user_password.py +++ b/src/sentry/users/api/endpoints/user_password.py @@ -9,6 +9,7 @@ from sentry.api.api_publish_status import ApiPublishStatus from sentry.api.base import control_silo_endpoint from sentry.auth import password_validation +from sentry.auth.principal import require_user from sentry.auth.twofactor import reset_2fa_rate_limits from sentry.ratelimits.config import RateLimitConfig from sentry.security.utils import capture_security_activity @@ -81,6 +82,7 @@ def put(self, request: Request, user: User) -> Response: result = serializer.validated_data + actor = require_user(request) user.set_password(result["passwordNew"]) user.refresh_session_nonce(request._request) user.clear_lost_passwords() @@ -89,7 +91,7 @@ def put(self, request: Request, user: User) -> Response: capture_security_activity( account=user, type="password-changed", - actor=request.user, + actor=actor, ip_address=request.META["REMOTE_ADDR"], send_email=True, ) diff --git a/src/sentry/users/api/endpoints/user_regions.py b/src/sentry/users/api/endpoints/user_regions.py index 69e4fb70a210..c5fd2622b3ee 100644 --- a/src/sentry/users/api/endpoints/user_regions.py +++ b/src/sentry/users/api/endpoints/user_regions.py @@ -8,6 +8,7 @@ from sentry.api.api_owners import ApiOwner from sentry.api.api_publish_status import ApiPublishStatus from sentry.api.base import control_silo_endpoint +from sentry.auth.principal import is_acting_user from sentry.auth.superuser import is_active_superuser from sentry.auth.system import is_system_auth from sentry.models.organizationmapping import OrganizationMapping @@ -30,7 +31,7 @@ class UserRegionEndpointPermissions(UserPermission): def has_object_permission( self, request: Request, view: Any, user: User | RpcUser | None = None ) -> bool: - if user and user.id == request.user.id and request.user.is_authenticated: + if user and is_acting_user(request.user, user.id) and request.user.is_authenticated: return True if is_system_auth(request.auth): return True diff --git a/tests/sentry/auth/test_principal.py b/tests/sentry/auth/test_principal.py index 66e93d3c0b99..f2fe7765313d 100644 --- a/tests/sentry/auth/test_principal.py +++ b/tests/sentry/auth/test_principal.py @@ -274,7 +274,7 @@ def _is_request_user_type_check(node: ast.AST) -> bool: # Lower these as callers move off. Exact, so the baseline can't drift back up. -REQUEST_USER_ID_BASELINE = 257 +REQUEST_USER_ID_BASELINE = 232 REQUEST_USER_TYPE_CHECK_BASELINE = 31 diff --git a/tests/sentry/auth_v2/utils/test_session.py b/tests/sentry/auth_v2/utils/test_session.py index 6e2df306b778..328a48eb38b9 100644 --- a/tests/sentry/auth_v2/utils/test_session.py +++ b/tests/sentry/auth_v2/utils/test_session.py @@ -1,11 +1,14 @@ from datetime import datetime from unittest.mock import Mock +from django.contrib.auth.models import AnonymousUser from django.contrib.sessions.backends.base import SessionBase from rest_framework.request import Request from sentry.auth_v2.utils.session import SessionBuilder, SessionSerializer +from sentry.service_accounts.principal import ServiceAccountPrincipal from sentry.testutils.cases import TestCase +from sentry.users.models.user import User EXPIRY_DATE = datetime(2023, 12, 31) @@ -114,7 +117,7 @@ def create_mock_user(self, **kwargs: bool) -> Mock: for key, value in properties.items(): mock_attributes[key] = value - self.request.user = Mock(**mock_attributes) + self.request.user = Mock(spec=User, **mock_attributes) return self.request.user def test_initialize_does_not_override_flags(self) -> None: @@ -152,7 +155,14 @@ def test_initialize_no_user(self) -> None: assert len(self.request.session.data) == 0 def test_initialize_anonymous_user(self) -> None: - self.create_mock_user(is_anonymous=True) + self.request.user = AnonymousUser() + self.session_builder.initialize_auth_flags() + assert len(self.request.session.data) == 0 + + def test_initialize_service_account(self) -> None: + self.request.user = ServiceAccountPrincipal( + service_account_id=1, organization_id=1, name="sa" + ) self.session_builder.initialize_auth_flags() assert len(self.request.session.data) == 0 diff --git a/tests/sentry/relocation/api/endpoints/test_index.py b/tests/sentry/relocation/api/endpoints/test_index.py index 8381636df2a6..2845053c12b0 100644 --- a/tests/sentry/relocation/api/endpoints/test_index.py +++ b/tests/sentry/relocation/api/endpoints/test_index.py @@ -6,6 +6,8 @@ import orjson from django.core.files.uploadedfile import SimpleUploadedFile +from django.test import override_settings +from django.urls import reverse from rest_framework import status from sentry.analytics.events.relocation_created import RelocationCreatedEvent @@ -29,7 +31,9 @@ ) from sentry.testutils.helpers.backups import generate_rsa_key_pair from sentry.testutils.helpers.datetime import freeze_time +from sentry.testutils.helpers.features import with_feature from sentry.testutils.helpers.options import override_options +from sentry.viewer_context import Actor, ActorType, ViewerContext, encode_viewer_context FRESH_INSTALL_PATH = get_fixture_path("backup", "fresh-install.json") TEST_DATE_ADDED = datetime(2023, 1, 23, 1, 23, 45, tzinfo=timezone.utc) @@ -276,6 +280,23 @@ def test_good_no_regular_user_with_query(self) -> None: assert len(response.data) == 1 assert response.data[0]["status"] == Relocation.Status.IN_PROGRESS.name + @override_settings(SEER_API_SHARED_SECRET="service-account-secret") + @with_feature("organizations:service-accounts") + def test_service_account_sees_no_relocations(self) -> None: + organization = self.create_organization(owner=self.owner) + service_account = self.create_service_account(organization=organization) + viewer_context = encode_viewer_context( + ViewerContext( + actor=Actor(type=ActorType.SERVICE_ACCOUNT, actor_id=service_account.id), + organization_id=organization.id, + ), + key="service-account-secret", + ) + + response = self.client.get(reverse(self.endpoint), HTTP_X_VIEWER_CONTEXT=viewer_context) + assert response.status_code == 200, response.content + assert response.data == [] + @override_options({"staff.ga-rollout": True}) def test_bad_unknown_status(self) -> None: self.login_as(user=self.staff_user, staff=True) diff --git a/tests/sentry/users/api/endpoints/test_user_authenticator_details.py b/tests/sentry/users/api/endpoints/test_user_authenticator_details.py index 8c96ab18fcd5..dd1a3914e6a1 100644 --- a/tests/sentry/users/api/endpoints/test_user_authenticator_details.py +++ b/tests/sentry/users/api/endpoints/test_user_authenticator_details.py @@ -441,6 +441,27 @@ def test_require_2fa__delete_with_multiple_auth__ok(self) -> None: assert not Authenticator.objects.filter(id=auth.id).exists() assert_security_email_sent("mfa-removed") + def test_superuser_removing_last_authenticator_logs_the_owner(self) -> None: + owner = self.create_user(email="owner@example.com") + interface = TotpInterface() + interface.enroll(owner) + assert interface.authenticator is not None + Authenticator.objects.auto_add_recovery_codes(owner) + superuser = self.create_user(email="admin@example.com", is_superuser=True) + self.login_as(user=superuser, superuser=True) + + with mock.patch("sentry.security.utils.logger") as logger: + self.get_success_response(owner.id, interface.authenticator.id, method="delete") + + removed = [ + call.kwargs["extra"] + for call in logger.info.call_args_list + if call.args == ("user.%s", "mfa-removed") + ] + # The TOTP device and its recovery codes, both the owner's + assert [extra["user_id"] for extra in removed] == [owner.id, owner.id] + assert {extra["actor_id"] for extra in removed} == {superuser.id} + def test_delete_last_authenticator_removes_recovery_codes(self) -> None: interface = TotpInterface() interface.enroll(self.user)