-
Notifications
You must be signed in to change notification settings - Fork 481
Add expiring offline access token support #1423
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
ecc5474 to
3f0c967
Compare
| def serialized_error(response) | ||
| body = {} | ||
| body["errors"] = response.body["errors"] if response.body["errors"] | ||
| body["error"] = response.body["error"] if response.body["error"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Let me know what you think
Because the error and error_description fields are specific to oauth requests I wonder if we could move this logic to a file where it make that more explicit.
For example, in RefreshToken or TokenExchange:
rescue ShopifyAPI::Errors::HttpResponseError => e
# Parse OAuth-specific error from response.body
oauth_error = e.response.body["error"]
oauth_description = e.response.body["error_description"]
# Re-raise with better message or handle specifically
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I slightly disagree on this since http_client is a generic class we provide in the library, it should be able to handle errors and be able to serialize it regardless of where it came from. Apps could use HTTPClient to implement their own OAuth strategy which they would now have to parse specific errors from the response themselves as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is a fair point. I don't feel too strongly either way so I am good to leave it as is. 👍
7642665 to
2846034
Compare
2846034 to
447b66c
Compare
Prevents sessions from losing refresh token data during copy operations, which would break token refresh functionality. Fixed merge conflict resolution: After merging main's refactored copy_attributes_from (direct assignment instead of JSON.parse), the new refresh_token fields were inadvertently omitted. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
What
Adds support for expiring offline access tokens with refresh tokens.
Related change in
shopify_appgem:Why
Enables apps to use time-limited offline access tokens for improved security through automatic token rotation.
Changes
Core Features:
expiring_offline_access_tokensin Context setupSessionclass withrefresh_token,refresh_token_expires, and expiration helpers (expired?,refresh_token_expired?)ShopifyAPI::Auth::RefreshToken.refresh_access_token- Exchange refresh token for new access tokenShopifyAPI::Auth::TokenExchange.migrate_to_expiring_token- Migrate existing non-expiring tokens to expiring tokens (one-time, irreversible per shop)OAuth Flows:
error,error_description) for better error messages