Skip to content

Commit

Permalink
if a transaction was active, ACL checks where skipped; this fixes tha…
Browse files Browse the repository at this point in the history 
…t, and fixes the MULTI command test to work now that DISCARD gets correctly blocked (microsoft#459)
  • Loading branch information
@kevin-montrose
kevin-montrose authored Jun 11, 2024
1 parent 5916135 commit 93145ee
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 28 deletions.
18 changes: 12 additions & 6 deletions libs/server/Resp/RespServerSession.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -312,13 +312,19 @@ private void ProcessMessages()
{
success = ProcessBasicCommands(cmd, count, ptr, ref lockableGarnetApi);
}
else success = cmd switch
else
{
RespCommand.EXEC => NetworkEXEC(ptr),
RespCommand.MULTI => NetworkMULTI(ptr),
RespCommand.DISCARD => NetworkDISCARD(),
_ => NetworkSKIP(cmd, count),
};
if (CheckACLPermissions(cmd, ptr, count, out success))
{
success = cmd switch
{
RespCommand.EXEC => NetworkEXEC(ptr),
RespCommand.MULTI => NetworkMULTI(ptr),
RespCommand.DISCARD => NetworkDISCARD(),
_ => NetworkSKIP(cmd, count),
};
}
}
}
else
{
Expand Down
62 changes: 40 additions & 22 deletions test/Garnet.test/Resp/ACL/RespCommandTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3833,16 +3833,27 @@ public void MultiACLs()
{
CheckCommands(
"MULTI",
[DoMulti]
[DoMulti],
skipPing: true
);

static void DoMulti(IServer server)
{
RedisResult val = server.Execute("MULTI");
Assert.AreEqual("OK", (string)val);
try
{
RedisResult val = server.Execute("MULTI");
Assert.AreEqual("OK", (string)val);
}
catch (RedisException e)
{
// The "nested MULTI" error response is also legal, if we're ACL'd for MULTI
if (e.Message == "ERR MULTI calls can not be nested")
{
return;
}

// if we got here, abort the transaction
server.Execute("DISCARD");
throw;
}
}
}

Expand Down Expand Up @@ -5775,7 +5786,8 @@ static void DoUnwatch(IServer server)
private static void CheckCommands(
string command,
Action<IServer>[] commands,
List<string> knownCategories = null
List<string> knownCategories = null,
bool skipPing = false
)
{
const string UserWithAll = "temp-all";
Expand Down Expand Up @@ -5841,22 +5853,22 @@ private static void CheckCommands(
{
ResetUserWithAll(defaultUserServer);

AssertAllPermitted(defaultUserServer, UserWithAll, allUserServer, commands, $"[{command}]: Denied when should have been permitted (user had +@all)");
AssertAllPermitted(defaultUserServer, UserWithAll, allUserServer, commands, $"[{command}]: Denied when should have been permitted (user had +@all)", skipPing);

SetUser(defaultUserServer, UserWithAll, [$"-@{category}"]);

AssertAllDenied(defaultUserServer, UserWithAll, allUserServer, commands, $"[{command}]: Permitted when should have been denied (user had -@{category})");
AssertAllDenied(defaultUserServer, UserWithAll, allUserServer, commands, $"[{command}]: Permitted when should have been denied (user had -@{category})", skipPing);
}

// Check adding category works
{
ResetUserWithNone(defaultUserServer);

AssertAllDenied(defaultUserServer, UserWithNone, nonUserServer, commands, $"[{command}]: Permitted when should have been denied (user had -@all)");
AssertAllDenied(defaultUserServer, UserWithNone, nonUserServer, commands, $"[{command}]: Permitted when should have been denied (user had -@all)", skipPing);

SetACLOnUser(defaultUserServer, UserWithNone, [$"+@{category}"]);

AssertAllPermitted(defaultUserServer, UserWithNone, nonUserServer, commands, $"[{command}]: Denied when should have been permitted (user had +@{category})");
AssertAllPermitted(defaultUserServer, UserWithNone, nonUserServer, commands, $"[{command}]: Denied when should have been permitted (user had +@{category})", skipPing);
}
}

Expand All @@ -5874,7 +5886,7 @@ private static void CheckCommands(

SetACLOnUser(defaultUserServer, UserWithAll, [$"-{commandAcl}"]);

AssertAllDenied(defaultUserServer, UserWithAll, allUserServer, commands, $"[{command}]: Permitted when should have been denied (user had -{commandAcl})");
AssertAllDenied(defaultUserServer, UserWithAll, allUserServer, commands, $"[{command}]: Permitted when should have been denied (user had -{commandAcl})", skipPing);
}

// Check adding command works
Expand All @@ -5883,7 +5895,7 @@ private static void CheckCommands(

SetACLOnUser(defaultUserServer, UserWithNone, [$"+{commandAcl}"]);

AssertAllPermitted(defaultUserServer, UserWithNone, nonUserServer, commands, $"[{command}]: Denied when should have been permitted (user had +{commandAcl})");
AssertAllPermitted(defaultUserServer, UserWithNone, nonUserServer, commands, $"[{command}]: Denied when should have been permitted (user had +{commandAcl})", skipPing);
}
}

Expand All @@ -5899,7 +5911,7 @@ private static void CheckCommands(

SetACLOnUser(defaultUserServer, UserWithAll, [$"-{subCommandAcl}"]);

AssertAllDenied(defaultUserServer, UserWithAll, allUserServer, commands, $"[{command}]: Permitted when should have been denied (user had -{subCommandAcl})");
AssertAllDenied(defaultUserServer, UserWithAll, allUserServer, commands, $"[{command}]: Permitted when should have been denied (user had -{subCommandAcl})", skipPing);
}

// Check adding subcommand works
Expand All @@ -5908,7 +5920,7 @@ private static void CheckCommands(

SetACLOnUser(defaultUserServer, UserWithNone, [$"+{subCommandAcl}"]);

AssertAllPermitted(defaultUserServer, UserWithNone, nonUserServer, commands, $"[{command}]: Denied when should have been permitted (user had +{subCommandAcl})");
AssertAllPermitted(defaultUserServer, UserWithNone, nonUserServer, commands, $"[{command}]: Denied when should have been permitted (user had +{subCommandAcl})", skipPing);
}

// Checking adding command but removing subcommand works
Expand All @@ -5917,7 +5929,7 @@ private static void CheckCommands(

SetACLOnUser(defaultUserServer, UserWithNone, [$"+{commandAcl}", $"-{subCommandAcl}"]);

AssertAllDenied(defaultUserServer, UserWithNone, nonUserServer, commands, $"[{command}]: Permitted when should have been denied (user had +{commandAcl} -{subCommandAcl})");
AssertAllDenied(defaultUserServer, UserWithNone, nonUserServer, commands, $"[{command}]: Permitted when should have been denied (user had +{commandAcl} -{subCommandAcl})", skipPing);
}

// Checking removing command but adding subcommand works
Expand All @@ -5926,7 +5938,7 @@ private static void CheckCommands(

SetACLOnUser(defaultUserServer, UserWithAll, [$"-{commandAcl}", $"+{subCommandAcl}"]);

AssertAllPermitted(defaultUserServer, UserWithAll, allUserServer, commands, $"[{command}]: Denied when should have been permitted (user had -{commandAcl} +{subCommandAcl})");
AssertAllPermitted(defaultUserServer, UserWithAll, allUserServer, commands, $"[{command}]: Denied when should have been permitted (user had -{commandAcl} +{subCommandAcl})", skipPing);
}
}
}
Expand Down Expand Up @@ -5956,27 +5968,33 @@ static void ResetUserWithNone(IServer defaultUserServer)
}

// Check that all commands succeed
static void AssertAllPermitted(IServer defaultUserServer, string currentUserName, IServer currentUserServer, Action<IServer>[] commands, string message)
static void AssertAllPermitted(IServer defaultUserServer, string currentUserName, IServer currentUserServer, Action<IServer>[] commands, string message, bool skipPing)
{
foreach (Action<IServer> cmd in commands)
{
Assert.True(CheckAuthFailure(() => cmd(currentUserServer)), message);
}

// Check we haven't desynced
Ping(defaultUserServer, currentUserName, currentUserServer);
if (!skipPing)
{
// Check we haven't desynced
Ping(defaultUserServer, currentUserName, currentUserServer);
}
}

// Check that all commands fail with NOAUTH
static void AssertAllDenied(IServer defaultUserServer, string currentUserName, IServer currentUserServer, Action<IServer>[] commands, string message)
static void AssertAllDenied(IServer defaultUserServer, string currentUserName, IServer currentUserServer, Action<IServer>[] commands, string message, bool skipPing)
{
foreach (Action<IServer> cmd in commands)
{
Assert.False(CheckAuthFailure(() => cmd(currentUserServer)), message);
}

// Check we haven't desynced
Ping(defaultUserServer, currentUserName, currentUserServer);
if (!skipPing)
{
// Check we haven't desynced
Ping(defaultUserServer, currentUserName, currentUserServer);
}
}

// Enable PING on user and issue PING on connection
Expand Down

0 comments on commit 93145ee

Please sign in to comment.