Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: add setting to remove default create on public schema #103061

Closed
wants to merge 1 commit into from

Conversation

e-mbrown
Copy link
Contributor

Resolves: #70266

Release note (sql change): remove_default_create_on_public controls whether users have default CREATE privileges on the public schema or not. When the setting is true users can only create tables on databases they own in the public schema. The cluster setting was added to match Postgres 15 behavior.

@e-mbrown e-mbrown requested review from rafiss and a team May 10, 2023 19:59
@e-mbrown e-mbrown requested review from a team as code owners May 10, 2023 19:59
@e-mbrown e-mbrown requested a review from a team May 10, 2023 19:59
@e-mbrown e-mbrown requested a review from a team as a code owner May 10, 2023 19:59
@e-mbrown e-mbrown requested review from lidorcarmel and michae2 and removed request for a team May 10, 2023 19:59
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@e-mbrown
Copy link
Contributor Author

Hmmm, seems like the change allowing

if sch.Name().Schema() == "public" {
			dbOwner := sch.Database().GetPrivileges().Owner()

might not be ideal because it's causing this error

pkg/sql/opt/testutils/testcat/test_catalog.go:314:9: cannot use &tc.testSchema (value of type *Schema) as type cat.Schema in return statement:
	*Schema does not implement cat.Schema (missing Database method)
pkg/sql/opt/testutils/testcat/test_catalog.go:567:20: cannot use &Schema{} (value of type *Schema) as type cat.Schema in variable declaration:
	*Schema does not implement cat.Schema (missing Database method)

Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown, @lidorcarmel, and @michae2)


pkg/sql/authorization.go line 839 at r1 (raw file):

			"cannot CREATE on schema %s", scDesc.GetName())
	case catalog.SchemaUserDefined:
		//It is possible to reach this point while reaching the public schema so

i don't follow why this change was needed


pkg/sql/vars.go line 387 at r1 (raw file):

	// Controls whether all user are granted the `CREATE` privilege on databases in
	// the public schema by default.
	`remove_default_create_on_public`: {

i think we should be using a cluster setting for this, not a session variable. since this is a security/auth thing, it's better for the behavior to be configured only in one place by someone who's allowed to set cluster settings, rather than allow each user to configure it for their own session

the cluster setting could be called sql.auth.public_schema_create_privilege.enabled


pkg/sql/vars.go line 400 at r1 (raw file):

			return formatBoolAsPostgresSetting(evalCtx.SessionData().RemoveDefaultCreateOnPublic), nil
		},
		GlobalDefault: displayPgBool(envutil.EnvOrDefaultBool("COCKROACH_NO_DEF_CREATE_ON_PUBLIC", false)),

nit: maybe name the environment variable COCKROACH_PUBLIC_SCHEMA_CREATE_PRIVILEGE_ENABLED (it's more clear if it matches the name of the cluster setting exactly)


pkg/sql/catalog/catpb/privilege.go line 192 at r1 (raw file):

// the public role, and ALL privileges for superusers. It is used for the
// public schema.
func NewPublicSchemaPrivilegeDescriptor(noCreatePrivOnDefault bool) *PrivilegeDescriptor {

nit: it's harder to read the code if a boolean variable has a "not" in the name. this could be named includeCreatePrivilege.


pkg/sql/opt/optbuilder/util.go line 510 at r1 (raw file):

	//the table is created in.
	if err := b.catalog.CheckPrivilege(b.ctx, sch, privilege.CREATE); err != nil {
		// When creating tables on databases in the public schema, check if the user

i don't follow why this is needed

@e-mbrown e-mbrown force-pushed the eb/remdef branch 2 times, most recently from 37bf487 to 5d8613e Compare May 11, 2023 17:25
Copy link
Contributor Author

@e-mbrown e-mbrown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @lidorcarmel, @michae2, and @rafiss)


pkg/sql/vars.go line 387 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i think we should be using a cluster setting for this, not a session variable. since this is a security/auth thing, it's better for the behavior to be configured only in one place by someone who's allowed to set cluster settings, rather than allow each user to configure it for their own session

the cluster setting could be called sql.auth.public_schema_create_privilege.enabled

Done.

Release note (sql change): `remove_default_create_on_public`
controls whether users have default `CREATE` privileges on
the public schema or not. When the setting is true users can
only create tables on databases they own in the public schema.
The cluster setting was added to match Postgres 15 behavior.
@rafiss
Copy link
Collaborator

rafiss commented May 18, 2023

replaced by #103598

@rafiss rafiss closed this May 18, 2023
@e-mbrown e-mbrown deleted the eb/remdef branch June 6, 2023 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql: remove default CREATE privilege on public schema [compat with PG 15]
3 participants