Skip to content

Add refresh token support to BearerTokenHandler #48595

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 3 commits into from
Jun 21, 2023
Merged

Add refresh token support to BearerTokenHandler #48595

merged 3 commits into from
Jun 21, 2023

Conversation

halter73
Copy link
Member

@halter73 halter73 commented Jun 3, 2023

  • Integrate with identity to check security stamp and refresh user from store

Fixes #47228

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-identity Includes: Identity and providers label Jun 3, 2023
/// <summary>
/// If set, the token must be valid for sign in to continue.
/// </summary>
public string? RefreshToken
Copy link
Contributor

Choose a reason for hiding this comment

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

This property seems really specific to the "API endpoints" stuff and will likely be confusing for users of different authentication handlers that also use AuthenticationProperties. Is a public API really needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's specific to the new BearerTokenHandler, but there are other AuthenticationProperties properties that don't apply to every hander. AFAIK IsPersistent and AllowRefresh are both only used by CookieAuthenticationHandler. I'd be fine with hiding ".refreshToken" in some constant if we could find a place it makes sense. Otherwise, I think this is alright.

I'm more worried about AddBearerToken getting confused with AddJwtBearer. I still think we might want to go with AddIdentityBearer like I originally proposed in #47227 rather than AddBearerToken so people don't confuse it with the AddJwtBearer. But you can technically use it without Identity, which is why we went with the current name in API review.

I welcome less confusing naming suggestions for AddBearerToken that's not Identity specific too. AddOpaqueBearer maybe?

Copy link
Contributor

@kevinchalet kevinchalet Jun 6, 2023

Choose a reason for hiding this comment

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

It's specific to the new BearerTokenHandler, but there are other AuthenticationProperties properties that don't apply to every hander. AFAIK IsPersistent and AllowRefresh are both only used by CookieAuthenticationHandler.

This problematic design was inherited from Katana and recent developments deliberately avoided adding new properties to the base AuthenticationProperties to avoid that. See https://github.com/dotnet/aspnetcore/blob/main/src/Security/Authentication/OAuth/src/OAuthChallengeProperties.cs and https://github.com/dotnet/aspnetcore/blob/main/src/Security/Authentication/Google/src/GoogleChallengeProperties.cs for examples of specialized AuthenticationProperties sub-classes. I'm not sure it's really worth having public properties, but if you wanted to, it's likely the design you'd want to adopt.

Regarding the name of the extensions, I don't have any particular opinion as I'm sadly unlikely to use this feature. If these "Identity API endpoints" were usable with existing projects like IdSrv or OpenIddict as I unsuccessfully suggested here, that would make them much more appealing, but heh 🤷🏻

Copy link
Member

Choose a reason for hiding this comment

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

@kevinchalet Do these current endpoints stop us from adding the ones that you think would be valuable? Also, did you write up any more details with what you think you would need?

Copy link
Contributor

Choose a reason for hiding this comment

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

@davidfowl well, despite being part of the Core Identity package, these "Identity API endpoints" aren't meant to be used without your custom "OAuth 2.0-like bearer token generation handler", AFAICT. Yet, these endpoints (and the future ones you plan to add like the password change/reset endpoints) would be useful to users of existing OAuth 2.0/OIDC solutions like OpenIddict.

Also, did you write up any more details with what you think you would need?

Here's a quick list of things that would be nice:

  • Having a clear separation between the token generation/validation handler and the Identity management API endpoints (e.g the /register endpoint isn't authenticated so it would be directly usable in an OpenIddict app without forcing the dev' to use your custom token generation handler).

  • It would make more sense to have these new things in a dedicated Identity package so that the .Core Identity package doesn't have to reference your custom token generation handler (many OpenIddict users use Identity, but they don't need/want to reference Microsoft.AspNetCore.Authentication.BearerToken).

  • For bonus points, it wouldn't be hard to make the underlying authentication scheme configurable for the token generation, so that the /login endpoint could be used with a different token server backend (let's say OpenIddict 😄)

Copy link
Member

Choose a reason for hiding this comment

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

@kevinchalet Can you file a design proposal issue? If it's not too disruptive to what we're trying to accomplish with these endpoints then it seems reasonable. Otherwise, we'll punt until .NET 9.

halter73 added 2 commits June 21, 2023 01:52
- Integrate with identity to check security stamp and refresh user from store
@halter73 halter73 marked this pull request as ready for review June 21, 2023 08:54
@halter73 halter73 requested a review from Tratcher as a code owner June 21, 2023 08:54
@halter73 halter73 enabled auto-merge (squash) June 21, 2023 16:25
@halter73 halter73 merged commit c5e43cf into main Jun 21, 2023
@halter73 halter73 deleted the halter73/47228 branch June 21, 2023 19:16
@ghost ghost added this to the 8.0-preview6 milestone Jun 21, 2023
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 token refresh endpoints to identity
5 participants