Updated SQL statements for new role options #8629
Conversation
|
This change is |
There was a problem hiding this comment.
I know this is still a WIP but left some thoughts.
Reviewed 1 of 19 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @Amruta-Ranade and @solongordon)
v20.2/alter-role.md, line 46 at r3 (raw file):
`VIEWACTIVITY`/`NOVIEWACTIVITY` | Allow or disallow a role to see other roles' queries and sessions using `SHOW QUERIES`, `SHOW SESSIONS`, and the Statements and Transactions pages in the Admin UI. Without this privilege, the `SHOW` commands only show the role's own data and the Admin UI pages are unavailable. <br><br>By default, the parameter is set to `NOVIEWACTIVITY` for all non-admin roles. `CONTROLCHANGEFEED`/`NOCONTROLCHANGEFEED` | Allow or disallow the role to run `CREATE CHANGEFEED` on tables they have `SELECT` privileges on. <br><br>By default, the parameter is set to `NOCONTROLCHANGEFEED` for all non-admin roles. `MODIFYCLUSTERSETTING`/`NOMODIFYCLUSTERSETTING` | Allow or disallow the role to to modify the cluster settings with the `sql.defaults` prefix. <br><br>By default, the parameter is set to `NOMODIFYCLUSTERSETTING` for all non-admin roles.
Would be nice not to duplicate all of these options with the CREATE ROLE page.
v20.2/alter-role.md, line 54 at r3 (raw file):
{{site.data.alerts.end}} ### Allow a role to log in to the database password
Incomplete comment?
v20.2/alter-role.md, line 69 at r3 (raw file):
~~~ ### Allow a role to create other roles and manage authentication methods for the new roles
Only setting CREATEROLE is not enough to manage authentication methods. This would require CREATELOGIN as well.
v20.2/alter-user.md, line 2 at r3 (raw file):
--- title: ALTER USER
I would recommend we just say that ALTER USER is a synonym for ALTER ROLE and point to that page. That's what Postgres does: https://www.postgresql.org/docs/current/sql-alteruser.html
v20.2/cancel-job.md, line 23 at r3 (raw file):
## Required privileges To cancel a job, the user must be a member of the `admin` role or must have the [`CONTROLJOB`](create-user.html#create-a-user-that-can-pause-resume-and-cancel-non-admin-jobs) parameter set.
Maybe note that non-admins cannot cancel admin jobs. (Same goes for canceling queries, sessions, etc. below)
v20.2/create-user.md, line 26 at r3 (raw file):
## Required privileges To create other users, the user must be a member of the `admin` role or have the [`CREATEROLE`](#create-a-user-that-can-create-other-users-and-manage-authentication-methods-for-the-new-users) parameter set.
As I mentioned on ALTER USER, I think it would be preferable to just specify that CREATE USER is an alias for CREATE ROLE WITH LOGIN and point to the CREATE ROLE page.
Amruta-Ranade
changed the title
There was a problem hiding this comment.
Oh, I forgot to take off the WIP tag before requesting a review. Sorry about that!
Reviewable status:
v20.2/alter-role.md, line 46 at r3 (raw file):
Previously, solongordon (Solon) wrote…
Would be nice not to duplicate all of these options with the CREATE ROLE page.
Yeah, I have a docs project issue to merge the ROLE/USER pages, but I won't get to it before the release. Hence the duplicate content across pages :)
v20.2/alter-role.md, line 54 at r3 (raw file):
Previously, solongordon (Solon) wrote…
Incomplete comment?
Good catch. Fixed.
v20.2/alter-role.md, line 69 at r3 (raw file):
Previously, solongordon (Solon) wrote…
Only setting CREATEROLE is not enough to manage authentication methods. This would require CREATELOGIN as well.
I thought a user with CREATEROLE has CREATELOGIN privileges by default? 🤔
v20.2/alter-user.md, line 2 at r3 (raw file):
Previously, solongordon (Solon) wrote…
I would recommend we just say that ALTER USER is a synonym for ALTER ROLE and point to that page. That's what Postgres does: https://www.postgresql.org/docs/current/sql-alteruser.html
Yep..follow-up project :)
v20.2/cancel-job.md, line 23 at r3 (raw file):
Previously, solongordon (Solon) wrote…
Maybe note that non-admins cannot cancel admin jobs. (Same goes for canceling queries, sessions, etc. below)
Done.
v20.2/create-user.md, line 26 at r3 (raw file):
Previously, solongordon (Solon) wrote…
As I mentioned on ALTER USER, I think it would be preferable to just specify that CREATE USER is an alias for CREATE ROLE WITH LOGIN and point to the CREATE ROLE page.
Yep..follow-up project :)
There was a problem hiding this comment.
Reviewable status:
v20.2/alter-role.md, line 69 at r3 (raw file):
Previously, Amruta-Ranade (Amruta Ranade) wrote…
I thought a user with CREATEROLE has CREATELOGIN privileges by default? 🤔
Nope, they're separate.
Basically the privileges which were previously encompassed by CREATEROLE in v20.1 have now been separated out into CREATEROLE and CREATELOGIN. For backward compatibility, we do have a migration to grant CREATELOGIN to any users who currently have CREATEROLE when a cluster is upgraded from v20.1 to v20.2. But going forward, they are separate privileges which need to be granted explicitly.
There was a problem hiding this comment.
Reviewable status:
v20.2/alter-role.md, line 69 at r3 (raw file):
Previously, solongordon (Solon) wrote…
Nope, they're separate.
Basically the privileges which were previously encompassed by CREATEROLE in v20.1 have now been separated out into CREATEROLE and CREATELOGIN. For backward compatibility, we do have a migration to grant CREATELOGIN to any users who currently have CREATEROLE when a cluster is upgraded from v20.1 to v20.2. But going forward, they are separate privileges which need to be granted explicitly.
Fixed as discussed on Slack.
There was a problem hiding this comment.
LGTM except I really really really think the 4x repeated role keyword table needs to be an include (unless there are tiny variations I am not understanding, which is ENTIRELY possible - if so, please disregard).
Every other comment is basically a variation on "suggest adding a link to this thing".
| ### Allow a role to log in to the database using a password | ||
|
|
||
| ~~~ sql | ||
| root@:26257/defaultdb> ALTER ROLE carl WITH LOGIN PASSWORD 'An0ther$tr0nGpassW0rD' VALID UNTIL '2021-10-10'; |
100% agree. However, I have an open issue to study and merge the user <> role equivalency and I was planning on cleaning up the redundancies as part of that docs project. |
|
Amruta Ranade <[email protected]> writes:
100% agree. However, I have an open issue to study and merge the user
<> role equivalency and I was planning on cleaning up the redundancies
as part of that docs project.
Ah ok!
|
Addresses #7954 and sub-issues: