SignalR Sample

[DovydasNavickas]

A sample project with basic ASOS and JwtBearerAuthentication for SignalR usage.

[kevinchalet]

Really good job, Dovydas! :+1: :clap:

I'm really concerned about the security aspects of flowing the access token in the query string, which could potentially lead to session fixation-like attacks. We should maybe consider updating the OnReceivingToken event to ensure we only retrieve the token from the query string if it the request path corresponds to the SignalR path?

[kevinchalet]

nit: consider adding a samples directory at the root of the solution :smile:

[DovydasNavickas]

I updated sample according to all comments except for moving to samples directory and checking if it's SignalR path in OnReceivingToken, because you can set custom path for SignalR. I'm gonna look if there is a way to figure out if the path is set to serve SignalR.

As for samples directory, I'm not sure that is really needed because the repository is already dedicated for samples anyway.

[kevinchalet]

As for samples directory, I'm not sure that is really needed because the repository is already dedicated for samples anyway.

ATM, it's not really needed since we have only one example :smile: But in the future, we'll probably want to avoid having too many folders in the root.

[DovydasNavickas]

As it does not make much of a difference and in case we are planning to move projects either way, I moved it already. If we leave the checking for SignalR connection for a bit later time, I think you can merge the PR.

[kevinchalet]

I updated sample according to all comments except for moving to samples directory and checking if it's SignalR path in OnReceivingToken, because you can set custom path for SignalR. I'm gonna look if there is a way to figure out if the path is set to serve SignalR.

Looks like you configured the SignalR middleware to bind the connection to /signalr. Maybe you could simply use that?

[DovydasNavickas]

Yeah, I could use '/signalr'. But there can be multiple SignalR endpoints, which makes it a bit tricky then. If I one looks at the sample and hardcodes '/signalr', later on adding more SignalR endpoints makes a necessity to add them into OnReceivingToken method.

[DovydasNavickas]

I've manually transpiled ES6 code to ES5 and now it works on all browsers OOTB. Maybe when I add UI manipulations I'll go back to ES6 with transpiler, but I think there is an unnecessary burden for this sample with console to have ES6 with transpiler, because most of developers will have backend knowledge, but not necessarily have frontend knowledge about ES6 and transpilers.

[kevinchalet]

Looks fine :+1:

One minor remark: the other repositories use K&R bracing style. Do you want me to update your PR before merging it or is it something you can easily do?

Also, don't hesitate to squash your commits :smile:

[DovydasNavickas]

Updated middlewares order and brace styling to K&R, though I like Allman style more myself (most of AspNet stack uses it). If everything is good to go, I will squash the commits then.

[DovydasNavickas]

https://github.com/aspnet-contrib/AspNet.Security.OpenIdConnect.Samples/pull/1#discussion_r45983059 What about now? It has to be good or I'm missing something here :D

[kevinchalet]

Minor one last remark, it's fine. Squash it and I'll merge it :+1:

[kevinchalet]

Merged, thanks! https://github.com/aspnet-contrib/AspNet.Security.OpenIdConnect.Samples/commit/06be66cd96d568ebc0db3f99370af61246282b6a :tada: