diff --git a/pkg/ccl/cloudccl/externalconn/testdata/create_drop_external_connection b/pkg/ccl/cloudccl/externalconn/testdata/create_drop_external_connection index c03464ae42f3..14a55065eac3 100644 --- a/pkg/ccl/cloudccl/externalconn/testdata/create_drop_external_connection +++ b/pkg/ccl/cloudccl/externalconn/testdata/create_drop_external_connection @@ -95,6 +95,51 @@ pq: only users with the EXTERNALCONNECTION system privilege are allowed to CREAT subtest end +subtest drop-external-storage-privilege + +exec-sql +CREATE EXTERNAL CONNECTION privileged AS 'nodelocal://1/foo' +---- + +# Create another External Connection. +exec-sql +CREATE EXTERNAL CONNECTION 'privileged-dup' AS 'nodelocal://1/foo' +---- + +exec-sql user=testuser +DROP EXTERNAL CONNECTION privileged +---- +pq: user testuser does not have DROP privilege on external_connection privileged + +inspect-system-table +---- +privileged STORAGE {"provider": "nodelocal", "simpleUri": {"uri": "nodelocal://1/foo"}} +privileged-dup STORAGE {"provider": "nodelocal", "simpleUri": {"uri": "nodelocal://1/foo"}} + +exec-sql +GRANT DROP ON EXTERNAL CONNECTION privileged TO testuser; +---- + +exec-sql user=testuser +DROP EXTERNAL CONNECTION privileged +---- + +# Try to drop the second external connection, testuser should be disallowed. +exec-sql user=testuser +DROP EXTERNAL CONNECTION 'privileged-dup' +---- +pq: user testuser does not have DROP privilege on external_connection privileged-dup + +inspect-system-table +---- +privileged-dup STORAGE {"provider": "nodelocal", "simpleUri": {"uri": "nodelocal://1/foo"}} + +exec-sql +DROP EXTERNAL CONNECTION 'privileged-dup' +---- + +subtest end + subtest basic-gs-kms exec-sql diff --git a/pkg/ccl/cloudccl/externalconn/testdata/multi-tenant/create_drop_external_connection b/pkg/ccl/cloudccl/externalconn/testdata/multi-tenant/create_drop_external_connection index a5f0eefdab77..4139bd1f5eb5 100644 --- a/pkg/ccl/cloudccl/externalconn/testdata/multi-tenant/create_drop_external_connection +++ b/pkg/ccl/cloudccl/externalconn/testdata/multi-tenant/create_drop_external_connection @@ -98,6 +98,51 @@ pq: only users with the EXTERNALCONNECTION system privilege are allowed to CREAT subtest end +subtest drop-external-storage-privilege + +exec-sql +CREATE EXTERNAL CONNECTION privileged AS 'nodelocal://1/foo' +---- + +# Create another External Connection. +exec-sql +CREATE EXTERNAL CONNECTION 'privileged-dup' AS 'nodelocal://1/foo' +---- + +exec-sql user=testuser +DROP EXTERNAL CONNECTION privileged +---- +pq: user testuser does not have DROP privilege on external_connection privileged + +inspect-system-table +---- +privileged STORAGE {"provider": "nodelocal", "simpleUri": {"uri": "nodelocal://1/foo"}} +privileged-dup STORAGE {"provider": "nodelocal", "simpleUri": {"uri": "nodelocal://1/foo"}} + +exec-sql +GRANT DROP ON EXTERNAL CONNECTION privileged TO testuser; +---- + +exec-sql user=testuser +DROP EXTERNAL CONNECTION privileged +---- + +# Try to drop the second external connection, testuser should be disallowed. +exec-sql user=testuser +DROP EXTERNAL CONNECTION 'privileged-dup' +---- +pq: user testuser does not have DROP privilege on external_connection privileged-dup + +inspect-system-table +---- +privileged-dup STORAGE {"provider": "nodelocal", "simpleUri": {"uri": "nodelocal://1/foo"}} + +exec-sql +DROP EXTERNAL CONNECTION 'privileged-dup' +---- + +subtest end + subtest basic-gs-kms exec-sql diff --git a/pkg/sql/drop_external_connection.go b/pkg/sql/drop_external_connection.go index 67a246ad6c27..e72dc911f4f9 100644 --- a/pkg/sql/drop_external_connection.go +++ b/pkg/sql/drop_external_connection.go @@ -13,10 +13,14 @@ package sql import ( "context" + "github.com/cockroachdb/cockroach/pkg/clusterversion" + "github.com/cockroachdb/cockroach/pkg/security/username" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" + "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" + "github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege" "github.com/cockroachdb/errors" ) @@ -54,16 +58,10 @@ func (p *planner) makeDropExternalConnectionEval( } func (p *planner) dropExternalConnection(params runParams, n *tree.DropExternalConnection) error { - // TODO(adityamaru): Check that the user has `DROP` privileges on the External - // Connection once we add support for it. Remove admin only check. - hasAdmin, err := params.p.HasAdminRole(params.ctx) - if err != nil { - return err - } - if !hasAdmin { - return pgerror.New( - pgcode.InsufficientPrivilege, - "only users with the admin role are allowed to DROP EXTERNAL CONNECTION") + if !p.ExecCfg().Settings.Version.IsActive(params.ctx, clusterversion.SystemExternalConnectionsTable) { + return pgerror.Newf(pgcode.FeatureNotSupported, + "External Connections are not supported until upgrade to version %v is finalized", + clusterversion.ByKey(clusterversion.SystemExternalConnectionsTable)) } // TODO(adityamaru): Add some metrics to track DROP EXTERNAL CONNECTION @@ -79,11 +77,22 @@ func (p *planner) dropExternalConnection(params runParams, n *tree.DropExternalC return errors.Wrap(err, "failed to resolve External Connection name") } + // Check that the user has DROP privileges on the External Connection object. + ecPrivilege := &syntheticprivilege.ExternalConnectionPrivilege{ + ConnectionName: name, + } + if err := p.CheckPrivilege(params.ctx, ecPrivilege, privilege.DROP); err != nil { + return err + } + + // DROP EXTERNAL CONNECTION is only allowed for users with the `DROP` + // privilege on this object. We run the query as `node` since the user might + // not have `SELECT` on the system table. if _ /* rows */, err = params.extendedEvalCtx.ExecCfg.InternalExecutor.ExecEx( params.ctx, dropExternalConnectionOp, params.p.Txn(), - sessiondata.InternalExecutorOverride{User: params.p.User()}, + sessiondata.InternalExecutorOverride{User: username.NodeUserName()}, `DELETE FROM system.external_connections WHERE connection_name = $1`, name, ); err != nil { return errors.Wrapf(err, "failed to delete external connection") diff --git a/pkg/sql/grant_revoke_system.go b/pkg/sql/grant_revoke_system.go index a793f51ee7c5..7ab47b32af6a 100644 --- a/pkg/sql/grant_revoke_system.go +++ b/pkg/sql/grant_revoke_system.go @@ -216,6 +216,11 @@ func (n *changeNonDescriptorBackedPrivilegesNode) makeSystemPrivilegeObject( } return ret, nil case privilege.ExternalConnection: + if !p.ExecCfg().Settings.Version.IsActive(ctx, clusterversion.SystemExternalConnectionsTable) { + return nil, errors.Newf("External Connections are not supported until upgrade to version %s is finalized", + clusterversion.SystemExternalConnectionsTable.String()) + } + var ret []syntheticprivilege.Object for _, externalConnectionName := range n.targets.ExternalConnections { // Ensure that an External Connection of this name actually exists. diff --git a/pkg/sql/logictest/testdata/logic_test/external_connection_privileges b/pkg/sql/logictest/testdata/logic_test/external_connection_privileges index 948e1f5f1736..6bf58aeb7d9b 100644 --- a/pkg/sql/logictest/testdata/logic_test/external_connection_privileges +++ b/pkg/sql/logictest/testdata/logic_test/external_connection_privileges @@ -12,26 +12,29 @@ SELECT * FROM system.privileges statement error pq: failed to resolve External Connection: external connection with name foo does not exist GRANT USAGE ON EXTERNAL CONNECTION foo TO testuser +statement error pq: failed to resolve External Connection: external connection with name foo does not exist +GRANT DROP ON EXTERNAL CONNECTION foo TO testuser + statement ok CREATE EXTERNAL CONNECTION foo AS 'nodelocal://1/foo' statement ok -GRANT USAGE ON EXTERNAL CONNECTION foo TO testuser +GRANT USAGE,DROP ON EXTERNAL CONNECTION foo TO testuser query TTTT SELECT * FROM system.privileges ---- -testuser /externalconn/foo {USAGE} {} +testuser /externalconn/foo {DROP,USAGE} {} statement ok -REVOKE USAGE ON EXTERNAL CONNECTION foo FROM testuser +REVOKE USAGE,DROP ON EXTERNAL CONNECTION foo FROM testuser query TTTT SELECT * FROM system.privileges ---- statement ok -GRANT USAGE ON EXTERNAL CONNECTION foo TO testuser +GRANT USAGE,DROP ON EXTERNAL CONNECTION foo TO testuser statement ok CREATE USER bar @@ -39,27 +42,27 @@ CREATE USER bar # Attempt to grant usage as testuser, this should fail since we did not specify WITH GRANT OPTION user testuser -statement error pq: user testuser missing WITH GRANT OPTION privilege on USAGE -GRANT USAGE ON EXTERNAL CONNECTION foo TO bar +statement error pq: user testuser missing WITH GRANT OPTION privilege on one or more of USAGE, DROP +GRANT USAGE,DROP ON EXTERNAL CONNECTION foo TO bar user root statement ok -GRANT USAGE ON EXTERNAL CONNECTION foo TO testuser WITH GRANT OPTION +GRANT USAGE,DROP ON EXTERNAL CONNECTION foo TO testuser WITH GRANT OPTION # Attempt to grant usage as testuser, this should succeed since we did specify WITH GRANT OPTION user testuser statement ok -GRANT USAGE ON EXTERNAL CONNECTION foo TO bar +GRANT USAGE,DROP ON EXTERNAL CONNECTION foo TO bar user root query TTTT SELECT * FROM system.privileges ORDER BY username ---- -bar /externalconn/foo {USAGE} {} -testuser /externalconn/foo {USAGE} {USAGE} +bar /externalconn/foo {DROP,USAGE} {} +testuser /externalconn/foo {DROP,USAGE} {DROP,USAGE} # Invalid grants on external connections. diff --git a/pkg/sql/privilege/privilege.go b/pkg/sql/privilege/privilege.go index ad8ab256fce2..f47804dd5094 100644 --- a/pkg/sql/privilege/privilege.go +++ b/pkg/sql/privilege/privilege.go @@ -122,7 +122,7 @@ var ( SequencePrivileges = List{ALL, USAGE, SELECT, UPDATE, CREATE, DROP, INSERT, DELETE, ZONECONFIG} SystemPrivileges = List{ALL, MODIFYCLUSTERSETTING, EXTERNALCONNECTION, VIEWACTIVITY, VIEWACTIVITYREDACTED, VIEWCLUSTERSETTING, CANCELQUERY, NOSQLLOGIN} VirtualTablePrivileges = List{ALL, SELECT} - ExternalConnectionPrivileges = List{ALL, USAGE} + ExternalConnectionPrivileges = List{ALL, USAGE, DROP} ) // Mask returns the bitmask for a given privilege.