Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Added support for enterprise app sso certificates #87

Merged
merged 1 commit into from
Jan 15, 2020
Merged

Added support for enterprise app sso certificates #87

merged 1 commit into from
Jan 15, 2020

Conversation

LukeFerris
Copy link
Contributor

When an app is registered as an Azure Enterprise app and SSO is turned on, Azure uses a different set of certificates to sign the resulting issued tokens.

The standard certificates are reached via:
https://login.microsoftonline.com/TENANT_ID/.well-known/openid-configuration.
However, when SSO is turned on, a different cert is used not present in the above list. Instead you can reach the correct certificate list using:
https://login.microsoftonline.com/TENANT_ID/.well-known/openid-configuration?appid=CLIENT_ID

As it happens, even if SSO is not turned on, the second of these URL options still returns the correct certificate list.

We bumped into this in a recent deployment and so I offer this addition to enable the package to work in both scenarios.

@codecov
Copy link

codecov bot commented Oct 31, 2019 •
edited
Loading

Codecov Report

Merging #87 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #87   +/-   ##
=======================================
  Coverage   81.81%   81.81%           
=======================================
  Files           6        6           
  Lines         407      407           
=======================================
  Hits          333      333           
  Misses         74       74
Impacted Files Coverage Δ
django_auth_adfs/config.py 84.46% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7626ca6...1d078e1. Read the comment docs.

@jobec
Copy link
Collaborator

jobec commented Oct 31, 2019

Found some refs to it here: https://docs.microsoft.com/en-us/azure/active-directory/develop/v1-protocols-openid-connect-code#openid-connect-metadata-document

Are you 100% sure it works in a fresh, vanilla Azure AD setup? Because if it doesn't, then sending it always will break existing implementations.

@LukeFerris
Copy link
Contributor Author

LukeFerris commented Oct 31, 2019 via email

@jobec
Copy link
Collaborator

jobec commented Oct 31, 2019

If you can ask that, that would be great.

From your tests I expect it to work, because we don't really need that much from those metadata. But a confirmation would also ease my mind.

@LukeFerris
Copy link
Contributor Author

LukeFerris commented Oct 31, 2019 via email

@jobec
Copy link
Collaborator

jobec commented Nov 12, 2019

Did you perhaps already got a reply on your question towards microsoft?

@LukeFerris
Copy link
Contributor Author

LukeFerris commented Dec 24, 2019 via email

@LukeFerris
Copy link
Contributor Author

LukeFerris commented Dec 24, 2019 via email

@LukeFerris
Copy link
Contributor Author

LukeFerris commented Jan 13, 2020 via email

@jobec jobec merged commit bc98a6d into snok:master Jan 15, 2020
mmcclelland1002 pushed a commit to mmcclelland1002/django-auth-adfs that referenced this pull request Nov 12, 2021
@jobec
Merge pull request snok#87 from LukeFerris/Support-enterprise-app-sso…
84cdb0b 
…-certificates

Added support for enterprise app sso certificates
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@LukeFerris @jobec