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

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

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@vinhub

vinhub commented May 28, 2015

…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.
WIP: Adding Office 365 login support using Azure Active Directory OAu…
…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.
@edx-webhook

This comment has been minimized.

Show comment
Hide comment
@edx-webhook

edx-webhook May 28, 2015

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.

edx-webhook commented May 28, 2015

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.

@antoviaque

This comment has been minimized.

Show comment
Hide comment
@antoviaque

antoviaque May 29, 2015

Member

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

Member

antoviaque commented May 29, 2015

@vinhub Python social auth is already being updated as part of the work on SSO/Shibboleth on #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

This comment has been minimized.

Show comment
Hide comment
@bradenmacdonald

bradenmacdonald May 29, 2015

Member

@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.
Member

bradenmacdonald commented May 29, 2015

@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.
@sarina

This comment has been minimized.

Show comment
Hide comment
@sarina

sarina Jun 1, 2015

Contributor

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

Contributor

sarina commented Jun 1, 2015

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

@vinhub

This comment has been minimized.

Show comment
Hide comment
@vinhub

vinhub Jun 1, 2015

@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?

vinhub commented Jun 1, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@sarina

sarina Jun 1, 2015

Contributor

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

Contributor

sarina commented Jun 1, 2015

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

@RobDolinMS

This comment has been minimized.

Show comment
Hide comment
@RobDolinMS

RobDolinMS Jun 2, 2015

@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.

RobDolinMS commented Jun 2, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@sarina

sarina Jun 8, 2015

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?

Contributor

sarina commented Jun 8, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@RobDolinMS

RobDolinMS Jun 10, 2015

@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.

RobDolinMS commented Jun 10, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@sarina

sarina Jun 15, 2015

Contributor

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

Contributor

sarina commented Jun 15, 2015

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

@RobDolinMS

This comment has been minimized.

Show comment
Hide comment
@RobDolinMS

RobDolinMS Jun 22, 2015

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

RobDolinMS commented Jun 22, 2015

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

@sarina

This comment has been minimized.

Show comment
Hide comment
@sarina

sarina Jun 22, 2015

Contributor

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

Contributor

sarina commented Jun 22, 2015

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

@sarina

This comment has been minimized.

Show comment
Hide comment
@sarina

sarina Jul 1, 2015

Contributor

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

Contributor

sarina commented Jul 1, 2015

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

This comment has been minimized.

Show comment
Hide comment
@RobDolinMS

RobDolinMS Jul 1, 2015

Thanks for closing @sarina.

RobDolinMS commented Jul 1, 2015

Thanks for closing @sarina.

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