Skip to content
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

Add observability for DEFAULT_AUTHENTICATION_CLASSES #32899

Closed
robrap opened this issue Aug 2, 2023 · 4 comments
Closed

Add observability for DEFAULT_AUTHENTICATION_CLASSES #32899

robrap opened this issue Aug 2, 2023 · 4 comments
Assignees

Comments

@robrap
Copy link
Contributor

robrap commented Aug 2, 2023

This is ticket is for adding observability for #32802.

  1. The first idea is to understand which endpoints are actually using the defaults today. We could do this by creating a DefaultSessionAuthentication class in edx-platform as a lightweight wrapper to SessionAuthentication, and that overrides authenticate(), similar to how it's done in JwtAuthentication, but simply adds the following monitoring before returning the result from super.
set_custom_attribute('using_default_authentication_classes', True)

We then update the setting in edx-platform to:

    'DEFAULT_AUTHENTICATION_CLASSES': [
        'some.path.DefaultSessionAuthentication',
        'rest_framework.authentication.BasicAuthentication'
    ],

The docstring of DefaultSessionAuthentication should explain that this should not be used anywhere except in DEFAULT_AUTHENTICATION_CLASSES.

  1. The second idea is to add monitoring to see if we can drop BasicAuthentication from this default list, since we don't really want it as a default. We could implement a DefaultBasicAuthentication class with a custom attribute like we have in BearerAuthentication.
@robrap
Copy link
Contributor Author

robrap commented Aug 3, 2023

Originally I was hoping to use DRF's successful_authenticator as detailed in this PR comment, but it is only available from the DRF request, not the WSGI request we have in the middleware.

@robrap
Copy link
Contributor Author

robrap commented Aug 3, 2023

@feanil: I wanted to see how and if we should use request.successful_authenticator inside the auth classes, and I'm seeing really odd behavior that makes me think I have no idea how DRF authentication happens.

Let me explain:

  1. I added a breakpoint() before we start doing JwtAuthentication here.
  2. I loaded the following in devstack: http://localhost:18000/api/toggles/v0/state/
  3. In the debugger, I looked at the request before performing JwtAuthentication and saw the following:
-> user_and_auth = super().authenticate(request)
(Pdb) request.user
<User: staff>
(Pdb) request.successful_authenticator
<rest_framework.authentication.SessionAuthentication object at 0x7fe469043160>

I confirmed that this view has JwtAuthentication before SessionAuthentication here. So, some code is authenticating the user before we even get there using SessionAuthentication. It's just confusing.

UPDATE: Additionally, adding a breakpoint() to SessionAuthentication, I never get stopped there. If a put a syntax error in it, it fails to load, so I know I'm using the code I am editing. I have no idea where or how the earlier authentication is happening on the request.

UPDATE 2:
I now understand:

  • The user is being set before getting to DRF in CacheBackedAuthenticationMiddleware. The DRF request does get this user from the start, before authenticating.
  • However, the DRF request doesn't have ._authenticator or .successful_authenticator set. I was calling successful_authenticator from in pdb, and didn't realize it was computing by authenticating at that time, but it was ignoring my breakpoints because I called it from pdb.

Conclusion: We probably should not use successful_authenticator.

@robrap
Copy link
Contributor Author

robrap commented Aug 14, 2023

I created the PR #33003, which basically implements this ticket.

@robrap robrap self-assigned this Aug 14, 2023
@robrap robrap added this to Arch-BOM Aug 14, 2023
@robrap robrap moved this to In Code Review in Arch-BOM Aug 14, 2023
@robrap
Copy link
Contributor Author

robrap commented Aug 15, 2023

This work has been completed.

@robrap robrap closed this as completed Aug 15, 2023
@github-project-automation github-project-automation bot moved this from In Code Review to Done in Arch-BOM Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant