Skip to content

Fix Session::isValid() to use inclusion-based scope validation#463

Open
rafaelstz wants to merge 1 commit into
Shopify:mainfrom
rafaelstz:feature/additional-scope
Open

Fix Session::isValid() to use inclusion-based scope validation#463
rafaelstz wants to merge 1 commit into
Shopify:mainfrom
rafaelstz:feature/additional-scope

Conversation

@rafaelstz
Copy link
Copy Markdown

WHY are these changes introduced?

Session::isValid() currently validates scopes with exact equality:

Context::$SCOPES->equals($this->scope)

This causes Utils::loadOfflineSession($shop) to return null when a stored offline session has all required app scopes plus additional granted optional scopes. The access token is still fully usable for Admin API calls that only require the configured scopes — extra granted scopes should not invalidate the session.

This bug can surface when a merchant grants optional/additional scopes during the OAuth flow (e.g. via Shopify's optional scopes feature), or when scope grants evolve over time without re-authentication. The session gets rejected even though it satisfies every scope the app actually needs.

WHAT is this pull request doing?

Replaces the exact-equality scope check in Session::isValid() with an inclusion-based check using the existing Scopes::has() method:

// Before
Context::$SCOPES->equals($this->scope)

// After
$sessionScopes = new Scopes($this->scope ?? []);
$sessionScopes->has(Context::$SCOPES)

A session is now valid as long as its granted scopes are a superset of the app's configured scopes. The access-token and expiry checks are unchanged.

This aligns the PHP SDK behaviour with the Node SDK's inclusion-based scope validation, avoiding the rejection of valid offline sessions that include optional or additional granted scopes.

Tests updated / added:

Scenario Expected
Session with exactly the configured scopes valid ✅
Session with configured scopes plus extra scopes valid ✅ (new)
Session missing one configured scope invalid ❌
Session without an access token invalid ❌
Expired session invalid ❌

The previous test testIsValidReturnsFalseIfScopesHaveChanged tested a scenario (session has fewer scopes than configured) that is still correctly false under the new logic; it has been renamed to testIsValidReturnsFalseIfScopesAreMissing for clarity.

Type of change

  • Patch: Bug (non-breaking change which fixes an issue)

Checklist

  • I have added a changelog entry, prefixed by the type of change noted above
  • I have added/updated tests for this change
  • I have updated the documentation for public APIs from the library (if applicable)

Sessions with all configured scopes plus additional granted scopes were
incorrectly treated as invalid because isValid() used equals() (exact
match) rather than has() (inclusion check) on the Scopes object.

This caused Utils::loadOfflineSession() to return null for sessions that
include optional or additional granted scopes, even though the access
token is fully valid for all Admin API calls that require the configured
scopes.

Change the check to:
  $sessionScopes->has(Context::$SCOPES)

so that a session is valid as long as its granted scopes are a superset
of the configured app scopes. The access-token and expiry checks are
unchanged. This aligns the PHP SDK behaviour with the Node SDK's
inclusion-based scope validation.
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.

1 participant