From e7552a76a7aa32762e0ce1f6af4f6c70b3e45d06 Mon Sep 17 00:00:00 2001 From: reesporte Date: Wed, 19 Jan 2022 12:08:05 -0600 Subject: [PATCH 1/2] fix bug where drop table wasn't being authorized also fixes bug in GetAuthorizedIndexList where perms weren't being properly compared --- authz/authorization.go | 4 ++-- server/grpc.go | 33 +++++++++++++++++++++++--- server/grpc_test.go | 53 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 85 insertions(+), 5 deletions(-) diff --git a/authz/authorization.go b/authz/authorization.go index a2127f33d..1cd0d3dc1 100644 --- a/authz/authorization.go +++ b/authz/authorization.go @@ -120,7 +120,7 @@ func (p *GroupPermissions) IsAdmin(groups []authn.Group) bool { func (p *GroupPermissions) GetAuthorizedIndexList(groups []authn.Group, desiredPermission Permission) (indexList []string) { // if user is admin, find all indexes in permissions file and return them - if admin := p.IsAdmin(groups); admin { + if p.IsAdmin(groups) { for groupId := range p.Permissions { for index := range p.Permissions[groupId] { indexList = append(indexList, index) @@ -132,7 +132,7 @@ func (p *GroupPermissions) GetAuthorizedIndexList(groups []authn.Group, desiredP for _, group := range groups { if _, ok := p.Permissions[group.GroupID]; ok { for index, permission := range p.Permissions[group.GroupID] { - if permission >= desiredPermission { + if permission.Satisfies(desiredPermission) { indexList = append(indexList, index) } } diff --git a/server/grpc.go b/server/grpc.go index f24823915..c57374020 100644 --- a/server/grpc.go +++ b/server/grpc.go @@ -30,6 +30,7 @@ import ( "google.golang.org/grpc/peer" "google.golang.org/grpc/reflection" "google.golang.org/grpc/status" + "vitess.io/vitess/go/vt/sqlparser" ) // GRPCHandler contains methods which handle the various gRPC requests. @@ -174,7 +175,13 @@ func (h *GRPCHandler) QuerySQL(req *pb.QuerySQLRequest, stream pb.Pilosa_QuerySQ return errors.Wrap(err, "parsing SQL") } - allowed := h.perms.GetAuthorizedIndexList(uinfo.(*authn.UserInfo).Groups, authz.Read) + perm := authz.Read + switch parsed.Statement.(type) { + case *sqlparser.DDL: // currently only used for DropTable + perm = authz.Admin + } + + allowed := h.perms.GetAuthorizedIndexList(uinfo.(*authn.UserInfo).Groups, perm) if !h.perms.IsAdmin(uinfo.(*authn.UserInfo).Groups) { if !isAllowed(parsed.Tables, allowed) { return status.Error(codes.PermissionDenied, "insufficient permissions to access requested tables") @@ -219,9 +226,29 @@ func (h *GRPCHandler) QuerySQL(req *pb.QuerySQLRequest, stream pb.Pilosa_QuerySQ func (h *GRPCHandler) QuerySQLUnary(ctx context.Context, req *pb.QuerySQLRequest) (*pb.TableResponse, error) { start := time.Now() uinfo := ctx.Value("userinfo") - if uinfo != nil && !h.perms.IsAdmin(uinfo.(*authn.UserInfo).Groups) { - ctx = context.WithValue(ctx, "indices", h.perms.GetAuthorizedIndexList(uinfo.(*authn.UserInfo).Groups, authz.Read)) + if uinfo != nil { + // authz + m := sql.NewMapper() + parsed, err := m.MapSQL(req.Sql) + if err != nil { + return nil, errors.Wrap(err, "parsing SQL") + } + + perm := authz.Read + switch parsed.Statement.(type) { + case *sqlparser.DDL: // currently only used for DropTable + perm = authz.Admin + } + + allowed := h.perms.GetAuthorizedIndexList(uinfo.(*authn.UserInfo).Groups, perm) + if !h.perms.IsAdmin(uinfo.(*authn.UserInfo).Groups) { + if !isAllowed(parsed.Tables, allowed) { + return nil, status.Error(codes.PermissionDenied, "insufficient permissions to access requested tables") + } + ctx = context.WithValue(ctx, "indices", allowed) + } } + results, err := h.execSQL(ctx, req.Sql) if err != nil { return nil, err diff --git a/server/grpc_test.go b/server/grpc_test.go index c2b993a96..acfa48943 100644 --- a/server/grpc_test.go +++ b/server/grpc_test.go @@ -1162,6 +1162,7 @@ admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` t.Fatal(err) } }) + t.Run("test-show-tables-unary-admin", func(t *testing.T) { response, err := gh.QuerySQLUnary(adminCtx, &pb.QuerySQLRequest{ Sql: "show tables", @@ -1190,6 +1191,55 @@ admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` t.Fatal(err) } }) + + t.Run("test-drop-table-unary-read", func(t *testing.T) { + _, err := gh.QuerySQLUnary(readCtx, &pb.QuerySQLRequest{ + Sql: "drop table deletable_index", + }) + if err == nil { + t.Fatal("expected error but got nil") + } + }) + + t.Run("test-drop-table-unary-write", func(t *testing.T) { + _, err := gh.QuerySQLUnary(writeCtx, &pb.QuerySQLRequest{ + Sql: "drop table deletable_index", + }) + if err == nil { + t.Fatal("expected error but got nil") + } + }) + + t.Run("test-drop-table-unary-admin", func(t *testing.T) { + _, err := gh.QuerySQLUnary(adminCtx, &pb.QuerySQLRequest{ + Sql: "drop table deletable_index", + }) + if err != nil { + t.Fatalf("expected nil error but got %v", err) + } + }) + + t.Run("test-drop-table-stream-read", func(t *testing.T) { + mock := &mockPilosa_QuerySQLServer{ctx: readCtx} + err := gh.QuerySQL(&pb.QuerySQLRequest{Sql: "drop table another_one"}, mock) + if err == nil { + t.Fatal("expected error but got nil") + } + }) + t.Run("test-drop-table-stream-write", func(t *testing.T) { + mock := &mockPilosa_QuerySQLServer{ctx: writeCtx} + err := gh.QuerySQL(&pb.QuerySQLRequest{Sql: "drop table another_one"}, mock) + if err == nil { + t.Fatal("expected error but got nil") + } + }) + t.Run("test-drop-table-stream-admin", func(t *testing.T) { + mock := &mockPilosa_QuerySQLServer{ctx: adminCtx} + err := gh.QuerySQL(&pb.QuerySQLRequest{Sql: "drop table another_one"}, mock) + if err != nil { + t.Fatalf("expected nil error but got %v", err) + } + }) } func TestCRUDIndexes(t *testing.T) { @@ -1506,6 +1556,9 @@ func setUpTestQuerySQLUnary(ctx context.Context, t *testing.T) (gh *server.GRPCH // delete_me m.MustCreateIndex(t, "delete_me", pilosa.IndexOptions{TrackExistence: true}) + m.MustCreateIndex(t, "another_one", pilosa.IndexOptions{TrackExistence: true}) + m.MustCreateIndex(t, "deletable_index", pilosa.IndexOptions{TrackExistence: true}) + return gh, func() { if err := m.API.DeleteIndex(ctx, joiner.Name()); err != nil { panic(err) From 61ef1aee4e694316810238236c84d01e714ede20 Mon Sep 17 00:00:00 2001 From: reesporte Date: Wed, 19 Jan 2022 12:55:07 -0600 Subject: [PATCH 2/2] fix older tests --- server/grpc_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/server/grpc_test.go b/server/grpc_test.go index acfa48943..3508d03ab 100644 --- a/server/grpc_test.go +++ b/server/grpc_test.go @@ -843,6 +843,8 @@ func TestQuerySQL(t *testing.T) { {"Table", "string"}, }, rows: []row{ + {[]columnResponse{"another_one"}}, + {[]columnResponse{"deletable_index"}}, {[]columnResponse{"delete_me"}}, {[]columnResponse{"grouper"}}, {[]columnResponse{"joiner"}}, @@ -881,6 +883,9 @@ func TestQuerySQL(t *testing.T) { {"Table", "string"}, }, rows: []row{ + {[]columnResponse{"another_one"}}, + {[]columnResponse{"deletable_index"}}, + {[]columnResponse{"grouper"}}, {[]columnResponse{"joiner"}}, },