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.

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

Add support for the special common/organizations/consumers Azure AD tenants #714

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ntherning
Copy link

When not targeting a specific Azure AD tenant (specified by a tenant GUID in
the discovery document URL) but rather one of the "common", "organizations"
or "consumers" multi-tenant aliases (see 1), discovery document parsing and ID
token validation require a few extra steps:

  • The discovery document's "issuer" value contains the special placeholder
    "{tenantid}". As '{' and '}' are invalid characters in URLs, AppAuth has to
    URL encode these characters before the issuer URL can be parsed by NSURL in
    OIDServiceDiscovery.m.
  • The same "{tenantid}" placeholder needs to be replaced with the actual
    tenant ID of the authenticated user, from the "tid" claim (see 2) of the ID
    token, before ID token validation is performed in OIDAuthorizationService.m.

1: https://docs.microsoft.com/en-us/azure/active-directory/develop/v2-protocols-oidc#fetch-the-openid-connect-metadata-document
2: https://docs.microsoft.com/en-us/azure/active-directory/develop/id-tokens#payload-claims

@ntherning
Copy link
Author

For full Azure AD support in AppAuth-iOS PR #710 also has to be merged. See my comment on that issue: #710 (comment)

@ntherning
Copy link
Author

I have signed the CLA.

@AristideVB
Copy link

any updates on this PR @ntherning

…enants

When not targeting a specific Azure AD tenant (specified by a tenant GUID in
the discovery document URL) but rather one of the "common", "organizations"
or "consumers" multi-tenant aliases (see 1), discovery document parsing and ID
token validation require a few extra steps:

* The discovery document's "issuer" value contains the special placeholder
  "{tenantid}". As '{' and '}' are invalid characters in URLs, AppAuth has to
  URL encode these characters before the issuer URL can be parsed by NSURL in
  OIDServiceDiscovery.m.
* The same "{tenantid}" placeholder needs to be replaced with the actual
  tenant ID of the authenticated user, from the "tid" claim (see 2) of the ID
  token, before ID token validation is performed in OIDAuthorizationService.m.

1: https://docs.microsoft.com/en-us/azure/active-directory/develop/v2-protocols-oidc#fetch-the-openid-connect-metadata-document
2: https://docs.microsoft.com/en-us/azure/active-directory/develop/id-tokens#payload-claims
@ntherning
Copy link
Author

Still waiting for someone to review this PR. I signed the CLA almost 2 years ago. Just now I rebased it onto master. Perhaps @mdmathias who have been committing to this repo lately can review it? As I mentioned in a previous comment, the #710 PR has to be merged too to have Azure working. In the Spamdrain app we have supported Azure OpenID Connect sign ins/ups for years using AppAuth-iOS patched with this PR (and #710) with great success.

@AristideVB
Copy link

Ok thank you @ntherning looking forward for the merge, great app by the way ! 🎉 🙂

Without using your branch do you know of a workaround to have https://login.microsoftonline.com/consumers/v2.0/.well-known/openid-configuration work ?

@ntherning
Copy link
Author

@AristideVB I'm afraid I don't know how to make it work with Azure without #710 and #714. The Spamdrain fork at https://github.com/spamdrain/AppAuth-iOS has a branch which adds #710 and #714 on top of the 1.6.2 release. Here's how we use that in our app's Podfile:

pod 'AppAuth', :git => 'https://github.com/spamdrain/AppAuth-iOS.git', :branch => '1.6.2+azure'

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.

2 participants