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

Persist addl claims+tokens w/ext providers #8310

Merged
merged 8 commits into from
Aug 31, 2018
Merged

Conversation

guardrex
Copy link
Collaborator

@guardrex guardrex commented Aug 28, 2018

Fixes #3744

Internal Review Topic

  • I made several uneducated guesses here. Let me know where I went off the rails. 🛠
  • I haven't validated the non-Google scopes. If they aren't known by our folks, should we trust the sources I found that provided the values, or should I configure providers and confirm they work? It's time-consuming to check them, and we have a massive amount of work to do 🏃😅. However, I'll validate them myself if we need it done.
  • Do you want a 1.x sample, too? If so, it would be best if it's produced for me or if someone points me to an existing 1.x identity sample where I can then attempt to apply Chris's notes: "The developer hooks into an event on the auth middeware, reads the user info blob, and creates new claims for the temp cookie."

@guardrex
Copy link
Collaborator Author

@HaoK Are you OOF right now? This can wait ... I can push the issue's completion back a sprint if we need to.

@HaoK
Copy link
Member

HaoK commented Aug 30, 2018

No sorry, I just lose track of mentions easily these days, I'll take a look

@HaoK
Copy link
Member

HaoK commented Aug 30, 2018

Can you double check the link to the internal review? it seems to just link back to this issue for me

@guardrex
Copy link
Collaborator Author

Sure ... stand-by.

@HaoK
Copy link
Member

HaoK commented Aug 30, 2018

I don't think a 1x sample is very important anymore, so I would wait to see if we get a lot of requests for that, I would imagine at this point the people who needed that already have figured it out by now

@guardrex
Copy link
Collaborator Author

I don't think a 1x sample is very important anymore,

😅 GOOD! lol ... that puts a smile on my ❤️.

@HaoK
Copy link
Member

HaoK commented Aug 30, 2018

Looks good to me, but I'm probably a bit too familiar with this flow since its based off the sample I wrote :) Adding @blowdart and @Tratcher for a pair of fresh eyes in case they see anything fishy.

Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

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

The next questions from users are going to be how to use the AccessToken to make API requests, and how to manage refreshing expired tokens independent of login. Unfortunately that's very provider specific.

* [Google authentication](xref:security/authentication/google-logins)
* [Microsoft authentication](xref:security/authentication/microsoft-logins)
* [Twitter authentication](xref:security/authentication/twitter-logins)
* [Other authentication providers](xref:security/authentication/otherlogins)
Copy link
Member

Choose a reason for hiding this comment

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

It would also work with OpenIdConnect, though we don't often show that used with Identity.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Bleh, that's worse than providing no link at all. How about https://github.com/Azure-Samples/active-directory-aspnetcore-webapp-openidconnect-v2?

as List<AuthenticationToken>;
tokens.Add(new AuthenticationToken()
{
Name = "TicketCreated",
Copy link
Member

Choose a reason for hiding this comment

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

You never explain why you're adding a custom AuthenticationToken here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@HaoK ??

Copy link
Member

Choose a reason for hiding this comment

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

That is just demonstrating how you can add custom tokens which get stored as part of SaveTokens so they show up on the identity side as a token

@guardrex
Copy link
Collaborator Author

@Tratcher How's that look?


[!code-csharp[](additional-claims/samples/2.x/AdditionalClaimsSample/Startup.cs?name=snippet_AddGoogle&highlight=9)]

When `OnPostConfirmationAsync` executes, store the access token ([ExternalLoginInfo.AuthenticationTokens](xref:Microsoft.AspNetCore.Identity.ExternalLoginInfo.AuthenticationTokens*)) from the external provider into the `ApplicationUser`'s `AuthenticationProperties`.
Copy link
Member

Choose a reason for hiding this comment

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

into --> in

1. Register the app and obtain a valid client ID and client secret for Google authentication. For more information, see <xref:security/authentication/google-logins>.
1. Provide the client ID and client secret to the app in the <xref:Microsoft.AspNetCore.Authentication.Google.GoogleOptions> of `Startup.ConfigureServices`.
1. Run the app and request the My Claims page. When the user isn't signed in, the app redirects to Google. Sign in with Google. Google redirects the user back to the app (`/Home/MyClaims`).
1. The user is authenticated, and the My Claims page is loaded. The gender claim is present under **User Claims** with the value obtained from Google. The access token appears in the **Authentication Properties**.
Copy link
Member

Choose a reason for hiding this comment

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

This last item doesn't feel like a step to use the sample app. It feels more like an outcome. Consider moving this text.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can roll this into the prior step. It all flows from the same user action (i.e., they run the app and request the page).

@guardrex guardrex merged commit 327b245 into master Aug 31, 2018
@guardrex guardrex deleted the guardrex/external-claims branch August 31, 2018 15:40
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