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

Session: return early if storeUserEmail is not populated #163

Merged
merged 4 commits into from
Oct 2, 2024

Conversation

polishq
Copy link
Contributor

@polishq polishq commented Sep 26, 2024

What problem is this solving?

A bug has been identified during the Helmet House project, having to do with B2B impersonation.

Part of the issue is that the vtex_session cookie has a longer expiration (~5 days) than the VtexidClientAutCookie (~1 day). Therefore, when returning to the store after 24 hours the user is likely to have an expired VtexidClientAutCookie but a non-expired session.

If the user had an active B2B impersonation session when they last used the site, when they return their session will become corrupted due to Storefront-Permission's setProfile logic which does not expect this scenario. This can cause undesirable side effects; for example in Helmet House's case, the user is in a "half-logged in" state when they should actually be prompted to log in again (the storefront is not meant to be accessible by anonymous users).

The existing logic in the Storefront Permissions setProfile flow is this:

  • check if there is a storeUserEmail in the session input authentication namespace
  • check if there is a user ID in the session input public.impersonate namespace, and if so, look up the email for that user
  • if neither of these emails are provided, return early

This PR adjusts the logic like so:

  • check if there is a storeUserEmail in the session input authentication namespace
  • if not, return early
  • check if there is a user ID in the session input public.impersonate namespace, and if so, look up the email for that user

This correctly results in the user with an expired cookie to be prompted to log in again. Once they've logged in, their impersonation session will continue, and this is fine.

How should this be manually tested?

The new app is linked in https://arthur--helmethouseprod.myvtex.com . Contact me on Slack for user credentials, or I can present a before/after demo of the fix.

Copy link

vtex-io-ci-cd bot commented Sep 26, 2024

Hi! I'm VTEX IO CI/CD Bot and I'll be helping you to publish your app! 🤖

Please select which version do you want to release:

  • Patch (backwards-compatible bug fixes)

  • Minor (backwards-compatible functionality)

  • Major (incompatible API changes)

And then you just need to merge your PR when you are ready! There is no need to create a release commit/tag.

  • No thanks, I would rather do it manually 😞

Copy link

github-actions bot commented Sep 26, 2024

Messages
📖 ❤️ Thanks!
📖

🎉 PR additions = 17, PR deletions = 8

Generated by 🚫 dangerJS against 77d13b5

@enzomerca enzomerca requested a review from a team September 27, 2024 11:55
@polishq
Copy link
Contributor Author

polishq commented Sep 30, 2024

Video explanation: https://www.loom.com/share/f3b0c3e7680e4856ae1c79270f121b53?sid=1c12b537-b82d-4898-920f-dd439316e92c

Also added a secondary fix for a token validation issue seen while impersonating. This was observed in the helmethouseprod account, when a custom app called the Storefront Permissions getUser query. Although the app was sending the storeUserAuthCookie, this was received/interpreted by Storefront Permissions as an adminUserAuthCookie, and failed the admin cookie validation. The fix is to fall back to the admin cookie when validating the user's store cookie, if the store cookie is not provided -- the cookie passes the store cookie validation.

Copy link

@enzomerca enzomerca merged commit 3d4670f into master Oct 2, 2024
12 checks passed
@enzomerca enzomerca deleted the fix/session-storeuseremail branch October 2, 2024 13:49
Copy link

vtex-io-ci-cd bot commented Oct 2, 2024

Your PR has been merged! App is being published. 🚀
Version 1.44.7 → 1.44.8

After the publishing process has been completed (check #vtex-io-releases) and doing A/B tests with the new version, you can deploy your release by running:

vtex deploy [email protected]

After that your app will be updated on all accounts.

For more information on the deployment process check the docs. 📖

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