Skip to content

[SignalR] Close connection on auth expiration #32431

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.

Sign up for GitHub

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

Jump to bottom
Merged
BrennanConroy merged 9 commits into from
Jul 13, 2021
Merged

[SignalR] Close connection on auth expiration #32431

BrennanConroy merged 9 commits into from
Jul 13, 2021

Conversation

@BrennanConroy
Copy link
Member

@BrennanConroy BrennanConroy commented May 5, 2021 •
edited
Loading

Part of #5297

Open questions:

  • If expiration of token happens, can we first run auth again to check if the token hasn't had its expiry updated?
  • What sort of hooks are there in auth to set expiration in case some 3rd party library doesn't set the ExpiresUtc property?
public class HttpConnectionDispatcherOptions
{
+    bool EnableAuthenticationExpiration { get; set; }
}

@BrennanConroy BrennanConroy added area-signalr Includes: SignalR clients and servers api-suggestion Early API idea and discussion, it is NOT ready for implementation labels May 5, 2021
@Tratcher
Copy link
Member

Tratcher commented May 5, 2021

  • If expiration of token happens, can we first run auth again to check if the token hasn't had its expiry updated?

How would you have acquired a new token without a new request?

  • What sort of hooks are there in auth to set expiration in case some 3rd party library doesn't set the ExpiresUtc property?

I don't think JWT auth will even validate a token without the exp property?

davidfowl
davidfowl reviewed May 5, 2021
View reviewed changes
@blowdart blowdart added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label May 6, 2021
@BrennanConroy BrennanConroy force-pushed the brecon/authexpiration branch from 067b4cf to 60dbf28 Compare June 28, 2021 22:01
@BrennanConroy BrennanConroy added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Jul 2, 2021
@ghost
Copy link

ghost commented Jul 2, 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.

@BrennanConroy BrennanConroy removed the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jul 2, 2021
@BrennanConroy BrennanConroy marked this pull request as ready for review July 2, 2021 18:21
@BrennanConroy BrennanConroy force-pushed the brecon/authexpiration branch from 5fa972c to 334e5a8 Compare July 2, 2021 19:19
@davidfowl
Copy link
Member

davidfowl commented Jul 8, 2021 •
edited
Loading

What's the state of this PR?

@BrennanConroy
Copy link
Member Author

BrennanConroy commented Jul 8, 2021

What's the state of this PR?

I think it's good to go, just needs review(s)

halter73
halter73 reviewed Jul 9, 2021
View reviewed changes
src/SignalR/server/Core/src/HubConnectionContext.cs Outdated

if (connectionContext.Features.Get<IConnectionLifetimeNotificationFeature>() is IConnectionLifetimeNotificationFeature lifetimeNotification)
{
// This feature is used by HttpConnectionManager to gracefully close the connection on authentication expiration
Copy link
Member

@halter73 halter73 Jul 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It seems weird to me to call it graceful when we're calling abort:

Suggested change
// This feature is used by HttpConnectionManager to gracefully close the connection on authentication expiration
// This feature is used by HttpConnectionManager to close the connection with a non-errored close message on authentication expiration.

Or should we include an error in the close message indicating that auth expired?

@halter73
Copy link
Member

halter73 commented Jul 10, 2021

Any particular reason for not adding an error to the close message?

@BrennanConroy
Copy link
Member Author

BrennanConroy commented Jul 10, 2021

Any particular reason for not adding an error to the close message?

I think it's a good idea, but the code isn't setup currently to allow it easily. I think we need to update how we do the CloseMessage in a different change. Do you remember if we had an issue tracking the CloseMessage race?

@pranavkm pranavkm 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 Jul 12, 2021
@halter73
Copy link
Member

halter73 commented Jul 12, 2021

Do you remember if we had an issue tracking the CloseMessage race?

I looked briefly, but didn't find one.

@BrennanConroy
Copy link
Member Author

BrennanConroy commented Jul 12, 2021

Do you remember if we had an issue tracking the CloseMessage race?

I looked briefly, but didn't find one.

Filed #34294

@BrennanConroy BrennanConroy merged commit f74c6f1 into main Jul 13, 2021
@BrennanConroy BrennanConroy deleted the brecon/authexpiration branch July 13, 2021 02:20
@ghost ghost added this to the 6.0-preview7 milestone Jul 13, 2021
@BrennanConroy BrennanConroy mentioned this pull request Jul 16, 2021
Open
@vicancy vicancy mentioned this pull request Aug 20, 2021
Closed
@ghost
Copy link

ghost commented Nov 13, 2021

Hi @mcguu. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

This was referenced Sep 27, 2022
Closed
Closed
@Y-Sindo Y-Sindo mentioned this pull request Oct 9, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@halter73 halter73 halter73 approved these changes

@Tratcher Tratcher Tratcher left review comments

@davidfowl davidfowl davidfowl approved these changes

@HaoK HaoK HaoK approved these changes

+1 more reviewer

@pranavkm pranavkm pranavkm left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api-approved API was approved in API review, it can be implemented area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer area-signalr Includes: SignalR clients and servers

Projects

None yet

Milestone

6.0-preview7

Development

Successfully merging this pull request may close these issues.

8 participants

@BrennanConroy @Tratcher @davidfowl @halter73 @pranavkm @HaoK @blowdart