[sql] Grant DB privileges in a non-deprecated way

[smklein]

Fixes #1304

For reference, I'm following the advice of https://www.cockroachlabs.com/docs/v21.2/grant.html#grant-privileges-on-all-tables-in-a-database-or-schema

I noticed that using the wildcard * only works after the tables have been created, so the statement is moved to the end of dbinit.sql.

[davepacheco]

For reference, I'm following the advice of https://www.cockroachlabs.com/docs/v21.2/grant.html#grant-privileges-on-all-tables-in-a-database-or-schema

I noticed that using the wildcard * only works after the tables have been created, so the statement is moved to the end of dbinit.sql.

I think this is different behavior from what's there before and not quite right. Previously, we granted privileges on the database. The docs say:

When a role or user is granted privileges for a database, new tables created in the database will inherit the privileges, but the privileges can then be changed.
The user does not get privileges to existing tables in the database.

So I think we want to keep it at the database level and do it first. With the change here, we're granting privileges on the specific things in the omicron.public schema that already exist. If an upgrade delivers a new table, the user won't have permissions on that table.

---

I also agree with your comment in #1304: it's fine to update the syntax, but it seems basically orthogonal to that issue in that it shouldn't cause a problem with the current version. (I'm also not clear on how this change updates the syntax but I'll dig into that some more.)

[smklein]

I intentionally did not apply the privileges to the database, reading https://www.cockroachlabs.com/docs/v21.2/grant.html#supported-privileges , it seems the set of privileges we want to apply are only applicable at a table level.

[davepacheco]

The message from #1304 was:

Stderr: NOTICE: GRANT INSERT, SELECT, UPDATE, DELETE ON DATABASE is deprecated.

It sounds like we interpreted this to mean "you aren't supposed to grant those specific privileges on a database". I don't think that's what it's saying because the message goes on to say:

This statement was automatically converted to USE omicron; ALTER DEFAULT PRIVILEGES GRANT INSERT, SELECT, UPDATE, DELETE ON TABLES TO omicron;

I think that's what we want to change this to. I believe that has the same effect as what we were doing before: it's a database-level grant that causes these permissions to be granted on all newly-created tables.

[smklein]

The message from #1304 was:

Stderr: NOTICE: GRANT INSERT, SELECT, UPDATE, DELETE ON DATABASE is deprecated.

It sounds like we interpreted this to mean "you aren't supposed to grant those specific privileges on a database". I don't think that's what it's saying because the message goes on to say:

This statement was automatically converted to USE omicron; ALTER DEFAULT PRIVILEGES GRANT INSERT, SELECT, UPDATE, DELETE ON TABLES TO omicron;

I think that's what we want to change this to. I believe that has the same effect as what we were doing before: it's a database-level grant that causes these permissions to be granted on all newly-created tables.

Got it, will give this a shot

[smklein]

Updated, ALTER DEFAULT PRIVILEGES GRANT INSERT, SELECT, UPDATE, DELETE ON TABLES to omicron; works

[davepacheco]

Based on the message, I'm afraid you might need a use omicron before this. I had previously avoided that in this file (that's why all the create table statements say omicron.public.rack) but I don't remember a particularly good reason...I may not have been super sure that it would behave correctly, especially if it failed for some reason. Or maybe would leave the connection in a state associated with omicron in a way that would break the next transaction? I'm not sure.

[davepacheco]

Saw your comment -- if it worked, great (though I'm not sure how)

[davepacheco]

Ah, I see now: since we're not using "FOR":

If you do not specify a FOR ... clause, CockroachDB alters the default privileges on objects created by the current user.

So this affects everything created by the user that's established this connection, not just the "omicron" database. I think that's fine for now.