New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(superuser): Add loggers to _needs_validation #66364
Conversation
b9a14e4
to
5cc92d1
Compare
5cc92d1
to
64f7a03
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #66364 +/- ##
==========================================
- Coverage 84.29% 84.29% -0.01%
==========================================
Files 5309 5309
Lines 237200 237198 -2
Branches 41038 41038
==========================================
- Hits 199957 199951 -6
- Misses 37024 37028 +4
Partials 219 219
|
DISABLE_SU_FORM_U2F_CHECK_FOR_LOCAL = getattr( | ||
settings, "DISABLE_SU_FORM_U2F_CHECK_FOR_LOCAL", False | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to remove this in an earlier PR b/c we no longer use this in this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to remove it from https://github.com/getsentry/sentry/pull/66364/files#diff-dcd4801baa22c78923c11bf52c23caa202d80d2990d78b0500507f78d9950c95R71 as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no I changed it recently so we use it in that file, which is what i'm debugging now
@@ -191,7 +191,7 @@ def test_su_access_logs(self, logger): | |||
superuser = Superuser(request, org_id=None) | |||
superuser.set_logged_in(request.user) | |||
assert superuser.is_active is True | |||
assert logger.info.call_count == 2 | |||
assert logger.info.call_count == 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷 personally don't think we need to assert logger calls at all
There's a bug where we're no longer requiring superuser access category and reason validation, which is caused by https://github.com/getsentry/sentry/pull/66043/files. I want to validate what those booleans are in order to find out if this is the cause of the bug.
There's a bug where we're no longer requiring superuser access category and reason validation, which is caused by https://github.com/getsentry/sentry/pull/66043/files.
I want to validate what those booleans are in order to find out if this is the cause of the bug.