Skip to content

feat(auth): add configurable scope enforcement with scope_mode option#625

Open
gocamille wants to merge 7 commits intomainfrom
camille/allow-scope-matching
Open

feat(auth): add configurable scope enforcement with scope_mode option#625
gocamille wants to merge 7 commits intomainfrom
camille/allow-scope-matching

Conversation

@gocamille
Copy link
Collaborator

This PR introduces configurable scope enforcement with a new scope_mode config option. This replaces the previous "fail-closed" logic where a token required all configured scopes to pass validation.

The new scope_mode supports three strategies:

  • require_all (default): the token must have all configured scopes
  • require_any: token must have at least one of the configured scopes
  • disabled: this skips scope enforcement entirely

This change addresses requests for more flexible scope matching and explicit support for downstream authorization.

Changes

  • Added ScopeMode enum to auth.rs with Disabled, RequireAll, and RequireAny variants
  • Updated oauth_validate middleware to enforce logic based on the selected mode
  • Added startup warning when scopes are configured but scope_mode is disabled

@gocamille gocamille requested review from a team as code owners February 5, 2026 22:26
@apollo-librarian
Copy link

apollo-librarian bot commented Feb 5, 2026

✅ Docs preview ready

The preview is ready to be viewed. View the preview

File Changes

0 new, 2 changed, 0 removed
* (developer-tools)/apollo-mcp-server/(latest)/auth.mdx
* (developer-tools)/apollo-mcp-server/(latest)/config-file.mdx

Build ID: fcc2076227452105dc02cddf
Build Logs: View logs

URL: https://www.apollographql.com/docs/deploy-preview/fcc2076227452105dc02cddf

@github-actions
Copy link

github-actions bot commented Feb 5, 2026

Changeset file added - thank you!

ScopeMode::Disabled => true,
ScopeMode::RequireAll => required.iter().all(|req| present.contains(req)),
ScopeMode::RequireAny => required.iter().any(|req| present.contains(req)),
}
Copy link

Choose a reason for hiding this comment

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

[Consider] In the test helper is_sufficient at lines 450-455, the empty scopes case for RequireAny returns false, which is inconsistent with the behavior for RequireAll and Disabled.

This is tested in scope_mode_empty_scopes_is_sufficient_for_all_modes (line 485), which explicitly expects RequireAny to return false when required scopes are empty. However, this seems counterintuitive - if no scopes are required, shouldn't any token be considered sufficient?

Consider whether the current behavior is intentional or if RequireAny with empty required scopes should return true (similar to RequireAll). If the current behavior is intentional, add a comment explaining why RequireAny with empty scopes fails while RequireAll succeeds.

See Chapter 1.6 for guidance on clarifying non-obvious behavior with comments.

);
tracing::Span::current().record("reason", "insufficient_scope");
tracing::Span::current().record("status_code", StatusCode::FORBIDDEN.as_u16());
// NOTE: WWW-Authenticate lists all configured scopes per RFC 6750.
Copy link

Choose a reason for hiding this comment

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

[Consider] The WWW-Authenticate header comment at lines 327-330 explains that the header lists all configured scopes even in require_any mode, which may be confusing to clients.

Per RFC 6750 section 3, the scope attribute in WWW-Authenticate indicates the required scope of the access token. When scope_mode is require_any, listing all scopes suggests the token needs all of them, which is misleading.

Consider one of these approaches:

  1. For require_any mode, only include the scopes that are actually missing (none of the configured scopes are present)
  2. Add a custom parameter to the header (e.g., scope_mode="require_any") to clarify the matching strategy
  3. If maintaining current behavior, ensure this is clearly documented in the auth.mdx file

The current implementation is functionally correct but may cause confusion for API consumers debugging 403 responses.

@claude
Copy link

claude bot commented Feb 5, 2026

Review Summary

This PR adds configurable scope enforcement via the new scope_mode option, replacing the previous fail-closed behavior. The implementation is clean, well-tested, and follows Rust idioms.

Findings

  • [Consider] Empty scopes edge case: The test at auth.rs:485 shows that RequireAny with empty required scopes returns false, while RequireAll returns true. This asymmetry may be intentional but deserves clarification.
  • [Consider] WWW-Authenticate header semantics: In require_any mode, the header lists all configured scopes, which may be confusing since only one is needed. Consider documenting this clearly or adjusting the header content.

Test Coverage Assessment

Test coverage is comprehensive:

  • All three scope modes have dedicated tests
  • Edge cases covered: empty scopes, no token scopes, partial matches, supersets
  • YAML deserialization tests ensure config parsing works correctly
  • The test helper is_sufficient consolidates scope validation logic across all tests

The tests follow Apollo's best practices from Chapter 5:

  • Descriptive test names that explain behavior
  • One assertion per test (mostly - a few could be split further)
  • Good use of test modules for organization

Final Recommendation

Approve with suggestions - The implementation is solid and ready to merge. The two "Consider" items are minor edge cases around clarity and documentation rather than correctness issues.


Reviewed by Claude Code Sonnet 4.5

@gocamille gocamille changed the title feat(auth): add configurable scope enforcement with scope_mode option feat(auth): add configurable scope enforcement with scope_mode option Feb 5, 2026
Copy link

@pragl pragl left a comment

Choose a reason for hiding this comment

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

Minor recommendations to align with our style guide. Otherwise, LGTM!

gocamille and others added 4 commits February 6, 2026 14:17
Co-authored-by: Parker <parker.ragland@apollographql.com>
Co-authored-by: Parker <parker.ragland@apollographql.com>
Co-authored-by: Parker <parker.ragland@apollographql.com>
Co-authored-by: Parker <parker.ragland@apollographql.com>
@claude
Copy link

claude bot commented Feb 6, 2026

Follow-up Review

The documentation improvements look good. The PR is ready to merge - the implementation is solid and test coverage is comprehensive.

The two [Consider] items from my previous review remain valid but are minor clarifications rather than blocking issues:

  1. Empty scopes edge case behavior in RequireAny mode (auth.rs:566)
  2. WWW-Authenticate header semantics in require_any mode (auth.rs:330-332)

Both are well-documented in the code, so these are optional enhancements rather than required changes.


Reviewed by Claude Code Sonnet 4.5

@claude
Copy link

claude bot commented Feb 6, 2026

Follow-up Review

The documentation improvements look excellent - they clearly explain the three scope modes with concrete examples and appropriate caution notes.

The implementation remains solid with comprehensive test coverage. The two [Consider] items from my previous review are minor clarifications rather than blocking issues:

  1. Empty scopes edge case (auth.rs:566): RequireAny with empty required scopes returns false, while RequireAll returns true. This is intentional behavior but the asymmetry could be documented.
  2. WWW-Authenticate header in require_any mode (auth.rs:330-332): The header lists all configured scopes even though only one is needed. The inline comment addresses this well.

Both items are optional enhancements - the current implementation is correct and ready to merge.

Final Recommendation

Approve - Ready to merge.


Reviewed by Claude Code Sonnet 4.5

Copy link
Member

@DaleSeo DaleSeo left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just a typo in the changelog.

…o_suport_flexible_matching_strategies_require_any_disabled.md

Co-authored-by: Dale Seo <5466341+DaleSeo@users.noreply.github.com>
@claude
Copy link

claude bot commented Feb 6, 2026

Final Review

This PR successfully implements configurable scope enforcement via the scope_mode option. The implementation is clean, well-tested, and ready to merge.

Summary

The changes replace the previous fail-closed behavior (requiring all scopes) with three flexible modes:

  • require_all (default): Token must have all configured scopes
  • require_any: Token must have at least one configured scope
  • disabled: Skip scope enforcement entirely

Implementation Quality

Strengths:

  • Clean enum-based design with sensible defaults
  • Comprehensive test coverage (15+ dedicated tests)
  • Clear documentation with examples and security warnings
  • Proper logging and diagnostics
  • Backward compatible (default behavior unchanged)

Test Coverage:

  • All three scope modes thoroughly tested
  • Edge cases covered: empty scopes, partial matches, supersets, no token scopes
  • YAML deserialization validated
  • Test helper is_sufficient consolidates validation logic effectively

Notes from Previous Reviews

The two [Consider] items I mentioned earlier remain valid but are minor clarifications:

  1. Empty scopes edge case (auth.rs:566): RequireAny with empty required scopes returns false, while RequireAll returns true. This asymmetry is intentional - any of an empty set is vacuously false, while all of an empty set is vacuously true. The behavior is mathematically correct.

  2. WWW-Authenticate header semantics (auth.rs:330-332): In require_any mode, the header lists all configured scopes even though only one is needed. The inline comment addresses this well, noting it matches RFC 6750 behavior.

Both items are optional enhancements - the current implementation is correct.

Final Recommendation

Approve - Ready to merge. The implementation follows Apollo's Rust best practices, has excellent test coverage, and the documentation clearly explains the security implications of each mode.


Reviewed by Claude Code Sonnet 4.5

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants