Provision user access to stores only when permitted

[gordonhirsch]

WHY are these changes introduced?

Modifies the fix for https://github.com/Shopify/shopify/issues/563572 to limit calls the BP provisioning mutation to organization owners and administrators.

WHAT is this pull request doing?

The mutation is available only to holders of the ondemand_access_to_stores permission. Other users should always be provisioned for access to a store and do not need the mutation called.

To avoid a separate GraphQL query to fetch user permissions, they are instead added to existing to store fetch calls by including

currentUser {
  organizationPermissions
}

in the existing query.

The additional information is added to OrganizationStore class so that is available when needed.

Ideally we would not be storing user-level information in a store object, but to avoid that, we'd need to

• Add yet another GraphQL call to the app dev flow, or
• Gather permission info more centrally for an authenticated user. This would require some refactoring and probably moving from destinations API to organizations API to gather some user info (beyond the scope of this PR)

How to test your changes?

Tophatting requires local dev and the following painful steps (sorry)

Add the user management scope to the CLI application.

• Visit https://identity.shop.dev/services/applications
• Search for shopify-cli
• Click Edit
• Under "scopes that may be requested", select the checkbox for the BP user-management checkbox (https://api.shopify.com/auth/organization.user-management)
• Save the changes. Ignore the error message.

Run BP using the rz-accessible-shops branch, rebasing from main as necessary.

Update shop/world

• Ensure that your local repo is recent enough to include this PR
• Add potentiaal dev shops to extra_server_names (details)

Ensure that dev@shopify.com has created a development store as described in the local dev doc

Log in to the dev store and add an admin user for the store.

• Navigate to Settings -> Users and click add user.
• Enter an email for one of the existing test users (e.g. alice@example.com).
• Assign the user to an organization administrator role
• At this point the user has been invited. To accept the invitation, follow the instructions here (run the commands via bin/rails c from the BP repo)

Create a store as the admin user

• Log in to dev dash as the admin user invited above (incognito)
• Create a store

Exercise the alternative methods of listing/selecting stores from the CLI. Both stores should be visible and selectable, and app dev should start up and auto-install the app on the selected store if necessary.

Post-release steps

Measuring impact

How do we know this change was effective? Please choose one:

• n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
• Existing analytics will cater for this addition
• PR includes analytics changes to measure impact

Checklist

• I've considered possible cross-platform impacts (Mac, Linux, Windows)
• I've considered possible documentation changes

[gordonhirsch]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• refactor to clearly separate org store and org user info #5605
• Provision user access to stores only when permitted #5596 👈 (View in Graphite)
• Refactor store fetch, remove dead code #5597
• Ensure user access to selected dev stores #5531
• Add BP mutation to ensure access to a dev store #5530
• Refactor app management client to prepare for new BP mutation #5529
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[github-actions]

Coverage report

St.❔	Category	Percentage	Covered / Total
🟡	Statements	76.53% (-0.09% 🔻)	9405/12289
🟡	Branches	71.67% (-0.14% 🔻)	4610/6432
🟡	Functions	76.27% (-0.07% 🔻)	2443/3203
🟡	Lines	77.05% (-0.07% 🔻)	8892/11540

Show new covered files 🐣

St.❔	File	Statements	Branches	Functions	Lines
🟢	... / bundle.ts	78.57%	75%	80%	84.62%

Show files with reduced coverage 🔻

St.❔	File	Statements	Branches	Functions	Lines
🟢	... / app.ts	87.5%	70.33% (-1.1% 🔻)	92.98%	90.21%
🟢	... / extension-instance.ts	84.52% (-0.6% 🔻)	80.16% (-2.38% 🔻)	92.45%	85.06%
🟢	... / flow_action.ts	90% (-10% 🔻)	100%	75% (-25% 🔻)	90% (-10% 🔻)
🟢	... / dev-session.ts	81.45% (-0.99% 🔻)	64.52%	90% (-0.63% 🔻)	83.78% (-0.96% 🔻)
🟢	... / serialize-fields.ts	95% (-2.5% 🔻)	87.5% (-3.13% 🔻)	100%	97.44%
🔴	... / app-management-client.ts	44.33% (-0.07% 🔻)	39.47% (-0.34% 🔻)	41.75% (-0.41% 🔻)	42.81% (-0.05% 🔻)
🔴	... / partners-client.ts	25% (-0.49% 🔻)	30% (-1.58% 🔻)	16.67%	24.67% (-0.5% 🔻)
🔴	... / partners.ts	58.62% (-12.21% 🔻)	50% (-25% 🔻)	57.14%	57.14% (-12.42% 🔻)

Test suite run success

2174 tests passing in 953 suites.

Report generated by 🧪jest coverage report action from 716984f

[github-actions]

We detected some changes at packages/*/src and there are no updates in the .changeset.
If the changes are user-facing, run "pnpm changeset add" to track your changes and include them in the next release CHANGELOG.

[grollest]

Approving in that the code looks to do what is described in the description, and that it is well tested but I lack the FED context to know if the code is correct/ideomatic and/or if there is full test coverage.

[gordonhirsch]

Ideally we would not be storing user-level information in a store object, but to avoid that, we'd need to

• Add yet another GraphQL call to the app dev flow, or
• Gather permission info more centrally for an authenticated user. This would require some refactoring and probably moving from destinations API to organizations API to gather some user info (beyond the scope of this PR)

Discussed this with @MitchDickinson earlier today. He suggested, as a "soft" preference that we have the underlying queries return the user and store data as separate objects rather than polluting OrganizationStore.

This is my incomplete attempt to do that. It's not as simple as I was hoping, and it is starting to strain my JS/TS skillset. If someone feels strongly enough about this approach to pair for a while, I'd be happy to do it; otherwise, I think we should go with the current solution.

[craigmichaelmartin]

Approving but am not able to tophat right now due to my local dev being borked currently.

[github-actions]

Differences in type declarations

We detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:

• Some seemingly private modules might be re-exported through public modules.
• If the branch is behind main you might see odd diffs, rebase main into this branch.

New type declarations

We found no new type declarations in this PR

Existing type declarations

packages/cli-kit/dist/public/node/api/business-platform.d.ts

@@ -40,6 +40,6 @@ export declare function businessPlatformOrganizationsRequest<T>(query: string, t
  * @param variables - GraphQL variables to pass to the query.
  * @returns The response of the query of generic type <T>.
  */
-export declare function businessPlatformOrganizationsRequestDoc<TResult>(query: TypedDocumentNode<TResult, GraphQLVariables> | TypedDocumentNode<TResult, Exact<{
+export declare function businessPlatformOrganizationsRequestDoc<TResult, TVariables extends Variables>(query: TypedDocumentNode<TResult, TVariables> | TypedDocumentNode<TResult, Exact<{
     [key: string]: never;
-}>>, token: string, organizationId: string, variables?: GraphQLVariables): Promise<TResult>;
\ No newline at end of file
+}>>, token: string, organizationId: string, variables?: TVariables): Promise<TResult>;
\ No newline at end of file