Skip to content
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

Add Salesforce B2C support #228

Merged
merged 15 commits into from
Dec 9, 2024
Merged

Add Salesforce B2C support #228

merged 15 commits into from
Dec 9, 2024

Conversation

alecgeatches
Copy link
Contributor

@alecgeatches alecgeatches commented Dec 2, 2024

This PR adds support for Salesforce B2C product search and selection by ID:

sfcc-product-search


Additionally, this adds support for the Salesforce B2C data source type in remote data settings:

sfcc-setup

Missing bits

This could be reviewed as-is, but I'd like to document some deficiencies and fragilities with the current code.

  1. get_request_headers() should be modified to return a array|WP_Error in case of runtime failure. Currently if authentication fails in SalesforceB2CAuth::generate_token(), we know that the subsequent request will fail, but we have no way to report an error without throwing. Instead, the request will fail and return a different error. We should update the inherited funtion signature and all implementors of get_request_headers() to return the array|WP_Error type. If an error occurs, fail early in QueryRunner::get_request_details() and return a better error message.

    Other integrations that do important logic in get_request_headers() may also be updated to return an early error on auth failure.

  2. There are no tests for this new data source.

  3. Currently we use an authentication fall-through for requests that looks like this:

    1. If we have a saved access token with a valid expiration, return it.
    2. If we have a saved refresh token with a valid expiration, use it to generate a new access token, and return it.
    3. If not, authenticate from scratch with client credentials and return that access token.

    There's a fallback for expired tokens, but if we have a saved authentication token that appears to be valid, but it still fails during the request, we won't be able to re-authenticate and try again. The auth token is a signed JWT with a known expiration time, so this is unlikely to fail unless there's a clock issue or something. However if it does fail we don't have a way to go back and re-run the whole request from scratch.

    Other authenticated endpoints like the Sheets API are also susceptible to the same thing. I think any solution would require an architecture change that allows a parent in the process to report an auth error to HttpQueryContext and re-run. I'm not sure if this is something we need to solve in this PR, but it's worth considering.

  4. There's not currently a "list" block associated with SalesforceB2C, and I'm not sure if that will be possible. From what I could find there's an endpoint for getProducts, but this requires passing in explicit IDs. There's also a productSearch that we use in the search block, but this requires a search term. We may need to ask for a category ID or something before listing.

I'll add a handful of notes in code below.

Testing

Breakpoints

I found the easiest way to test the auth flow was to set breakpoints in inc/Integrations/SalesforceB2C/Auth/SalesforceB2CAuth.php's generate_token() function.

Token expiry

The access_token and refresh_token expire times are really long (30 minutes and 30 days, respectively), so I also modified the $*_token_expires_in variables on these lines to a much shorter time:

For example:

private static function save_refresh_token( string $refresh_token, int $expires_in, string $organization_id, string $client_id ): void {
    // Expires 10 seconds early as a buffer for request time and drift
-   $refresh_token_expires_in = $expires_in - 10;
+   $refresh_token_expires_in = 60

Clearing cache

To clear cache, run:

wp-env run cli wp cache flush

@alecgeatches alecgeatches self-assigned this Dec 2, 2024
$client_auth_url = sprintf( '%s/shopper/auth/v1/organizations/%s/oauth2/token', $endpoint, $organization_id );

// Even though we're using a refresh token, authentication is still required to receive a new secret
$client_credentials = base64_encode( sprintf( '%s:%s', $client_id, $client_secret ) );
Copy link
Contributor Author

@alecgeatches alecgeatches Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a bit silly - in order to refresh our auth token using a refresh token, we still need to pass in our client ID and secret. From what I can tell, this is due to the OAuth spec. See the "Client Authentication" section on Refreshing Access Tokens:

If the client was issued a secret, then the client must authenticate this request.

Because we receive a secret in the initial grant, we still need it again here.

It makes me wonder - is it really necessary to save and reuse the refresh token? We'll still sending client credentials every 30 minutes anyway, why not just use the client_credentials grant every time and remove this function entirely?

The current setup seems like the "correct" flow, but the refresh token feels pointless.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the primary reason to use the refresh token is because you're generating revokable tokens on the Authorization Server. So imagine we used the client credentials to get new tokens every time instead of just refreshing the one... and then the user goes in to manage their issued tokens and revoke some... and they see a list of 10,000 tokens because we've created a new one on every refresh.

$data_source_config = $this->get_data_source()->to_array();

return sprintf(
'%s/search/shopper-search/v1/organizations/%s/product-search?siteId=RefArchGlobal&q=%s',
Copy link
Contributor Author

@alecgeatches alecgeatches Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parameters currently passed directly in the URL string should be passed somewhere cleaner, if possible.

@@ -50,6 +50,11 @@ export const AddDataSourceDropdown = () => {
label: SUPPORTED_SERVICES_LABELS.shopify,
value: 'shopify',
},
{
icon: HttpIcon,
Copy link
Contributor Author

@alecgeatches alecgeatches Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note we're still using the HttpIcon here instead of something more specific. This should be fixed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with merging this and getting another PR for some design updates if needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a commit that includes an icon, however the icon is licensed under the MIT license so we should get it replaced with something custom probably.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The icon came from here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I switched to the icon provided by Salesforce in their brand guide, though it isn't sizing well for our use case. You might be able to try again. I had to remove a bunch of stuff from the SVG contents to get it to work at all.

@maxschmeling
Copy link
Contributor

I pushed a small commit to rename get_refresh_token_key to get_refresh_token_cache_key and fix some lint issues that I probably introduced with broken editor auto formatting.

@maxschmeling
Copy link
Contributor

I think we can abstract this some more for using OAuth with other services, but we can do that in a PR when we add another one. This is a great start. A little more work is necessary to "productionize" this just like we need to do with the other data sources, but this looks pretty good.

@alecgeatches alecgeatches marked this pull request as ready for review December 9, 2024 18:18
@maxschmeling maxschmeling self-requested a review December 9, 2024 19:44
@alecgeatches alecgeatches merged commit 4e1031d into trunk Dec 9, 2024
11 checks passed
@alecgeatches alecgeatches deleted the add/sfcc-token-management branch December 9, 2024 21:25
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.

2 participants