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 a new OnCheckSlidingExpiration event to control renewal #33016

Merged
merged 3 commits into from
May 27, 2021

Conversation

Tratcher
Copy link
Member

@Tratcher Tratcher commented May 25, 2021

Fixes #32269 @brockallen @Rinsen

Today an auth cookie is automatically renewed if it's more than 50% expired. We've had two complaints about this:
A) People want to set a different renewal interval, usually more frequent. Renewals can be forced today with OnValidatePrincipal, but not delayed or suppressed.
B) People want to suppress renewal for some types of request such as SPA apps that are pinging to check if their cookie is still valid.

This fix adds a new event OnCheckSlidingExpiration that allows people to override the default behavior to refresh more or less often. The default sliding expiration behavior remains the same. We opted not to modify OnValidatePrincipal to avoid compat issues as well as conflicting events (Identity uses that one for SSO).

Unrelated: Some sample fixup including using the new minimal host APIs.

@Tratcher Tratcher added this to the 6.0-preview6 milestone May 25, 2021
@Tratcher Tratcher requested a review from HaoK May 25, 2021 22:41
@Tratcher Tratcher self-assigned this May 25, 2021
@ghost ghost added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label May 25, 2021
@Tratcher Tratcher added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label May 25, 2021
@ghost
Copy link

ghost commented May 25, 2021

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@@ -174,8 +182,6 @@ private async Task<AuthenticateResult> ReadCookieTicket()
return AuthenticateResult.Fail("Ticket expired");
}

CheckForRefresh(ticket);
Copy link
Member Author

Choose a reason for hiding this comment

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

I moved this to clarify that it's only relevant to Authenticate scenarios. SignIn and SignOut also call ReadCookieTicket, but only to populate the _sessionKey.


// Don't renew on API endpoints that use JWT.
var authData = context.HttpContext.GetEndpoint()?.Metadata.GetMetadata<IAuthorizeData>();
if (authData != null && string.Equals(authData.AuthenticationSchemes, "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.

You could consider moving this to auth samples instead of manual and adding a test for this scenario (always nicer to have coverage)

https://github.com/dotnet/aspnetcore/blob/main/src/Security/test/AuthSamples.FunctionalTests/CookiesTests.cs

Copy link
Member Author

Choose a reason for hiding this comment

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

I mainly wanted to show @brockallen that there were other patterns than matching request paths.

@Rinsen
Copy link

Rinsen commented May 26, 2021

This looks great! I guess it will be shipped in .NET 6?

@Tratcher
Copy link
Member Author

This looks great! I guess it will be shipped in .NET 6?

Yes, this will be a 6.0 API.

@Tratcher Tratcher added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels May 27, 2021
@Tratcher Tratcher enabled auto-merge (squash) May 27, 2021 23:23
@Tratcher Tratcher merged commit 62998ac into dotnet:main May 27, 2021
@Tratcher Tratcher deleted the tratcher/cookierenew branch May 27, 2021 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suppress cookie sliding
4 participants