Skip to content

[fix](fe) Add user existence check in DropRowPolicyCommand#63631

Open
heguanhui wants to merge 1 commit into
apache:masterfrom
heguanhui:fix/drop-row-policy-check-user-exist
Open

[fix](fe) Add user existence check in DropRowPolicyCommand#63631
heguanhui wants to merge 1 commit into
apache:masterfrom
heguanhui:fix/drop-row-policy-check-user-exist

Conversation

@heguanhui
Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: close #63322

Problem Summary: DropRowPolicyCommand does not validate whether the user specified in the DROP ROW POLICY statement actually exists. This is inconsistent with CreatePolicyCommand, which checks user existence via doesUserExist(). Without this validation, a typo in the user name could silently match no policy, or the user might mistakenly believe the drop succeeded. Also, the auth check should be performed before user existence check to prevent information leakage.

Release note

DROP ROW POLICY now validates that the specified user exists, returning an error if it does not. This makes the behavior consistent with CREATE ROW POLICY.

Check List (For Author)

  • Test
    • Unit Test
  • Behavior changed:
    • Yes. DROP ROW POLICY now throws AnalysisException when the specified user does not exist, whereas previously it would proceed without validation.
  • Does this need documentation?
    • No.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

@hello-stephen
Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

### What problem does this PR solve?

Issue Number: close apache#63322

Problem Summary: DropRowPolicyCommand does not validate whether the user
specified in the DROP ROW POLICY statement actually exists. This is inconsistent
with CreatePolicyCommand, which checks user existence via doesUserExist().
Without this validation, a typo in the user name could silently match no policy,
or the user might mistakenly believe the drop succeeded. Also, the auth check
should be performed before user existence check to prevent information leakage.

### Release note

DROP ROW POLICY now validates that the specified user exists, returning an error
if it does not. This makes the behavior consistent with CREATE ROW POLICY.

### Check List (For Author)

- Test: Unit Test, Regression test
    - Added testValidateUserNotExist in DropRowPolicyCommandTest
    - Added regression test in test_row_policy.groovy
- Behavior changed: Yes. DROP ROW POLICY now throws AnalysisException when the
  specified user does not exist, whereas previously it would proceed without
  validation.
- Does this need documentation: No
@heguanhui heguanhui force-pushed the fix/drop-row-policy-check-user-exist branch from 662ecbd to a91be8d Compare May 25, 2026 14:12
@heguanhui
Copy link
Copy Markdown
Contributor Author

run buildall

@hello-stephen
Copy link
Copy Markdown
Contributor

TPC-H: Total hot run time: 31503 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit a91be8deac20de89895890a6f3a8b4806c18db58, data reload: false

------ Round 1 ----------------------------------
orders	Doris	NULL	NULL	0	0	0	NULL	0	NULL	NULL	2023-12-26 18:27:23	2023-12-26 18:42:55	NULL	utf-8	NULL	NULL	
============================================
q1	17708	4108	4091	4091
q2	q3	10758	1414	825	825
q4	4688	475	350	350
q5	7776	2274	2145	2145
q6	276	175	138	138
q7	964	777	659	659
q8	9440	1707	1585	1585
q9	6732	5023	4964	4964
q10	6457	2257	1865	1865
q11	444	273	245	245
q12	693	433	300	300
q13	18198	3290	2723	2723
q14	267	260	234	234
q15	q16	820	768	716	716
q17	999	973	914	914
q18	6840	5800	5565	5565
q19	1254	1286	1096	1096
q20	550	410	274	274
q21	5749	2594	2512	2512
q22	425	360	302	302
Total cold run time: 101038 ms
Total hot run time: 31503 ms

----- Round 2, with runtime_filter_mode=off -----
orders	Doris	NULL	NULL	150000000	42	6422171781	NULL	22778155	NULL	NULL	2023-12-26 18:27:23	2023-12-26 18:42:55	NULL	utf-8	NULL	NULL	
============================================
q1	4422	4309	4304	4304
q2	q3	4565	4944	4380	4380
q4	2204	2273	1430	1430
q5	4504	4380	4924	4380
q6	266	205	159	159
q7	2004	2014	1663	1663
q8	2607	2338	2371	2338
q9	8040	8086	8042	8042
q10	4850	4938	4292	4292
q11	593	423	405	405
q12	742	777	560	560
q13	3363	3587	3011	3011
q14	297	317	292	292
q15	q16	714	740	685	685
q17	1440	1377	1395	1377
q18	8126	7427	6923	6923
q19	1121	1122	1098	1098
q20	2246	2232	1939	1939
q21	5363	4713	4654	4654
q22	545	455	414	414
Total cold run time: 58012 ms
Total hot run time: 52346 ms

@hello-stephen
Copy link
Copy Markdown
Contributor

TPC-DS: Total hot run time: 172836 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit a91be8deac20de89895890a6f3a8b4806c18db58, data reload: false

query5	4315	660	518	518
query6	368	214	204	204
query7	4225	606	321	321
query8	339	232	221	221
query9	8842	4182	4115	4115
query10	472	347	302	302
query11	5815	2543	2236	2236
query12	189	129	124	124
query13	1282	630	450	450
query14	6251	5544	5254	5254
query14_1	4600	4555	4518	4518
query15	213	206	186	186
query16	992	445	435	435
query17	1129	745	612	612
query18	2451	502	373	373
query19	223	207	164	164
query20	143	136	132	132
query21	221	144	127	127
query22	13680	13529	13413	13413
query23	17409	16458	16275	16275
query23_1	16513	16303	16326	16303
query24	7560	1779	1309	1309
query24_1	1341	1354	1348	1348
query25	592	497	442	442
query26	1312	320	182	182
query27	2672	568	366	366
query28	4416	2042	2014	2014
query29	1016	646	526	526
query30	301	251	205	205
query31	1151	1084	970	970
query32	96	80	73	73
query33	543	373	305	305
query34	1201	1192	653	653
query35	797	793	680	680
query36	1413	1399	1256	1256
query37	153	103	95	95
query38	3231	3161	3087	3087
query39	952	944	895	895
query39_1	879	887	886	886
query40	225	150	123	123
query41	67	63	64	63
query42	108	108	108	108
query43	333	336	306	306
query44	
query45	218	205	206	205
query46	1071	1235	745	745
query47	2377	2425	2306	2306
query48	404	416	304	304
query49	624	498	396	396
query50	1004	343	260	260
query51	4395	4346	4279	4279
query52	103	103	92	92
query53	259	289	206	206
query54	320	270	267	267
query55	94	92	87	87
query56	303	306	315	306
query57	1453	1429	1350	1350
query58	303	274	269	269
query59	1583	1699	1484	1484
query60	322	332	309	309
query61	161	156	155	155
query62	700	647	555	555
query63	264	204	207	204
query64	2483	834	636	636
query65	
query66	1717	480	363	363
query67	29836	29818	29650	29650
query68	
query69	452	339	292	292
query70	1061	1022	986	986
query71	312	269	264	264
query72	3004	2746	2421	2421
query73	815	728	425	425
query74	5140	4957	4870	4870
query75	2721	2615	2286	2286
query76	2313	1155	772	772
query77	403	406	337	337
query78	12559	12524	12072	12072
query79	1472	1060	738	738
query80	1342	553	463	463
query81	517	286	238	238
query82	1000	159	119	119
query83	349	280	258	258
query84	274	143	114	114
query85	935	531	478	478
query86	456	360	356	356
query87	3461	3386	3268	3268
query88	3671	2761	2762	2761
query89	454	402	343	343
query90	1912	185	176	176
query91	182	165	136	136
query92	77	82	74	74
query93	1536	1465	939	939
query94	722	351	301	301
query95	683	464	346	346
query96	1119	759	356	356
query97	2744	2764	2673	2673
query98	233	227	228	227
query99	1203	1163	1042	1042
Total cold run time: 256164 ms
Total hot run time: 172836 ms

@hello-stephen
Copy link
Copy Markdown
Contributor

FE Regression Coverage Report

Increment line coverage 62.50% (5/8) 🎉
Increment coverage report
Complete coverage report

@morrySnow morrySnow self-requested a review May 26, 2026 07:26
PrivPredicate.GRANT.getPrivs().toString());
}
tableNameInfo.analyze(ctx.getNameSpaceContext());
if (user != null) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in doris, we maybe use external Authentication Service or use a temporary user. So users do not necessarily always exist in Doris.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just to maintain consistency with the user existence verification when creating the policy

@github-actions github-actions Bot added the approved Indicates a PR has been approved by one committer. label May 26, 2026
@github-actions
Copy link
Copy Markdown
Contributor

PR approved by at least one committer and no changes requested.

@github-actions
Copy link
Copy Markdown
Contributor

PR approved by anyone and no changes requested.

@morrySnow morrySnow self-requested a review May 26, 2026 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by one committer. reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants