Skip to content

ju/ednx/JD-1: Microsite aware key-secret pairs for oauth#433

Merged
mariajgrimaldi merged 1 commit intoednx/juniper+edunextfrom
ju/ednx/JD-1
Nov 12, 2020
Merged

ju/ednx/JD-1: Microsite aware key-secret pairs for oauth#433
mariajgrimaldi merged 1 commit intoednx/juniper+edunextfrom
ju/ednx/JD-1

Conversation

@mariajgrimaldi
Copy link
Contributor

@mariajgrimaldi mariajgrimaldi commented Oct 8, 2020

Description

This PR allows each tenant to have its own key-secret pair.

Changes and justification

  • Changes KEY_FIELD from KEY_FIELDS = ('backend_name') to KEY_FIELDS = ('site_id', 'backend_name',): this allows to create differents OauthProviderConfig per site.

For example:

With KEY_FIELDS = ('backend_name'), we could only have a max of active OauthProviderConfig equals the number of available backends.
Say that we have an active OauthProviderConfig for Google and Facebook, if we add a new one for Google even with different data but with the same backend -for Google-, the old config will be updated and the new one will be the active.

Here some proof of that:

  1. I had a Provider Configuration with the backend: google-oauth2 for the site .ngrok.io
    Screenshot from 2020-10-13 12-09-47

  2. After I added a new Config for test-cert with the same backend, this happened:
    Screenshot from 2020-10-13 12-10-33

But with KEY_FIELDS = ('site_id', 'backend_name') we can have the same max but per site, so we can have for each site a new config but with different backends.

For this, I followed this PR: /edx/edx-platform/pull/17276

  • Pass site_id to current method calls: after changing KEY_FIELDS the current will use them as lookup fields to find the current configs, so now we have to pass the site_id.

  • Override provider_id to cast field: adding site_id to KEY_FIELDS makes provider_id to change and add site_id (which is a number).

  • Change get_setting so the key can be searched in settings: this makes it more flexible to store key/secrets, they can be stored using the OauthProviderConfig or the tenant settings.

Consequences

  • current method override is no longer used
  • THIRD_PARTY_AUTH_ENABLED_PROVIDERS is no longer used
  • From what I understand, KEY_FIELDS makes unique active configurations. So, if in PROD there are two configurations with the same SITE and BACKEND the active configuration will be the latest one.
  • Because we change how current is called, some tests fail. I already fixed them.

Data changes
https://docs.google.com/document/d/1CMoG_YoaYIuIGLQG2kDbvMQ4k0QNpu4GXAd3bSplCxA

Questions and possible answers

  • why not use site_id and slug instead of backend? Well, here's the reason:
    Here
    oauth2_backend_names = OAuth2ProviderConfig.key_values('backend_name', flat=True)
    instead of searching for backend_name, that the max is the number of backends available, we would need to look for slugs that normally exceed the number of backends available

LMS logs during the login process using Google:
Screenshot from 2020-10-14 08-50-16

Copy link
Member

@felipemontoya felipemontoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to understand the consequences of the key change as well as the data changes that we would need to do in the PROD data for this to work with the current clients

@mariajgrimaldi mariajgrimaldi force-pushed the ju/ednx/JD-1 branch 3 times, most recently from a9ab879 to 62e7b1c Compare October 16, 2020 17:56
@mariajgrimaldi mariajgrimaldi changed the title [WIP] ju/ednx/JD-1: Microsite aware key-secret pairs for oauth ju/ednx/JD-1: Microsite aware key-secret pairs for oauth Oct 19, 2020
"ENABLE_THIRD_PARTY_AUTH",
settings.FEATURES.get("ENABLE_THIRD_PARTY_AUTH")
# This forces the module to be enabled on a per tenant basis
settings.FEATURES.get("ENABLE_THIRD_PARTY_AUTH_FOR_TEST", False)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this necessary, there is a consequence if this returns None instead of False ?

Copy link
Contributor Author

@mariajgrimaldi mariajgrimaldi Oct 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cherry-picked this from ir/ednx/JD-1. I'm gonna do some tests to check if there's a consequence

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you know why is this change? I can't figure it out. @felipemontoya

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember something about this needing to be turned on when the server starts (aka the yml file) and then be turned off for all the tenants which dont have a need for it.

@mariajgrimaldi mariajgrimaldi force-pushed the ju/ednx/JD-1 branch 3 times, most recently from 3088c65 to c42442b Compare October 22, 2020 01:19
@mariajgrimaldi mariajgrimaldi force-pushed the ju/ednx/JD-1 branch 2 times, most recently from 0e185ce to 642cf20 Compare October 29, 2020 13:09
@mariajgrimaldi
Copy link
Contributor Author

do you agree to merge this? @felipemontoya

Copy link
Member

@felipemontoya felipemontoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I missed something or we already tried and something was not working, but in this particular point I think we should consider again modifying the .current method.

"ENABLE_THIRD_PARTY_AUTH",
settings.FEATURES.get("ENABLE_THIRD_PARTY_AUTH")
# This forces the module to be enabled on a per tenant basis
settings.FEATURES.get("ENABLE_THIRD_PARTY_AUTH_FOR_TEST", False)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember something about this needing to be turned on when the server starts (aka the yml file) and then be turned off for all the tenants which dont have a need for it.

@mariajgrimaldi mariajgrimaldi force-pushed the ju/ednx/JD-1 branch 3 times, most recently from 8f9377b to 132411a Compare November 4, 2020 14:20
- Change KEY_FIELD from KEY_FIELDS = ('backend_name') to KEY_FIELDS = ('site_id', 'backend_name')
- Override provider_id to cast field
- Override current method to pass site_id
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.

3 participants