roleccl: enable GRANT/REVOKE for roles without a license #45325
Conversation
|
This change is |
|
cc @mattcrdb |
|
cc @taroface - this change will also be backported so the corresponding doc update will be multi-version. |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @knz, @RichardJCai, and @rohany)
pkg/ccl/roleccl/role.go, line 85 at r1 (raw file):
// non-licensed users to add/remove users from the admin role, so // they can grant administrative privileges to user accounts that // are not superusers like "root".
Nit: Are we concerned at all about users going from enterprise to non-enterprise? They would be able to grant existing roles in that case.
Good question. Asking @nstewart:
|
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/roleccl/role.go, line 236 at r1 (raw file):
if err := utilccl.CheckEnterpriseEnabled( p.ExecCfg().Settings, p.ExecCfg().ClusterID(), p.ExecCfg().Organization(), "REVOKE <role>",
Need to remove this check too.
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/roleccl/role.go, line 236 at r1 (raw file):
Previously, RichardJCai (Richard Cai) wrote…
Need to remove this check too.
Also probably want to add tests to sql/logictest/testdata/logic_test/role
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/roleccl/role.go, line 236 at r1 (raw file):
Previously, RichardJCai (Richard Cai) wrote…
Also probably want to add tests to sql/logictest/testdata/logic_test/role
Good catch!
The GRANT and REVOKE statements are already tested in that logic test file.
Unfortunately we do not have testing infrastructure for checking that licensed features are available to non-licensed users and vice-versa. As this PR will be backported, I am reluctant to add new infrastructure as it would make the backport more complicated to manage. (This would be an exception of course.)
I'll file an issue.
There was a problem hiding this comment.
Reviewable status:
19.1 and 19.2 since the previous changes introduced a serious UX regression. |
Before, the HTTP endpoints would return 500 (Internal server error) when they require an admin user and a non-admin user was logged in. This patch changes it to make them return 403 (Forbidden) instead, which is the standard "permission denied" error code. Release note (general change): HTTP endpoints now report status 403 (Forbidden) instead of 500 (Internal server error) when the authenticated user has insufficient privileges to use the endpoint.
Release note (security update): Non-licensed users are now able to add more principals to the special superuser role/group `admin`. (Creation of additional roles is still a licensed feature). Release note (sql change): It is now possible to use `GRANT` and `REVOKE` to add users to the `admin` role without a valid license. This change aims to enable use of the admin UI and other privileged features without a license.
|
bors r=RichardJCai |
Build succeeded |
Fixes #45275.
This was discussed (and agreed upon) with @nstewart and @piyush-singh.
Release note (security update): Non-licensed users are now
able to add more principals to the special superuser role/group
admin. (Creation of additional roles is still a licensed feature.)Release note (sql change): It is now possible to use
GRANTandREVOKEto add users to theadminrole without a validlicense. This change aims to enable use of the admin UI and other
privileged features without a license.