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: Drop Role does not properly check if target user does not exist #134538

Closed
Dedej-Bergin opened this issue Nov 7, 2024 · 2 comments · Fixed by #134850
Closed

sql: Drop Role does not properly check if target user does not exist #134538

Dedej-Bergin opened this issue Nov 7, 2024 · 2 comments · Fixed by #134850
Assignees
Labels
branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 branch-release-24.1 Used to mark GA and release blockers, technical advisories, and bugs for 24.1 branch-release-24.2 Used to mark GA and release blockers, technical advisories, and bugs for 24.2 branch-release-24.3 Used to mark GA and release blockers, technical advisories, and bugs for 24.3 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-support Would prevent or help troubleshoot a customer escalation - bugs, missing observability/tooling, docs P-2 Issues/test failures with a fix SLA of 3 months T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@Dedej-Bergin
Copy link
Contributor

Dedej-Bergin commented Nov 7, 2024

Describe the problem

If we set the role as admin and we try to drop a user that does not exist, we give the impression that the command succeeded instead of properly erroring out.

To Reproduce

CREATE ROLE IF NOT EXISTS migrator;
CREATE USER goose_catalogs WITH PASSWORD '123';
CREATE DATABASE catalogs_staging;
USE catalogs_staging; 
ALTER DEFAULT PRIVILEGES FOR ALL ROLES GRANT CREATE, DROP, SELECT, INSERT, UPDATE, DELETE ON tables TO migrator;
GRANT CREATE, DROP, SELECT, INSERT, UPDATE, DELETE ON ALL TABLES IN SCHEMA catalogs_staging.public TO migrator;
GRANT CREATE ON SCHEMA catalogs_staging.public TO migrator;
GRANT migrator TO goose_catalogs WITH ADMIN OPTION;
GRANT SYSTEM CONTROLJOB, CREATEROLE, CREATELOGIN TO goose_catalogs;
show users;


SET ROLE goose_catalogs;
SHOW ROLE;
DROP USER IF EXISTS goose_catalogs_staging_202410212225;
-- we get the proper error that we should get


SET ROLE admin;
SHOW ROLE;
DROP USER IF EXISTS goose_catalogs_staging_202410212225;
-- no error and it looks like the drop succeeded

Environment:
CockroachDB Version: v24.1.6

Additional context
Was first hit by this customer here: https://github.com/cockroachlabs/support/issues/3118

Jira issue: CRDB-44125

Epic CRDB-43310

@Dedej-Bergin Dedej-Bergin added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-support Would prevent or help troubleshoot a customer escalation - bugs, missing observability/tooling, docs T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) labels Nov 7, 2024
@Dedej-Bergin Dedej-Bergin self-assigned this Nov 7, 2024
Copy link

blathers-crl bot commented Nov 7, 2024

Hi @Dedej-Bergin, please add branch-* labels to identify which branch(es) this C-bug affects.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@exalate-issue-sync exalate-issue-sync bot added the P-2 Issues/test failures with a fix SLA of 3 months label Nov 7, 2024
Dedej-Bergin added a commit to Dedej-Bergin/cockroach that referenced this issue Nov 11, 2024
Previously when we are a non-admin user and we run drop role if exists
on a user that does not exist we would get an error that the user does
not exist.  This is incosistent with other if exists commands and it
does not make sense to get an error since by typing if exists we expect
that the user may not exist.  These code changes take care of that.

Fixes: cockroachdb#134538

Release note (bug fix): When we are a non-admin user and we run drop
role if exists on a user that does not exist we no longer get an error
message.
Dedej-Bergin added a commit to Dedej-Bergin/cockroach that referenced this issue Nov 11, 2024
Previously when we are a non-admin user and we run drop role if exists
on a user that does not exist we would get an error that the user does
not exist.  This is incosistent with other if exists commands and it
does not make sense to get an error since by typing if exists we expect
that the user may not exist.  These code changes take care of that.

Fixes: cockroachdb#134538

Release note (bug fix): When you are a non-admin user and you run drop
role if exists on a user that does not exist you no longer get an error
message.
craig bot pushed a commit that referenced this issue Nov 12, 2024
134850: sql: Drop Role does not properly check if target user does not exist r=Dedej-Bergin a=Dedej-Bergin

Previously when we are a non-admin user and we run drop role if exists on a user that does not exist we would get an error that the user does not exist.  This is incosistent with other if exists commands and it does not make sense to get an error since by typing if exists we expect that the user may not exist.  These code changes take care of that.

Loom Video Description: https://www.loom.com/share/cd75d692ef3940be9b3fd395dc911f71?sid=ad6b27e1-620f-45b1-be10-9b733408fae5 

Fixes: #134538

Release note (bug fix): When you are a non-admin user and you run drop role if exists on a user that does not exist you no longer get an error message.

Co-authored-by: Bergin Dedej <[email protected]>
craig bot pushed a commit that referenced this issue Nov 12, 2024
134850: sql: Drop Role does not properly check if target user does not exist r=Dedej-Bergin a=Dedej-Bergin

Previously when we are a non-admin user and we run drop role if exists on a user that does not exist we would get an error that the user does not exist.  This is incosistent with other if exists commands and it does not make sense to get an error since by typing if exists we expect that the user may not exist.  These code changes take care of that.

Loom Video Description: https://www.loom.com/share/cd75d692ef3940be9b3fd395dc911f71?sid=ad6b27e1-620f-45b1-be10-9b733408fae5 

Fixes: #134538

Release note (bug fix): When you are a non-admin user and you run drop role if exists on a user that does not exist you no longer get an error message.

Co-authored-by: Bergin Dedej <[email protected]>
@craig craig bot closed this as completed in 696869a Nov 12, 2024
Copy link

blathers-crl bot commented Nov 12, 2024

Based on the specified backports for linked PR #134850, I applied the following new label(s) to this issue: branch-release-23.2, branch-release-24.1, branch-release-24.2, branch-release-24.3. Please adjust the labels as needed to match the branches actually affected by this issue, including adding any known older branches.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@blathers-crl blathers-crl bot added branch-release-24.1 Used to mark GA and release blockers, technical advisories, and bugs for 24.1 branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 branch-release-24.2 Used to mark GA and release blockers, technical advisories, and bugs for 24.2 branch-release-24.3 Used to mark GA and release blockers, technical advisories, and bugs for 24.3 labels Nov 12, 2024
blathers-crl bot pushed a commit that referenced this issue Nov 12, 2024
Previously when we are a non-admin user and we run drop role if exists
on a user that does not exist we would get an error that the user does
not exist.  This is incosistent with other if exists commands and it
does not make sense to get an error since by typing if exists we expect
that the user may not exist.  These code changes take care of that.

Fixes: #134538

Release note (bug fix): When you are a non-admin user and you run drop
role if exists on a user that does not exist you no longer get an error
message.
blathers-crl bot pushed a commit that referenced this issue Nov 12, 2024
Previously when we are a non-admin user and we run drop role if exists
on a user that does not exist we would get an error that the user does
not exist.  This is incosistent with other if exists commands and it
does not make sense to get an error since by typing if exists we expect
that the user may not exist.  These code changes take care of that.

Fixes: #134538

Release note (bug fix): When you are a non-admin user and you run drop
role if exists on a user that does not exist you no longer get an error
message.
blathers-crl bot pushed a commit that referenced this issue Nov 12, 2024
Previously when we are a non-admin user and we run drop role if exists
on a user that does not exist we would get an error that the user does
not exist.  This is incosistent with other if exists commands and it
does not make sense to get an error since by typing if exists we expect
that the user may not exist.  These code changes take care of that.

Fixes: #134538

Release note (bug fix): When you are a non-admin user and you run drop
role if exists on a user that does not exist you no longer get an error
message.
blathers-crl bot pushed a commit that referenced this issue Nov 12, 2024
Previously when we are a non-admin user and we run drop role if exists
on a user that does not exist we would get an error that the user does
not exist.  This is incosistent with other if exists commands and it
does not make sense to get an error since by typing if exists we expect
that the user may not exist.  These code changes take care of that.

Fixes: #134538

Release note (bug fix): When you are a non-admin user and you run drop
role if exists on a user that does not exist you no longer get an error
message.
craig bot pushed a commit that referenced this issue Nov 12, 2024
134724: metric: truncate help text in prom output r=arjunmahishi a=dhartunian

We have some long multiline help text defined for metrics that makes the prometheus output quite large. This change automatically truncates help text at the first newline during prometheus output. We can still access the full text in our docs and code.

Resolves: CRDB-43497
Epic: None

Release note (ops change): the metrics scrape HTTP endpoint at `/ _status/vars` will now truncate HELP text at the first newline, reducing the metadata for metrics with large descriptions. Customers can still access these descriptions via our docs.

134902: tablemetadatacache,ui: change error messaging for failed tmj update r=kyle-a-wong a=kyle-a-wong

Previously, error messages from the table metaddata update job were surfaced in the UI on the table overview page. These errors are usually overly verbose and don't provide any actionable info to the end user.

In addition to this, error messages can be very big if the error is a result of a span stats fanout failure, which can contain errors for each fanned out request.

Now, db-console will simply report that an error occured in the job and that the data is stale. The error returned from failed SpanStats requests will now have a generic "An error has occurred while fetching span stats."

Epic: None
Release note: None

Old error messaging:
<img width="338" alt="image" src="https://github.com/user-attachments/assets/382ce140-3778-48f4-a300-fe5fcaed5faa">

New error messaging:
<img width="434" alt="image" src="https://github.com/user-attachments/assets/21df4f80-dd0e-4d95-87a1-a64752b0d867">


134955: kvserver/rangefeed: introduce Disconnector interface  r=stevendanna a=wenyihu6

This patch introduces the `Disconnector` interface, implemented by the
`registration` interface. Although currently unused, future commits will pass this
interface to `node.MuxRangefeed`, allowing disconnection of individual
registrations at the node level.

Part of: #110432
Release note: none

Co-authored-by: Steven Danna [email protected]

134979: sql: avoid role existence check in DROP ROLE when it's not necessary r=rafiss a=rafiss

We only need to make the RoleExists call when the `IF EXISTS` clause is used.

This check was recently added in 696869a.

informs: #134538
Release note: None

135000: Revert "schemachanger: refactor subzone config elements" r=annrpom a=annrpom

Reverts #133879

Epic: none

Release note: None

Co-authored-by: David Hartunian <[email protected]>
Co-authored-by: Kyle Wong <[email protected]>
Co-authored-by: Wenyi Hu <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>
Co-authored-by: Annie Pompa <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 branch-release-24.1 Used to mark GA and release blockers, technical advisories, and bugs for 24.1 branch-release-24.2 Used to mark GA and release blockers, technical advisories, and bugs for 24.2 branch-release-24.3 Used to mark GA and release blockers, technical advisories, and bugs for 24.3 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-support Would prevent or help troubleshoot a customer escalation - bugs, missing observability/tooling, docs P-2 Issues/test failures with a fix SLA of 3 months T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant