Skip to content

Add MapIdentityApi<TUser>() #47414

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

Merged
merged 2 commits into from
Apr 27, 2023
Merged

Add MapIdentityApi<TUser>() #47414

merged 2 commits into from
Apr 27, 2023

Conversation

halter73
Copy link
Member

Addresses #47227

@ghost ghost added the area-identity Includes: Identity and providers label Mar 25, 2023
@halter73 halter73 requested review from javiercn and Tratcher March 30, 2023 16:07
@halter73 halter73 marked this pull request as ready for review March 30, 2023 16:07
@halter73 halter73 requested review from a team and wtgodbe as code owners March 30, 2023 16:07
@ghost
Copy link

ghost commented Apr 13, 2023

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no breaking changes are introduced, please leave an /azp run comment here to rerun the CI pipeline and confirm success before merging the change.

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Apr 13, 2023
Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

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

Looks good to me

@halter73 halter73 changed the title Add MapIdentity<TUser>() Add MapIdentityApi<TUser>() Apr 26, 2023
@halter73
Copy link
Member Author

/backport to release/8.0-preview4

@halter73 halter73 removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Apr 26, 2023
@github-actions
Copy link
Contributor

Started backporting to release/8.0-preview4: https://github.com/dotnet/aspnetcore/actions/runs/4812793273

@github-actions
Copy link
Contributor

@halter73 backporting to release/8.0-preview4 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Add MapIdentityApi<TUser>()
.git/rebase-apply/patch:1330: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
Using index info to reconstruct a base tree...
M	src/Identity/Core/src/PublicAPI.Unshipped.txt
M	src/Identity/Identity.slnf
M	src/Security/Security.slnf
Falling back to patching base and 3-way merge...
Auto-merging src/Security/Security.slnf
Auto-merging src/Identity/Identity.slnf
CONFLICT (content): Merge conflict in src/Identity/Identity.slnf
Auto-merging src/Identity/Core/src/PublicAPI.Unshipped.txt
CONFLICT (content): Merge conflict in src/Identity/Core/src/PublicAPI.Unshipped.txt
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add MapIdentityApi<TUser>()
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@github-actions
Copy link
Contributor

@halter73 an error occurred while backporting to release/8.0-preview4, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

@halter73 halter73 merged commit d4430f0 into main Apr 27, 2023
@halter73 halter73 deleted the halter73/47227 branch April 27, 2023 19:06
@ghost ghost added this to the 8.0-preview5 milestone Apr 27, 2023
public static class IdentityApiEndpointsIdentityBuilderExtensions
{
/// <summary>
/// Adds configuration ans services needed to support <see cref="IdentityApiEndpointRouteBuilderExtensions.MapIdentityApi{TUser}(IEndpointRouteBuilder)"/>
Copy link
Member

Choose a reason for hiding this comment

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

ans => and

#pragma warning restore IDE0060 // Remove unused parameter
: SignInAuthenticationHandler<BearerTokenOptions>(optionsMonitor, loggerFactory, urlEncoder)
{
private const string BearerTokenPurpose = $"Microsoft.AspNetCore.Authentication.BearerToken:v1:BearerToken";
Copy link
Member

Choose a reason for hiding this comment

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

- $


var ticket = BearerTokenProtector.Unprotect(token);

if (ticket?.Properties?.ExpiresUtc is null)
Copy link
Member

Choose a reason for hiding this comment

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

store ExpiresUtc in a variable, it's parsed on-demand.


if (properties.ExpiresUtc is null)
{
properties.ExpiresUtc ??= utcNow + Options.BearerTokenExpiration;
Copy link
Member

Choose a reason for hiding this comment

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

- ??


properties ??= new();

if (properties.ExpiresUtc is null)
Copy link
Member

Choose a reason for hiding this comment

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

store locally

{
var authorization = Request.Headers.Authorization.ToString();

return authorization.StartsWith("Bearer ", StringComparison.Ordinal)
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

This is new, and I like being strict until we see a client that cannot send "Bearer" with the standard casing.

Copy link
Member

Choose a reason for hiding this comment

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

This is the kind of thing clients are guaranteed to get wrong sooner or later. There's no reason to be strict about it.

/// <summary>
/// Specifies events which the bearer token handler invokes to enable developer control over the authentication process.
/// </summary>
public class BearerTokenEvents
Copy link
Member

@Tratcher Tratcher Apr 27, 2023

Choose a reason for hiding this comment

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

There should also be an OnSignIn event that would let you take over the response generation, or at least mess with the identity before the token is created.

private static readonly AuthenticateResult FailedUnprotectingToken = AuthenticateResult.Fail("Unprotected token failed");
private static readonly AuthenticateResult TokenExpired = AuthenticateResult.Fail("Token expired");

private ISecureDataFormat<AuthenticationTicket> BearerTokenProtector
Copy link
Member

Choose a reason for hiding this comment

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

You can do this one in PostConfigure options to avoid creating it per request.

if (options.TicketDataFormat == null)
{
// Note: the purpose for the data protector must remain fixed for interop to work.
var dataProtector = options.DataProtectionProvider.CreateProtector("Microsoft.AspNetCore.Authentication.Cookies.CookieAuthenticationMiddleware", name, "v2");
options.TicketDataFormat = new TicketDataFormat(dataProtector);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-identity Includes: Identity and providers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add API endpoints for generating identity tokens
6 participants