Skip to content

WIP: Adding Office 365 login support using Azure Active Directory OAu… - #8248

Closed
vinhub wants to merge 1 commit into
openedx:masterfrom
microsoftarchive:azuread-wip
Closed

WIP: Adding Office 365 login support using Azure Active Directory OAu…#8248
vinhub wants to merge 1 commit into
openedx:masterfrom
microsoftarchive:azuread-wip

Conversation

@vinhub

@vinhub vinhub commented May 28, 2015

Copy link
Copy Markdown

…th2 provider and related changes

The code is working, but it is WIP because we have a couple of issues that we would like to get reviewed:

  1. Python social auth currently does not have support for Azure AD provider. We have sent a PR to them for that in parallel and it is currently being reviewed. But this means that in order for edX to use Azure AD provider, you have to upgrade to the new version of python social auth. In this PR, we are pointing to a version of python social auth with our changes since our PR has still not been merged in. Note that python social auth 0.2.x may have some backward compatibility issues with 0.1.x version and we noticed one and have added a patch to edX code for that. (We noticed these issues with Google’s provider also.) We will update the PR after python social auth people have merged our changes and also you have reviewed our patch to edX to make them work.
  2. We need to show an “Office 365” icon in the login button but there is no FontAwesome icon glyph for Office 365. So we have added support for png icons. Also, since in our case the provider name is AzureAD and login name is Office 365 (with a space in it), we have added a UINAME property in addition to NAME property in the provider and adjusted various pieces of UI that use them. We have made sure the other providers NAME and UINAME are the same, so they should be unaffected.

…th2 provider and related changes

The code is working, but it is WIP because we have a couple of issues that we would like to get reviewed:

1.	Python social auth currently does not have support for Azure AD provider. We have sent a PR to them for that in parallel and it is currently being reviewed. But this means that in order for edX to use Azure AD provider, you have to upgrade to the new version of python social auth. In this PR, we are pointing to a version of python social auth with our changes since our PR has still not been merged in. Note that python social auth 0.2.x may have some backward compatibility issues with 0.1.x version and we noticed one and have added a patch to edX code for that. (We noticed these issues with Google’s provider also.) We will update the PR after python social auth people have merged our changes and also you have reviewed our patch to edX to make them work.
2.	We need to show an “Office 365” icon in the login button but there is no FontAwesome icon glyph for Office 365. So we have added support for png icons. Also, since in our case the provider name is AzureAD and login name is Office 365 (with a space in it), we have added a UINAME property in addition to NAME property in the provider and adjusted various pieces of UI that use them. We have made sure the other providers NAME and UINAME are the same, so they should be unaffected.
@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @vinhub! I've created OSPR-618 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

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

  • supporting documentation
  • edx-code email 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 still be done via the Github pull request interface. As a reminder, our process documentation is here.

We can't start reviewing your pull request until you've submitted a signed contributor agreement or indicated your institutional affiliation and added yourself to the AUTHORS file. Please see the CONTRIBUTING file for more information.

@openedx-webhooks openedx-webhooks added open-source-contribution PR author is not from Axim or 2U needs triage product review PR requires product review before merging and removed needs triage labels May 28, 2015
@antoviaque

Copy link
Copy Markdown
Contributor

@vinhub Python social auth is already being updated as part of the work on SSO/Shibboleth on https://github.com/edx/edx-platform/pull/8140 - it would be important to sync up with @bradenmacdonald before considering merging this, especially as both rely on changes in the upstream project.

@bradenmacdonald

Copy link
Copy Markdown
Contributor

@vinhub This will be a nice contribution!

As @antoviaque mentioned, I've been doing a lot of work around third party auth as part of edX's Shibboleth/SSO project.

If you're feeling a bit adventurous, you may want to try developing this on top of the code that's in #8155 (which includes other code from #8140). That code is still under review and on a separate feature branch, but we're hoping to get it all merged to master in the near future. The benefits for you from that work already done are:

  • It's already upgraded to python-social-auth 0.2.7
  • You can now add new providers like this simply by using the Django admin view and entering in the information.
  • UINAME is not required because each provider now has a name and an id specified in the database, and the names can contain spaces.
  • The can add a new python-social-auth OAuth2 backend (such as this new Office 365 backend) just by adding its full path (e.g. social.backends.azuread.AzureADOAuth2) to the new THIRD_PARTY_AUTH_BACKENDS setting. This means, for example, that you can use custom backends that are not even located in the python-social-auth package.

@openedx-webhooks openedx-webhooks added community manager review and removed product review PR requires product review before merging labels May 29, 2015
@sarina

sarina commented Jun 1, 2015

Copy link
Copy Markdown
Contributor

@vinhub thanks for your contributor's agreement! Can you please sync with @bradenmacdonald regarding your questions, and respond to his as well? Thanks!

@openedx-webhooks openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed community manager review labels Jun 1, 2015
@vinhub

vinhub commented Jun 1, 2015

Copy link
Copy Markdown
Author

@sarina, @bradenmacdonald
Thanks for your comments. Your suggestion looks very interesting and we are looking into PR #8155. We are under a bit of time pressure, so do you have an approximate ETA for when #8155 (or its successor) might get accepted into the platform?

@sarina

sarina commented Jun 1, 2015

Copy link
Copy Markdown
Contributor

@vinhub can you explain your time pressure with a bit more detail? I don't know the background of this project.

@RobDolinMS

Copy link
Copy Markdown

@sarina Our team (me, @vinhub , and others from http://www.MSOpenTech.com/ ) is trying to coordinate the available of this functionality with some other Microsoft + Educational OSS interoperability and announcements. If @bradenmacdonald 's #8155 gets picked-up in the next 5-7 days, that would be awesome. If it's more like 30+ days, that would be non-optimal for us. Thanks.

@sarina

sarina commented Jun 8, 2015

Copy link
Copy Markdown
Contributor

@RobDolinMS - I understand you spoke with Beth last week regarding this pull request. Is there anything you need me to do as a follow up from that?

@RobDolinMS

Copy link
Copy Markdown

@sarina - Thanks for checking. We've connected with @bradenmacdonald via email and I believe his changes will satisfy most of the goals of this PR.

@sarina

sarina commented Jun 15, 2015

Copy link
Copy Markdown
Contributor

@RobDolinMS thanks for the update. Does this pull request still need to be reviewed, then, if Braden's changes will satisfy your requirements?

@RobDolinMS

Copy link
Copy Markdown

@sarina Thanks for checking. When #8155 and #8599 are merged, this PR does not need to be reviewed.

@sarina

sarina commented Jun 22, 2015

Copy link
Copy Markdown
Contributor

@RobDolinMS great, thanks. Can this PR be closed, then?

@sarina

sarina commented Jul 1, 2015

Copy link
Copy Markdown
Contributor

OK, I didn't receive a response and the PRs in question have been merged so I'm closing this. Thanks!

@sarina sarina closed this Jul 1, 2015
@RobDolinMS

Copy link
Copy Markdown

Thanks for closing @sarina.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

open-source-contribution PR author is not from Axim or 2U waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants