Merge pull request #1975 from cockroachdb/marc/table_privileges

Add table privileges: #1830

sql/create.go

@@ -70,6 +70,9 @@ func (p *planner) CreateTable(n *parser.CreateTable) (planNode, error) {
 	if err != nil {
 		return nil, err
 	}
+	// Inherit permissions from the database descriptor.
+	desc.PrivilegeDescriptor = dbDesc.PrivilegeDescriptor
+
 	if err := desc.AllocateIDs(); err != nil {
 		return nil, err
 	}

sql/database.go

@@ -26,9 +26,11 @@ import (
 
 func makeDatabaseDesc(p *parser.CreateDatabase) structured.DatabaseDescriptor {
 	return structured.DatabaseDescriptor{
-		Name:  p.Name.String(),
-		Read:  []string{security.RootUser},
-		Write: []string{security.RootUser},
+		Name: p.Name.String(),
+		PrivilegeDescriptor: structured.PrivilegeDescriptor{
+			Read:  []string{security.RootUser},
+			Write: []string{security.RootUser},
+		},
 	}
 }
 

sql/descriptor.go

@@ -23,14 +23,24 @@ import (
 	"github.com/cockroachdb/cockroach/client"
 	"github.com/cockroachdb/cockroach/keys"
 	"github.com/cockroachdb/cockroach/proto"
+	"github.com/cockroachdb/cockroach/sql/parser"
+	"github.com/cockroachdb/cockroach/structured"
+	"github.com/cockroachdb/cockroach/util"
 	gogoproto "github.com/gogo/protobuf/proto"
 )
 
-// descriptorProto is the interface needed by writeDescriptor and getDescriptor.
+// descriptorProto is the interface implemented by both DatabaseDescriptor
+// and TableDescriptor.
+// TODO(marc): this is getting rather large.
 type descriptorProto interface {
 	gogoproto.Message
+	Grant(*parser.Grant) error
+	Revoke(*parser.Revoke) error
+	Show() (structured.UserPrivilegeList, error)
+	HasPrivilege(string, parser.PrivilegeType) bool
 	GetID() uint32
 	SetID(uint32)
+	GetName() string
 	Validate() error
 }
 
@@ -91,3 +101,36 @@ func (p *planner) getDescriptor(key proto.Key, descriptor descriptorProto) error
 
 	return descriptor.Validate()
 }
+
+// getDescriptorFromTargetList examines a TargetList and fetches the
+// appropriate descriptor.
+// TODO(marc): support multiple targets.
+func (p *planner) getDescriptorFromTargetList(targets parser.TargetList) (descriptorProto, error) {
+	if targets.Databases != nil {
+		if len(targets.Databases) == 0 {
+			return nil, errNoDatabase
+		} else if len(targets.Databases) != 1 {
+			return nil, util.Errorf("TODO(marc): multiple targets not implemented")
+		}
+		descriptor, err := p.getDatabaseDesc(targets.Databases[0])
+		if err != nil {
+			return nil, err
+		}
+		return descriptor, nil
+	}
+
+	if targets.Tables == nil {
+		return nil, util.Errorf("no targets speciied")
+	}
+
+	if len(targets.Tables) == 0 {
+		return nil, errNoTable
+	} else if len(targets.Tables) != 1 {
+		return nil, util.Errorf("TODO(marc): multiple targets not implemented")
+	}
+	descriptor, err := p.getTableDesc(targets.Tables[0])
+	if err != nil {
+		return nil, err
+	}
+	return descriptor, nil
+}

sql/grant.go

@@ -22,7 +22,6 @@ import (
 
 	"github.com/cockroachdb/cockroach/keys"
 	"github.com/cockroachdb/cockroach/sql/parser"
-	"github.com/cockroachdb/cockroach/util"
 )
 
 // Grant adds privileges to users.
@@ -36,34 +35,25 @@ import (
 //   Notes: postgres requires the object owner.
 //          mysql requires the "grant option" and the same privileges, and sometimes superuser.
 func (p *planner) Grant(n *parser.Grant) (planNode, error) {
-	if len(n.Targets.Targets) == 0 {
-		return nil, errEmptyDatabaseName
-	}
-	if len(n.Targets.Targets) != 1 {
-		return nil, util.Errorf("TODO(marc): multiple targets not implemented")
-	}
-
-	// Lookup the database descriptor.
-	// TODO(marc): iterate over n.Targets.Targets once the grammar supports more than one.
-	dbDesc, err := p.getDatabaseDesc(n.Targets.Targets[0])
+	descriptor, err := p.getDescriptorFromTargetList(n.Targets)
 	if err != nil {
 		return nil, err
 	}
 
-	if !dbDesc.HasPrivilege(p.user, parser.PrivilegeWrite) {
+	if !descriptor.HasPrivilege(p.user, parser.PrivilegeWrite) {
 		return nil, fmt.Errorf("user %s does not have %s privilege on database %s",
-			p.user, parser.PrivilegeWrite, dbDesc.Name)
+			p.user, parser.PrivilegeWrite, descriptor.GetName())
 	}
 
-	if err := dbDesc.Grant(n); err != nil {
+	if err := descriptor.Grant(n); err != nil {
 		return nil, err
 	}
 
 	// Now update the descriptor.
 	// TODO(marc): do this inside a transaction. This will be needed
 	// when modifying multiple descriptors in the same op.
-	descKey := keys.MakeDescMetadataKey(dbDesc.ID)
-	if err := p.db.Put(descKey, dbDesc); err != nil {
+	descKey := keys.MakeDescMetadataKey(descriptor.GetID())
+	if err := p.db.Put(descKey, descriptor); err != nil {
 		return nil, err
 	}
 

sql/grant_test.go

@@ -26,7 +26,7 @@ import (
 	"github.com/cockroachdb/cockroach/util/leaktest"
 )
 
-func TestGrant(t *testing.T) {
+func TestGrantDatabase(t *testing.T) {
 	defer leaktest.AfterTest(t)
 	s, sqlDB, kvDB := setup(t)
 	defer cleanup(s, sqlDB)

sql/parser/grant.go

@@ -91,14 +91,18 @@ var (
 	}
 )
 
-// TargetList represents a list of targets of a given type.
+// TargetList represents a list of targets.
+// Only one field may be non-nil.
 type TargetList struct {
-	Type    TargetType
-	Targets NameList
+	Databases NameList
+	Tables    QualifiedNames
 }
 
 func (tl TargetList) String() string {
-	return fmt.Sprintf("%s %s", tl.Type, tl.Targets)
+	if tl.Databases != nil {
+		return fmt.Sprintf("DATABASE %s", tl.Databases)
+	}
+	return fmt.Sprintf("%s", tl.Tables)
 }
 
 func (node *Grant) String() string {

sql/parser/parse_test.go

@@ -74,17 +74,30 @@ func TestParse(t *testing.T) {
 		{`SHOW INDEX FROM a`},
 		{`SHOW INDEX FROM a.b.c`},
 		{`SHOW TABLES FROM a; SHOW COLUMNS FROM b`},
+
+		// Tables are the default, but can also be specified with
+		// GRANT x ON TABLE y. However, the stringer does not output TABLE.
 		{`SHOW GRANTS`},
+		{`SHOW GRANTS ON foo`},
+		{`SHOW GRANTS ON foo, db.foo`},
 		{`SHOW GRANTS ON DATABASE foo, bar`},
 		{`SHOW GRANTS ON DATABASE foo FOR bar`},
 		{`SHOW GRANTS FOR bar, baz`},
 
+		// Tables are the default, but can also be specified with
+		// GRANT x ON TABLE y. However, the stringer does not output TABLE.
+		{`GRANT READ ON foo TO root`},
+		{`GRANT WRITE, READ ON foo, db.foo TO root, bar`},
 		{`GRANT READ ON DATABASE foo TO root`},
 		{`GRANT ALL ON DATABASE foo TO root, test`},
 		{`GRANT READ, WRITE ON DATABASE bar TO foo, bar, baz`},
 		{`GRANT READ, WRITE ON DATABASE db1, db2 TO foo, bar, baz`},
 		{`GRANT READ, WRITE ON DATABASE db1, db2 TO "test-user"`},
 
+		// Tables are the default, but can also be specified with
+		// REVOKE x ON TABLE y. However, the stringer does not output TABLE.
+		{`REVOKE READ ON foo FROM root`},
+		{`REVOKE WRITE, READ ON foo, db.foo FROM root, bar`},
 		{`REVOKE READ ON DATABASE foo FROM root`},
 		{`REVOKE ALL ON DATABASE foo FROM root, test`},
 		{`REVOKE READ, WRITE ON DATABASE bar FROM foo, bar, baz`},