fix issue with incorrect user detection when Invoking for User #747

ivankarpey · 2017-08-22T08:44:37Z

At the current moment in DefaultHubLifetimeManager<THub>.InvokeUserAsync user detected by Name, not by Id, even if userId is required via method argument list. That's incorrect behavior and could lead to data leaks to other user, since it's not guaranteed that user name is unique. Id should be used there as it required via method arguments.

dnfclas · 2017-08-22T08:44:41Z

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

moozzyk · 2017-08-22T17:22:08Z

src/Microsoft.AspNetCore.SignalR/DefaultHubLifetimeManager.cs

@@ -104,7 +104,7 @@ public override Task InvokeGroupAsync(string groupName, string methodName, objec
        public override Task InvokeUserAsync(string userId, string methodName, object[] args)
        {
            return InvokeAllWhere(methodName, args, connection => 
-                string.Equals(connection.User.FindFirst(ClaimTypes.NameIdentifier).Value, userId, StringComparison.Ordinal));
+                string.Equals(connection.User.FindFirstValue(ClaimTypes.NameIdentifier), userId, StringComparison.Ordinal));


How about connection.User.FindFirst(ClaimTypes.NameIdentifier)?.Value and not taking the dependency on Microsoft.Extensions.Identity.Core?

-- removed -- Sorry, missed .? operator in your peace of code. Ok, I will do that.

moozzyk · 2017-08-22T17:28:39Z

test/Microsoft.AspNetCore.SignalR.Tests/TestClient.cs

-            Connection.User = new ClaimsPrincipal(new ClaimsIdentity(new[] { new Claim(ClaimTypes.Name, Interlocked.Increment(ref _id).ToString()) }));
+
+            string claimValue = Interlocked.Increment(ref _id).ToString();
+            List<Claim> claims = new List<Claim>{ new Claim(ClaimTypes.Name, claimValue) };


moozzyk · 2017-08-22T17:28:46Z

test/Microsoft.AspNetCore.SignalR.Tests/TestClient.cs

@@ -38,7 +38,15 @@ public TestClient(bool synchronousCallbacks = false)
            _transport = ChannelConnection.Create<byte[]>(input: transportToApplication, output: applicationToTransport);

            Connection = new DefaultConnectionContext(Guid.NewGuid().ToString(), _transport, Application);
-            Connection.User = new ClaimsPrincipal(new ClaimsIdentity(new[] { new Claim(ClaimTypes.Name, Interlocked.Increment(ref _id).ToString()) }));
+
+            string claimValue = Interlocked.Increment(ref _id).ToString();


moozzyk · 2017-08-22T17:30:00Z

test/Microsoft.AspNetCore.SignalR.Tests/TestClient.cs

@@ -28,7 +28,7 @@ public class TestClient : IDisposable, IInvocationBinder
        public Channel<byte[]> Application { get; }
        public Task Connected => ((TaskCompletionSource<bool>)Connection.Metadata["ConnectedTask"]).Task;

-        public TestClient(bool synchronousCallbacks = false)
+        public TestClient(bool synchronousCallbacks = false, bool shouldHaveId = false)


consider renaming toshouldHaveId to addClaimId - as it is now it is not clear what the id refers to

moozzyk · 2017-08-22T17:31:08Z

test/Microsoft.AspNetCore.SignalR.Tests/TestClient.cs

@@ -53,6 +61,7 @@ public TestClient(bool synchronousCallbacks = false)
            }
        }

+


nit: remove

moozzyk · 2017-08-22T17:34:27Z

/cc @BrennanConroy - can you also take a look?

BrennanConroy · 2017-08-22T17:45:52Z

Redis will have a similar issue I believe

SignalR/src/Microsoft.AspNetCore.SignalR.Redis/RedisHubLifetimeManager.cs

Line 157 in 79d8a9d

    
           var userChannel = typeof(THub).FullName + ".user." + connection.User.Identity.Name;

davidfowl · 2017-08-23T05:45:20Z

We need to introduce that service so we can share the logic.

…ations

moozzyk · 2017-08-23T16:20:35Z

test/Microsoft.AspNetCore.SignalR.Tests/TestClient.cs

@@ -52,7 +60,7 @@ public TestClient(bool synchronousCallbacks = false)
                Application.Out.TryWrite(memoryStream.ToArray());
            }
        }
-
+        


nit: remove

In my editor there is single line freespace as in other places across the file. Not sure why it shows line added there.

It is a line containing whitespaces - if you are using VS you can install this extension and you will never miss it.

moozzyk · 2017-08-23T16:22:18Z

test/Microsoft.AspNetCore.SignalR.Tests/TestClient.cs

@@ -28,7 +28,7 @@ public class TestClient : IDisposable, IInvocationBinder
        public Channel<byte[]> Application { get; }
        public Task Connected => ((TaskCompletionSource<bool>)Connection.Metadata["ConnectedTask"]).Task;

-        public TestClient(bool synchronousCallbacks = false)
+        public TestClient(bool synchronousCallbacks = false, bool addClaimId = false)


I was just thinking - is there any reason to not add the claimId always (i.e. can we even not pass this parameter)?

Having this param permanent will require to fix test case with unauthorized user, since it's currently check that user have no NameIdentifier claim (well, testcase not throwing unauthorized exception if user have those claim, but case expect it will be thrown).

So avoid that kind of parameter will probably require some test code changes and introducing 2 types of clients - authorized and unauthorized. Wasn't sure if that's something to make sense to do, so decide just start with additional parameter.

Leave as it is then for now.

BrennanConroy · 2017-08-23T16:28:22Z

src/Microsoft.AspNetCore.SignalR.Redis/RedisHubLifetimeManager.cs

@@ -187,7 +188,7 @@ public override Task OnConnectedAsync(HubConnectionContext connection)

            if (connection.User.Identity.IsAuthenticated)
            {
-                var userChannel = _channelNamePrefix + ".user." + connection.User.Identity.Name;
+                var userChannel = _channelNamePrefix + ".user." + connection.User.FindFirst(ClaimTypes.NameIdentifier)?.Value;


If there isn't a value then we probably shouldn't be subscribing to _channelNamePrefix + ".user."

Also, you need to have this below where we publish to the user channel

Is it a valid case that user is Authenticated but don't have NameIdentifier claim?

Also, you need to have this below where we publish to the user channel

Sorry, not sure I get that correctly. You mean InvokeUserAsync method?

@BrennanConroy Ok, let's assume it's possible case, that user authenticated, but still have no NameIdentifier claim. What behavior you expect to see there. Just do nothing or throwing InvalidOperationException ?

moozzyk · 2017-08-25T20:03:22Z

@davidfowl made me aware about this issue: #604. Looks like this is the way to go here...

ivankarpey · 2017-08-25T21:04:13Z

@moozzyk Should I implement DefaultUserIdProvider, add it to signalr configuration dependency injections and than change this part of code to use this provider?

davidfowl · 2017-08-25T21:04:58Z

@moozzyk Should I implement DefaultUserIdProvider, add it to signalr configuration dependency injections and than change this part of code to use this provider?

Yep!

BrennanConroy · 2017-09-21T18:37:52Z

@ivankarpey Any progress on this change?

ivankarpey · 2017-09-30T16:56:51Z

Sorry Guys. Was on vac. I will provide updated version before Monday.

davidfowl · 2017-10-05T23:28:17Z

src/Microsoft.AspNetCore.SignalR.Core/DefaultHubLifetimeManager.cs

-                return string.Equals(connection.User.Identity.Name, userId, StringComparison.Ordinal);
-            });
+            return InvokeAllWhere(methodName, args, connection => 
+                string.Equals(_userIdProvider.GetUserId(connection), userId, StringComparison.Ordinal));


Lets run this in OnConnectedAsync and cache it on the connection object itself.

davidfowl · 2017-10-05T23:28:30Z

src/Microsoft.AspNetCore.SignalR.Core/IUserIdProvider.cs

+{
+    public interface IUserIdProvider
+    {
+        string GetUserId(HubConnectionContext conn);


nit: connection

davidfowl · 2017-10-05T23:29:06Z

src/Microsoft.AspNetCore.SignalR.Redis/RedisHubLifetimeManager.cs

@@ -245,9 +249,9 @@ public override Task OnConnectedAsync(HubConnectionContext connection)
                previousConnectionTask = WriteAsync(connection, message);
            });

-            if (connection.User.Identity.IsAuthenticated)
+            if (connection.User.Identity.IsAuthenticated && _userIdProvider.GetUserId(connection) != null)


Don't check for authenticated, just check for null and store the result in a local so it's not executed twice.

davidfowl · 2017-10-06T00:13:01Z

src/Microsoft.AspNetCore.SignalR.Core/HubConnectionContext.cs

@@ -25,12 +26,15 @@ public class HubConnectionContext
        private readonly ConnectionContext _connectionContext;
        private readonly CancellationTokenSource _connectionAbortedTokenSource = new CancellationTokenSource();
        private readonly TaskCompletionSource<object> _abortCompletedTcs = new TaskCompletionSource<object>();
+        private readonly IUserIdProvider _userIdProvider;
+        private string _userIdCache = null;


Super nit: Just call this _userId.

davidfowl · 2017-10-06T00:15:26Z

src/Microsoft.AspNetCore.SignalR.Core/HubConnectionContext.cs

@@ -67,6 +71,18 @@ public virtual void Abort()
            Task.Factory.StartNew(_abortedCallback, this);
        }

+        public string UserIdentifier


I'm not comfortable with the lazy evaluation of this. I think it should be set by the HubEndPoint after negotiate.

Well, I don't really like the idea of having public setter for that sort of things. Since I think that for end-user connection context should be something "immutable". Don't you think so?

I agree with that too, make it an internal setter. We'll need to see what mocking this object looks like though.

davidfowl · 2017-10-06T01:02:33Z

src/Microsoft.AspNetCore.SignalR.Redis/RedisHubLifetimeManager.cs

@@ -245,9 +245,9 @@ public override Task OnConnectedAsync(HubConnectionContext connection)
                previousConnectionTask = WriteAsync(connection, message);
            });

-            if (connection.User.Identity.IsAuthenticated)
+            if (!String.IsNullOrEmpty(connection.UserIdentifier))


nit: lowercase s in string

davidfowl

LGTM!

BrennanConroy · 2017-10-06T15:19:12Z

src/Microsoft.AspNetCore.SignalR.Core/DefaultHubLifetimeManager.cs

@@ -14,7 +14,7 @@ public class DefaultHubLifetimeManager<THub> : HubLifetimeManager<THub>
    {
        private long _nextInvocationId = 0;
        private readonly HubConnectionList _connections = new HubConnectionList();
-
+        


nit: remove empty space

BrennanConroy · 2017-10-06T15:20:20Z

src/Microsoft.AspNetCore.SignalR.Core/DefaultHubLifetimeManager.cs

@@ -166,7 +163,7 @@ public override Task OnDisconnectedAsync(HubConnectionContext connection)
                if (connection.Output.TryWrite(hubMessage))
                {
                    break;
-                }
+                } 


nit: remove space

Changes were made

fix issue with incorrect user detection when Invoking for User

07c4e79

dnfclas added the cla-not-required label Aug 22, 2017

ivankarpey added 2 commits August 22, 2017 12:55

fix failed testcases

3408e09

use proper extension method to avoid potential null reference exception

4572645

moozzyk previously requested changes Aug 22, 2017

View reviewed changes

ivankarpey added 3 commits August 23, 2017 15:08

fix for channel name in redis version + follow SignalR team recommend…

f534b4e

…ations

merge with latest changes from dev branch

0da1a24

remove unncessary freespace

fe6c28a

moozzyk reviewed Aug 23, 2017

View reviewed changes

BrennanConroy reviewed Aug 23, 2017

View reviewed changes

remove whitespaces

3270d9e

ivankarpey added 2 commits October 6, 2017 00:17

merge with latest changes from dev branch

9445232

introduce IUserIdProvider to resolve user id

b93e024

davidfowl reviewed Oct 5, 2017

View reviewed changes

Move IUserIdProvider from HubLifetimeManager to HubConnectionContext

aee0337

davidfowl reviewed Oct 6, 2017

View reviewed changes

setting user id to connection context in hubendpoint

1548fe8

davidfowl reviewed Oct 6, 2017

View reviewed changes

davidfowl approved these changes Oct 6, 2017

View reviewed changes

String -> string

8913166

BrennanConroy approved these changes Oct 6, 2017

View reviewed changes

ivankarpey and others added 2 commits October 6, 2017 21:33

remove whitespaces

cbeb3c1

remove whitespace

012766f

davidfowl merged commit 665f166 into aspnet:dev Oct 6, 2017

		@@ -53,6 +61,7 @@ public TestClient(bool synchronousCallbacks = false)
		}
		}

fix issue with incorrect user detection when Invoking for User #747

fix issue with incorrect user detection when Invoking for User #747

Conversation

ivankarpey commented Aug 22, 2017 • edited Loading

dnfclas commented Aug 22, 2017

Choose a reason for hiding this comment

ivankarpey Aug 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moozzyk commented Aug 22, 2017

BrennanConroy commented Aug 22, 2017 • edited Loading

davidfowl commented Aug 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivankarpey Aug 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moozzyk commented Aug 25, 2017

ivankarpey commented Aug 25, 2017

davidfowl commented Aug 25, 2017

BrennanConroy commented Sep 21, 2017

ivankarpey commented Sep 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl Oct 6, 2017 • edited Loading

Choose a reason for hiding this comment

davidfowl Oct 6, 2017 • edited Loading

Choose a reason for hiding this comment

ivankarpey Oct 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl Oct 6, 2017 • edited Loading

Choose a reason for hiding this comment

davidfowl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivankarpey commented Aug 22, 2017 •

edited

Loading

ivankarpey Aug 22, 2017 •

edited

Loading

BrennanConroy commented Aug 22, 2017 •

edited

Loading

ivankarpey Aug 23, 2017 •

edited

Loading

davidfowl Oct 6, 2017 •

edited

Loading

davidfowl Oct 6, 2017 •

edited

Loading

ivankarpey Oct 6, 2017 •

edited

Loading

davidfowl Oct 6, 2017 •

edited

Loading