Add token exchange concern

zzooeeyy commented 3 months ago

What this PR does

https://github.com/Shopify/develop-app-access/issues/144

Tophat: 📹 https://videobin.shopify.io/v/J4j8l2

Happy path for token exchange

Add token exchange concern to retrieve current session. This replaces "LoginProtection" logic when "use_new_embedded_auth_strategy" is turned on.
"EnsureHasSession" concern can be used with token exchange.

Note on what else needs to be implemented in other tasks:

Handle invalid or missing session tokens
Handle invalid access tokens
Handle post auth tasks (webhooks, jobs, etc)
Add support to use id_token from URL params.
Ensure billing works with token exchange

Impact on existing apps:

None - All the code is gated behind the configuration flag: config.wip_new_embedded_auth_strategy

Checklist

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

[ will update when the feature is complete ] Update CHANGELOG.md if the changes would impact users
[ will update when the feature is complete ] Update README.md, if appropriate.
[ will update when the feature is complete ] Update any relevant pages in /docs, if necessary

paulomarg commented 3 months ago

I think this makes sense, assuming the TODOs will be resolved before we release :)

ragalie commented 3 months ago

This looks good to me! One question: should we have unit tests around the TokenExchange class?

paulomarg commented 3 months ago

Great callout, we should definitely add some tests for the new paths.

zzooeeyy commented 3 months ago

This looks good to me! One question: should we have unit tests around the TokenExchange class?

Yes we totally should, I was going to tackle that after we handle all the special invalid token cases. That way we can ensure we have all the test cases and ways to recover in 1 go... I've added that another task so we don't forget about it:

https://github.com/Shopify/develop-app-access/issues/267

ragalie commented 3 months ago

I was going to tackle that after we handle all the special invalid token cases

I'd encourage you to test the happy path in the happy path PR, and the sad path in the sad path PR! Doing all the tests at once can make it harder to discover what caused errors, whereas doing them more iteratively makes it more obvious when a change is breaking an assumption.

zzooeeyy commented 3 months ago

Thanks @ragalie & @paulomarg -- I've updated this PR with tests!

Shopify / shopify_app