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

[DEPR] Remove EdxRestApiClient usage in edx-enterprise #42

Closed
Tracked by #37
feanil opened this issue Mar 8, 2022 · 2 comments · Fixed by openedx/edx-enterprise#1532
Closed
Tracked by #37

[DEPR] Remove EdxRestApiClient usage in edx-enterprise #42

feanil opened this issue Mar 8, 2022 · 2 comments · Fixed by openedx/edx-enterprise#1532
Assignees
Labels
depr Proposal for deprecation & removal per OEP-21

Comments

@feanil
Copy link

feanil commented Mar 8, 2022

"Capturing Notes from Slack from Nimisha about what needs to be done here to deal with the JWT tokens in use right now in edx-enterprise:

OAuthAPIClient is intended to be used for backend-to-backend  API calls. (See this ADR.)That is, it works for when Microservice A wants to call Microservice B on behalf of itself (A).It does not work for when Microservice A wants to call Microservice B on behalf of a specific user (U). (edited) 

We typically have frontends needing to call APIs on behalf of users (U).For backend-to-backend, we typically have these calls made in asynchronous celery tasks (in the server’s context) - not within a user’s call.

It looks like the user passed here is a service-user and not an end-user: https:/edx/edx-enterprise/blob/b55a50473775be18505565b99d7966389fb81ca9/enterprise/api_client/discovery.py#L426 

 So… while some additional refactoring will be needed, it shouldn’t require code changes on the Discovery-side.

 Here’s what I believe needs to happen:Configure OAuth Settings ([@Rraposa|https://edx-internal.slack.com/team/UDA40RS1J] Is there an easier way to do this?)

  1. edx-enterprise needs to define its own ENTERPRISE_BACKEND_SERVICE_EDX_OAUTH2_KEY and ENTERPRISE_BACKEND_SERVICE_EDX_OAUTH2_SECRET. These ^ are to be shared only with the “User Service” and no other service.
  2. You would define values for these on Devstack. For Prod and Stage, you would not have raw access to them, only access to their encrypted forms that you’ll need to configure. You can see how this is done in other services when you search for BACKEND_SERVICE_EDX_OAUTH2_KEY and BACKEND_SERVICE_EDX_OAUTH2_SECRET in https:/edx/edx-internal|https:/edx/edx-internal .
  3. Note: edx-enterprise will want to prefix its configuration settings with ENTERPRISE since edx-enterprise is currently a “plugin” in edx-platform instead of its own service - to prevent name collisions with other plugins.
  4. Then, add these configured Keys and Secrets in devstack (using provisioning) and each of the necessary environments (stage, prod) using the directions provided at https:/edx/course-discovery/blob/master/docs/advanced.rst#oauth2

Code Changes

Refactor 

CourseCatalogApiServiceClient to no longer get the service user.

Refactor 

CourseCatalogApiClient to no longer require a user.

Refactor 

course_discovery_api_client to no longer build JWTs. Instead use the above configuration settings to call OAuthAPIClient"

Other Info

Original Jira Issue: https://openedx.atlassian.net/browse/DEPR-134

@feanil feanil mentioned this issue Mar 8, 2022
14 tasks
@github-actions github-actions bot added the depr Proposal for deprecation & removal per OEP-21 label Mar 8, 2022
dyudyunov added a commit to dyudyunov/devstack that referenced this issue Apr 13, 2022
dyudyunov added a commit to raccoongang/edx-platform that referenced this issue Apr 13, 2022
This is a part of openedx/public-engineering#42

- add settings for enterprise-backend-service DOT application
- update utils used by enterprise to get rid of EdxRestAPIClient
- original utils stays in the code (to keep edx-platform api
clients working) till the
openedx/public-engineering#39 deprecation
work will be done
dianakhuang pushed a commit to openedx-unsupported/devstack that referenced this issue Apr 14, 2022
johnnagro pushed a commit to openedx/edx-platform that referenced this issue Apr 21, 2022
…0240)

* refactor: enterprise dependencies for EdxRestAPIClient replacement

This is a part of openedx/public-engineering#42

- add settings for enterprise-backend-service DOT application
- update utils used by enterprise to get rid of EdxRestAPIClient
- original utils stays in the code (to keep edx-platform api
clients working) till the
openedx/public-engineering#39 deprecation
work will be done

* fix: fix typo in the docstring
@feanil
Copy link
Author

feanil commented Apr 22, 2022

@dyudyunov can you confirm that this can be closed? It looks like all the relevant PRs have merged.

@dyudyunov
Copy link

hi, @feanil
confirming, this can be closed

@feanil feanil closed this as completed Apr 22, 2022
jawad-khan pushed a commit to openedx/edx-platform that referenced this issue Jun 14, 2022
…0240)

* refactor: enterprise dependencies for EdxRestAPIClient replacement

This is a part of openedx/public-engineering#42

- add settings for enterprise-backend-service DOT application
- update utils used by enterprise to get rid of EdxRestAPIClient
- original utils stays in the code (to keep edx-platform api
clients working) till the
openedx/public-engineering#39 deprecation
work will be done

* fix: fix typo in the docstring
nsprenkle pushed a commit to openedx-unsupported/devstack that referenced this issue Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
depr Proposal for deprecation & removal per OEP-21
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants