Skip to content

Conversation

@haasonsaas
Copy link
Contributor

@haasonsaas haasonsaas commented Oct 19, 2025

Summary

  • ensure cache scope validation accepts organization-wide scopes when repository-specific scopes are absent

Testing

  • uv run pytest (fails: missing optional dependencies such as httpx, yaml, structlog, fastapi, redis, pydantic, hypothesis during collection)

https://chatgpt.com/codex/tasks/task_e_68f52e796df48326a86cf7159d30a4d9

Greptile Overview

Updated On: 2025-10-31 05:45:19 UTC

Greptile Summary

Reordered the cache scope validation logic in validate_cache_scope to prioritize organization-wide scopes over repository-specific scopes, enabling tokens with org-level permissions (e.g., pull:org-1) to access any repository within their organization.

Key changes:

  • Logic reordering in security.py:175-209: Moved org-wide scope check (pull:org-{id}) to execute before repo-specific scope check, fixing the previous issue where org-wide tokens were incorrectly denied when a repository parameter was provided
  • Test alignment: Updated test_manifest_roundtrip to verify org-wide scoped tokens can access repositories without requiring repo-specific scope suffixes
  • Test infrastructure: Added saml2 module mocks to conftest.py to prevent import errors during test collection

Permission hierarchy verified:

  • Legacy scopes (read_write, read, write) continue to work as before
  • Org-wide scopes (e.g., pull:org-1) grant access to all repos within the organization
  • Repo-specific scopes (e.g., pull:org-1/demo) still restrict access to individual repos
  • Org boundary enforcement remains intact via repository prefix validation

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The logic change is well-reasoned and correctly implements a permission hierarchy where org-wide scopes grant broader access than repo-specific scopes. The change maintains backward compatibility with legacy scopes and existing repo-specific tokens. Organization boundary enforcement remains intact through the separate repository prefix validation. Tests have been updated to verify the new behavior, and existing tests (like test_validate_cache_scope_repo) continue to pass with the new logic.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
src/nimbus/common/security.py 5/5 Reordered cache scope validation to check org-wide scopes before repo-specific scopes, enabling org-level tokens to access any repo within their organization
tests/test_docker_cache.py 5/5 Updated test expectations to verify org-wide scope tokens can successfully access repositories without repo-specific scope suffixes
tests/conftest.py 5/5 Added mock saml2 modules to prevent import errors during test collection when optional SAML dependencies are not installed

Sequence Diagram

sequenceDiagram
    participant Client
    participant DockerCacheAPI
    participant validate_repository_access
    participant validate_cache_scope

    Client->>DockerCacheAPI: GET /v2/org-1/demo/manifests/latest
    Note over Client,DockerCacheAPI: Token has scope: pull:org-1,push:org-1
    
    DockerCacheAPI->>validate_repository_access: Check access for org-1/demo
    validate_repository_access->>validate_repository_access: Verify org prefix matches token org_id
    Note over validate_repository_access: Extract repo_suffix = "demo"
    
    validate_repository_access->>validate_cache_scope: validate(operation="pull", org_id=1, repo="demo")
    
    Note over validate_cache_scope: Step 1: Check legacy scopes
    validate_cache_scope->>validate_cache_scope: read_write? read? write?
    
    Note over validate_cache_scope: Step 2: Check org-wide scope (NEW)
    validate_cache_scope->>validate_cache_scope: Is "pull:org-1" in scopes?
    Note over validate_cache_scope: Match found!
    
    validate_cache_scope-->>validate_repository_access: True
    validate_repository_access-->>DockerCacheAPI: Access granted
    DockerCacheAPI-->>Client: 200 OK
Loading

@haasonsaas
Copy link
Contributor Author

Verification

  • Ran uv run python -m pytest --cov=src --cov-report=term-missing --cov-fail-under=65
  • Result: 292 passed, 3 skipped, coverage 66%

Behavior change

  • Before this change, validate_cache_scope immediately looked for repo-specific scopes whenever a repository suffix was supplied. Tokens scoped only at the org level (e.g. pull:org-1) were rejected for repo operations because the function returned early.
  • We now treat org-wide scopes as sufficient for repository access when no repo-specific scope is present, and only fall back to repo-specific scopes if the broader scope is absent.

Why org-level scopes differ

  • Org-level scopes are meant to allow access across all repositories in the org. Unlike repo-specific scopes, they intentionally omit the repository suffix and should still satisfy repo operations.
  • The cache server still prefers repo-specific scopes when provided, but org-wide scopes serve as a higher-level permission that should not require duplicating every repo suffix.

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

3 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants