feat: allow oauth configuration per site and backend #32656

navinkarkera · 2023-07-05T15:44:35Z

Description

Currently, we can only add oauth configuration for a given backend to only a single site. This PR includes site_id in KEY_FIELDS for oauth configuration provider allowing a backend configuration for each site.

Supporting information

Mostly copied from eduNEXT/edunext-platform#601 except for minor changes like ignoring site_id in provider id as site_id is already considered while fetching the correct configuration.

Testing instructions

Get lms devstack up and running using make lms-up
Add some sites via admin: http://localhost:18000/admin/sites/site/
Add dummy oauth config for multiple sites: http://localhost:18000/admin/third_party_auth/oauth2providerconfig/
Each site should have a current configuration.
Without this PR, only one current configuration is enabled even if sites are different.

openedx-webhooks · 2023-07-05T15:44:40Z

Thanks for the pull request, @navinkarkera! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

common/djangoapps/third_party_auth/tests/test_provider.py

kaustavb12

👍

Already reviewed and tested the internal PR open-craft#542

This PR is identical to that.

mphilbrick211 · 2023-07-17T23:28:20Z

Hi @navinkarkera - is this ready for review?

* feat: allow oauth configuration per site and backend Allows admins to configure same oauth backend for multiple sites (cherry picked from commit efd2ef9) * fix: skip pipeline if oauth provider for site is not setup (cherry picked from commit 29f8494) * refactor: remove site_id from provider_id (cherry picked from commit 236f7a5) --------- Cherry pick of #542 Upstream PR openedx#32656 [BB-7589](https://tasks.opencraft.com/browse/BB-7589) --------- Co-authored-by: Navin Karkera <navin@opencraft.com>

mphilbrick211 · 2023-09-12T18:20:54Z

Hi @navinkarkera! Flagging the new tests that have popped up, per the recent update: https://discuss.openedx.org/t/minor-change-to-edx-platform-check-statuses/11131

mphilbrick211 · 2023-09-14T14:06:24Z

Hi @openedx/arch-bom! is this something you can please review/merge for us? Thanks!

robrap · 2023-09-14T15:53:10Z

Before digging into details, I made a comment related on this DEPR openedx/platform-roadmap#21 (comment) on site_configuration, because it would be useful to know direction around this.

robrap

Thank you. Mostly minor comments, and hoping that new tests would help me better understand the actual core changes of this PR.

common/djangoapps/third_party_auth/tests/test_provider.py

common/djangoapps/third_party_auth/pipeline.py

common/djangoapps/third_party_auth/models.py

navinkarkera · 2023-09-28T09:59:06Z

@robrap I have updated the PR to address your comments, please have a look when you find some time.

robrap

I'd like to see some of the minor improvements to the test if you can, but none of my comments are blocking. Thank you!

common/djangoapps/third_party_auth/tests/test_provider.py

robrap · 2023-09-29T15:06:10Z

@navinkarkera: Remind me if you'll need this merged for you, and let me know if you are ready for that. Thank you!

Allows admins to configure same oauth backend for multiple sites (cherry picked from commit be4b676) refactor: remove site_id from provider_id (cherry picked from commit 7bbe8e8) fix: skip pipeline if oauth provider for site is not setup (cherry picked from commit ec738bc) docs: update test docstrings test: add test for oauth config per site

navinkarkera · 2023-10-09T14:33:14Z

@robrap Thanks! This is ready to be merged.

openedx-webhooks · 2023-10-10T15:33:27Z

@navinkarkera 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

edx-pipeline-bot · 2023-10-10T19:36:23Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

edx-pipeline-bot · 2023-10-10T19:58:09Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2023-10-10T19:58:09Z

2U Release Notice: This PR has been deployed to the edX production environment.

Allows admins to configure same oauth backend for multiple sites. This change includes site_id in KEY_FIELDS for oauth configuration provider allowing a backend configuration for each site. (cherry picked from commit 565b34e)

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jul 5, 2023

navinkarkera force-pushed the navin/multsite-oauth-config-master branch 2 times, most recently from 04197d4 to a473c5d Compare July 7, 2023 14:52

navinkarkera commented Jul 7, 2023

View reviewed changes

common/djangoapps/third_party_auth/tests/test_provider.py Outdated Show resolved Hide resolved

navinkarkera marked this pull request as ready for review July 7, 2023 16:44

navinkarkera force-pushed the navin/multsite-oauth-config-master branch from a473c5d to 8d29434 Compare July 7, 2023 16:45

kaustavb12 approved these changes Jul 8, 2023

View reviewed changes

kaustavb12 mentioned this pull request Aug 15, 2023

feat: allow oauth configuration per site and backend open-craft/edx-platform#579

Merged

navinkarkera force-pushed the navin/multsite-oauth-config-master branch from 8d29434 to facb9c8 Compare September 13, 2023 10:28

mphilbrick211 requested a review from a team September 14, 2023 14:06

robrap mentioned this pull request Sep 14, 2023

Deprecation and Removal of site_configuration openedx/platform-roadmap#21

Closed

robrap reviewed Sep 14, 2023

View reviewed changes

timmc-edx assigned robrap Sep 25, 2023

robrap assigned navinkarkera and unassigned robrap Sep 26, 2023

robrap added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Sep 26, 2023

navinkarkera force-pushed the navin/multsite-oauth-config-master branch from facb9c8 to 0a46ea7 Compare September 28, 2023 09:34

navinkarkera requested a review from robrap September 28, 2023 09:58

robrap approved these changes Sep 28, 2023

View reviewed changes

common/djangoapps/third_party_auth/tests/test_provider.py Outdated Show resolved Hide resolved

common/djangoapps/third_party_auth/tests/test_provider.py Outdated Show resolved Hide resolved

navinkarkera force-pushed the navin/multsite-oauth-config-master branch from 5331bd7 to d91a17e Compare September 29, 2023 14:47

mphilbrick211 removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Oct 2, 2023

navinkarkera force-pushed the navin/multsite-oauth-config-master branch from d91a17e to bd55bfe Compare October 9, 2023 14:32

robrap merged commit 565b34e into openedx:master Oct 10, 2023
45 of 61 checks passed

Agrendalath deleted the navin/multsite-oauth-config-master branch November 9, 2023 15:04

0x29a mentioned this pull request Dec 31, 2023

feat: port code drift for open-craft/edx-platform [BB-8366] open-craft/edx-platform#614

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow oauth configuration per site and backend #32656

feat: allow oauth configuration per site and backend #32656

navinkarkera commented Jul 5, 2023 •

edited

Loading

openedx-webhooks commented Jul 5, 2023 •

edited

Loading

kaustavb12 left a comment

mphilbrick211 commented Jul 17, 2023

mphilbrick211 commented Sep 12, 2023

mphilbrick211 commented Sep 14, 2023

robrap commented Sep 14, 2023

robrap left a comment

navinkarkera commented Sep 28, 2023

robrap left a comment

robrap commented Sep 29, 2023

navinkarkera commented Oct 9, 2023

openedx-webhooks commented Oct 10, 2023

edx-pipeline-bot commented Oct 10, 2023

edx-pipeline-bot commented Oct 10, 2023

edx-pipeline-bot commented Oct 10, 2023

feat: allow oauth configuration per site and backend #32656

feat: allow oauth configuration per site and backend #32656

Conversation

navinkarkera commented Jul 5, 2023 • edited Loading

Description

Supporting information

Testing instructions

openedx-webhooks commented Jul 5, 2023 • edited Loading

kaustavb12 left a comment

Choose a reason for hiding this comment

mphilbrick211 commented Jul 17, 2023

mphilbrick211 commented Sep 12, 2023

mphilbrick211 commented Sep 14, 2023

robrap commented Sep 14, 2023

robrap left a comment

Choose a reason for hiding this comment

navinkarkera commented Sep 28, 2023

robrap left a comment

Choose a reason for hiding this comment

robrap commented Sep 29, 2023

navinkarkera commented Oct 9, 2023

openedx-webhooks commented Oct 10, 2023

edx-pipeline-bot commented Oct 10, 2023

edx-pipeline-bot commented Oct 10, 2023

edx-pipeline-bot commented Oct 10, 2023

navinkarkera commented Jul 5, 2023 •

edited

Loading

openedx-webhooks commented Jul 5, 2023 •

edited

Loading