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

[Feature Request] Allow to pass tenantId when requesting application access token #413

Closed
stvonolf opened this issue Aug 6, 2020 · 6 comments
Assignees
Labels
enhancement New feature or request fixed
Milestone

Comments

@stvonolf
Copy link

stvonolf commented Aug 6, 2020

Is your feature request related to a problem? Please describe.
We would use a multi-tenant application which is authorized in several tenants. In each tenant, the application would be added in the Dynamics CRM as an app user. For each of those, we need to request the token using the tenant authority.

Describe the solution you'd like
Add tenantId parameter to GetAccessTokenForAppAsync - similar as in GetAccessTokenForUserAsync

Describe alternatives you've considered
n/a

Additional context
https://github.com/AzureAD/microsoft-identity-web/blob/master/src/Microsoft.Identity.Web/TokenAcquisition.cs

__

Addition. Given that for client credentials, we can only pass one scope (resource/.default), we'd want to change the signature to pass only one string (instead of a collection)

=> This is a breaking change. So proposing to take it (PM would say easy) in 0.3.0-preview
cc: @jennyf19 @henrik-me @pmaytak

@stvonolf stvonolf added the enhancement New feature or request label Aug 6, 2020
@jmprieur
Copy link
Collaborator

jmprieur commented Aug 6, 2020

@stvonolf how urgent is it for you?

@stvonolf
Copy link
Author

stvonolf commented Aug 6, 2020

If this is a use-case you want to support, it would be good to have it until the end of August if possible.

@stvonolf
Copy link
Author

stvonolf commented Aug 6, 2020

We should clarify first though, if requesting application tokens for multiple tenants is considered an anti-pattern.

@jmprieur jmprieur added this to the 0.3.0-preview milestone Aug 7, 2020
@jmprieur
Copy link
Collaborator

jmprieur commented Aug 9, 2020

Checked with @hpsin and here is what we agreed to:

  • Change the signature of GetAccessTokenForUserAsync to take a string (instead of a IEnumerable<string>) as there is only one possible string for a given resource of App Id URI AppIdUri: "AppIdUri/.default". We can check that the resource ends in "./default"
  • Add an additional optional parameter tenant to support this scenario, and verify that this tenant is not organizations (and of course common and consumers, which don't make sense)
public async Task<string> GetAccessTokenForAppAsync(string scope, string? tenant = null)

@jmprieur jmprieur self-assigned this Aug 9, 2020
@jmprieur
Copy link
Collaborator

jmprieur commented Aug 9, 2020

Taking it as there is an API breaking change so I'd like it to be in 0.3.0-preview

jmprieur added a commit that referenced this issue Aug 9, 2020
Checked with @hpsin and here is what we agreed to:
- Change the signature of `GetAccessTokenForUserAsync` to take a `string` (instead of a `IEnumerable<string>`) as there is only one possible string for a given resource of App Id URI AppIdUri: "AppIdUri/.default". Check that the resource ends in "./default"
- Add an additional optional parameter `tenant` to support this scenario, and verify that this tenant is not organizations (and of course common and consumers, which don't make sense)

```CSharp
public async Task<string> GetAccessTokenForAppAsync(string scope, string? tenant = null)
```
jmprieur added a commit that referenced this issue Aug 10, 2020
… (#424)

* Make GetTokenForAppAsync less confusing and allow to pass tenantId #413

Checked with @hpsin and here is what we agreed to:
- Change the signature of `GetAccessTokenForUserAsync` to take a `string` (instead of a `IEnumerable<string>`) as there is only one possible string for a given resource of App Id URI AppIdUri: "AppIdUri/.default". Check that the resource ends in "./default"
- Add an additional optional parameter `tenant` to support this scenario, and verify that this tenant is not organizations (and of course common and consumers, which don't make sense)

```CSharp
public async Task<string> GetAccessTokenForAppAsync(string scope, string? tenant = null)
```

* Update src/Microsoft.Identity.Web/ITokenAcquisition.cs
Co-authored-by: jennyf19 <jeferrie@microsoft.com>

* Addressing PR feedback:
- Adding an aka.ms link to the error messages (https://aka.ms/ms-id-web/daemon-scenarios)
- using constants for the meta-tenants
- Testing all the meta tenant
Thanks @jennyf19 for this PR feedback

* Addressing @hpsin 's PR feedback.
jennyf19 added a commit that referenced this issue Aug 12, 2020
* Initial commit (does not build)

* Updating Web API.

Todo:
- see if we could have some commonalities between the Web app and Web API builder (configuration?)
- Enfoce configuration for CallsWebAPI can only be called if configuration for the AddMicrosoftWebApp/Api

* Improving the API

* Improving the API.

* Updating unit tests so that they build
Fixing a few.

* Renamings an clean-up discussed with DevDiv
- Renaming AddMicrosoftWebApp to AddMicrosoftIdentityPlatformWebApp,
- Renaming AddMicrosoftWebApi to AddMicrosoftIdentityPlatformWebApi,
 - MicrososoftAppCallingWebApiAuthenticationBuilder.AddInXXXTokenCaches etc ... return their parent builder.

* More renaming

* Test fix.

* Renaming of more overrides

* Updating the templates

* initial commit api w/microsoftIdentity

* merge conflicts + update templates

* Making the templates work with 0.3.*-*

* few more updates

* Make GetTokenForAppAsync less confusing and allow to pass tenantId #413 (#424)

* Make GetTokenForAppAsync less confusing and allow to pass tenantId #413

Checked with @hpsin and here is what we agreed to:
- Change the signature of `GetAccessTokenForUserAsync` to take a `string` (instead of a `IEnumerable<string>`) as there is only one possible string for a given resource of App Id URI AppIdUri: "AppIdUri/.default". Check that the resource ends in "./default"
- Add an additional optional parameter `tenant` to support this scenario, and verify that this tenant is not organizations (and of course common and consumers, which don't make sense)

```CSharp
public async Task<string> GetAccessTokenForAppAsync(string scope, string? tenant = null)
```

* Update src/Microsoft.Identity.Web/ITokenAcquisition.cs
Co-authored-by: jennyf19 <jeferrie@microsoft.com>

* Addressing PR feedback:
- Adding an aka.ms link to the error messages (https://aka.ms/ms-id-web/daemon-scenarios)
- using constants for the meta-tenants
- Testing all the meta tenant
Thanks @jennyf19 for this PR feedback

* Addressing @hpsin 's PR feedback.

* fix tests

* add xml comments

* few spelling changes

* renaming of CallsWebApi to EnableTokenAcquisitionToCallDownstreamApi

* - Adding a missing renaming for AddMicrosoftWebApp => AddMicrosoftIdentityWebApp
- Updating the ITokenAcquisition.GetTokenForAppAsync signature to match the class.

* Fixing the TodoListService controller in WebAppCallsWebApiCallsGraph

* pr feedback

* reverting pr feedback

Co-authored-by: Jean-Marc Prieur <jmprieur@microsoft.com>
Co-authored-by: pmaytak <34331512+pmaytak@users.noreply.github.com>
@jennyf19
Copy link
Collaborator

Included in 0.3.0-preview release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request fixed
Projects
None yet
Development

No branches or pull requests

3 participants