Skip to content

Commit

Permalink
chore(staff): Switch to checking options instead of feature flag (#67852
Browse files Browse the repository at this point in the history
)

Note: this is the second step described in
https://github.com/getsentry/getsentry/pull/13463

---
### Summary
Switch from feature flag to using staff option.

There are two options:
1. [User Email Option](#67742) -
this is set in the [options
automator](https://github.com/getsentry/sentry-options-automator/pull/974)
to limit staff to specific users
2. [GA Option](#67851) - this
will be used to turn staff on for all sentry employees (has not been set
in options automator yet)

To easily switch the tests to over from the feature flag, we override
the GA option when testing, which doesn't require us to know the user's
email (simplifies testing process quite a bit)
  • Loading branch information
schew2381 authored Mar 28, 2024
1 parent 4825f88 commit 483456b
Show file tree
Hide file tree
Showing 25 changed files with 119 additions and 106 deletions.
7 changes: 3 additions & 4 deletions src/sentry/api/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,13 @@
from rest_framework.views import APIView
from sentry_sdk import Scope

from sentry import analytics, features, options, tsdb
from sentry import analytics, options, tsdb
from sentry.api.api_owners import ApiOwner
from sentry.api.api_publish_status import ApiPublishStatus
from sentry.api.exceptions import StaffRequired, SuperuserRequired
from sentry.apidocs.hooks import HTTP_METHOD_NAME
from sentry.auth import access
from sentry.auth.staff import has_staff_option
from sentry.models.environment import Environment
from sentry.ratelimits.config import DEFAULT_RATE_LIMIT_CONFIG, RateLimitConfig
from sentry.silo import SiloLimit, SiloMode
Expand Down Expand Up @@ -256,9 +257,7 @@ def permission_denied(self, request, message=None, code=None):
permissions = self.get_permissions()
if request.user.is_authenticated and len(permissions) == 1:
permission_cls = permissions[0]
enforce_staff_permission = features.has(
"auth:enterprise-staff-cookie", actor=request.user
)
enforce_staff_permission = has_staff_option(request.user)

# TODO(schew2381): Remove SuperuserOrStaffFeatureFlaggedPermission
# from isinstance checks once feature flag is removed.
Expand Down
7 changes: 3 additions & 4 deletions src/sentry/api/endpoints/user_authenticator_details.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,14 @@
from rest_framework.request import Request
from rest_framework.response import Response

from sentry import features
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.api.bases.user import OrganizationUserPermission, UserEndpoint
from sentry.api.decorators import sudo_required
from sentry.api.serializers import serialize
from sentry.auth.authenticators.u2f import decode_credential_id
from sentry.auth.staff import is_active_staff
from sentry.auth.staff import has_staff_option, is_active_staff
from sentry.auth.superuser import is_active_superuser
from sentry.models.authenticator import Authenticator
from sentry.models.user import User
Expand Down Expand Up @@ -165,9 +164,9 @@ def delete(self, request: Request, user: User, auth_id, interface_device_id=None
return Response(status=status.HTTP_204_NO_CONTENT)

# We should only be able to delete the last auth method through the
# _admin portal, which is indicated by staff. After the feature flag is
# _admin portal, which is indicated by staff. After the option is
# removed, this will only check for is_active_staff.
if features.has("auth:enterprise-staff-cookie", actor=request.user):
if has_staff_option(request.user):
check_remaining_auth = not is_active_staff(request)
else:
check_remaining_auth = not is_active_superuser(request)
Expand Down
10 changes: 5 additions & 5 deletions src/sentry/api/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
TwoFactorRequired,
)
from sentry.auth import access
from sentry.auth.staff import is_active_staff
from sentry.auth.staff import has_staff_option, is_active_staff
from sentry.auth.superuser import SUPERUSER_ORG_ID, is_active_superuser
from sentry.auth.system import is_system_auth
from sentry.models.orgauthtoken import is_org_auth_token_auth, update_org_auth_token_last_used
Expand Down Expand Up @@ -118,13 +118,13 @@ def is_not_2fa_compliant(self, request, *args, **kwargs) -> bool:

# NOTE(schew2381): This is a temporary permission that does NOT perform an OR
# between SuperuserPermission and StaffPermission. Instead, it uses StaffPermission
# if the feature flag is enabled, and otherwise uses SuperuserPermission. We
# if the option is enabled for the user, and otherwise checks SuperuserPermission. We
# need this to handle the transition for endpoints that will only be accessible to
# staff but not superuser, that currently use SuperuserPermission. Once the
# feature is rolled out, we can delete this permission and use StaffPermission
# staff but not superuser, that currently use SuperuserPermission. Once staff is
# released to the everyone, we can delete this permission and use StaffPermission
class SuperuserOrStaffFeatureFlaggedPermission(BasePermission):
def has_permission(self, request: Request, view: object) -> bool:
enforce_staff_permission = features.has("auth:enterprise-staff-cookie", actor=request.user)
enforce_staff_permission = has_staff_option(request.user)

if enforce_staff_permission:
return StaffPermission().has_permission(request, view)
Expand Down
14 changes: 5 additions & 9 deletions src/sentry/auth/elevated_mode.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@

from rest_framework.request import Request

from sentry import features


class InactiveReason(str, Enum):
INVALID_IP = "invalid-ip"
Expand Down Expand Up @@ -50,19 +48,17 @@ def on_response(cls, response) -> None:
pass


# TODO(schew2381): Delete this method after the feature flag is removed
# TODO(schew2381): Delete this method after the option is removed
def has_elevated_mode(request: Request) -> bool:
"""
This is a temporary helper method that checks if the user on the request has
the staff feature flag enabled. If so, it checks is_active_staff and
otherwise defaults to checking is_active_superuser.
the staff option enabled. If so, it checks is_active_staff and otherwise
defaults to checking is_active_superuser.
"""
from sentry.auth.staff import is_active_staff
from sentry.auth.staff import has_staff_option, is_active_staff
from sentry.auth.superuser import is_active_superuser

enforce_staff_permission = features.has("auth:enterprise-staff-cookie", actor=request.user)

if enforce_staff_permission:
if has_staff_option(request.user):
return is_active_staff(request)

return is_active_superuser(request)
16 changes: 16 additions & 0 deletions src/sentry/auth/staff.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
from django.utils.crypto import constant_time_compare, get_random_string
from rest_framework.request import Request

from sentry import options
from sentry.auth.elevated_mode import ElevatedMode, InactiveReason
from sentry.auth.system import is_system_auth
from sentry.utils.auth import has_completed_sso
Expand Down Expand Up @@ -51,6 +52,21 @@ def is_active_staff(request: HttpRequest | Request) -> bool:
return staff.is_active


# TODO(schew2381): Delete after staff is GA'd and the options are removed
def has_staff_option(user) -> bool:
"""
This checks two options, the first being whether or not staff has been GA'd.
If not, it falls back to checking the second option which by email specifies which
users staff is enabled for.
"""
if options.get("staff.ga-rollout"):
return True

if (email := getattr(user, "email", None)) is None:
return False
return email in options.get("staff.user-email-allowlist")


class Staff(ElevatedMode):
allowed_ips = frozenset(ipaddress.ip_network(str(v), strict=False) for v in ALLOWED_IPS)

Expand Down
13 changes: 6 additions & 7 deletions src/sentry/sentry_apps/apps.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,9 @@
from rest_framework.exceptions import ValidationError
from sentry_sdk.api import push_scope

from sentry import analytics, audit_log, features
from sentry import analytics, audit_log
from sentry.api.helpers.slugs import sentry_slugify
from sentry.auth.staff import has_staff_option
from sentry.constants import SentryAppStatus
from sentry.coreapi import APIError
from sentry.db.postgres.transactions import in_test_hide_transaction_boundary
Expand Down Expand Up @@ -69,16 +70,14 @@ def expand_events(rolled_up_events: list[str]) -> set[str]:
)


# TODO(schew2381): Delete this method after the feature flag is removed
# TODO(schew2381): Delete this method after staff is GA'd and the options are removed
def _is_elevated_user(user) -> bool:
"""
This is a temporary helper method that checks if the user can become staff
if the feature flag is enabled. Otherwise, it defaults to checking that the
user can become a superuser.
if staff mode is enabled. Otherwise, it defaults to checking that the user
can become a superuser.
"""
return (
user.is_staff if features.has("auth:enterprise-staff-cookie", user) else user.is_superuser
)
return user.is_staff if has_staff_option(user) else user.is_superuser


@dataclasses.dataclass
Expand Down
16 changes: 8 additions & 8 deletions tests/sentry/api/endpoints/relocations/test_abort.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
from sentry.api.endpoints.relocations.abort import ERR_NOT_ABORTABLE_STATUS
from sentry.models.relocation import Relocation
from sentry.testutils.cases import APITestCase
from sentry.testutils.helpers.features import with_feature
from sentry.testutils.helpers.options import override_options
from sentry.utils.relocation import OrderedTask

TEST_DATE_ADDED = datetime(2023, 1, 23, 1, 23, 45, tzinfo=timezone.utc)
Expand Down Expand Up @@ -34,7 +34,7 @@ def setUp(self):
latest_task_attempts=1,
)

@with_feature("auth:enterprise-staff-cookie")
@override_options({"staff.ga-rollout": True})
def test_good_staff_abort_in_progress(self):
self.login_as(user=self.staff_user, staff=True)
self.relocation.status = Relocation.Status.PAUSE.value
Expand All @@ -53,7 +53,7 @@ def test_good_superuser_abort_in_progress(self):
assert response.data["status"] == Relocation.Status.FAILURE.name
assert response.data["step"] == Relocation.Step.PREPROCESSING.name

@with_feature("auth:enterprise-staff-cookie")
@override_options({"staff.ga-rollout": True})
def test_good_staff_abort_paused(self):
self.login_as(user=self.staff_user, staff=True)
self.relocation.status = Relocation.Status.PAUSE.value
Expand All @@ -72,7 +72,7 @@ def test_good_superuser_abort_paused(self):
assert response.data["status"] == Relocation.Status.FAILURE.name
assert response.data["step"] == Relocation.Step.PREPROCESSING.name

@with_feature("auth:enterprise-staff-cookie")
@override_options({"staff.ga-rollout": True})
def test_bad_staff_already_succeeded(self):
self.login_as(user=self.staff_user, staff=True)
self.relocation.status = Relocation.Status.SUCCESS.value
Expand All @@ -91,7 +91,7 @@ def test_bad_superuser_already_succeeded(self):
assert response.data.get("detail") is not None
assert response.data.get("detail") == ERR_NOT_ABORTABLE_STATUS

@with_feature("auth:enterprise-staff-cookie")
@override_options({"staff.ga-rollout": True})
def test_bad_staff_already_failed(self):
self.login_as(user=self.staff_user, staff=True)
self.relocation.status = Relocation.Status.FAILURE.value
Expand All @@ -110,7 +110,7 @@ def test_bad_superuser_already_failed(self):
assert response.data.get("detail") is not None
assert response.data.get("detail") == ERR_NOT_ABORTABLE_STATUS

@with_feature("auth:enterprise-staff-cookie")
@override_options({"staff.ga-rollout": True})
def test_bad_staff_not_found(self):
self.login_as(user=self.staff_user, staff=True)
does_not_exist_uuid = uuid4().hex
Expand All @@ -121,8 +121,8 @@ def test_bad_superuser_not_found(self):
does_not_exist_uuid = uuid4().hex
self.get_error_response(str(does_not_exist_uuid), status_code=404)

@with_feature("auth:enterprise-staff-cookie")
def test_superuser_fails_with_flag(self):
@override_options({"staff.ga-rollout": True})
def test_superuser_fails_with_option(self):
self.login_as(user=self.superuser, superuser=True)
self.get_error_response(self.relocation.uuid, status_code=403)

Expand Down
4 changes: 2 additions & 2 deletions tests/sentry/api/endpoints/relocations/test_cancel.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
)
from sentry.models.relocation import Relocation
from sentry.testutils.cases import APITestCase
from sentry.testutils.helpers.features import with_feature
from sentry.testutils.helpers.options import override_options
from sentry.utils.relocation import OrderedTask

TEST_DATE_ADDED = datetime(2023, 1, 23, 1, 23, 45, tzinfo=timezone.utc)
Expand Down Expand Up @@ -38,7 +38,7 @@ def setUp(self):
latest_task_attempts=1,
)

@with_feature("auth:enterprise-staff-cookie")
@override_options({"staff.ga-rollout": True})
def test_good_staff_cancel_in_progress_at_next_step(self):
staff_user = self.create_user(is_staff=True)
self.login_as(user=staff_user, staff=True)
Expand Down
12 changes: 6 additions & 6 deletions tests/sentry/api/endpoints/relocations/test_details.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

from sentry.models.relocation import Relocation
from sentry.testutils.cases import APITestCase
from sentry.testutils.helpers.features import with_feature
from sentry.testutils.helpers.options import override_options
from sentry.utils.relocation import OrderedTask

TEST_DATE_ADDED = datetime(2023, 1, 23, 1, 23, 45, tzinfo=timezone.utc)
Expand Down Expand Up @@ -39,14 +39,14 @@ def test_good_superuser_found(self):
response = self.get_success_response(self.relocation.uuid, status_code=200)
assert response.data["uuid"] == str(self.relocation.uuid)

@with_feature("auth:enterprise-staff-cookie")
def test_good_staff_found_with_flag(self):
@override_options({"staff.ga-rollout": True})
def test_good_staff_found_with_option(self):
self.login_as(user=self.staff_user, staff=True)
response = self.get_success_response(self.relocation.uuid, status_code=200)
assert response.data["uuid"] == str(self.relocation.uuid)

@with_feature("auth:enterprise-staff-cookie")
def test_bad_superuser_fails_with_flag(self):
@override_options({"staff.ga-rollout": True})
def test_bad_superuser_fails_with_option(self):
self.login_as(user=self.superuser, superuser=True)
self.get_error_response(self.relocation.uuid, status_code=403)

Expand All @@ -55,7 +55,7 @@ def test_bad_superuser_not_found(self):
does_not_exist_uuid = uuid4().hex
self.get_error_response(str(does_not_exist_uuid), status_code=404)

@with_feature("auth:enterprise-staff-cookie")
@override_options({"staff.ga-rollout": True})
def test_bad_staff_not_found(self):
self.login_as(user=self.staff_user, staff=True)
does_not_exist_uuid = uuid4().hex
Expand Down
18 changes: 10 additions & 8 deletions tests/sentry/api/endpoints/relocations/test_index.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
from sentry.testutils.factories import get_fixture_path
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.utils import json
from sentry.utils.relocation import OrderedTask
Expand Down Expand Up @@ -104,7 +103,7 @@ def setUp(self):

self.success_uuid = Relocation.objects.get(status=Relocation.Status.SUCCESS.value)

@with_feature("auth:enterprise-staff-cookie")
@override_options({"staff.ga-rollout": True})
def test_good_staff_simple(self):
self.login_as(user=self.staff_user, staff=True)
response = self.get_success_response(status_code=200)
Expand Down Expand Up @@ -529,9 +528,10 @@ def test_good_with_invalid_autopause_option(
sender=RelocationIndexEndpoint,
)

@override_options({"relocation.enabled": False, "relocation.daily-limit.small": 1})
@override_options(
{"relocation.enabled": False, "relocation.daily-limit.small": 1, "staff.ga-rollout": True}
)
@patch("sentry.tasks.relocation.uploading_complete.delay")
@with_feature("auth:enterprise-staff-cookie")
def test_good_staff_when_feature_disabled(
self,
uploading_complete_mock: Mock,
Expand Down Expand Up @@ -948,8 +948,9 @@ def test_bad_missing_owner(

assert relocation_link_promo_code_signal_mock.call_count == 0

@override_options({"relocation.enabled": True, "relocation.daily-limit.small": 1})
@with_feature("auth:enterprise-staff-cookie")
@override_options(
{"relocation.enabled": True, "relocation.daily-limit.small": 1, "staff.ga-rollout": True}
)
def test_bad_staff_nonexistent_owner(
self, relocation_link_promo_code_signal_mock: Mock, analytics_record_mock: Mock
):
Expand Down Expand Up @@ -1150,8 +1151,9 @@ def test_bad_throttle_if_daily_limit_reached(
sender=RelocationIndexEndpoint,
)

@override_options({"relocation.enabled": True, "relocation.daily-limit.small": 1})
@with_feature("auth:enterprise-staff-cookie")
@override_options(
{"relocation.enabled": True, "relocation.daily-limit.small": 1, "staff.ga-rollout": True}
)
def test_good_no_throttle_for_staff(
self, relocation_link_promo_code_signal_mock: Mock, analytics_record_mock: Mock
):
Expand Down
4 changes: 2 additions & 2 deletions tests/sentry/api/endpoints/relocations/test_pause.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
)
from sentry.models.relocation import Relocation
from sentry.testutils.cases import APITestCase
from sentry.testutils.helpers.features import with_feature
from sentry.testutils.helpers.options import override_options
from sentry.utils.relocation import OrderedTask

TEST_DATE_ADDED = datetime(2023, 1, 23, 1, 23, 45, tzinfo=timezone.utc)
Expand Down Expand Up @@ -41,7 +41,7 @@ def setUp(self):
latest_task_attempts=1,
)

@with_feature("auth:enterprise-staff-cookie")
@override_options({"staff.ga-rollout": True})
def test_good_staff_pause_asap(self):
self.login_as(user=self.staff_user, staff=True)
response = self.get_success_response(self.relocation.uuid, status_code=200)
Expand Down
Loading

0 comments on commit 483456b

Please sign in to comment.