search

panva / openid-client

OAuth 2 / OpenID Connect Client API for JavaScript Runtimes
MIT License
1.83k stars 392 forks source link

feat: add support for azure ad v2 multitenant apps #148

Closed ulrikstrid closed 5 years ago

ulrikstrid commented 5 years ago

This PR adds support for Azure AD v2 multitenant apps. Flags are documented in README.

codecov[bot] commented 5 years ago

Codecov Report

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

@@          Coverage Diff          @@
##           master   #148   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          18     18           
  Lines         798    803    +5     
=====================================
+ Hits          798    803    +5
panva commented 5 years ago

Hello @ulrikstrid

I'm really reluctant to be adding vendor specific workarounds to a library that aims to be interoperable between certified solutions, which Azure AD is.

That being said let's give it a chance, if you wouldn't mind, fixing the test syntax not to use trailing commas (eslint should pick up on this for you) and writing down an understandable reasoning behind this change. What use case is trying to solve and why is it needed, with code examples please. I will then ping the responsible person from Microsoft for consult.

Thank you for putting your effort into this and bearing with the above.

ulrikstrid commented 5 years ago

I tried to fix the linting issues but might take another go at it as it seems I didn't get it to work.

Our use case is that we want to create a multi tenant application where other companies can login with their own Azure AD to our application. This is, as stated in #84, currently not possible because Microsoft does some non-standard things in their discovery document. They return issuer URL that should have {tentantid} substituted for what comes back from when trying to auth.

The returned issuer is always https://login.microsoftonline.com/{tenantid}/v2.0". The tenantId is put in the tid key of the payload that is returned and the url is changed to use that to validate it's the same. As I understand it IdentityServer just disables the issuer check but that seems to be even more unsafe than what we did in this PR.

This PR allows to use a array of valid tenantId's, if that list is omited we just match the tid with the tenantId in the iss to make sure they at least are the same.

Our usage code is this:

  const adIssuer = await Issuer.discover(
    `https://login.microsoftonline.com/common/v2.0/.well-known/openid-configuration`
  );

  const adClient = new adIssuer.Client({
    redirect_uri: REDIRECT_URI,
    client_id: CLIENT_ID,
    client_secret: CLIENT_SECRET,
    token_endpoint_auth_method: "client_secret_basic",
    isAzureADv2Multitenant: true,
    azureADv2ValidTenantIds: ["74d3d36e-789f-4168-99dd-4a6052d32573"] // GUID changed from our real GUID
  });

Anything else you want me to clarify?

ulrikstrid commented 5 years ago

The test is failing because node 6.x.x doesn't have async/await. I'll see if I can change the test to just use catch instead.

panva commented 5 years ago

@ulrikstrid before i reach out to Mike, can't you do discovery on a route which is specific to your tenant id?

I seem to recall https://login.microsoftonline.com/common/v2.0/.well-known/openid-configuration does have a tenant specific result when you replace common with the tenant id. (i could validate that if you just didn't hide your real tenant id)

ulrikstrid commented 5 years ago

When I change common to my tenant id I get back a issuer with my tenant id in it, but then I need to have separate routes for each of our customers and I would really like to avoid that as that is a lot more work than adding a new customer to the database.

panva commented 5 years ago

I seem to recall login.microsoftonline.com/common/v2.0/.well-known/openid-configuration does have a tenant specific result when you replace common with the tenant id.

that is indeed the case

When I change common to my tenant id I get back a issuer with my tenant id in it, but then I need to have separate routes for each of our customers and I would really like to avoid that as that is a lot more work than adding a new customer to the database.

Since the PR suggests you know the tenantIds upfront i think you can get without it. At authorization request time, do you know the tid already? Is that something you have configured in your customer db?

ulrikstrid commented 5 years ago

I don't know the tid that the current user will have until they come back.

panva commented 5 years ago

So are you blindly accepting users from tenant ids not belonging to your current customer?

ulrikstrid commented 5 years ago

Anyone can get to the login, but with the changes in this PR I can match them to the list of tenant ids I provide in the configuration azureADv2ValidTenantIds. So anyone can try to login, but if they come back with the wrong tenant id they will not come in.

panva commented 5 years ago

Does a customer of yours provide his tenant id?

ulrikstrid commented 5 years ago

We will know tenant id's ahead of time as we need to register them as customers in the application. But we don't know them for each connection. That's why we can provide the list of valid tenant ids

simongottschlag commented 5 years ago

The main goal of this is to have one "Azure AD Application" that is configured to be multi tenant. This means we can connect an application using node-openid-client to it and use Azure AD for authentication of all users (from any tenant) with this.

For some usecases, this is something that you would like to do (to use Azure AD as an OP just like you use GitHub and Google).

In our case, we don't want to allow access to all Azure AD tenants, but specific ones. On our side, we will store the customers with an Azure AD tenant ID - which we will pick up for all customers and generate the array to use with azureADv2ValidTenantIds. We will also do matching that a user that is a member of a customer also is coming in from the correct tenant id - but that's nothing to do with OIDC.

ssypi commented 5 years ago

I think its pointless to compare tid and iss and it only makes this azure-specific and unnecessary complex. If someone wants to do that, they can do that afterwards in their own code. What I propose is to have something like "ignoreIssuerValidation" or "validateIssuer: false" flags instead for ignoring it. Additionally, you could have something like "validIssuers" array for having multiple issuers instead of ignoring it completely.

This way it is not tied to Azure AD and can be used in other cases while still allowing multiple Azure tenants to be supplied to the validIssuers array. They just have to be in full iss format instead of just the tenant-id.

This kind of approach is what Microsoft also suggests in their documentation for multi-tenanted applications.

https://docs.microsoft.com/fi-fi/azure/active-directory/develop/howto-convert-app-to-be-multi-tenant#update-your-code-to-handle-multiple-issuer-values

For example, if a multi-tenant application only allows sign-in from specific tenants who have signed up for their service, then it must check either the issuer value or the tid claim value in the token to make sure that tenant is in their list of subscribers. If a multi-tenant application only deals with individuals and doesn’t make any access decisions based on tenants, then it can ignore the issuer value altogether. In the multi-tenant samples, issuer validation is disabled to enable any Azure AD tenant to sign in.

panva commented 5 years ago

The solution i'm leaning towards is to refactor the ID Token validation so that it allows for some prototype overloading on specific parts. That's one thing.

The other is i'll probably expose (as part of this library or a new one, not sure yet) another issuer/client constructor just for AAD.

panva commented 5 years ago

Just checking in, i haven't forgotten about this, I'm just now planning to revive the work on 3.x (due in April with node 12 release) and i'll include this there.

acoll commented 5 years ago

I had to add support for MS login across multiple tenants today and just ended up forking this project and adding my own little dirty patch to define custom iss validation. Let me know if I can help on testing or implementing proper support.

panva commented 5 years ago

Coming back to this as I progress on v3.x

There are a few points of feedback i need confirmed from you.

1) Based on discussions with MS's Standards Architect I will only allow the special AAD issuer validation handling if the Issuer was discovered through Issuer.discover(...) where the resolved AS metadata URL is equal to (see below line [1]). Issuer instances created through the regular constructor will not get this applied. I was told that "so long as the client uses the discovered metadata the issuer validation can be omitted" hence this restriction. 2) Clients created through this Issuer will have an altered iss ID Token validation handling such as (see below line [2]) 3) I will not implement this custom azureADv2ValidTenantIds handling since you can implement this on your application's level after getting the tokenset from authorizationCallback

[1]: https://login.microsoftonline.com/common/v2.0/.well-known/openid-configuration [2]: 

const azureADv2Issuer = this.issuer.issuer.replace('{tenantid}', payload.tid);
assert.equal(payload.iss, azureADv2Issuer, 'unexpected iss value');

Thoughts?