Merge pull request #8805 from planetscale/improve-acl-errors

Slight improvement of the ACL error message

go/test/endtoend/encryption/encryptedtransport/encrypted_transport_test.go

@@ -189,8 +189,8 @@ func TestSecureTransport(t *testing.T) {
 	require.NoError(t, err)
 	err = vterrors.FromVTRPC(qr.Error)
 	require.Error(t, err)
-	assert.Contains(t, err.Error(), "table acl error")
-	assert.Contains(t, err.Error(), "cannot run Select on table")
+	assert.Contains(t, err.Error(), "Select command denied to user")
+	assert.Contains(t, err.Error(), "for table 'vt_insert_test' (ACL check error)")
 
 	// now restart vtgate in the mode where we don't use SSL
 	// for client connections, but we copy effective caller id
@@ -214,8 +214,8 @@ func TestSecureTransport(t *testing.T) {
 	require.NoError(t, err)
 	err = vterrors.FromVTRPC(qr.Error)
 	require.Error(t, err)
-	assert.Contains(t, err.Error(), "table acl error")
-	assert.Contains(t, err.Error(), "cannot run Select on table")
+	assert.Contains(t, err.Error(), "Select command denied to user")
+	assert.Contains(t, err.Error(), "for table 'vt_insert_test' (ACL check error)")
 
 	// 'vtgate client 1' is authorized to access vt_insert_test
 	callerID := &vtrpc.CallerID{
@@ -236,8 +236,8 @@ func TestSecureTransport(t *testing.T) {
 	require.NoError(t, err)
 	err = vterrors.FromVTRPC(qr.Error)
 	require.Error(t, err)
-	assert.Contains(t, err.Error(), "table acl error")
-	assert.Contains(t, err.Error(), "cannot run Select on table")
+	assert.Contains(t, err.Error(), "Select command denied to user")
+	assert.Contains(t, err.Error(), "for table 'vt_insert_test' (ACL check error)")
 
 	clusterInstance.Teardown()
 }

go/test/endtoend/mysqlserver/mysql_server_test.go

@@ -193,8 +193,8 @@ func TestSelectWithUnauthorizedUser(t *testing.T) {
 
 	_, err = conn.ExecuteFetch("SELECT * from vt_insert_test limit 1", 1, false)
 	require.NotNilf(t, err, "error expected, got nil")
-	assert.Contains(t, err.Error(), "table acl error")
-	assert.Contains(t, err.Error(), "cannot run Select on table")
+	assert.Contains(t, err.Error(), "Select command denied to user")
+	assert.Contains(t, err.Error(), "for table 'vt_insert_test' (ACL check error)")
 }
 
 func connectDB(t *testing.T, vtParams mysql.ConnParams, params ...string) *sql.DB {

go/vt/vttablet/endtoend/acl_test.go

@@ -19,9 +19,10 @@ package endtoend
 import (
 	"bytes"
 	"encoding/json"
-	"strings"
 	"testing"
 
+	"gotest.tools/assert"
+
 	"vitess.io/vitess/go/sqltypes"
 	"vitess.io/vitess/go/vt/vttablet/endtoend/framework"
 	"vitess.io/vitess/go/vt/vttablet/tabletserver/rules"
@@ -32,7 +33,7 @@ import (
 func TestTableACL(t *testing.T) {
 	client := framework.NewClient()
 
-	aclErr := "table acl error"
+	aclErr := "command denied to user 'dev' for table"
 	execCases := []struct {
 		query string
 		err   string
@@ -101,9 +102,7 @@ func TestTableACL(t *testing.T) {
 			}
 			continue
 		}
-		if err == nil || !strings.HasPrefix(err.Error(), tcase.err) {
-			t.Errorf("Execute(%s): Error: %v, must start with %s", tcase.query, err, tcase.err)
-		}
+		assert.ErrorContains(t, err, tcase.err)
 	}
 
 	streamCases := []struct {
@@ -131,9 +130,7 @@ func TestTableACL(t *testing.T) {
 			}
 			continue
 		}
-		if err == nil || !strings.HasPrefix(err.Error(), tcase.err) {
-			t.Errorf("Error: %v, must start with %s", err, tcase.err)
-		}
+		assert.ErrorContains(t, err, tcase.err)
 	}
 }
 

go/vt/vttablet/tabletserver/query_executor.go

@@ -444,7 +444,11 @@ func (qre *QueryExecutor) checkAccess(authorized *tableacl.ACLResult, tableName
 		}
 
 		if qre.tsv.qe.strictTableACL {
-			errStr := fmt.Sprintf("table acl error: %q %v cannot run %v on table %q", callerID.Username, callerID.Groups, qre.plan.PlanID, tableName)
+			groupStr := ""
+			if len(callerID.Groups) > 0 {
+				groupStr = fmt.Sprintf(", in groups [%s],", strings.Join(callerID.Groups, ", "))
+			}
+			errStr := fmt.Sprintf("%s command denied to user '%s'%s for table '%s' (ACL check error)", qre.plan.PlanID.String(), callerID.Username, groupStr, tableName)
 			qre.tsv.Stats().TableaclDenied.Add(statsKey, 1)
 			qre.tsv.qe.accessCheckerLogger.Infof("%s", errStr)
 			return vterrors.Errorf(vtrpcpb.Code_PERMISSION_DENIED, "%s", errStr)

go/vt/vttablet/tabletserver/query_executor_test.go

@@ -822,17 +822,15 @@ func TestQueryExecutorMessageStreamACL(t *testing.T) {
 
 	callerID = &querypb.VTGateCallerID{
 		Username: "u2",
+		Groups:   []string{"non-admin"},
 	}
 	qre.ctx = callerid.NewContext(context.Background(), nil, callerID)
 	// Should fail because u2 does not have permission.
 	err = qre.MessageStream(func(qr *sqltypes.Result) error {
 		return io.EOF
 	})
 
-	want := `table acl error: "u2" [] cannot run MessageStream on table "msg"`
-	if err == nil || err.Error() != want {
-		t.Errorf("qre.MessageStream(msg) error: %v, want %s", err, want)
-	}
+	assert.EqualError(t, err, `MessageStream command denied to user 'u2', in groups [non-admin], for table 'msg' (ACL check error)`)
 	if code := vterrors.Code(err); code != vtrpcpb.Code_PERMISSION_DENIED {
 		t.Fatalf("qre.Execute: %v, want %v", code, vtrpcpb.Code_PERMISSION_DENIED)
 	}
@@ -972,10 +970,8 @@ func TestQueryExecutorTableAclDualTableExempt(t *testing.T) {
 	if code := vterrors.Code(err); code != vtrpcpb.Code_PERMISSION_DENIED {
 		t.Fatalf("qre.Execute: %v, want %v", code, vtrpcpb.Code_PERMISSION_DENIED)
 	}
-	wanterr := "table acl error"
-	if !strings.Contains(err.Error(), wanterr) {
-		t.Fatalf("qre.Execute: %v, want %s", err, wanterr)
-	}
+
+	assert.EqualError(t, err, `SelectImpossible command denied to user 'basic_username' for table 'test_table' (ACL check error)`)
 
 	// table acl should be ignored when querying against dual table
 	query = "select @@version_comment from dual limit 1"
@@ -1014,6 +1010,7 @@ func TestQueryExecutorTableAclExemptACL(t *testing.T) {
 	username := "u2"
 	callerID := &querypb.VTGateCallerID{
 		Username: username,
+		Groups:   []string{"eng", "beta"},
 	}
 	ctx := callerid.NewContext(context.Background(), nil, callerID)
 
@@ -1039,10 +1036,7 @@ func TestQueryExecutorTableAclExemptACL(t *testing.T) {
 	if code := vterrors.Code(err); code != vtrpcpb.Code_PERMISSION_DENIED {
 		t.Fatalf("qre.Execute: %v, want %v", code, vtrpcpb.Code_PERMISSION_DENIED)
 	}
-	wanterr := "table acl error"
-	if !strings.Contains(err.Error(), wanterr) {
-		t.Fatalf("qre.Execute: %v, want %s", err, wanterr)
-	}
+	assert.EqualError(t, err, `Select command denied to user 'u2', in groups [eng, beta], for table 'test_table' (ACL check error)`)
 
 	// table acl should be ignored since this is an exempt user.
 	username = "exempt-acl"