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

feat(palm)!: backport instructor APIs conversion to DRF #702

Draft
wants to merge 6 commits into
base: opencraft-release/palm.1
Choose a base branch
from

Conversation

Agrendalath
Copy link
Member

@Agrendalath Agrendalath commented Oct 22, 2024

TODO: Add testing instructions.

curl -X POST -d "grant_type=password&client_id=test&client_secret=test&username=<email>&password=<password>&token_type=jwt" http://localhost:18000/oauth2/access_token

Authorization: JWT <token>

http://localhost:18000/courses/course-v1:edX+DemoX+Demo_Course/instructor/api/modify_access
form-data:

unique_student_identifier:audit
rolename:staff
action:allow // (or "revoke")

http://localhost:18000/courses/course-v1:edX+DemoX+Demo_Course/instructor/api/list_course_role_members
form-data:

rolename:staff

Private-ref: BB-9250

robrap and others added 6 commits October 22, 2024 19:10
Added new authentication classes to be used in
DEFAULT_AUTHENTICATION_CLASSES that include
observability. This will enable us to see more
about the endpoints using the defaults, to help
us make choices about changes in the defaults.

We make the DRF default of Session and Basic
Authentication explicit by setting
DEFAULT_AUTHENTICATION_CLASSES explicitly.

(cherry picked from commit b9134c6)
Removed BasicAuthentication as a default from DRF
endpoints that have not overridden the authentication
classes. It appears this is not in use, and was just
implicitly a default because it came from DRF's
defaults.

See DEPR: openedx#33028

(cherry picked from commit 7113624)
By default DRF sets 'DEFAULT_AUTHENTICATION_CLASSES' to:

```
[
    'rest_framework.authentication.SessionAuthentication',
    'rest_framework.authentication.BasicAuthentication'
]
```

We also want to allow for JWT Authentication as a valid default auth
choice.  This will allow users to send JWT tokens in the authorization
header to any existing API endpoints and access them. If any APIs have
set custom authentication classes, this will not override that.

I believe this is a fairly safe change to make since it only adds one
authentication class and does not impact authorization of any of the
endpoints that might be affected.

Note: This change changes the default for both the LMS and CMS because
`cms/envs/common.py` imports this value from the LMS.

BREAKING CHANGE: For any affected endpoint that also required the user
to be authenticated, the endpoint will now return a 401 in place of a
403 when the user is not authenticated.

- See [these DRF docs](https://github.com/encode/django-rest-framework/blob/master/docs/api-guide/authentication.md#unauthorized-and-forbidden-responses) for a deeper explanation about why this changes.

- Here is [an example endpoint](https://github.com/openedx/edx-platform/blob/b8ecfed67dc0520b8c4d95de3096b35acc083611/openedx/core/djangoapps/embargo/views.py#L20-L21) that does not override defaults and checks for IsAuthenticated.

Generally speaking, this is should not be a problem. An issue would
appear only if the caller of the endpoint is specifically handling 403s
in a way that would be missed for 401s.

(cherry picked from commit 7af2b1d)
Adding generic permission class. Added standard authentication classes.

(cherry picked from commit 39dd3c0)
* feat: upgrading simple api to drf compatible.

(cherry picked from commit 99760f8)
@Agrendalath Agrendalath self-assigned this Oct 22, 2024
@Agrendalath Agrendalath changed the title feat!(palm): :backport instructor DRF APIs feat(palm)!: :backport instructor DRF APIs Oct 22, 2024
@Agrendalath Agrendalath changed the title feat(palm)!: :backport instructor DRF APIs feat(palm)!: backport instructor DRF APIs Oct 22, 2024
@Agrendalath Agrendalath changed the title feat(palm)!: backport instructor DRF APIs feat(palm)!: backport instructor APIs conversion to DRF Oct 22, 2024
@Agrendalath Agrendalath marked this pull request as draft October 22, 2024 17:50
@Cup0fCoffee
Copy link
Member

@Agrendalath What is the purpose of this PR? Should I just ignore it in the context of the upgrade?

@Agrendalath
Copy link
Member Author

@Cup0fCoffee, it's okay to ignore this one. These commits should be present in Sumac anyway.

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

Successfully merging this pull request may close these issues.

5 participants