feat(staff): Create initial staff class for _admin mode #61653

schew2381 · 2023-12-13T00:03:58Z

In future PRs, I will create + test the staff middleware and index endpoint

Summary

This PR creates a new staff class that is modeled off of the existing Superuser class. I removed all the code that was added to support the superuser modal because _admin will not require that, only 2FA.

Similarities

is_active_staff is not a class method but a helper function, that checks for staff the same way as is_active_superuser checks for superuser
get_session_data is identical
set_logged_out and _set_logged_out are identical
on_response is identical

Differences

set_logged_in for Staff vs Superuser
- Staff does not exist for self-hosted, is not enabled automatically upon login, and does not require a form to enable (only SSO/2FA)
- Therefore Staff does not have a _needs_validation function
- Staff also does not have form checks
is_active class property does not check for an expiry time depending on in you switched orgs b/c staff mode is not tied to any org
- Thus we don't need _check_expired_on_org_change
STAFF_ORG_ID, instead of setting this is init, we know keep it as a file constant because it should never change

Closes https://github.com/getsentry/team-enterprise/issues/5

codecov · 2023-12-13T00:29:00Z

Codecov Report

Merging #61653 (008b4e6) into master (e331269) will increase coverage by 0.86%.
Report is 3 commits behind head on master.
The diff coverage is 82.05%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #61653      +/-   ##
==========================================
+ Coverage   80.35%   81.22%   +0.86%     
==========================================
  Files        5187     5188       +1     
  Lines      228691   228847     +156     
  Branches    38396    38428      +32     
==========================================
+ Hits       183770   185885    +2115     
+ Misses      39050    37294    -1756     
+ Partials     5871     5668     -203

Files	Coverage Δ
src/sentry/auth/staff.py	`82.05% <82.05%> (ø)`

... and 218 files with indirect coverage changes

cathteng

i still need to look at tests

src/sentry/auth/staff.py

src/sentry/auth/superuser.py

src/sentry/auth/staff.py

cathteng

lgtm but this is also an ever evolving project

src/sentry/auth/staff.py

ykamo001

I know I'm a little late to this PR, sorry! But happy to go over the comments more if you want. Let me know if I've misunderstood something, or don't have the right context.

src/sentry/auth/staff.py

ykamo001 · 2023-12-19T22:03:36Z

src/sentry/auth/staff.py

+def is_active_staff(request: Request) -> bool:
+    if is_system_auth(getattr(request, "auth", None)):
+        return True
+    stf = getattr(request, "staff", None) or Staff(request)


considering that Staff initializer can throw errors, would it be more prudent to have this around a try/except so that we don't have unintended behaviors or errors? Something along the lines of:

try: stf = getattr(request, "staff", None) or Staff(request) except Exception as err: logger.error(f"unexpected error trying to determine active staff user, defaulting to False", exc_info=err) return False return stf.is_active

ykamo001 · 2023-12-19T22:13:30Z

src/sentry/auth/staff.py

+        # if the user has changed
+        if str(self.request.user.id) != self.uid:
+            return False
+        return self._is_active


After reading the code all the way, I now know _populate is at the end supposed to help establish this value before being referenced, but if that's the case, it would be better in the __init__ to establish this value first as a default for safer code. Reading this code from top to bottom, it's not clear that the value gets dynamically added to the instance; I thought "Oh no, we're referencing a value that doesn't exist!" haha

We have to go 2 levels deep (from populate -> set_logged_in/out) to know this is a class variable we can use in this class, and if something happens in that chain, we could be blowing up here/throwing an error.

src/sentry/auth/staff.py

ykamo001 · 2023-12-19T22:29:03Z

src/sentry/auth/staff.py

+            )
+            return
+
+        session_token = data.get("tok")


nit: do we have constants for these keys/references?

src/sentry/auth/staff.py

ykamo001 · 2023-12-19T22:34:30Z

src/sentry/auth/staff.py

+            return False, "invalid-ip"
+        return True, None
+
+    def get_session_data(self, current_datetime=None):


There is a lot of checks and calculations happening in this function. Can we break those apart? I think the code would be much easier to read, and the flow/logic much more feasible to follow in the mind!

We also seem to be populating some keys and not others if a condition fails. For example, if idl is less than current, we will return early and not populate the rest of the keys (exp). Is that what we expect? Should the rest of the keys still be defaulted so they exist? Could we pull the populating/checking of those keys out to another function so that we could handle that decision more easily?

ykamo001 · 2023-12-19T22:38:20Z

src/sentry/auth/staff.py

+
+        return data
+
+    def _populate(self, current_datetime=None):


Could we name this function to something else? Populate is very generic, I'm not sure what it's supposed to be populating, and if it's okay to rerun this function or add to another private function.

If this is only meant to be run once, could we add some guard rails/checks for that? Or restructure this function so that it returns the values we want to populate and the __init__ can be responsible for adding them to the class?

ykamo001 · 2023-12-19T22:40:59Z

src/sentry/auth/staff.py

+            if not self.is_active:
+                if self._inactive_reason:
+                    logger.warning(
+                        "staff.%s",
+                        self._inactive_reason,
+                        extra={
+                            "ip_address": request.META["REMOTE_ADDR"],
+                            "user_id": request.user.id,
+                        },
+                    )
+                else:
+                    logger.warning(
+                        "staff.inactive-unknown-reason",
+                        extra={
+                            "ip_address": request.META["REMOTE_ADDR"],
+                            "user_id": request.user.id,
+                        },
+                    )


Do we need to do this logic in this function at this level? Right now, is_activate property needs _populate to finish first so that _is_active gets added to the class, now _populate needs is_active to function correctly. It's creating this cyclic dependency effect, and it would be best to pull that out.

ykamo001 · 2023-12-19T22:46:53Z

Super exciting work here Seiji!!

Create staff middleware and tests. The code has been copied and adjusted over from the superuser middleware. Requires #61653 Closes https://github.com/getsentry/team-enterprise/issues/6

schew2381 self-assigned this Dec 13, 2023

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Dec 13, 2023

vercel bot deployed to Preview December 13, 2023 00:06 View deployment

vercel bot deployed to Preview December 13, 2023 00:51 View deployment

schew2381 force-pushed the seiji/feat/create-staff-class branch from 25f422c to 2743786 Compare December 13, 2023 18:13

vercel bot deployed to Preview December 13, 2023 18:16 View deployment

vercel bot deployed to Preview December 13, 2023 18:48 View deployment

schew2381 force-pushed the seiji/feat/create-staff-class branch from 8b7317e to a5a300e Compare December 14, 2023 23:20

vercel bot deployed to Preview December 14, 2023 23:23 View deployment

vercel bot deployed to Preview December 15, 2023 00:05 View deployment

vercel bot deployed to Preview December 15, 2023 22:07 View deployment

schew2381 requested a review from a team December 15, 2023 22:08

schew2381 assigned cathteng Dec 15, 2023

schew2381 requested a review from leedongwei December 15, 2023 22:08

schew2381 unassigned cathteng Dec 15, 2023

schew2381 requested a review from cathteng December 15, 2023 22:08

vercel bot deployed to Preview December 15, 2023 22:11 View deployment

cathteng reviewed Dec 15, 2023

View reviewed changes

src/sentry/auth/staff.py Show resolved Hide resolved

src/sentry/auth/superuser.py Outdated Show resolved Hide resolved

src/sentry/auth/staff.py Outdated Show resolved Hide resolved

src/sentry/auth/staff.py Show resolved Hide resolved

vercel bot deployed to Preview December 15, 2023 22:49 View deployment

vercel bot deployed to Preview December 15, 2023 22:53 View deployment

vercel bot deployed to Preview December 15, 2023 22:57 View deployment

schew2381 marked this pull request as ready for review December 15, 2023 22:57

schew2381 requested a review from a team as a code owner December 15, 2023 22:57

schew2381 force-pushed the seiji/feat/create-staff-class branch from d027250 to 439733b Compare December 18, 2023 18:05

vercel bot deployed to Preview December 18, 2023 18:08 View deployment

schew2381 mentioned this pull request Dec 18, 2023

feat(staff): Create staff middleware #61956

Merged

cathteng approved these changes Dec 18, 2023

View reviewed changes

schew2381 added 2 commits December 18, 2023 17:38

create initial class

51888f4

first test

bbf4965

schew2381 added 9 commits December 18, 2023 17:38

add failing superuser test to check org_id variable

09d9343

add sso test

b811e20

finish up to test_changed_user

819cc23

finish all tests

f44d24c

remove final prints in test superuser

a940525

fix typing and remove org_id

45de444

fix tests b/c we don't need to mock SSO check anymore

1cfabf6

remove breakpoint

aaf55dc

add back disabling SSO check for local dev testing

008b4e6

schew2381 force-pushed the seiji/feat/create-staff-class branch from 439733b to 008b4e6 Compare December 18, 2023 22:38

vercel bot deployed to Preview December 18, 2023 22:41 View deployment

schew2381 merged commit aa13083 into master Dec 18, 2023
49 of 50 checks passed

schew2381 deleted the seiji/feat/create-staff-class branch December 18, 2023 23:51

leedongwei reviewed Dec 19, 2023

View reviewed changes

src/sentry/auth/staff.py Show resolved Hide resolved

ykamo001 reviewed Dec 19, 2023

View reviewed changes

schew2381 mentioned this pull request Dec 20, 2023

nit: Cleanup staff and superuser #62103

Merged

github-actions bot locked and limited conversation to collaborators Jan 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(staff): Create initial staff class for _admin mode #61653

feat(staff): Create initial staff class for _admin mode #61653

schew2381 commented Dec 13, 2023 •

edited

Loading

codecov bot commented Dec 13, 2023 •

edited

Loading

cathteng left a comment

cathteng left a comment

ykamo001 left a comment

ykamo001 Dec 19, 2023

ykamo001 Dec 19, 2023

ykamo001 Dec 19, 2023

ykamo001 Dec 19, 2023

ykamo001 Dec 19, 2023

ykamo001 Dec 19, 2023

ykamo001 commented Dec 19, 2023

feat(staff): Create initial staff class for _admin mode #61653

feat(staff): Create initial staff class for _admin mode #61653

Conversation

schew2381 commented Dec 13, 2023 • edited Loading

Summary

Similarities

Differences

codecov bot commented Dec 13, 2023 • edited Loading

Codecov Report

cathteng left a comment

Choose a reason for hiding this comment

cathteng left a comment

Choose a reason for hiding this comment

ykamo001 left a comment

Choose a reason for hiding this comment

ykamo001 Dec 19, 2023

Choose a reason for hiding this comment

ykamo001 Dec 19, 2023

Choose a reason for hiding this comment

ykamo001 Dec 19, 2023

Choose a reason for hiding this comment

ykamo001 Dec 19, 2023

Choose a reason for hiding this comment

ykamo001 Dec 19, 2023

Choose a reason for hiding this comment

ykamo001 Dec 19, 2023

Choose a reason for hiding this comment

ykamo001 commented Dec 19, 2023

schew2381 commented Dec 13, 2023 •

edited

Loading

codecov bot commented Dec 13, 2023 •

edited

Loading