WL-286 Add support for multi-tenancy

[douglashall]

We need to be able to configure SOCIAL_AUTH settings per site.

Related PR:

openedx/ecommerce#543

[douglashall]

@clintonb @rlucioni This is ready for review.

[clintonb]

@douglashall I noticed that the tag v0.1.4 links to a commit on this branch. Tags/releases should be made after merging.

[douglashall]

@clintonb Cool. I wasn't sure of your tagging process. I can update after we merge.

[clintonb]

We follow the PEP8 style for imports. Imports should be grouped as follows:

1. standard library imports
2. related third party imports
3. local application/library specific imports

Each group should be alphabetized, and you should put a blank line between each group of imports.

[douglashall]

Sounds good. Should be all set.

[clintonb]

These changes should be applied to a new auth backend (e.g. EdXOpenIdConnectMultiSite. Overloading EdXOpenIdConnect leads to issues with backwards-incompatibility.

[douglashall]

Can you tell me more about the backwards-incompatibility issues you foresee? Are there other IDAs that are using this backend that wouldn't want to support multitenancy?

[clintonb]

If I were to drop this code into Otto today, without changing settings, would it work? If someone is sub-classing EdXOpenIdConnect, and decide to upgrade, will they have issues? It seems cleaner to me to make a new subclass so that it is obvious which backend is being used and which settings need to be updated. The hybrid approach will lead to confusion.

[clintonb]

Semantic versioning: 0.2.0

[douglashall]

I guess I'm not clear on who exactly is using this library. Seems like it would just be us and we should be in control of updating the settings of consumers. However, I will defer and create a separate backend in order to move this along.

[clintonb]

@douglashall thinking about dependencies...the SOCIAL_AUTH_STRATEGY can accept any Python path. That means the strategy doesn't necessarily need to live in this repo if we are concerned about dependency injection/contamination/etc. The strategy could live in edx/ecommerce, and you need only update EdXOpenIdConnect to use the new methods.

[douglashall]

Good point. Yes, I think I will move this to ecommerce. Any suggestions for where to put it? ecommerce.oauth.strategies, maybe?

[clintonb]

s/oauth/social_auth

[douglashall]

Sounds good. Thanks.

[clintonb]

You'll want to look at python-social-auth for ideas regarding testing. The method is pretty straightforward, so no need to go crazy beyond just making sure we handle existing settings and properly fail when a setting doesn't exist.

[douglashall]

@clintonb This is ready for a final review. We will need to get this merged and get the 0.2.0 version pushed to pypi before the tests will run for openedx/ecommerce#543.

[clintonb]

Why not inherit from EdXOpenIdConnect? Doing so would get rid of a lot of duplicated code.

[douglashall]

All set.

[clintonb]

The current implementation is fine aside from the unnecessary duplication. Thinking back to another option we discussed, I think this could be simplified even further:

class EdXOpenIdConnect(OpenIdConnectAuth):
  @property
  def ID_TOKEN_ISSUER(self)
    return self.setting('URL_ROOT')

  @property
  def AUTHORIZATION_URL(self)
    return '{0}/authorize/'.format(self.ID_TOKEN_ISSUER)

This approach would maintain backwards-compatibility while enabling a new strategy to support site-specific settings.

[douglashall]

@clintonb Alright, I think we have made this as concise as possible. Thanks for your help on this.

[clintonb]

I believe these changes can be applied directly to EdXOpenIdConnect. There is no longer a need for the child class since we have backwards-compatibility.

[clintonb]

Also, tests are needed for these new properties.

[clintonb]

👍, pending final comments addressed.

[douglashall]

@mattdrayer Could you give this a second review?

[clintonb]

Note that the consuming service will need to have this value set (in addition to URL_ROOT). This value should be set in configuration.

[clintonb]

It seems this advice was never heeded, and now I have one more thing to fix for LEARNER-693. Come on!

[douglashall]

Hi Clinton, Sincere apologies if this is causing you pain. I really appreciate all of the great work that you do and do not want to make it difficult for you to continue that work. My hope is to learn all I can from you. It’s not clear to me from this comment what the problem is or what needs to be done to solve it. Would you mind describing the problem you are running into in more detail and the solution which you are implementing so that we can avoid pain for everyone in the future? Sorry again and thanks, - Doug

…

On Apr 26, 2017, at 9:58 AM, Clinton Blackburn ***@***.***> wrote: @clintonb commented on this pull request. In auth_backends/backends.py <#9 (comment)>: > @@ -34,6 +30,22 @@ class EdXOpenIdConnect(OpenIdConnectAuth): auth_complete_signal = django.dispatch.Signal(providing_args=["user", "id_token"]) + @Property + def ID_TOKEN_ISSUER(self): + return self.setting('ISSUER') It seems this advice was never heeded, and now I have one more thing to fix for LEARNER-693. Come on! — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#9 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AANtenPosmkflgy6fEjf596HOaSTrrN5ks5rz02EgaJpZM4HXj6F>.

[clintonb]

Sorry for lashing out. There is no "ISSUER" setting as far as I can tell. I'm not sure how/why logins were working in the past. Now login is broken on credentials.stage.edx.org because the setting cannot be found. Was the issuer ever configured? Clinton Blackburn On Wed, Apr 26, 2017 at 10:07 AM, Douglas Hall <notifications@github.com> wrote:

…

Hi Clinton, Sincere apologies if this is causing you pain. I really appreciate all of the great work that you do and do not want to make it difficult for you to continue that work. My hope is to learn all I can from you. It’s not clear to me from this comment what the problem is or what needs to be done to solve it. Would you mind describing the problem you are running into in more detail and the solution which you are implementing so that we can avoid pain for everyone in the future? Sorry again and thanks, - Doug > On Apr 26, 2017, at 9:58 AM, Clinton Blackburn ***@***.***> wrote: > > @clintonb commented on this pull request. > > In auth_backends/backends.py <https://github.com/edx/auth- backends/pull/9#discussion_r113458416>: > > > @@ -34,6 +30,22 @@ class EdXOpenIdConnect(OpenIdConnectAuth): > > auth_complete_signal = django.dispatch.Signal(providing_args=["user", "id_token"]) > > + @Property > + def ID_TOKEN_ISSUER(self): > + return self.setting('ISSUER') > It seems this advice was never heeded, and now I have one more thing to fix for LEARNER-693. Come on! > > — > You are receiving this because you modified the open/close state. > Reply to this email directly, view it on GitHub < #9 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ AANtenPosmkflgy6fEjf596HOaSTrrN5ks5rz02EgaJpZM4HXj6F>. > — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA3krrDwuItU9cml5_bUjAhB9gJCkPdoks5rz0-JgaJpZM4HXj6F> .

[douglashall]

No worries. Is this “ISSUER” setting in edxapp or credentials? Sorry, trying to refresh my memory about how this all works.

…

On Apr 26, 2017, at 10:20 AM, Clinton Blackburn ***@***.***> wrote: Sorry for lashing out. There is no "ISSUER" setting as far as I can tell. I'm not sure how/why logins were working in the past. Now login is broken on credentials.stage.edx.org because the setting cannot be found. Was the issuer ever configured? Clinton Blackburn On Wed, Apr 26, 2017 at 10:07 AM, Douglas Hall ***@***.***> wrote: > Hi Clinton, > > Sincere apologies if this is causing you pain. I really appreciate all of > the great work that you do and do not want to make it difficult for you to > continue that work. My hope is to learn all I can from you. > > It’s not clear to me from this comment what the problem is or what needs > to be done to solve it. Would you mind describing the problem you are > running into in more detail and the solution which you are implementing so > that we can avoid pain for everyone in the future? > > Sorry again and thanks, > > - Doug > > > On Apr 26, 2017, at 9:58 AM, Clinton Blackburn ***@***.***> > wrote: > > > > @clintonb commented on this pull request. > > > > In auth_backends/backends.py <https://github.com/edx/auth- > backends/pull/9#discussion_r113458416>: > > > > > @@ -34,6 +30,22 @@ class EdXOpenIdConnect(OpenIdConnectAuth): > > > > auth_complete_signal = django.dispatch.Signal(providing_args=["user", > "id_token"]) > > > > + @Property > > + def ID_TOKEN_ISSUER(self): > > + return self.setting('ISSUER') > > It seems this advice was never heeded, and now I have one more thing to > fix for LEARNER-693. Come on! > > > > — > > You are receiving this because you modified the open/close state. > > Reply to this email directly, view it on GitHub < > #9 (comment)>, or > mute the thread <https://github.com/notifications/unsubscribe-auth/ > AANtenPosmkflgy6fEjf596HOaSTrrN5ks5rz02EgaJpZM4HXj6F>. > > > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#9 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/AA3krrDwuItU9cml5_bUjAhB9gJCkPdoks5rz0-JgaJpZM4HXj6F> > . > — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#9 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AANteufdHq4bi6DyZOBr68X3v3qQr6ijks5rz1LBgaJpZM4HXj6F>.

[clintonb]

Both. edxapp, as the issuer, needs to know what value to set. All of the IDAs need to know what value to expect.