Revoke token auth error

[vijaysawant]

Problem Statement

Revoking registry access token doesn't revoke client's access to registry repos.
https://issues.redhat.com/browse/SAT-38785

Solution

Upstream katello PR Katello/katello#11625
Newly added test will check the behaviour after token revoke

Related Issues

PRT test Cases example

trigger: test-robottelo
pytest: tests/foreman/cli/test_container_management.py -k 'test_positive_revoke_registry_token_prevents_access'

Summary by Sourcery

Tests:

• Add CLI container management test that verifies revoking a registry personal access token prevents podman pushes until the user logs out and logs back in.

[sourcery-ai]

Reviewer's Guide

Adds an end-to-end podman-based CLI test to verify that revoking a registry personal access token prevents further image pushes until the user logs out and logs back in, ensuring the regression in SAT-38785 is covered.

File-Level Changes

Change	Details	Files
Add an integration test that validates registry token revocation blocks podman pushes until a fresh login occurs.	Create a new organization and product with lowercase names and register cleanup finalizer to log out, remove images, and delete created resources. Log into the Satellite podman registry with admin credentials, pull a minimal Fedora image from an external registry, and derive its image ID. Tag and push the image to the Satellite registry using an org/product/repo-based registry path, asserting the initial push succeeds. Locate the admin user via CLI and revoke their registry personal access token using the user access_token revoke action. Tag the image with a new tag and assert that pushing fails with an authentication-related error after token revocation. Log out of the registry, then log back in with admin credentials and assert that pushing the newly tagged image now succeeds.	tests/foreman/cli/test_container_management.py

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[vijaysawant]

trigger: test-robottelo
pytest: tests/foreman/cli/test_container_management.py -k 'test_positive_revoke_registry_token_prevents_access'

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• The cleanup finalizer is declared as a decorator with @request.addfinalizer, but addfinalizer expects a callable passed as an argument (e.g. define _cleanup and then call request.addfinalizer(_cleanup)), otherwise the cleanup may not run as intended.
• Consider capturing and asserting the result of target_sat.cli.User.access_token(..., action='revoke', ...) (e.g. checking exit status or response) so the test explicitly fails if the revoke operation itself does not succeed rather than only inferring it from later push behavior.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The cleanup finalizer is declared as a decorator with `@request.addfinalizer`, but `addfinalizer` expects a callable passed as an argument (e.g. define `_cleanup` and then call `request.addfinalizer(_cleanup)`), otherwise the cleanup may not run as intended.
- Consider capturing and asserting the result of `target_sat.cli.User.access_token(..., action='revoke', ...)` (e.g. checking exit status or response) so the test explicitly fails if the revoke operation itself does not succeed rather than only inferring it from later push behavior.

## Individual Comments

### Comment 1
<location> `tests/foreman/cli/test_container_management.py:597-606` </location>
<code_context>
+        )
+        assert result.status == 0, f'Failed to login to registry: {result.stderr}'
+
+        # Pull a small test image from external registry
+        external_image = 'registry.fedoraproject.org/fedora-minimal:latest'
+        result = target_sat.execute(f'podman pull {external_image}')
+        assert result.status == 0, f'Failed to pull external image: {result.stderr}'
+
+        # Get the image ID
+        result = target_sat.execute('podman images fedora-minimal -q')
+        assert result.status == 0
+        image_id = result.stdout.strip().split('\n')[0]
+        assert image_id, 'Failed to get image ID'
+
+        # Tag image for Satellite registry
</code_context>

<issue_to_address>
**suggestion (testing):** Pulling from an external registry can introduce flakiness; consider handling transient network issues or documenting expectations.

Since this test depends on `registry.fedoraproject.org` being reachable, failures may come from external outages rather than regressions in token handling. If possible, consider adding a retry around `podman pull` or marking the test skip/xfail when the registry is unavailable (e.g., based on the error). Otherwise, a brief comment noting that this external dependency is intentional would help future readers.

Suggested implementation:

```python
        # Pull a small test image from external registry.
        # Note: This depends on registry.fedoraproject.org being reachable; we retry
        # to mitigate transient network issues / external outages rather than
        # treating them as regressions in Satellite token handling.
        external_image = 'registry.fedoraproject.org/fedora-minimal:latest'
        max_pull_attempts = 3
        for attempt in range(1, max_pull_attempts + 1):
            result = target_sat.execute(f'podman pull {external_image}')
            if result.status == 0:
                break
            if attempt == max_pull_attempts:
                assert False, (
                    f'Failed to pull external image after {max_pull_attempts} attempts; '
                    'this may be due to transient network issues or the external '
                    f'registry being unavailable: {result.stderr}'
                )
            time.sleep(5)

```

To fully apply this change, you also need to:
1. Ensure `import time` is present at the top of `tests/foreman/cli/test_container_management.py`. If it's not there yet, add it alongside the other standard library imports.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[Satellite-QE]

PRT Result

Build Number: 14249
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/cli/test_container_management.py -k test_positive_revoke_registry_token_prevents_access --external-logging
Test Result : =========== 1 passed, 8 deselected, 6 warnings in 853.85s (0:14:13) ============

[qcjames53]

This looks pretty solid to me, Vijay. The description is spot on and the reproducer steps make sense to me. I had two small changes requested. Would you also mind addressing the failing automation? Can we ignore CI code quality because it's known to be broken?

[vsedmik]

One question regarding the testing steps

[vijaysawant]

trigger: test-robottelo
pytest: tests/foreman/cli/test_container_management.py -k 'test_positive_revoke_registry_token_prevents_access'

[Satellite-QE]

PRT Result

Build Number: 14278
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/cli/test_container_management.py -k test_positive_revoke_registry_token_prevents_access --external-logging
Test Result : =========== 1 passed, 8 deselected, 6 warnings in 874.87s (0:14:34) ============

[jnagare-redhat]

LGTM

[amolpati30]

Could you please squash the commits or drop the ones that are not required?

[Gauravtalreja1]

Same here, use podman_logout()

[vijaysawant]

target_sat.podman_login(
    username=settings.server.admin_username,
    password=settings.server.admin_password,
    registry=target_sat.hostname)

target_sat.podman_logout(registry=target_sat.hostname)

methods podman_login() and podman_logout() exist, but they're designed for a different use case (IOP/Red Hat cloud registry)
Since the auth file approach might cache credentials differently (which could affect the token revocation test), I would keep the direct execute() approach for this specific test.

[Gauravtalreja1]

Ah, but I don't think it would work differently when using tokens, and for using this methods, this can be generalised to cover such use cases in future

[vijaysawant]

trigger: test-robottelo
pytest: tests/foreman/cli/test_container_management.py -k 'test_positive_revoke_registry_token_prevents_access'

[vijaysawant]

trigger: test-robottelo
pytest: tests/foreman/cli/test_container_management.py -k 'test_positive_revoke_registry_token_prevents_access'

[Satellite-QE]

PRT Result

Build Number: 14310
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/cli/test_container_management.py -k test_positive_revoke_registry_token_prevents_access --external-logging
Test Result : =========== 1 passed, 8 deselected, 6 warnings in 764.99s (0:12:44) ============

[vsedmik]

Looks good to me! One tiny proposal to make it even nicer:

[vijaysawant]

trigger: test-robottelo
pytest: tests/foreman/cli/test_container_management.py -k 'test_positive_revoke_registry_token_prevents_access'

[Satellite-QE]

PRT Result

Build Number: 14311
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/cli/test_container_management.py -k test_positive_revoke_registry_token_prevents_access --external-logging
Test Result : ========== 1 passed, 8 deselected, 10 warnings in 1298.63s (0:21:38) ===========

[vsedmik]

LGTM