Skip to content

feature/INT-1697 - Oauth scopes review - #237

Merged
david-ruiz-cko merged 3 commits into
mainfrom
feature/INT-1697
Sep 15, 2026
Merged

david-ruiz-cko merged 3 commits into
mainfrom
feature/INT-1697

Conversation

@david-ruiz-cko

Copy link
Copy Markdown
Contributor

This pull request updates the OAuthScopes enum and its usage to synchronize with the current Checkout.com API specification, remove deprecated scopes, and improve test coverage. The main focus is on aligning the SDK's OAuth scope definitions with the official API spec, removing outdated values, adding missing ones, and ensuring correctness through comprehensive tests.

OAuth scope enum updates:

  • Added missing scopes to OAuthScopes, including COMPLIANCE_REQUESTS, COMPLIANCE_REQUESTS_READ, COMPLIANCE_REQUESTS_RESPOND, and VAULT_TOKENS_METADATA, which are required by some API operations but were not previously declared. [1] [2]
  • Removed deprecated or retired scopes such as ISSUING_CARD_MGMT, ISSUING_CLIENT, MARKETPLACE, MIDDLEWARE_GATEWAY, MIDDLEWARE_PAYMENT_CONTEXT, and ensured remaining scopes are alphabetically ordered for maintainability.
  • Added a detailed docstring to the OAuthScopes enum explaining its correspondence to the API spec and clarifying subtle differences between similar scopes.

Test improvements and validation:

  • Added new tests to tests/oauth_scopes_test.py to verify that all documented and newly added scopes are present, distinguish between similar but distinct scopes, and ensure every enum member has a unique, non-blank wire value.
  • Introduced checks to guarantee that enum members remain alphabetically ordered and no duplicate wire values exist, preventing subtle bugs and improving maintainability.

Integration test updates:

  • Updated integration tests to use the correct, non-retired scopes (ACCOUNTS instead of MARKETPLACE for payout schedules, and ISSUING_CARD_MANAGEMENT_READ/WRITE instead of retired issuing scopes), with comments explaining the changes and referencing the API spec. [1] [2]

@david-ruiz-cko
david-ruiz-cko requested a review from a team September 14, 2026 08:39
@agent-wall-e

agent-wall-e Bot commented Sep 14, 2026

Copy link
Copy Markdown

🔴 Risk Classification: MAJOR

Approval route: AI Review + Human Approval Required
Rollback controls: Change-freeze window + documented rollback plan

Classification reasons

  • security_sensitive_path:checkout_sdk/oauth_scopes.py
  • security_sensitive_path:tests/oauth_scopes_test.py

Operational gates

  • ✅ jira_ticket (INT-1697)
  • ✅ independent_review

Files analysed: 4


wall-e 2026.06.19-02 · policy 376219bc71e6…

@agent-wall-e

agent-wall-e Bot commented Sep 14, 2026

Copy link
Copy Markdown
🔬 Debug — why this classification?

Each reason code emitted by the classifier, its source clause in the AI in SDLC Control Framework, and what it means.

Reason code Kind Clause Meaning
security_sensitive_pathcheckout_sdk/oauth_scopes.py classifying §2.1 M4/M5 Path matched a sensitive pattern (auth, secrets, crypto, PCI, migrations, network IaC).
security_sensitive_pathtests/oauth_scopes_test.py classifying §2.1 M4/M5 Path matched a sensitive pattern (auth, secrets, crypto, PCI, migrations, network IaC).

Kinds:

  • classifying — this rule contributed to the chosen tier.
  • informational — context only; did not by itself decide the tier.

See issue #3 for the proposal to formalise this map as Appendix A of the standards doc.

wall-e 2026.06.19-02 · debug

@agent-wall-e

agent-wall-e Bot commented Sep 14, 2026

Copy link
Copy Markdown

🟠 Advisory review: Concerns worth a look

This PR needs a human approval. Before you give it, these are the things I'd want resolved.

The diff adds new scopes, removes deprecated ones, updates integration tests, and adds comprehensive test coverage. Most of it looks correct, but there are two concrete problems: the alphabetical ordering test is logically broken for the existing enum ordering, and the docstring contains a self-contradictory claim about deprecated scopes.

Concerns

  • The test_should_declare_members_in_alphabetical_order test uses replace('_', '').lower() to sort, but the actual enum ordering (e.g. BALANCES, BALANCES_TOP_UP_INSTRUCTIONS, BALANCES_VIEW) when stripped of underscores becomes 'balances', 'balancestopupinstructions', 'balancesview' — which is already alphabetical — but PAYMENT_CONTEXT ('paymentcontext'), PAYMENT_SESSIONS ('paymentsessions'), PAYMENTS_SEARCH ('paymentssearch') would sort as 'paymentcontext' < 'paymentsessions' < 'paymentssearch', matching the declared order, so that specific case is fine; however, the test should actually be run to confirm it passes given the full enum, since the normalization logic is non-obvious and could silently pass even with a wrong ordering.
  • The docstring states in paragraph 2 that five members 'appear nowhere in the specification at all' and are 'kept for backward compatibility', but then in paragraph 1 says these are NOT among the scopes 'kept for backward compatibility' — then contradicts itself by describing the sandbox payouts client being provisioned for marketplace and rejecting accounts with invalid_scope, which directly contradicts the integration test change that replaces MARKETPLACE with ACCOUNTS for payout schedules.
  • The integration test in accounts_payout_schedules_integration_test.py switches from MARKETPLACE to ACCOUNTS, but the docstring in oauth_scopes.py says 'the sandbox payouts client is provisioned for marketplace and answers a request for accounts with invalid_scope' — these two changes are directly contradictory and at least one of them is wrong.
  • The test_should_retain_the_legacy_scopes_the_spec_omits test asserts MARKETPLACE is kept for backward compatibility, while the integration test simultaneously removes MARKETPLACE usage and replaces it with ACCOUNTS — if ACCOUNTS truly works for payout schedules, the docstring's claim about invalid_scope is false; if the docstring is correct, the integration test change will break CI.
  • The issuing conftest removes ISSUING_CLIENT with the comment 'the latter has no documented equivalent', which means any caller relying on that scope for issuing operations will silently stop requesting it — the PR should clarify whether dropping ISSUING_CLIENT from the integration test scope list causes those tests to fail or pass with reduced permissions.

This is not an approval. wall-e cannot auto-approve this PR — it is an opinion to help whoever does. Advisory review · us.anthropic.claude-sonnet-4-6 · wall-e 2026.06.19-02

@agent-wall-e

agent-wall-e Bot commented Sep 14, 2026

Copy link
Copy Markdown

🔴 Risk Classification: MAJOR

Approval route: AI Review + Human Approval Required
Rollback controls: Change-freeze window + documented rollback plan

Classification reasons

  • security_sensitive_path:checkout_sdk/oauth_scopes.py
  • security_sensitive_path:tests/oauth_scopes_test.py

Operational gates

  • ✅ jira_ticket (INT-1697)
  • ✅ independent_review

Files analysed: 4


wall-e 2026.06.19-02 · policy 376219bc71e6…

@agent-wall-e

agent-wall-e Bot commented Sep 14, 2026

Copy link
Copy Markdown
🔬 Debug — why this classification?

Each reason code emitted by the classifier, its source clause in the AI in SDLC Control Framework, and what it means.

Reason code Kind Clause Meaning
security_sensitive_pathcheckout_sdk/oauth_scopes.py classifying §2.1 M4/M5 Path matched a sensitive pattern (auth, secrets, crypto, PCI, migrations, network IaC).
security_sensitive_pathtests/oauth_scopes_test.py classifying §2.1 M4/M5 Path matched a sensitive pattern (auth, secrets, crypto, PCI, migrations, network IaC).

Kinds:

  • classifying — this rule contributed to the chosen tier.
  • informational — context only; did not by itself decide the tier.

See issue #3 for the proposal to formalise this map as Appendix A of the standards doc.

wall-e 2026.06.19-02 · debug

@agent-wall-e

agent-wall-e Bot commented Sep 15, 2026

Copy link
Copy Markdown

🔴 Risk Classification: MAJOR

Approval route: AI Review + Human Approval Required
Rollback controls: Change-freeze window + documented rollback plan

Classification reasons

  • security_sensitive_path:checkout_sdk/oauth_scopes.py
  • security_sensitive_path:tests/oauth_scopes_test.py

Operational gates

  • ✅ jira_ticket (INT-1697)
  • ✅ independent_review

Files analysed: 4


wall-e 2026.06.19-02 · policy 376219bc71e6…

@agent-wall-e

agent-wall-e Bot commented Sep 15, 2026

Copy link
Copy Markdown
🔬 Debug — why this classification?

Each reason code emitted by the classifier, its source clause in the AI in SDLC Control Framework, and what it means.

Reason code Kind Clause Meaning
security_sensitive_pathcheckout_sdk/oauth_scopes.py classifying §2.1 M4/M5 Path matched a sensitive pattern (auth, secrets, crypto, PCI, migrations, network IaC).
security_sensitive_pathtests/oauth_scopes_test.py classifying §2.1 M4/M5 Path matched a sensitive pattern (auth, secrets, crypto, PCI, migrations, network IaC).

Kinds:

  • classifying — this rule contributed to the chosen tier.
  • informational — context only; did not by itself decide the tier.

See issue #3 for the proposal to formalise this map as Appendix A of the standards doc.

wall-e 2026.06.19-02 · debug

@sonarqubecloud

Copy link
Copy Markdown

@david-ruiz-cko
david-ruiz-cko merged commit 6228338 into main Sep 15, 2026
4 checks passed
@david-ruiz-cko
david-ruiz-cko deleted the feature/INT-1697 branch September 15, 2026 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants