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 message handlers for downstream api auth #1276

Merged
merged 4 commits into from
Jul 23, 2021

Conversation

qetza
Copy link
Contributor

@qetza qetza commented Jun 20, 2021

Authentication message handlers to inject authorization header using token acquisition.

@ghost
Copy link

ghost commented Jun 20, 2021

CLA assistant check
All CLA requirements met.

@qetza qetza changed the title Add message handlers for downstream api auth (#1131) Add message handlers for downstream api auth Jun 20, 2021
@qetza
Copy link
Contributor Author

qetza commented Jun 20, 2021

This is for the feature request #1131

Copy link
Collaborator

@jmprieur jmprieur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @qetza
This looks good to me.

How would you use the message handler?
Would you have a sample app? unit tests?

Thanks!

@qetza
Copy link
Contributor Author

qetza commented Jun 21, 2021

Hi @jmprieur,
I will add some unit tests and copy an existing downstream webapi sample and rewrite it with using the message handler instead of the DownstreamWebApi

@qetza
Copy link
Contributor Author

qetza commented Jul 4, 2021

Hi @jmprieur,
I've refactor the code to have base options class between downstream and delegating handlers, fix the issue with existing header and added some unit tests.
For the sample part as they are in another repo i'll do a PR there later.

@jmprieur
Copy link
Collaborator

jmprieur commented Jul 4, 2021

Thanks @qetza

@qetza qetza force-pushed the messagehandlers branch from e46bd1b to 18b2052 Compare July 9, 2021 06:51
@qetza
Copy link
Contributor Author

qetza commented Jul 9, 2021

I've rebased the code on the lastest version of master to resolve merge conflict.

@jmprieur
Copy link
Collaborator

Sorry @qetza : I missed that you had added tests.
We'll review. thanks for the head-up.

namespace Microsoft.Identity.Web
{
/// <summary>
/// A DelegatingHandler implementation that add an authorization header with a token for the application.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add

nit: adds

Copy link
Collaborator

@jennyf19 jennyf19 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@jennyf19
Copy link
Collaborator

thanks @qetza i, too, missed the updates here. Could you resolve the merge conflict and then we will merge it? thanks so much.

@jennyf19 jennyf19 added this to the 1.15.0 milestone Jul 22, 2021
@qetza qetza force-pushed the messagehandlers branch from 18b2052 to 7ad3452 Compare July 23, 2021 07:45
@qetza
Copy link
Contributor Author

qetza commented Jul 23, 2021

@jennyf19, i've rebased the branch and fixed the merge conflict (on Microsoft.Identity.Web.xml as usual)

@jennyf19 jennyf19 merged commit 721643b into AzureAD:master Jul 23, 2021
@NWessel
Copy link

NWessel commented Aug 20, 2021

@qetza Could you provide a link to a repo where you use it?
I really want to try it out right now :)

@qetza qetza deleted the messagehandlers branch August 22, 2021 07:34
@qetza
Copy link
Contributor Author

qetza commented Aug 22, 2021

Hi @NWessel,
I've submitted a pull request on the sample repo with an example of using the MicrosoftIdentityUserAuthenticationMessageHandler class.

You can see the code here:
https://github.com/qetza/active-directory-aspnetcore-webapp-openidconnect-v2/blob/142a315a36293e25ba5e4fd42640a1a624519d13/3-WebApp-multi-APIs/Startup.cs#L49

@NWessel
Copy link

NWessel commented Aug 22, 2021

Just checked it out, looks like what i would expect it to be ^^
Probably could make the scope optional as well, and take the base url of the request and add /.default

So baseUrl + "/.default"
If no scope is provided.

What is the service name for ? I'm not sure I quite understand the usecase :)

@qetza
Copy link
Contributor Author

qetza commented Aug 22, 2021

You can always create your own extension method without a scope and do the magic in there 😄

The service name is used internally to link and retrieve the options if you have multiple clients which needs different options.

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.

4 participants