Skip to content

feat: Integration test for verifying grant revocation#573

Open
konac-hamza wants to merge 3 commits intoopenstack-experimental:mainfrom
konac-hamza:feature/integration-test-grant-revocation
Open

feat: Integration test for verifying grant revocation#573
konac-hamza wants to merge 3 commits intoopenstack-experimental:mainfrom
konac-hamza:feature/integration-test-grant-revocation

Conversation

@konac-hamza
Copy link
Collaborator

@konac-hamza konac-hamza commented Feb 19, 2026

No description provided.

gtema
gtema requested changes Feb 20, 2026
View reviewed changes
tests/integration/src/assignment/grant/revoke.rs

use eyre::Result;
use tracing_test::traced_test;

Copy link
Collaborator

@gtema gtema Feb 20, 2026

Choose a reason for hiding this comment

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

please keep the spaces between import groups. First group is typically std (+ whatever is std nowadays like tracing, eyre), second group are the other foreign crates, 3rd are the own crates (i.e. when a project spreads across multiple crates, what in our case is going to be openstack_keystone_api_types, openstack_keystone_storage, etc). The last group are the imports from the current crate itself.

tests/integration/src/assignment/grant/revoke.rs Outdated
access_rules: None,
name: Uuid::new_v4().to_string(),
project_id: "project_a".into(),
roles: vec![Role {
Copy link
Collaborator

@gtema gtema Feb 20, 2026

Choose a reason for hiding this comment

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

you should be having multiple roles. Explanation further down.

tests/integration/src/assignment/grant/revoke.rs Outdated
.get_token_provider()
.encode_token(&post_revoke_token)?;

assert!(
Copy link
Collaborator

@gtema gtema Feb 20, 2026

Choose a reason for hiding this comment

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

intention with having multiple roles is to still have a working authentication just ensuring the revoked role is not present anymore.

tests/integration/src/assignment/grant/revoke.rs

// CHECK 3: existing auth (issued before revocation) is no longer accepted
assert!(
matches!(
Copy link
Collaborator

@gtema gtema Feb 20, 2026

Choose a reason for hiding this comment

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

this shows the real problem and is exactly the purpose of the test. The token must be considered as a revoked and not that the user is not having roles. In the combination with having more roles initially this would lead to the vulnerability.
I am not sure why exactly, but we should get the TokenProviderError::TokenRevoked error

tests/integration/src/assignment/grant.rs
use eyre::Report;
use sea_orm::{DbConn, entity::*};
use std::sync::Arc;

Copy link
Collaborator

@gtema gtema Feb 20, 2026

Choose a reason for hiding this comment

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

same comment regarding the imports sorting is applicable here.

@konac-hamza konac-hamza force-pushed the feature/integration-test-grant-revocation branch from eedc423 to 95f5696 Compare February 25, 2026 23:57
@goOdyaga
Copy link

goOdyaga commented Mar 10, 2026

@gtema
I thought that reason of the fails about the new implementations. So I didn't try to change anything in the PR. Do I need to change anything in the request:?

@gtema
Copy link
Collaborator

gtema commented Mar 10, 2026

I was looking at it yesterday and found few issues:

  • appcreds (as used in the test) need to keep original roles for the token revolution check, while dropping unavailable roles in the response. I addressed this yesterday in a different PR by introducing new method for calling roles on token. This requires PR rebase
  • token revolution works with a seconds precision (instead of microseconds) and that requires a 1second wait after revoking role before trying to auth again
  • sqlite (most likely sqlx driver) has some weird bug that token issued_at is compared wrongly. For MySQL/postgres works correct. I want to try to chase it down today

@gtema
Copy link
Collaborator

gtema commented Mar 10, 2026

it should be fine now to resolve the conflicts and update the test: there is a similar test verifying the revoke provider with the difference that it explicitly creates revocation event instead of revoking the concrete role and it does not verify the "auth" aspect like this test does

@konac-hamza konac-hamza force-pushed the feature/integration-test-grant-revocation branch from d5483ca to 8436997 Compare March 10, 2026 21:23
gtema
gtema requested changes Mar 11, 2026
View reviewed changes
tests/integration/src/assignment/grant/revoke.rs
.is_ok(),
"Token should be valid before revocation"
);
tokio::time::sleep(std::time::Duration::from_secs(1)).await;
Copy link
Collaborator

@gtema gtema Mar 11, 2026

Choose a reason for hiding this comment

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

that one is not necessary

tests/integration/src/assignment/grant/revoke.rs
!grant_exists(&state, &user.id, "project_a", "role_revoke_auth", true).await?,
"Grant should not exist after revocation"
);
tokio::time::sleep(std::time::Duration::from_secs(1)).await;
Copy link
Collaborator

@gtema gtema Mar 11, 2026

Choose a reason for hiding this comment

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

add a comment here, that since token revocation is working with a seconds precision we need to wait for a new second before granting new token to prevent it being also eventually revoked

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gtema gtema gtema requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@konac-hamza @goOdyaga @gtema