sql: add parser grammar and wire up for SHOW USERS clauses#166416
Conversation
|
Merging to
|
rafiss
left a comment
There was a problem hiding this comment.
@rafiss made 6 comments.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on sanchit-CRL and souravcrl).
-- commits line 23 at r4:
nit: the PR body says "informs" -- ideally these would be consistent
pkg/sql/delegate/show_roles.go line 67 at r4 (raw file):
tsStr := tree.AsStringWithFlags(n.Options.LastAccessOlderThan, tree.FmtBareStrings) whereExprs = append(whereExprs, fmt.Sprintf( "u.estimated_last_login_time < %s::TIMESTAMPTZ",
nit: it could be helpful to add a comment saying that users with a NULL estimated_last_login_time will be excluded from the result.
pkg/sql/sem/tree/show.go line 1013 at r4 (raw file):
// ShowUsersOptions describes options for the SHOW USERS statement. type ShowUsersOptions struct { Source Expr // SOURCE = 'ldap:...'
nit: this just echoes the syntax rather than explaining semantics. Consider adding what the field represents (e.g., "filters users by their PROVISIONSRC role option value").
pkg/sql/logictest/testdata/logic_test/user line 256 at r4 (raw file):
subtest end subtest show_users_provisioning_filters
please add an additional test case that verifies that the "last access before" syntax actually works.
you can use the let syntax in the logic test framework to capture the login time. for example:
# The estimated_last_login_time is not guaranteed to be populated synchronously,
# so we poll until testuser's entry was updated with the last login time.
query I retry
SELECT count(*) FROM system.users WHERE estimated_last_login_time IS NOT NULL AND username = 'testuser2'
----
1
let $login_time
SELECT estimated_last_login_time FROM system.users WHERE username = 'testuser2'
query TTT
SELECT username, options, member_of FROM [SHOW USERS WITH LAST LOGIN BEFORE $login_time::timestamptz + '1 minute'::interval]
pkg/sql/delegate/show_roles_test.go line 175 at r4 (raw file):
// escaped via lexbase.EscapeSQLString. n := &tree.ShowUsers{ Options: &tree.ShowUsersOptions{
nit: the SQL injection test only covers Source, not LastAccessOlderThan. Add a case with a malicious timestamp value like "2024-01-01' OR 1=1 --" to TestDelegateShowRolesExtendedSQLInjection.
pkg/sql/parser/sql.y line 10258 at r4 (raw file):
$$.val = &tree.ShowUsersOptions{Source: $3.expr()} } | LAST ACCESS TIME OLDER THAN a_expr
nit: this syntax is a little verbose. how about SHOW USERS WITH LAST LOGIN BEFORE a_expr
(consider updating the go struct names if you change this syntax)
4f37fb6 to
16308ac
Compare
souravcrl
left a comment
There was a problem hiding this comment.
@souravcrl made 6 comments and resolved 4 discussions.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on rafiss and sanchit-CRL).
pkg/sql/delegate/show_roles.go line 67 at r4 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: it could be helpful to add a comment saying that users with a NULL
estimated_last_login_timewill be excluded from the result.
done
pkg/sql/parser/sql.y line 10258 at r4 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: this syntax is a little verbose. how about
SHOW USERS WITH LAST LOGIN BEFORE a_expr(consider updating the go struct names if you change this syntax)
done
pkg/sql/sem/tree/show.go line 1013 at r4 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: this just echoes the syntax rather than explaining semantics. Consider adding what the field represents (e.g., "filters users by their PROVISIONSRC role option value").
done
pkg/sql/logictest/testdata/logic_test/user line 256 at r4 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
please add an additional test case that verifies that the "last access before" syntax actually works.
you can use the
letsyntax in the logic test framework to capture the login time. for example:# The estimated_last_login_time is not guaranteed to be populated synchronously, # so we poll until testuser's entry was updated with the last login time. query I retry SELECT count(*) FROM system.users WHERE estimated_last_login_time IS NOT NULL AND username = 'testuser2' ---- 1 let $login_time SELECT estimated_last_login_time FROM system.users WHERE username = 'testuser2' query TTT SELECT username, options, member_of FROM [SHOW USERS WITH LAST LOGIN BEFORE $login_time::timestamptz + '1 minute'::interval]
done
pkg/sql/delegate/show_roles_test.go line 175 at r4 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: the SQL injection test only covers
Source, notLastAccessOlderThan. Add a case with a malicious timestamp value like "2024-01-01' OR 1=1 --" to TestDelegateShowRolesExtendedSQLInjection.
done
pkg/sql/delegate/show_roles.go line 84 at r8 (raw file):
var limitClause string if n.Limit != nil && n.Limit.Count != nil {
@rafiss I had a question if we should also validate n.Limit.Offset here? cc: @sanchit-CRL
souravcrl
left a comment
There was a problem hiding this comment.
@souravcrl made 1 comment.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on rafiss and sanchit-CRL).
Previously, rafiss (Rafi Shamim) wrote…
nit: the PR body says "informs" -- ideally these would be consistent
Apologies, this has already merged. I will take care from next time.
16308ac to
6cdc8ca
Compare
rafiss
left a comment
There was a problem hiding this comment.
nice work!
@rafiss made 2 comments.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on sanchit-CRL).
pkg/sql/delegate/show_roles.go line 84 at r8 (raw file):
Previously, souravcrl wrote…
@rafiss I had a question if we should also validate
n.Limit.Offsethere? cc: @sanchit-CRL
validation seems reasonable to add. what did you have in mind?
souravcrl
left a comment
There was a problem hiding this comment.
@souravcrl made 1 comment.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on rafiss and sanchit-CRL).
pkg/sql/delegate/show_roles.go line 84 at r8 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
validation seems reasonable to add. what did you have in mind?
I am not sure exactly how to achieve this. Is there an existing sql statement leveraging this and should we try to clone that here?
cbfcdca to
762c2ff
Compare
rafiss
left a comment
There was a problem hiding this comment.
@rafiss made 1 comment.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on sanchit-CRL).
pkg/sql/delegate/show_roles.go line 84 at r8 (raw file):
I'm still not sure what your idea is -- what are you thinking of validating?
I am not sure exactly how to achieve this. Is there an existing sql statement leveraging this
What does "this" refer to?
souravcrl
left a comment
There was a problem hiding this comment.
@souravcrl made 1 comment.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on rafiss, sanchit-CRL, and spilchen).
pkg/sql/delegate/show_roles.go line 84 at r8 (raw file):
Previously you mentioned
validation seems reasonable to add
So, I wanted to see if we are doing this elsewhere in the code and how this could improve the implementation. This was originally @sanchit-CRL 's idea that we should have this, so adding him here to chime in his thoughts.
spilchen
left a comment
There was a problem hiding this comment.
@spilchen partially reviewed 3 files and made 4 comments.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on rafiss, sanchit-CRL, and souravcrl).
pkg/sql/delegate/delegate.go line 156 at r13 (raw file):
case *tree.ShowUsers: return d.delegateShowRolesExtended(t)
one thing I found confusing was before SHOW ROLES and SHOW USERS were interchangeable. But now the extended syntax only applies to SHOW USERS, not SHOW ROLES. Perhaps that is intentional and by design. But the extended support was added to the function name delegateShowRolesExtended. So, I am a little confused.
I think one of two things has to happen:
- extend the
SHOW ROLESsyntax to have the extended options - rename
delegateShowRolesExtendedtodelegateShowUsersExtendedso that it's clear this extended syntax only applies toSHOW USERS
pkg/sql/logictest/testdata/logic_test/user line 256 at r13 (raw file):
subtest end subtest show_users_provisioning_filters
do we have a test that has multiple options with SHOW USER? e.g. SOURCE = ..., LAST LOGIN BEFORE ...
pkg/sql/parser/testdata/show line 669 at r13 (raw file):
parse SHOW USERS WITH SOURCE = 'ldap:ldap.example.com', LAST LOGIN BEFORE '2024-01-01'
can we have a test for duplicate options? I believe it should error out
pkg/sql/delegate/show_roles.go line 0 at r13 (raw file):
the changes in this file was reverted. But I see a TODO in there that should probably removed now:
// TODO(sourav): Wire this into the delegate dispatch for ShowUsers;
// currently only exercised by unit tests.
func (d *delegator) delegateShowRolesExtended(n *tree.ShowUsers) (tree.Statement, error) {
|
Nice catch -- we should continue to treat |
souravcrl
left a comment
There was a problem hiding this comment.
@souravcrl made 1 comment.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on rafiss, sanchit-CRL, and spilchen).
pkg/sql/delegate/delegate.go line 156 at r13 (raw file):
Previously, spilchen wrote…
one thing I found confusing was before
SHOW ROLESandSHOW USERSwere interchangeable. But now the extended syntax only applies toSHOW USERS, notSHOW ROLES. Perhaps that is intentional and by design. But the extended support was added to the function namedelegateShowRolesExtended. So, I am a little confused.I think one of two things has to happen:
- extend the
SHOW ROLESsyntax to have the extended options- rename
delegateShowRolesExtendedtodelegateShowUsersExtendedso that it's clear this extended syntax only applies toSHOW USERS
Thanks for the note! You're right — SHOW ROLES and SHOW USERS have always been interchangeable, so it doesn't make sense for the extended syntax to only apply to SHOW USERS. I went with option 1: extending
SHOW ROLES to support the same provisioning clauses.
▎ The changes are split across 3 PRs (stacked on top of this one), mirroring the structure of the original SHOW USERS PRs:
▎ - #168022 — Extends the ShowRoles AST node with Options and Limit fields
▎ - #168023 — Refactors delegateShowRolesExtended to accept (options, limit) params so it serves both SHOW USERS and SHOW ROLES, and wires up the SHOW ROLES dispatch path
▎ - #168024 — Adds parser grammar for SHOW ROLES [WITH ] [LIMIT ], parser tests (including duplicate option error cases), and e2e logic tests covering SHOW ROLES with SOURCE, LAST LOGIN
BEFORE, and combined options
sanchit-CRL
left a comment
There was a problem hiding this comment.
@sanchit-CRL made 1 comment.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on rafiss, souravcrl, and spilchen).
pkg/sql/delegate/show_roles.go line 84 at r8 (raw file):
Previously, souravcrl wrote…
Previously you mentioned
validation seems reasonable to add
So, I wanted to see if we are doing this elsewhere in the code and how this could improve the implementation. This was originally @sanchit-CRL 's idea that we should have this, so adding him here to chime in his thoughts.
My question in the other PR was whether we should reject n.Limit.Offset if that has been provided in the query explicitly. So should that validation be added here.
bdea500 to
d4f3a9d
Compare
souravcrl
left a comment
There was a problem hiding this comment.
@souravcrl made 3 comments.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on rafiss and spilchen).
pkg/sql/delegate/show_roles.go line at r13 (raw file):
Previously, spilchen wrote…
the changes in this file was reverted. But I see a TODO in there that should probably removed now:
// TODO(sourav): Wire this into the delegate dispatch for ShowUsers; // currently only exercised by unit tests. func (d *delegator) delegateShowRolesExtended(n *tree.ShowUsers) (tree.Statement, error) {
done
pkg/sql/logictest/testdata/logic_test/user line 256 at r13 (raw file):
Previously, spilchen wrote…
do we have a test that has multiple options with
SHOW USER? e.g.SOURCE = ..., LAST LOGIN BEFORE ...
have added.
pkg/sql/parser/testdata/show line 669 at r13 (raw file):
Previously, spilchen wrote…
can we have a test for duplicate options? I believe it should error out
done
ee61b62 to
ff93c78
Compare
|
Detected infrastructure failure (matched: self-hosted runner lost communication with the server). Automatically rerunning failed jobs. (run link) |
Add grammar rules to sql.y for the SHOW USERS WITH ... syntax:
SHOW USERS WITH SOURCE = <expr>, LAST LOGIN BEFORE <expr> LIMIT <n>
Options are parsed into ShowUsersOptions via the show_users_option
production rule. Multiple options are comma-separated and validated
for duplicates via CombineWith.
Update parser testdata with parse round-trip tests for all clause
combinations.
Epic: CRDB-52460
Release note (sql change): SHOW USERS now supports optional
filtering clauses for user provisioning workflows:
SHOW USERS [WITH <options>] [LIMIT <n>]
Options (comma-separated):
- SOURCE = <string>: filter users by their provisioning source
(PROVISIONSRC role option value), e.g. 'ldap:ldap.example.com'.
- LAST LOGIN BEFORE <timestamp>: filter users whose estimated
last login time is before the given timestamp. Users who have
never logged in (NULL estimated_last_login_time) are excluded.
Examples:
-- Find all LDAP-provisioned users
SHOW USERS WITH SOURCE = 'ldap:ldap.example.com'
-- Find dormant users who haven't logged in since Jan 2024
SHOW USERS WITH LAST LOGIN BEFORE '2024-01-01'
-- Combine filters with a limit
SHOW USERS WITH SOURCE = 'ldap:ldap.example.com',
LAST LOGIN BEFORE '2024-06-01' LIMIT 100
The original SHOW USERS behavior (no clauses) is unchanged.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ff93c78 to
c3bb649
Compare
This commit connects the SHOW USERS extended syntax (WITH SOURCE, WITH LAST LOGIN BEFORE, LIMIT) to the delegation layer that generates the underlying SQL query. It also adds end-to-end logic tests covering the new filter clauses. Epic: CRDB-45081 Release note: None
c3bb649 to
80b98d5
Compare
sql: add parser grammar for SHOW USERS provisioning clauses
Extend the SQL parser grammar to support optional WITH clauses
and LIMIT on the SHOW USERS statement:
SHOW USERS [WITH ] [LIMIT ]
Options:
SOURCE =
LAST LOGIN BEFORE
Options are comma-separated and parsed into ShowUsersOptions AST
nodes defined in a prior commit. The grammar follows the same
pattern as opt_with_show_backup_options for consistency.
sql: wire up SHOW USERS delegation and add e2e tests
Wire up the TryDelegate dispatch to route ShowUsers AST nodes
through delegateShowRolesExtended, which applies provisioning
filters when WITH options are present.
Add end-to-end logic tests that exercise the full SHOW USERS
WITH clause pipeline from parsing through delegation to execution:
Informs #150604
Release note (sql change): SHOW USERS now supports optional
filtering clauses for user provisioning workflows:
SHOW USERS [WITH ] [LIMIT ]
Options (comma-separated):
(PROVISIONSRC role option value), e.g. 'ldap:ldap.example.com'.
last login time is before the given timestamp. Users who have
never logged in (NULL estimated_last_login_time) are excluded.
Examples:
-- Find all LDAP-provisioned users
SHOW USERS WITH SOURCE = 'ldap:ldap.example.com'
-- Find dormant users who haven't logged in since Jan 2024
SHOW USERS WITH LAST LOGIN BEFORE '2024-01-01'
-- Combine filters with a limit
SHOW USERS WITH SOURCE = 'ldap:ldap.example.com',
LAST LOGIN BEFORE '2024-06-01' LIMIT 100
The original SHOW USERS behavior (no clauses) is unchanged.