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

Feedback #4

Open
tpeczek opened this issue Aug 6, 2017 · 63 comments
Open

Feedback #4

tpeczek opened this issue Aug 6, 2017 · 63 comments

Comments

@tpeczek
Copy link
Owner

tpeczek commented Aug 6, 2017

This issue is for general feedback regarding this project.

tpeczek added a commit that referenced this issue Aug 21, 2017
tpeczek added a commit that referenced this issue Sep 30, 2017
@tpeczek tpeczek reopened this Sep 30, 2017
@Lanayx
Copy link

Lanayx commented Mar 4, 2019

Hi! First of all want to thank you for this great library. I'm pretty happy with it's functionality, there are just several things that I missed to use it as a nuget package.

  1. Logging. I'd like to configure logging not only on connect/disconnect events, but on send events as well. Also added Description field in ServerSentEventsClient for logging purpose as well, since Id is not enough
  2. More flexible client initialization. In my case I want to use string instead of Guid, and take it from user's Claims after authentication, not just doing Guid.NewGuid(). String is more flexible because many people use numbers as UserId or other techniques, or generics could be used there as well. Here is how I modified the constructor:
internal ServerSentEventsClient(HttpContext context)
{
    User = context.User ?? throw new ArgumentNullException(nameof(context.User));
    Id = User.Claims.First(claim => claim.Type == JwtRegisteredClaimNames.Sub).Value;
    Description = context.Request.Headers["User-Agent"];
    _response = context.Response ?? throw new ArgumentNullException(nameof(context.Response));
    IsConnected = true;
}

@tpeczek
Copy link
Owner Author

tpeczek commented Mar 4, 2019

Hi @Lanayx

I'm happy you find this useful regardles if you use it through NuGet or your own fork :).

Thank you for your feedback. I'll give some thinking to your remarks. My intial thought is that those things best fit outside the library, but maybe after further consideration I'll see a nice place for them.

@seniorquico
Copy link

... In my case I want to use string instead of Guid, and take it from user's Claims after authentication, not just doing Guid.NewGuid(). ...

@Lanayx, with your custom constructor, how do you manage scenarios where the same user (same sub claim from your JWT) opens multiple, simultaneous connections? Did you also make an adjustment to the ConcurrentDictionary keys in ServerSentEventsService to support multiple, simultaneous connections?

@Lanayx
Copy link

Lanayx commented Mar 20, 2019

@seniorquico thanks for raising the question. In my case only one last connection is used (last active tab), but I guess this should be configurable and one may want to have notifications for each tab

@henry-chris
Copy link

henry-chris commented Mar 27, 2019

I'm new-ish to this library and SSE. I'm wanting to create more of a real-time collaboration app where a group of users subscribe to a Redis pub/sub channel. I wouldn't want to send a Publish event to every user; only certain subsets at a time (each user subscribes to a document ID basically).

Not sure if I could tackle that with this library. I'm trying to figure out if I'd need dynamically mapped SSE events (outside of the .MapServerSentEvents<NotificationsServerSentEventsService>() or if i'd need to do something else.

Currently looking through the code, but any advice in how to use/extend your library for that would be helpful. Or if it really wasn't meant to do this.

EDIT: Ahh, maybe a better option might be to create NotificationsService which holds a dictionary of ServerSentEventsServices and create a new one for each document session. Then just pick the right SseEventsService from the dictionary when I need to publish. Based on a short review of your articles/source code.

@tpeczek
Copy link
Owner Author

tpeczek commented Mar 28, 2019

@henry-chris My gut tells that under you scenario hides a requirement for some kind of groups functionality, which would be a nice thing to add.

But that will take time, so first I would like to try help you with hat library currently provides. Can you elaborate on how subscribing to different documents happens? Different URLs?

@seniorquico
Copy link

In our application, we allow clients to subscribe to/unsubscribe from many topics/groups at any time. We initially thought about exploring how to wire up the middleware with a parameterized URL to identify the topic/group identifier. That would've allowed clients to subscribe and unsubscribe by simply starting and stopping different EventSources. However, after some initial testing, we found a significant number of our clients still connecting with user agents that don't support HTTP/2 (which unlocks multiplexed requests and responses). As such, we chose to find a solution that worked with a single EventSource.

We created a controller that exposes CRUD-based methods for a client's topic/group subscriptions (simply because SSE provides only a simplex comm channel after the initial setup). The controller uses all of the normal ASP.NET tools to apply auth checks/permissions. A separate backend application is used to keep track of the client's topic/group subscriptions in addition to which ASP.NET Core server the client is connected. When any of the microservices in our application push an event to a topic/group, the event is relayed to the appropriate ASP.NET Core server(s) and on to the appropriate client(s).

The backend application is built on Microsoft Orleans, which already provides full pub/sub event streaming infrastructure. Our ASP.NET Core servers subscribe to a single Orleans stream identified by a random, server-specific GUID. We model each client as an actor. When the controller method is called to subscribe to a topic/group, the actor subscribes to the associated Orleans stream. When the actor receives an event, it simply relays the message to the stream identified by the server-specific GUID. (I'm actively working on releasing this implementation as an open source project on GitHub.)

The idea of managing subscriptions via a CRUD controller could be adapted to an in-memory dictionary or Redis storage. However, our stream topology may not be a great fit for Redis streams. You could look at the Redis backplane for SignalR for some implementation inspiration.

@henry-chris
Copy link

@tpeczek I have been looking at this off and on and "Yes" it seems to need some type of group functionality. I'm still working out the details, but we handle several different storage repositories and the underlying document metadata is different in most of them. Best I can tell though, the ID would end up being a concatenation of two GUIDs that would be supplied from the client/browser as either a number or a string (doesn't really matter which). I could not use straight document URLs, but they could be part of the ID.

Everytime a user opens our app we would load up the document and an accompanying 'Markup' document in the '.xfdf' format. This is basically a PDF/Office document markup service. At the same time we would register an EventSource to either a single Web API endpoint or a parameter-ized one.

We would send the GROUP ID along with email and/or any other data needed. This would be cached in redis in some way (as a Set/List under the GROUP ID maybe).

Then when a user adds/modifies/deletes any markups a "Command" is sent to the server which contains the diff + operation of the markups. We would send the GROUP ID with it. We'd then want to sent the SSE out to only that specific group and send the "Command" as the message.

So, I was thinking each GROUP ID would be a redis channel with associated pub/sub. The problem seems to be generating the redis channels dynamically and in the correct way. I took a little time to toy with that but didn't get very far, yet.

@seniorquico Thanks for the input. Yes, a single EventSource seems to be the way to go. I was looking into building a shared service/web worker, also, as some of our customers have multiple tabs open (So we don't hit the ~6 connection limit).

I honestly haven't seen Orleans. I will be sure to take a look at it. Also, I'm hoping not to use SignalR if possible. Just a personal preference.

@tpeczek
Copy link
Owner Author

tpeczek commented Apr 1, 2019

@seniorquico Looks like you have put together a nice piece of software, really impresive.

@henry-chris From SSE perspective it looks like you shoudl be able to build what you need based on ClientConnected event. The ServerSentEventsClientConnectedArgs provides a HttpRequest for the connecting user so you can use it to apply any criteria you need and build your own grouping on top of that. From that point it boils down to sending to correct group (which of course is a separate issue).

@henry-chris
Copy link

@tpeczek I'm not sure how to parse your comment. Building a group is not really a problem. The problem is how to only send specific events to specific groups using your package (which I think you alluded to at the end?).

I'm currently looking into this:

Seems like the main thing that would need to be re-written is the ForAllClientsAsync method. Instead of doing it for all clients, have a separate method that takes a list of clients to send. This seems like it would need the least change for now as only my service and the `SseKeepaliveService' may need to be modified. I'd just need to keep a mapping of Group->Clients to parse all clients of a group on a certain server.

As you stated maybe I could use the ClientConnected/ClientDisconnected events to add/remove from my mappings and register a single other service which keeps those mappings and can be registered through DI.

May have holes in my logic here. This also requires rewriting some internal functions/classes. I'm also on Azure, so I'll likely need the KeepAliveService.

If I didn't re-write any internal code I can't see a good way to use the library. It would almost be easier to write new services/middleware.

The If & How of possibly working some of this into the library is another matter. I may be going about it the wrong way.

@tpeczek
Copy link
Owner Author

tpeczek commented Apr 2, 2019

@henry-chris Let me clear my previous comment. From ClientConnected events you have access to IServerSentEventsClient and HttpRequest. Managing groups means maintaining a set of IServerSentEventsClient collections, where each collection represents a different a group. When you need to send to a specific group you just iterate a specific collection and call IServerSentEventsClient.SendEventAsync for all clients in that group.

@seniorquico
Copy link

seniorquico commented Apr 2, 2019

@henry-chris It sounds like you understand the need to build your own grouping infrastructure and some possible implementation ideas. Great! Next, expose a method to get the list of client identifiers that belong to a target group (maybe by some "IGroupManager" that's also in your DI container). Then, use something like the following extension methods to distribute an event to the needed clients:

public static Task SendEventAsync(this IServerSentEventsService service, string text, IEnumerable<Guid> clientIds) =>
    service.SendEventAsync(text, clientIds, CancellationToken.None);

public static Task SendEventAsync(this IServerSentEventsService service, string text, IEnumerable<Guid> clientIds, CancellationToken cancellationToken) =>
    Task.WhenAll(
        clientIds
            .Select(clientId => service.GetClient(clientId))
            .Where(client => client != null)
            .Select(client =>client.SendEventAsync(data, cancellationToken)),
        cancellationToken);

Simply get the needed services from the DI container and call the extension method:

var service = services.GetService<IServerSentEventsService>();
var groupManager = services.GetService<IGroupManager>();

IEnumerable<Guid> clientIds = groupManager.GetClientsInGroup(...);
service.SendEventAsync("group message", clientIds);

No changes to the library are necessary for the above. (But, they could be added to the library or even directly to the IServerSentEventsService API as an enhancement.) If you're sending events at a super high-frequency, the dictionary lookups in GetClient could become a problem (but I'd test to first confirm). If that's the case, then you may want to go "lower level". Instead of looking up the client from a list of Guid, just save lists of clients (the IServerSentEventsClient instances). Then, you don't even need the above extension methods:

var groupManager = services.GetService<IGroupManager>();

IEnumerable<IServerSentEventsClient> clients = groupManager.GetClientsInGroup(...);
Task.WhenAll(clients.Select(client => client.SendEventAsync("group message")));

However, all of the above is good only for distributing events to clients connected directly to the local server. If you live in a single-server environment, then you're good to go. If you're in a multi-server environment, then that's where Redis (or some other messaging infrastructure) can come into play. Start with a non-optimized solution:

Each ASP.NET Core server subscribes to a single Redis pub/sub channel at startup (use a singleton service). When an ASP.NET Core server receives a message from the Redis channel, relay it as an event to the IServerSentEventsService. When you want to send an event, instead of using IServerSentEventsService directly, publish it as a message to the Redis channel. (The message should include the target client identifier or target group identifier.)

After that's working, look into optimizations that fit your usage patterns. Again, I'd recommend looking into how SignalR leverages Redis as a backplane for inspiration. (I wasn't suggesting above that you use Redis 😄.) The basic premise is close to the above, but they get fancy with multiple channels.

@henry-chris
Copy link

@tpeczek Right, so if I send my GroupId over with the HttpRequest I can catch both the Client and Group with the Connected Event. That's what I'm thinking at least. After that I can either setup my own service/classes to send events through OR Fork your library and build it into it. From your responses, it's looking like the former is the best option up front.

I had questions about the KeepAlive service (I didn't want to spawn a bunch of them), but looking over the code it seems that only a Singleton is spawned.

That seems to be the only real caveat as nothing else is actually a background task.

@tpeczek
Copy link
Owner Author

tpeczek commented Apr 2, 2019

@henry-chris Please go through @seniorquico comment. It's a very detailed description of what I had in mind. It gives you the exact functionality you need. Of course it would be much better if that would be built in the library and I've created an issue for it (#20), but as my resources are limited it will take some time.

Regarding KeepAlive, that shouldn't cause any issues. You don't need multiple IServerSentEventsService (most likely one will do) so there will also be only one ServerSentEventsKeepaliveService. Also ServerSentEventsKeepaliveService is not something you manage by yourself, it's managed under the hood by library based on the options you pass to AddServerSentEvents.

@henry-chris
Copy link

@tpeczek, @seniorquico Thanks, yes my trouble was finding the right way to go about this. I wanted to make sure I wasn't spawning a bunch of Background services that weren't needed or would cause overhead. At first I was just trying to debug through the library to see where/how I could hook into. It does look like @seniorquico has a very simple solution that I did not see right away. I will focus on that.

Yes I'm on a distributed App Service in Azure. Just starting to use Redis to deal with this specific use case. I was hoping to stay away from more abstractions like SignalR until I learned more about the internals of how everything works at a base level.

I'll let you know how it goes.

@henry-chris
Copy link

Just as an update. Everything is working well. I went with a similar approach to what you two suggested. Working well with Redis over 3-4 server nodes with document specific groupings.

I've forked the repo locally and have been making small changes to integrate groups a little more. It'll probably be awhile, but I'll at least give you a link when it's done (if you are interested)

I've just got to figure out how to integrate OIDC IdentityServerAuthentication middleware for Authorization against the open connection stream. Haven't messed with that much yet, but it looks like I can build a PolicyEvaluator of some sort.

Thanks for the help.

@tpeczek
Copy link
Owner Author

tpeczek commented Apr 25, 2019

@henry-chris Happy to hear that things are working out for you. Please share your code when it will be ready - it might be useful for me.

Regarding authorization, you are not the first with this question and I have an answer for you here: https://www.tpeczek.com/2019/01/aspnet-core-middleware-and-authorization.html

@henry-chris
Copy link

I do have one other question.

What is the reason that this library primarily uses middleware for the notifcation routing? I suppose you couldn't have a NuGet package that has built in controllers.

I ask because it seems that I am adding multiple middlewares which are evaluated during every request, whereas a controller with a variable endpoint could (I believe) spawn the long-running SseClients just as well. It would also only evaluate authorization if a route/controller was hit, as well as being much easier to tie into the built in Authorization.

I could be wrong though.

@tpeczek
Copy link
Owner Author

tpeczek commented Apr 26, 2019

@henry-chris

Although possible, handling things like SSE in controller would be incorrect from framework philosophy/architecture perspective. Controller are ment to handle standard request/response flow, preferable in API and view scenrios (the view scenerio is slowly becoming less important with grow of Razor Page and SPA). Middlewares have been introduced to give flexibility for other scenarios like WebSockets (and SSE is much more like WebSockets than API) as they have already proven this capability in Node.js.

Controllers are often seen as first place to implement something, because MVC is nicely coupled with routing, authentication, authroization etc. The ASP.NET Core team has noticed htat issue and in result as part of 3.0 those mechanism will be decoupled from MVC and put in front of middleware (Endpoint Routing) to easy scenarios like the one being context for the post I've gave you.

Now I'm not sure what you mean by "multiple middlewares which are evaluated during every request", but if you have set up everything correctly (SSE has its own path and you are branching pipeline) everything should run only when needed.

@tpeczek
Copy link
Owner Author

tpeczek commented Apr 26, 2019

@henry-chris

One more thign regardign authorization. I've directed you to that blog post with assumption that you need something very specific. But this library has built in support for typical authorization scenarios. in case it happened that you've missed it, here is the relevant part of documentation: https://tpeczek.github.io/Lib.AspNetCore.ServerSentEvents/articles/authorization.html

@henry-chris
Copy link

@tpeczek Understood. I have seen SSE and Websockets implemented via controllers as a jump off point, so naturally I'm asking. I do see the benefit in Middleware over API approach here, but since it's less supported it seems easier to just easier to take the API route. I don't see much actual downside, though I'm no expert. Good to know that 3.0 will change this somewhat.

I was unaware of the ability to branch the pipeline. I expected all middleware to be ran through on each request. I will look more deeply into that, thanks.

Yes, I've checked the blog post and the html article. Sadly, we use header based JWT Bearer tokens which makes it more difficult to use SSE. Native implementations do not support custom headers, but some polyfills do. I'm attempting to test performance/reliability of some polyfills now and am working out the best approach to take depending on those results.

@alexiwonder
Copy link

@tpeczek How is it possible to gracefully close the connection from the server?
Thanks.

@tpeczek
Copy link
Owner Author

tpeczek commented Aug 30, 2020

Hi @alexiwonder,

This hs been raised before in #27.

TL;DR SSE philosophy doesn't forsee such an operation as server is supposed to be a never ending stream of evetns, but if you want you can create a "logical" operation for close.

@alexiwonder
Copy link

Thanks @tpeczek.
Before I raised the question, I was wondering how sending 204 No-Content to signal the client stop re-connecting is ever possible in ASP.Net Core when a connection is already established. I gave it a a go and found that it's not doable to change the http status code once the first response has been flushed. (As I'm new to .Net Core, can you please confirm?).
So yes! the best bet is to implement a logical operation for close.

@tpeczek
Copy link
Owner Author

tpeczek commented Sep 1, 2020

@alexiwonder That 204 status code is a tricky thing in Server-Sent Events. The correct flow to use it goes like this:

  • Client connects and receives events
  • Server decides to disconnect the client and terminates the connection
  • Due to auto-reconnect client automatically makes connection the server
  • Server responds with 204 which prevents client from further attempts to reconnect

So this is a form of a disconnecting mechanic, but it requires a logical mechanism to detect that disconnected client is attempting to reconnect.

I was thinking some time ago about trying to build some API for it, but the more I thought about the more complicated it was becoming (how to identify clients in a way to information is retained for long enough but not too long so client can connect later etc.)

For future reference I've created an issue to track that the idea exists: #39

@billhong-just
Copy link

Hi, @tpeczek

Thanks for such a quick and detailed answer.
I have created an issue for this. (#51)

Thanks ^^

@Luigi6821
Copy link

Luigi6821 commented Mar 23, 2023

Hi, @tpeczek
I am new on SSE but looking at your great library I believe I could use it for achieving what I need.
I tried to use it and I am able to send/receive notification sent from my back-end api service.
What I am not able to do is try to associate the requesting client to my internal repository in order to send certain notifications to it.
Basically, trying to briefly summarize, the client (any browser) connect to server using the example url
http://localhost:[port]/Notifications/sse-notifications-license/{argument-key}
where {argument-key} is a key that client MUST supply in order to receive notification about its own license.
So my need is try to associate clientId to {argument-key} to client Id in order to implement something like
_eventservicenotification.SendAsync({argument-key}, ...). In this way only that client will receive information about its own
license.
I have tried to intercept OnClientConnected event but seems to me that arguments received are not present (I can't find {argument-key}.
For sure I am doing something wrong...
Can you help me ?
Thanks in advance
Luigi

@tpeczek
Copy link
Owner Author

tpeczek commented Mar 23, 2023

Hi @Luigi6821,

There is an extensibility point in the library which will most likely be suitable for your need - Client Identifier Provider. It allows you to implement a class which will be available for providing client id based on HttpContext object. In your case you could simply extract that argument-key from URL and return it as client id.

There is a little sample available here: https://tpeczek.github.io/Lib.AspNetCore.ServerSentEvents/articles/disconneting-clients.html#client-identifier-provider

The one potential issue I can see here are errors if it happens that client attempts to establish multiple parallel connections with the same argument-key. Client id must be unique, so if that is the case you will have to somehow modify the approach, so the client ids are unique but you can map them back to argument-key.

@Luigi6821
Copy link

Hi, @tpeczek
First of all thank you for your prompt reply.
I tried your solution but I still have some troubles.
Basically the route value I receive from controller like following:
[ActionName("sse-notifications-license")]
[Route("/Notifications/[action]/{appKey}")]
[AcceptVerbs("GET")]
public IActionResult Receiver(string appKey)
{
// AppKey value is in this.Request.RouteValues
return View("Receiver");
}

Is not present in http context of customized ClientIdProvider
public Guid AcquireClientId(HttpContext context)
{
// context.Request.RoutValues is empty
}

I am sure I am doing something wrong.
Please help me.

Thanks again

@tpeczek
Copy link
Owner Author

tpeczek commented Mar 25, 2023

Hi @Luigi6821,

It looks like you might be confusing endpoints. Any controller endpoint you may have in your application is not related to any SSE endpoint you might have. Requests are also separated. This means that you must properly define the route when you are registering your SSE endpoint:

public class Startup
{
    ...

    public void Configure(IApplicationBuilder app, IHostEnvironment env)
    {
        ...

        app
            ...
            .UseRouting()
            .UseEndpoints(endpoints =>
            {
                endpoints.MapServerSentEvents("/sse-notifications/{appKey}");

                ...
            });
    }
}

You also need to make sure that the client side code is providing the value while creating EventSource.

var source = new EventSource("/sse-notifications/app-key");

@Luigi6821
Copy link

Hi @Luigi6821,

It looks like you might be confusing endpoints. Any controller endpoint you may have in your application is not related to any SSE endpoint you might have. Requests are also separated. This means that you must properly define the route when you are registering your SSE endpoint:

public class Startup
{
    ...

    public void Configure(IApplicationBuilder app, IHostEnvironment env)
    {
        ...

        app
            ...
            .UseRouting()
            .UseEndpoints(endpoints =>
            {
                endpoints.MapServerSentEvents("/sse-notifications/{appKey}");

                ...
            });
    }
}

You also need to make sure that the client side code is providing the value while creating EventSource.

var source = new EventSource("/sse-notifications/app-key");

Hi @tpeczek ,
Thank you again!
I imagined something like that but I can't know the user appKey until he calls controller endpoint
/Notifications/[action]/{appKey}.
So I can't register endpoints.MapServerSentEvents("/sse-notifications/{appKey}") before user uses it.
The supplied appKey identifies the user license and should be associated to the clientId (guid) in some ways in order to send
notification of license usages to that "listening" user.
Hope to have better clarified my needs

Regards
Luigi

@Luigi6821
Copy link

Hi @tpeczek,
Maybe I found something useful doing the following:

<script> var appKey = "@Model.AppKey"; var notifications = document.getElementById('notifications'); var showNotification = function(data) { var preformatedContainer = document.createElement('pre'); preformatedContainer.appendChild(document.createTextNode(data)); notifications.appendChild(preformatedContainer); notifications.appendChild(document.createElement('hr')); }; var source = new EventSource('/sse-notifications-license?appKey=' + appKey); source.onmessage = function (event) { showNotification(event.data); }; source.addEventListener('alert', function(event) { alert(event.data); }); </script>

In this way The OnConnected SSE service event receives a QueryString containing supplied appKey and also the client related to it.
Is this a possible solution?
Thanks
Luigi

@tpeczek
Copy link
Owner Author

tpeczek commented Mar 25, 2023

@Luigi6821

The MapServerSentEvents in the snippet I've provided supports route template. The endpoints.MapServerSentEvents("/sse-notifications/{appKey}"); is the exact code (well the initial part of your path might be different of course) which will make appKey available through RouteValues.

@tpeczek
Copy link
Owner Author

tpeczek commented Mar 25, 2023

Hi @tpeczek, Maybe I found something useful doing the following:

<script> var appKey = "@Model.AppKey"; var notifications = document.getElementById('notifications'); var showNotification = function(data) { var preformatedContainer = document.createElement('pre'); preformatedContainer.appendChild(document.createTextNode(data)); notifications.appendChild(preformatedContainer); notifications.appendChild(document.createElement('hr')); }; var source = new EventSource('/sse-notifications-license?appKey=' + appKey); source.onmessage = function (event) { showNotification(event.data); }; source.addEventListener('alert', function(event) { alert(event.data); }); </script>

In this way The OnConnected SSE service event receives a QueryString containing supplied appKey and also the client related to it. Is this a possible solution? Thanks Luigi

Yes, this frontend change is in general correct - you need to provide appKey from model to EventSource constructor.

You can do this with query string, but the approach with using OnConnected is a little bit hacky. I would suggest route base approach with client identifier provider. But, ultimately, both will work so you can choose the one which suits you better.

@ssteiner
Copy link

Is there a way to run this when there's no default authentication scheme? I'm on an App that still doesn't use the standard authentication mechanism in ASP.NET Core.. so when a client tries to connect to the SSE endpoint, it gets tripped up in the ServerSentEventsMiddleware's AuthorizeAsync method with

System.InvalidOperationException: No authenticationScheme was specified, and there was no DefaultChallengeScheme found. The default schemes can be set using either AddAuthentication(string defaultScheme) or AddAuthentication(Action configureOptions).

The authorization code being custom, I need IServerSentEventsService.ClientConnected to fire to read the request properties that allows me to decide whether to client is really allowed or to disconnect it again.

@tpeczek
Copy link
Owner Author

tpeczek commented Nov 24, 2023

Is there a way to run this when there's no default authentication scheme? I'm on an App that still doesn't use the standard authentication mechanism in ASP.NET Core.. so when a client tries to connect to the SSE endpoint, it gets tripped up in the ServerSentEventsMiddleware's AuthorizeAsync method with

System.InvalidOperationException: No authenticationScheme was specified, and there was no DefaultChallengeScheme found. The default schemes can be set using either AddAuthentication(string defaultScheme) or AddAuthentication(Action configureOptions).

The authorization code being custom, I need IServerSentEventsService.ClientConnected to fire to read the request properties that allows me to decide whether to client is really allowed or to disconnect it again.

The ServerSentEventsMiddleware will not perform authentication if ServerSentEventsOptions.Authorization is null (which is the default).

That said, I wouldn't advise using IServerSentEventsService.ClientConnected for authentication because it runs after the connection has been already established and doesn't provide tools (beyond hackery) to deny it. I would rather suggest using ASP.NET Core authentication and authorization extensibility points to get the functionality you want (for example autherization handlers). You can read more here:

@ssteiner
Copy link

I don't plan to do this but as a stopgap solution. The old mechanism with pushing data over a Chunked Channel (in a way, rather similar to SSE - but not standardizes) doesn't seem to work anymore in ASP.NET Core, the three attempts I made to fix all failed, so I figured since I'm already using your lib in two other products, why not go down that route. It's for dev clients only in a dev environment. Prod Clients won't use SSE for now - it's just that swapping out the custom (10+ year old) auth scheme is more a 'in a sprint' undertaking, than a 'in a few hours'..

@tpeczek
Copy link
Owner Author

tpeczek commented Nov 24, 2023

I don't plan to do this but as a stopgap solution. The old mechanism with pushing data over a Chunked Channel (in a way, rather similar to SSE - but not standardizes) doesn't seem to work anymore in ASP.NET Core, the three attempts I made to fix all failed, so I figured since I'm already using your lib in two other products, why not go down that route. It's for dev clients only in a dev environment. Prod Clients won't use SSE for now - it's just that swapping out the custom (10+ year old) auth scheme is more a 'in a sprint' undertaking, than a 'in a few hours'..

Understood. In that case I would suggest one more approach to consider (as IServerSentEventsService.ClientConnected will really put you into hard to clean up situation) - how about registering a small middleware on the same route before the ServerSentEventsMiddleware? This will enable you to short circuit the request based on the conditions you need before the ServerSentEventsMiddleware will start building a client out of it.

@ssteiner
Copy link

Do you have an example I could build upon for that middleware? Might be worth a look.

@tpeczek
Copy link
Owner Author

tpeczek commented Nov 28, 2023

Do you have an example I could build upon for that middleware? Might be worth a look.

This post of mine should get you started. It goes too far for your needs (because it drags in IAuthorizationService which you don't want, instead you want to do your own thing) and shows some age (we are talking ASP.NET Core 3 times), but should get you going.

https://www.tpeczek.com/2019/01/aspnet-core-middleware-and-authorization.html

@ssteiner
Copy link

ssteiner commented Nov 29, 2023

well, tried with your first approach. Added my own IAuthorizationService too.. so that calling await authorizationService.AuthorizeAsync return Success. Yet, calling the /sse-endpoint now returns in a 404

My Startup.Cs

    services.AddAuthorization(options =>
    {
        options.AddPolicy("PolicyName", policy => policy.RequireClaim("SessionId"));
    });

    var sseKeepAliveInterval = Configuration.GetValue("AppSettings:SseKeepAliveInterval", 15);
    services.AddServerSentEvents(options =>
    {
        options.ReconnectInterval = 5000;
        options.KeepaliveInterval = sseKeepAliveInterval;
        options.KeepaliveMode = ServerSentEventsKeepaliveMode.Always;
    });

    app.UseAuthentication();
    app.Map("/sse-endpoint", branchedApp =>
    {
        branchedApp.UseAuthorization("PolicyName");
    });
    app.MapServerSentEvents("/sse-endpoint", new ServerSentEventsOptions
    {
        Authorization = ServerSentEventsAuthorization.Default
    });
    var sseService = app.ApplicationServices.GetService<IServerSentEventsService>();

If I set Authorization back to null, and remove app.Map, the client can connect again. So it seems app.Map and app.MapServerSentEvents interfere with each other

@tpeczek
Copy link
Owner Author

tpeczek commented Nov 29, 2023

Ok, some thoughts on this snippet.

If you are defining a policy (it seems that you do), you shouldn't be needing an additional middleware. This should be enough:

app.MapServerSentEvents("/sse-endpoint", new ServerSentEventsOptions
{
    Authorization = new ServerSentEventsAuthorization { Policy = "PolicyName" }
});

If you want an additional middleware, you should register Server-Sent Events in that branched pipeline without authorization.

app.Map("/sse-endpoint", branchedApp =>
{
    branchedApp.UseAuthorization("PolicyName");
    branchedApp.UseServerSentEvents();
});

Picking one of those most likely will get you the right result (unless there is some additional important context not present in the snippet).

@ssteiner
Copy link

I tried the second approach first and it works :)
I do inject the required claims into httpContext.User in my middleware (so that my own implementation of IAuthorizationService can check for the existence of my custom claims, and then return Success (claim present) or Failed (claim not present). Not sure there's anywhere else do do that.

@tpeczek
Copy link
Owner Author

tpeczek commented Nov 29, 2023

That sounds good, just triple check everything because you know - security ;)

@Droni
Copy link

Droni commented Dec 12, 2023

Hi. Why the library in nuget is not strongly named?

@tpeczek
Copy link
Owner Author

tpeczek commented Dec 12, 2023

Hi. Why the library in nuget is not strongly named?

Hi @Droni,

The honest answer is that there was never a compelling reason to do so.

In case of .NET Core and .NET, strong-naming has no benefits - the runtime never validates the strong-name signature and doesn't use the strong-name for assembly binding.

The only remaining case where there is a potential value is ASP.NET Core 2.1 on .NET Framework, but there are also drawbacks and usage for that specific case was never high enough to justify them.

@Droni
Copy link

Droni commented Dec 12, 2023

Hm. Thanks for your reply. I was currently using your library in my project which uses strong names and I got a compilation error. To solve this problem, various ways are proposed, including decompiling and rebuilding the library. But I think it would be better to use strong names from the very beginning :)

P.S. I use .Net 8

@tpeczek
Copy link
Owner Author

tpeczek commented Dec 12, 2023

An interesting question to ask would be why the project is requiring strong-naming.

In case of .NET Framework strong-naming was enabling relying on assemblies in GAC and avoiding assemblies conflicts, but in .NET there is none of that.

I could start signing the library but, as it is considered a breaking change, I would need a compelling reason to do that. This is the first time when the subject has been even raised.

@Droni
Copy link

Droni commented Dec 13, 2023

All commercial products use a signature. This is more to show that the library is not fake and clearly indicates its ownership. For example, when Microsoft started using NewtonSoft.Json, they couldn't do it if the library wasn't signed. If you look into the future and if you want your development to become more popular, then sooner or later you will have to think about it.

@tpeczek
Copy link
Owner Author

tpeczek commented Dec 13, 2023

Well that's not entirely true (but mostly was back in .NET Framework days). First of all, strong naming doesn't indicate ownership, it just creates an identity. In fact the Microsoft recommendation on strong naming is CONSIDER not DO. At the same time the recommendation is that if you do, you should keep the key in source control so maintainers can recompile with the same one.

image

Also, as I previously written, strong naming has no value in case of .NET Core and .NET and can be easily suppressed.

image

This is coming straight from Open-Source Library Guidance : https://learn.microsoft.com/en-us/dotnet/standard/library-guidance/strong-naming

And from experience, strong naming in current days is a consideration only for projects which have legacy ties to .NET Framework. If such projects will start looking for broader adoption of this library, or there will be specific project with a compelling need, I will be happy to consider that change.

@ssteiner
Copy link

ssteiner commented Mar 7, 2024

is there a particular reason why we can modify the reconnect interval on IServerSentEventsService but not the keepalive configuration?

Also, is there a way to disable SSE again at runtime? I figure use app.UseWhen in Startup.cs, but I'd have to somehow shut down the connections and cleanup.

@tpeczek
Copy link
Owner Author

tpeczek commented Mar 8, 2024

Hi @ssteiner,

If you are asking for a capability to modify keepalive after the endpoint has been spin up, than it is not supported because it would have strong consequences on the hosted service which is performing the job (it essentially would have to be reinitialised with different setup).

From broader perspective, keepalives are an infrastructure concept - they are there to mitigate any intermediaries you may have configured along the path, so there shouldn't be a need to change that configuration at runtime.

Regarding the second question, I'm not sure I fully understand what you are trying to achieve there and not sure how can I guide you. Removing endpoints while application is working sounds unusual, so while disconnecting clients is supported, removing the endpoint is not backed in any way.

@ssteiner
Copy link

ssteiner commented Mar 8, 2024

Here's what I'm trying to do: I'd like to be able to turn on/off the SSE functionality at runtime. So, initially, SSE is off, meaning the sse endpoint uri would return 404. Then, somebody decides we want to use SSE now (which we use for dynamic GUI updates), so they turn it on (at runtime), and from that point on, the sse endpoint uri would be accessible.

So I tried something like this

app.UseWhen(context => { var toggle = context.RequestServices.GetRequiredService<StartupConfig>(); return toggle.EnableSse; }, builder => { app.MapServerSentEvents(sseUri, new ServerSentEventsOptions { Authorization = ServerSentEventsAuthorization.Default }); });

But, regardless of the the value of toggle.EnableSee, the SSE endpoint is always accessible on sseUri with this setup (this runs in Startup.cs in the Configure method.

My thinking was: why spin up something that isn't being used.

@tpeczek
Copy link
Owner Author

tpeczek commented Mar 9, 2024

Technically you may be able to achieve what you want, but it's completely outside of the scope of the library internals.

Instead of using MapServerSentEvents extensions provided by the library, you will rather have to use MapWhen to register middleware manually.

There also may be unforeseen consequences you will have to resolve.

@ssteiner
Copy link

Okay, I guess it's not mean to be used like that so I'll revert to the static activation/configuration/deactivation at startup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests