Add new auth strategy docs & set token exchange as default configuration from rails generator #1834

zzooeeyy · 2024-04-29T22:55:08Z

What this PR does

https://github.com/Shopify/develop-app-access/issues/291
https://github.com/Shopify/develop-app-access/issues/216
Added documentations for token exchange embedded auth strategy (mostly copied from the Remix app).
- README section
- Changelog
- Session & Authentication guides
- Upgrading guide
Modified the rails configuration generator to enable this new auth strategy config by default if it's an embedded app.

Before and after with the configuration on/off

The embedded app is using previous OAuth with callbacks in "authorization code grant" flow when config.new_embedded_auth_strategy = false.
The embedded app is using the new zero redirect OAuth with "token exchange" flow when config.new_embedded_auth_strategy = true

New app install

Auth code flow (old)	New embedded app auth (new)
auth-code-flow-install.mp4	token-exchange-install.mp4
There are multiple redirects back and forth from app to Shopify to retrieve both the offline and online access tokens. Took about 15s from "install" to home page being displayed.	There are no redirects happening at all from the app after the install. Home page displayed in about 5s

Adding new access scope

Auth code flow (old)	New embedded app auth (new)
auth-code-flow-new-scope-720.mov	token-exchange-new-scope.mp4
When a new scope has been deployed, the old authorization code flow can't pick it up until the server has been restarted. Then the user has to go through many redirects to get the new access tokens with the new access scopes.	When a new scope has been deployed, the new scope grant will be automatically prompted. Once the user grants approval to the new scopes, the operation will continue.

Expired online access token

Auth code flow (old)	New embedded app auth (new)
auth-code-flow-expired-online-token.mp4	token-exchange-expired-online-token.mp4
When the online access token has expired. The app redirected multiple times to retrieve the new access tokens, and the XHR operation from the front end didn't get completed.	When the online access token has expired, the new auth strategy automatically fetches the new access token before handling the actual API request with no redirects.

Invalidated online access token

Auth code flow (old)	New embedded app auth (new)
auth-code-flow-invalid-access-token.mp4	token-exchange-invalid-access-token.mp4
When the online access token has been invalidated, the app could not handle XHR requests until the app has been reloaded and redirected multiple times for OAuth to retrieve new access tokens	When the online access token has been invalidated, the server handles the 401 HTTP unauthorized error and fetches new access token through token exchange before retrying the controller action again. The result is a seamless server side handling of this error with zero redirects.

Checklist

Before submitting the PR, please consider if any of the following are needed:

Update CHANGELOG.md if the changes would impact users
Update README.md, if appropriate.
Update any relevant pages in /docs, if necessary
For security fixes, the Disclosure Policy must be followed.

rachel-carvalho · 2024-05-01T13:33:17Z

lib/generators/shopify_app/install/templates/shopify_app.rb.tt

@@ -4,6 +4,8 @@ ShopifyApp.configure do |config|
  config.scope = "<%= @scope %>" # Consult this page for more scope options:
                                  # https://help.shopify.com/en/api/getting-started/authentication/oauth/scopes
  config.embedded_app = <%= embedded_app? %>
+  config.new_embedded_auth_strategy = <%= embedded_app? %>


I think we realized we need to also enable token expiration check to use the new strategy, right?

If that's the case, would it make sense to turn that on in the background, and fail if an invalid combination is explicitly provided?

Yea I thought about enabling it, but I thought it might be confusing for apps that aren't using online tokens....
And Paulo brings up a good point, I'm not sure why we have this check anyways, it should just not get a new token automatically if there are no online tokens or if online token doesn't have an expiry date column, and wouldn't need a config for it?

And I just double checked, if the app has a User table but they don't have "expire_at" column, with the check enabled, it'll actually raise an NotImplementedError.. 🤔

And I just found this change to the template -- Shopify/shopify-app-template-ruby#24

Looks offline tokens are preferred instead of using online token?

paulomarg

I had a few thoughts, but nothing blocking.

paulomarg · 2024-05-01T12:50:26Z

README.md

@@ -129,6 +129,50 @@ These routes are configurable. See the more detailed [*Engine*](/docs/shopify_ap

 To learn more about how this gem authenticates with Shopify, see [*Authentication*](/docs/shopify_app/authentication.md).

+### New embedded app authorization strategy


I wonder if we need to give some context as to what "new" means here - in the future this might not be so clear :)

paulomarg · 2024-05-01T12:53:07Z

README.md

+- [Using Shopify managed installation](https://shopify.dev/docs/apps/auth/installation#shopify-managed-installation)
+- [Configuring access scopes through the Shopify CLI](https://shopify.dev/docs/apps/tools/cli/configuration)


Not sure how much value these links add here, since they were mentioned above and this is a fairly short section. Fine if you want to keep them, but just wanted to raise this :)

paulomarg · 2024-05-01T13:08:50Z

docs/shopify_app/authentication.md

+
+OAuth process by exchanging the current user's [session token (shopify id token)](https://shopify.dev/docs/apps/auth/session-tokens) for an
+[access token](https://shopify.dev/docs/apps/auth/access-token-types/online.md) to make
+authenticated Shopify API queries. This can replace authorization code grant flow completely if you also take advantage of [Shopify managed installation](https://shopify.dev/docs/apps/auth/installation#shopify-managed-installation).


This makes it sound like managed install is optional, but that's not the case, right?

paulomarg · 2024-05-01T13:09:58Z

docs/shopify_app/authentication.md

+authenticated Shopify API queries. This can replace authorization code grant flow completely if you also take advantage of [Shopify managed installation](https://shopify.dev/docs/apps/auth/installation#shopify-managed-installation).
+
+To enable token exchange authorization strategy, you can follow the steps in ["New embedded app authorization strategy"](/README.md#new-embedded-app-authorization-strategy).
+Upon completion of the token exchange to get the access token, [post authenticated tasks](#post-authenticate-tasks) will be run.


Is there any difference in behaviour as to when post auth tasks are run, when compared to auth code? If so, this might be a good place to call that out.

Nope it'll run the same post auth tasks!

Right, but I meant more in the sense of whether there are any scenarios where the auth code flow would trigger the tasks and this won't. I guess there aren't any?

I don't think so, both will happen right after the app gets a new token. Auth code flow calls it from CallbackController and this is called right after token exchange

paulomarg · 2024-05-01T13:22:00Z

docs/shopify_app/authentication.md

+with [token exchange](#token-exchange) instead of the authorization code grant flow.
+
+
+Authorization code grant flow is the previous OAuth flow that requires the app to redirect the user 


I would try to avoid temporal references here as they will eventually become outdated.

paulomarg · 2024-05-01T13:22:39Z

docs/shopify_app/authentication.md

+> [!NOTE]
+> If you are building an embedded app, we **strongly** recommend using [Shopify managed installation](https://shopify.dev/docs/apps/auth/installation#shopify-managed-installation)
+with [token exchange](#token-exchange) instead of the authorization code grant flow.


I wonder if we need the warning here as well if we already have it above, WDYT?

paulomarg · 2024-05-01T13:24:06Z

docs/shopify_app/authentication.md

+The ShopifyApp gem then redirect the merchant to Shopify, to ask for permission to install the app. (See [ShopifyApp::SessionsController.redirect_to_begin_oauth](https://github.com/Shopify/shopify_app/blob/main/app/controllers/shopify_app/sessions_controller.rb#L76-L96)
+for detailed implementation)
+
+### OAuth callback

 >️ **Note:** In Shopify App version 8.4.0, we have extracted the callback logic in its own controller. If you are upgrading from a version older than 8.4.0 the callback action and related helper methods were defined in `ShopifyApp::SessionsController` ==> you will have to extend `ShopifyApp::CallbackController` instead and port your logic to the new controller.


This is a fairly old warning, I wonder if we still need to have the version reference here, or if we want to make it more "permanent".

docs/shopify_app/sessions.md

paulomarg · 2024-05-01T13:31:29Z

docs/shopify_app/sessions.md

+  include ShopifyApp::EnsureHasSession
+
+  def index
+    with_token_refetch(current_shopify_session, shopify_id_token) do


If we're using methods provided by EnsureHasSession, do we still need to explicitly pass the session and id token as params? Maybe we could do that internally and keep the params as an override (or not?), that way calling this is a bit simpler.

paulomarg · 2024-05-01T13:35:04Z

docs/shopify_app/sessions.md

+end
+```
+
+If the error is being rescued in the action, it's still possible to make use of `with_token_refetch` provided by `EnsureHasSession` so that a new access token is fetched and the code is executed again with it. This will also update the session parameter with the new attributes.


I can see this being a bit confusing for people in the sense that the code might be run twice. Should we explicitly state that only the query should be executed inside the block, and no business logic? Should we have a way to do this in the client itself, something like

def index client = Client.new client.query({ **options, refetch_id_token: shopify_id_token }) end

I'm mostly just bikeshedding here, wondering if we can go a little bit further to simplify the app's code.

zzooeeyy added the #gsd:35360 label Apr 29, 2024

zzooeeyy force-pushed the add-docs branch 2 times, most recently from b3bd298 to a8e5e4e Compare April 29, 2024 23:01

zzooeeyy changed the title ~~Add docs~~ Add new auth strategy docs & set token exchange as default configuration from rails generator Apr 29, 2024

zzooeeyy and others added 11 commits April 30, 2024 13:26

Add README introduction to new embedded app auth strategy

3430e7e

update session.md for with_token_refetch

8578d01

fix with_token_refetch example so client is inside block

6c10eb1

Add authentication docs for token exchange

4cc1172

Add more detail for auth code flow

381c3a0

Add section reference to handling invalid access tokensgs

7595610

Add changelog entry

7d0a958

Remove WIP from token exchange

f74c771

Enable new auth strategy as default from generator

74759b6

Update changelog

caf2dad

Rename new_embedded_auth_strategy configuration

b2ff7ad

zzooeeyy force-pushed the add-docs branch from 31c1a3e to b2ff7ad Compare April 30, 2024 19:32

zzooeeyy added 2 commits April 30, 2024 14:54

Fix doc links and wording

c6d4e3f

Add upgrade docs

08f1555

zzooeeyy marked this pull request as ready for review April 30, 2024 23:56

zzooeeyy requested a review from a team as a code owner April 30, 2024 23:56

rachel-carvalho reviewed May 1, 2024

View reviewed changes

paulomarg approved these changes May 1, 2024

View reviewed changes

Clarify docs

75e39bb

zzooeeyy force-pushed the add-docs branch from 9bb28b1 to 75e39bb Compare May 2, 2024 15:43

zzooeeyy merged commit 44079b0 into main May 2, 2024
6 of 7 checks passed

zzooeeyy deleted the add-docs branch May 2, 2024 15:54

shopify-shipit bot temporarily deployed to rubygems May 2, 2024 20:10 Inactive

zzooeeyy mentioned this pull request May 3, 2024

[draft] Token exchange docs #1829

Closed

4 tasks

zzooeeyy mentioned this pull request Jul 19, 2024

Oauth error failed_grant_with_invalid_scopes #1839

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new auth strategy docs & set token exchange as default configuration from rails generator #1834

Add new auth strategy docs & set token exchange as default configuration from rails generator #1834

zzooeeyy commented Apr 29, 2024 •

edited

Loading

rachel-carvalho May 1, 2024

paulomarg May 1, 2024

zzooeeyy May 1, 2024

zzooeeyy May 1, 2024

zzooeeyy May 1, 2024

paulomarg left a comment

paulomarg May 1, 2024

paulomarg May 1, 2024

paulomarg May 1, 2024

paulomarg May 1, 2024

zzooeeyy May 1, 2024

paulomarg May 1, 2024

zzooeeyy May 1, 2024

paulomarg May 1, 2024

paulomarg May 1, 2024

paulomarg May 1, 2024

paulomarg May 1, 2024

paulomarg May 1, 2024

		@@ -129,6 +129,50 @@ These routes are configurable. See the more detailed [Engine](/docs/shopify_ap

		To learn more about how this gem authenticates with Shopify, see [Authentication](/docs/shopify_app/authentication.md).

		### New embedded app authorization strategy

		- [Using Shopify managed installation](https://shopify.dev/docs/apps/auth/installation#shopify-managed-installation)
		- [Configuring access scopes through the Shopify CLI](https://shopify.dev/docs/apps/tools/cli/configuration)

		with [token exchange](#token-exchange) instead of the authorization code grant flow.


		Authorization code grant flow is the previous OAuth flow that requires the app to redirect the user

Add new auth strategy docs & set token exchange as default configuration from rails generator #1834

Add new auth strategy docs & set token exchange as default configuration from rails generator #1834

Conversation

zzooeeyy commented Apr 29, 2024 • edited Loading

What this PR does

Before and after with the configuration on/off

New app install

Adding new access scope

Expired online access token

Invalidated online access token

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulomarg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zzooeeyy commented Apr 29, 2024 •

edited

Loading