[improvement](auth) Scope mysql.user rows to the caller and mask password columns - #67444
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Request changes. The password masking and optional Thrift plumbing are structurally consistent, but two authorization-boundary defects and one deterministic style-gate failure remain.
Critical checkpoint conclusions:
- Goal, data correctness, and security: not yet achieved. A non-GRANT caller is matched by username instead of the exact authenticated user@host account, exposing another same-name account's host, privilege flags, and password-policy state. Separately, ADMIN/GRANT privileges supplied by authentication-integration session roles are lost across the BE callback, so a supported role administrator is incorrectly scoped as an ordinary user. The password-derived column indexes 23 and 27 are correct and always masked on the new/new path.
- Scope and clarity: the six-file change is focused, and every changed path plus the relevant identity, role, scanner, and password-policy producers/consumers was traced.
- Concurrency and lifecycle: no new shared mutable state, unsafe lock ordering, use-after-free path, or callback-lifetime issue was found; the existing Auth and password-policy locking remains intact.
- Compatibility, configuration, persistence, and data writes: there is no configuration, edit-log, storage-format, transaction, or persistent-data change. The optional Thrift field is wire-compatible; the documented new-FE/old-BE fail-closed result and old-FE/new-BE legacy behavior are not separate defects in this patch.
- Parallel paths and FE/BE propagation: the exact user identity reaches the callback, but the immutable session-mapped role set does not; that omission is called out inline. Native persisted and legacy LDAP role paths remain reconstructible.
- Tests and checks: the new unit/regression coverage verifies root, a distinct normal username, missing identity, and both mask columns, but it does not cover the two failing authorization cases. On exact head aac29f0, FE UT and Clang Formatter pass; CheckStyle fails because TShowUserResult is now unused. Other CI jobs are still pending. No local build or test was run because this was a review-only workflow.
- Performance and observability: no material regression or missing diagnostic was found for this small metadata scan.
- Additional user focus: none was provided.
Two independent review rounds converged with no unresolved or undeduplicated candidates. The three blocking findings are attached inline.
| // the identity subject, so a caller sees every same-named entry regardless of | ||
| // its host pattern. Password material is masked below, so nothing sensitive | ||
| // leaks even across host variants of the same name. | ||
| if (!canSeeAll && (currentUser == null || !userIdent.getQualifiedUser() |
Contributor
There was a problem hiding this comment.
[P1] Scope the self case to the exact authenticated account. Doris stores multiple User entries for one name when their hosts differ, and ConnectContext.currentUserIdentity is the exact user@host entry that matched login. With a restricted u@hostA and a separate privileged u@hostB, this username-only comparison admits both rows and exposes host, global privilege flags, and password-policy state from the other account. Please compare the complete UserIdentity here and add a same-name/different-host regression.
| } | ||
|
|
||
| struct TShowUserRequest { | ||
| 1: optional Types.TUserIdentity current_user_ident // to filter rows by the requesting user's privileges |
Contributor
There was a problem hiding this comment.
[P1] Preserve session-mapped roles across this callback. Authentication integrations keep their granted roles only in the originating ConnectContext, and Auth.getRolesByUserWithLdap consults that set only on the session thread. The BE callback reaches a Thrift worker with just this identity, so an OIDC/JIT user whose mapped role has global ADMIN_PRIV or GRANT_PRIV is evaluated as a normal user and cannot see all rows. Please carry the immutable authenticated-role set (or a trusted precomputed authorization decision) through the plan/request and cover a mapped-role administrator.
|
|
||
| @Test | ||
| public void testShowUser() { | ||
| public void testShowUser() throws Exception { |
Contributor
There was a problem hiding this comment.
[P1] Remove the now-unused TShowUserResult import. This rewritten test no longer declares a TShowUserResult, and the CheckStyle job on this exact head fails at line 69 with UnusedImports. Please remove that import so the required FE style gate passes.
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Member
Author
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 16825 ms |
Contributor
TPC-DS: Total hot run time: 81853 ms |
Contributor
ClickBench: Total hot run time: 14.58 s |
…word columns Return mysql.user rows according to the requesting user's privileges: role administrators (ADMIN_PRIV or GRANT_PRIV) still see every account, while other users see only their own account. The authentication_string and password_policy.history_passwords columns are always rendered as *** for every caller, including accounts with an empty password. The caller identity is threaded through TShowUserRequest (mirroring the sibling schema-table scanners) so the FE can filter rows; a request without an identity returns no rows. Add coverage: FrontendServiceImplTest#testShowUser asserts the admin, normal-user, and no-identity behaviors, and a new test_mysql_user_visibility regression checks the end-to-end admin vs normal-user visibility and masking.
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 16722 ms |
Contributor
TPC-DS: Total hot run time: 81801 ms |
Contributor
ClickBench: Total hot run time: 14.55 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
924060929
approved these changes
Sep 3, 2026
github-actions Bot
pushed a commit
that referenced
this pull request
Sep 3, 2026
…word columns (#67444) ### What this PR does Adjusts what `mysql.user` returns so the visible rows follow the requesting user's privileges, and keeps password-derived columns out of the result entirely. - Rows are now scoped to the caller: role administrators (`ADMIN_PRIV` or `GRANT_PRIV`) still see every account; other users see only their own account. - The `authentication_string` and `password_policy.history_passwords` columns are always rendered as `***` for every caller, including accounts with an empty password. To make row scoping possible, the caller identity is threaded through `TShowUserRequest` (the same pattern already used by the sibling schema-table scanners such as `user_privileges` and `processlist`), so the FE can filter rows. A request without an identity returns no rows. ### Compatibility - `current_user_ident` is an `optional` Thrift field, wire-compatible in both directions. - No metadata / editlog / storage-format change, so downgrade is clean. - During a rolling window where a new FE talks to an old BE that does not set the field, `mysql.user` returns no rows (fail-closed) until the BE is also upgraded; upgrading BE before FE avoids this. ### Tests - `FrontendServiceImplTest#testShowUser` asserts the administrator, normal-user, and no-identity behaviors, including that the password columns are masked. - `regression-test/suites/auth_p0/test_mysql_user_visibility.groovy` covers the end-to-end admin-vs-normal-user visibility and masking.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Adjusts what
mysql.userreturns so the visible rows follow the requesting user's privileges, and keeps password-derived columns out of the result entirely.ADMIN_PRIVorGRANT_PRIV) still see every account; other users see only their own account.authentication_stringandpassword_policy.history_passwordscolumns are always rendered as***for every caller, including accounts with an empty password.To make row scoping possible, the caller identity is threaded through
TShowUserRequest(the same pattern already used by the sibling schema-table scanners such asuser_privilegesandprocesslist), so the FE can filter rows. A request without an identity returns no rows.Compatibility
current_user_identis anoptionalThrift field, wire-compatible in both directions.mysql.userreturns no rows (fail-closed) until the BE is also upgraded; upgrading BE before FE avoids this.Tests
FrontendServiceImplTest#testShowUserasserts the administrator, normal-user, and no-identity behaviors, including that the password columns are masked.regression-test/suites/auth_p0/test_mysql_user_visibility.groovycovers the end-to-end admin-vs-normal-user visibility and masking.