-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[BugFix] Fix an issue when the privilege is merged,casue root privilge dropped. #46411
Conversation
@@ -279,7 +287,7 @@ public void merge(PrivilegeCollectionV2 other) { | |||
} else { | |||
List<PrivilegeEntry> typeList = typeToPrivilegeEntryList.get(typeId); | |||
for (PrivilegeEntry entry : otherList) { | |||
grantObjectToList(entry.actionSet, entry.object, entry.withGrantOption, typeList); | |||
grantObjectToList(entry.actionSet, entry.object, entry.withGrantOption, typeList, true); | |||
} // for privilege entry in other.list | |||
} | |||
} // for typeId, privilegeEntryList in other |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The most risky bug in this code is:
Mixing semantics of isGrant
with deep/shallow copy logic potentially leads to incorrect privilege management.
You can modify the code like this:
- grantObjectToList(entry.actionSet, entry.object, entry.withGrantOption, typeList);
+ grantObjectToList(entry.actionSet, entry.object, entry.withGrantOption, typeList, true);
Specifically, ensure isDeepCopy
parameter usage is consistent and aligned with the intention behind cloning or referencing ActionSet
objects. This change ensures that whenever we're merging privileges from one collection to another (merge
method), we're consciously deciding whether to clone (isDeepCopy = true
) or merely reference (isDeepCopy = false
) the ActionSet
, preventing accidental state manipulation across different privilege entries.
034d314
to
bf9f4ed
Compare
…ge dropped. Signed-off-by: edwinhzhang <[email protected]>
…ge dropped. Signed-off-by: edwinhzhang <[email protected]>
dde1190
to
74baba2
Compare
Quality Gate passedIssues Measures |
[BE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[FE Incremental Coverage Report]✅ pass : 2 / 2 (100.00%) file detail
|
hi @nshangyiming plz reivew the code |
@Mergifyio backport branch-3.3 |
@Mergifyio backport branch-3.2 |
@Mergifyio backport branch-3.1 |
@Mergifyio backport branch-3.0 |
@Mergifyio backport branch-2.5 |
✅ Backports have been created
|
✅ Backports have been created
|
✅ Backports have been created
|
✅ Backports have been created
|
✅ Backports have been created
|
…e dropped. (#46411) Signed-off-by: edwinhzhang <[email protected]> (cherry picked from commit 2391dd9)
…e dropped. (#46411) Signed-off-by: edwinhzhang <[email protected]> (cherry picked from commit 2391dd9)
…e dropped. (#46411) Signed-off-by: edwinhzhang <[email protected]> (cherry picked from commit 2391dd9)
…e dropped. (#46411) Signed-off-by: edwinhzhang <[email protected]> (cherry picked from commit 2391dd9)
…e dropped. (#46411) Signed-off-by: edwinhzhang <[email protected]> (cherry picked from commit 2391dd9) # Conflicts: # fe/fe-core/src/main/java/com/starrocks/privilege/PrivilegeCollection.java
…e dropped. (backport #46411) (#46566) Co-authored-by: zhanghe <[email protected]>
…e dropped. (backport #46411) (#46568) Co-authored-by: zhanghe <[email protected]>
…e dropped. (backport #46411) (#46569) Co-authored-by: zhanghe <[email protected]>
…e dropped. (backport #46411) (#46567) Co-authored-by: zhanghe <[email protected]>
Why I'm doing:
What I'm doing:
Fixes #issue
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: