Feature/system assignment group - #796
konac-hamza wants to merge 1 commit into
Conversation
9289461 to
b3a55d7
Compare
|
@konac-hamza are you still working on the PR? |
Because of you closed the sub-issue, I am not working. Should I complete the PR? |
|
ah, there was an accidental duplication, issue #755 describes what is still missing for system assignments. So yes, please, let us complete system role assignments to users/groups |
I will start the PR as soon as possible. Thanks for your notify. |
b3a55d7 to
0810e33
Compare
0810e33 to
9f3d930
Compare
Signed-off-by: Hamza Konac <hamza.konac@tubitak.gov.tr>
9f3d930 to
0cb661b
Compare
gtema
left a comment
gtema
left a comment
There was a problem hiding this comment.
the formatting of the code is very broken
gtema
left a comment
gtema
left a comment
There was a problem hiding this comment.
oh sorry, this is the new styling in github that literally make impression of empty lines between every change line
gtema
left a comment
gtema
left a comment
There was a problem hiding this comment.
Automated review (caveman-review). Core change — guarding the group-grant revocation event — is sound and matches Python Keystone's post-#1662514 behavior. Main issues: (1) the revoke_by_id=true path re-introduces the over-broad revocation the Python bug fix removed and is not equivalent to Python's option of the same name; (2) the change silently alters revoke behavior for the existing GroupDomain/GroupProject paths; (3) no unit test for the new branch; plus a duplicated license header in list.rs and assorted nits. Details inline.
| //! System group role: list. | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
| // | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
There was a problem hiding this comment.
Duplicate Apache license header and //! System group role: list. doc comment (lines 15-29 repeat lines 1-13 + 15). Delete lines 15-29.
| let group = group?.ok_or_else(|| { | ||
| info!("Group {} was not found", group_id); | ||
| KeystoneApiError::NotFound { | ||
| resource: "group".into(), |
There was a problem hiding this comment.
resource: "group" here, but grant/check/revoke in this PR all use resource: "grant" for the same not-found case. Pick one.
identifier: "" (all four handlers) drops the id from the 404 body and logs — pass group_id / role_id.
| let roles: Vec<Role> = assignments | ||
| .into_iter() | ||
| .map(|a| a.try_into()) | ||
| .collect::<Result<std::collections::HashSet<_>, _>>()? |
There was a problem hiding this comment.
HashSet<Role> → Vec makes the role order in the response non-deterministic across calls. Collect, then sort by id before returning for a stable response.
| tag="role_assignments" | ||
| )] | ||
| #[tracing::instrument( | ||
| name = "api::system_group_role_list", |
There was a problem hiding this comment.
nit: instrument name api::system_group_role_list (also _check) vs api::v3::system_group_role_grant / _revoke. Unify the prefix.
| let query_params = RoleAssignmentListParameters { | ||
| group_id: Some(group_id.clone()), | ||
| system_id: Some("system".into()), | ||
| effective: Some(true), |
There was a problem hiding this comment.
q: effective: Some(true) here vs Some(false) in list.rs. Group effective-expansion is the known-unimplemented path (list_users_in_group), so effective=true is a no-op for a group actor today. Align to false or add a comment explaining why check differs.
There was a problem hiding this comment.
I followed the system user assignment handlers where check.rs uses Some(true) and list.rs uses Some(false). Since effective expansion for groups is currently a no-op (as you noted), aligning both to Some(false) would be more accurate. I'll update check.rs to use Some(false) and add a comment.
| ) | ||
| .await?; | ||
|
|
||
| let grant = AssignmentBuilder::default() |
There was a problem hiding this comment.
No pre-revoke existence check. Python delete_grant calls check_grant_role_id first and raises RoleAssignmentNotFound (404) when the group holds no such grant (keystone/assignment/core.py, test_backends.py assert_role_assignment_not_found_exception). Here the handler fetches group + role, enforces policy, then calls revoke_grant unconditionally → 204 for a no-op revoke. Diverges from Python and from REST norms. Confirm whether the existing system/user revoke handler has the same gap before sizing the fix.
There was a problem hiding this comment.
I looked Python and read methods which you mentioned. In Rust there is no provider method that check assignment directly, so should I check them like how check.rs handle or implement provider method? also Should I fix gap in system/user revoke handler?
There was a problem hiding this comment.
check from api is implemented as a narrow list and filter. I guess it would make sense to implement a new shortcut in the provider, so yes, please create the dedicated check method which works like current check API methods
| // Only create revocation event for group assignments if revoke_by_id is enabled. | ||
| // By default group revocations do not create revocation events since token | ||
| // validation rebuilds assignments at validation time. | ||
| // Reference: Python Keystone bug #1662514 | ||
| let is_group_assignment = matches!( | ||
| &grant.r#type, | ||
| AssignmentType::GroupDomain | ||
| | AssignmentType::GroupProject | ||
| | AssignmentType::GroupSystem | ||
| ); | ||
|
|
||
| let revoke_by_id = ctx | ||
| .state() | ||
| .config_manager | ||
| .config | ||
| .read() | ||
| .await | ||
| .token | ||
| .revoke_by_id; | ||
| if !is_group_assignment || revoke_by_id { | ||
| // ADR 0034 §4: the central revocation event stays on the global revoke | ||
| // provider, unrouted — it is not an assignment-backend operation. | ||
| ctx.state() | ||
| .provider | ||
| .get_revoke_provider() | ||
| .create_revocation_event(ctx, revocation_event) | ||
| .await?; | ||
| // ADR 0031 "Tokens": revoking a grant cascades revocation of every | ||
| // token carrying that role - `"cascade"`, not a direct user request. | ||
| crate::token::TOKEN_METRICS.revoked_total.inc(["cascade"]); | ||
| } |
There was a problem hiding this comment.
Cross-checked against Python Keystone (opendev.org/openstack/keystone @ 5f1d13f38), bug #1662514, fix 9a4391c49.
The diagnosis is correct: Rust currently builds an over-broad revoke event for group grants (RevocationEventCreate { user_id: None, role_id: Some, project/domain } at L802) — exactly the pre-2017 Python bug. Guarding it is the right fix, and validation-time recomputation makes it safe: calculate_effective_roles (auth.rs:666) runs every validation and the SQL driver expands user_id → list_groups_of_user → group actors for domain/project/system scope (assignment-driver-sql/src/lib.rs:289). The PR's "known limitation" is the reverse direction (group → members) and does not affect token validation.
Three problems:
-
🔴
revoke_by_id=trueis not equivalent to Python'srevoke_by_id=True— it re-introduces the bug. Modern Python gates only_invalidate_token_cache(...)on the flag: a blanket, non-destructive cache flush (still-valid tokens survive;test_v3_auth.pytoken3now expectsOK). Python never re-persists the broad(scope, role)event, flag on or off. Thistruebranch callscreate_revocation_eventwithuser_id: None→ a hard revocation viais_token_revokedthat permanently kills every token carrying that role on that scope, including users with a direct assignment. Enabling the flag = "revert #1662514". Fix: resolve group members and emit per-user-scoped events, or rename/document the flag as "restores broad scope+role revocation for group grants; NOT equivalent to Python[token] revoke_by_id". -
Scope creep.
is_group_assignmentalso coversGroupDomain | GroupProject, changing revoke behavior for pre-existing endpoints, not just the new system/group one. Call this out in the PR description and in the security-model / ADR 0031 — revocation is a MUST-READ area. -
No test for the new branch.
test_revoke_grant(L448) only exercisesUserProject. Add:GroupSystem+revoke_by_id=false→create_revocation_eventnot called;GroupProjectsame; any group +revoke_by_id=true→ event created;UserProjectunchanged.
| | AssignmentType::GroupSystem | ||
| ); | ||
|
|
||
| let revoke_by_id = ctx |
There was a problem hiding this comment.
Reading config.read().await.token.revoke_by_id per revoke is fine. But note the default (false, config/token.rs) is the opposite of Python's (keystone/conf/token.py:82 → default=True). false is the right call for Rust since the true path is the buggy one — but the PR description's "(default: false)" claim about Python is wrong; state the real reason Rust diverges.
| /// Controls whether token revocation is enabled for group role | ||
| /// assignments. When disabled (default), group role revocations do not | ||
| /// create revocation events since token validation rebuilds assignments | ||
| /// at validation time. Enabling this creates revocation events for group | ||
| /// assignments but may cause overly broad token invalidation. | ||
| /// Matches Python Keystone's [token] revoke_by_id option. | ||
| #[serde(default = "default_revoke_by_id")] | ||
| pub revoke_by_id: bool, |
There was a problem hiding this comment.
"Matches Python Keystone's [token] revoke_by_id option" is misleading:
- Python default is
True, notfalse. - Python's flag gates a non-destructive token-cache flush; it never persists the broad
(scope, role)revocation event. Hererevoke_by_id=truere-enables exactly that broad event (see service.rs comment). The Rust flag means "restore broad scope+role revocation for group grants", which is the pre-#1662514 behavior, not Python parity.
Also: this option is not surfaced in the sample / reference config. Add it next to [token] expiration with help text that describes the actual Rust behavior.
| @@ -0,0 +1,29 @@ | |||
| # METADATA | |||
| # description: Policy for checking group roles on system | |||
| package identity.system.group.role.check | |||
There was a problem hiding this comment.
These new policy files use 8-space indentation; the rest of policy/ uses tabs. check_test.rego, grant.rego, list.rego, revoke.rego also lack a trailing newline. opa fmt --diff in pre-commit will fail. Run opa fmt -w policy/resource/system/group/.
2 participants
#757 System group role assignment API
This PR covers CRUD operations for system role assignments of group actors.
When listing roles for a system group, the API was returning all system role assignments in the database instead of only those belonging to the requested group. The root cause was that
group_idwas never added to the actors filter inlist_assignments, whileuser_idwas handled correctly. With an empty actors list, the database query had no filter and returned every system assignment. The fix was addinggroup_idto the actors list the same wayuser_idis handled.When revoking a group role assignment, the implementation was creating a revocation event with only
role_idset and nouser_id. This caused all tokens holding that role to be invalidated, not just tokens of users in the revoked group. This matches Python Keystone bug #1662514, which was fixed by only creating revocation events for group assignments whenrevoke_by_id = trueis set in config (default:false). The fix addsrevoke_by_idto the token config and skips revocation events for group assignments by default.Known Limitations
Effective role listing with
group_idandeffective=trueis not yet fully implemented. Python Keystone handles this case by expanding the group into its members vialist_users_in_groupand including their direct role assignments in the result. The Rust identity backend does not yet implementlist_users_in_group, so this expansion is currently missing. This will be tracked and implemented separately.