Skip to content
This repository has been archived by the owner on Apr 11, 2024. It is now read-only.

Return and delete session in validateAuthCallback #217

Merged
merged 12 commits into from
Oct 13, 2021

Conversation

Paulinakhew
Copy link
Contributor

@Paulinakhew Paulinakhew commented Jul 13, 2021

WHY are these changes introduced?

  • Consider whether it is possible to never fall back to loading the current session from a cookie on embedded apps (and make sure we expire the OAuth cookie right away).

WHAT is this pull request doing?

  • returns the current session at the end of validateAuthCallback
  • immediately destroy the cookie session for embedded apps to help prevent build-up

Type of change

  • Patch: Bug (non-breaking change which fixes an issue)
  • Minor: New feature (non-breaking change which adds functionality)
  • Major: Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • I have added a changelog entry, prefixed by the type of change noted above
  • I have added/updated tests for this change
  • I have documented new APIs/updated the documentation for modified APIs (for public APIs)

Note to self

    // Begin auth
    // Check that we DO have a cookie (that expires 1m in the future with grace period)
    // Set up the test callback query
    // Call validateAuthCallback
    // Check that the cookie has the right id
    // Check that the cookie has the right expiration date
    // expect(cookies.expires).toBe("the right value");

@Paulinakhew Paulinakhew requested a review from paulomarg July 13, 2021 15:57
src/auth/oauth/oauth.ts Outdated Show resolved Hide resolved
src/auth/oauth/test/oauth.test.ts Outdated Show resolved Hide resolved
@Paulinakhew Paulinakhew requested a review from paulomarg July 13, 2021 17:45
src/auth/oauth/oauth.ts Outdated Show resolved Hide resolved
src/auth/oauth/test/oauth.test.ts Outdated Show resolved Hide resolved
src/auth/oauth/test/oauth.test.ts Outdated Show resolved Hide resolved
@Paulinakhew
Copy link
Contributor Author

Hi @paulomarg I still need to write tests but I'm re-requesting your review

@Paulinakhew Paulinakhew requested a review from paulomarg July 19, 2021 19:21
@Paulinakhew Paulinakhew marked this pull request as ready for review July 28, 2021 17:20
@Paulinakhew Paulinakhew requested a review from a team as a code owner July 28, 2021 17:20
src/auth/oauth/oauth.ts Outdated Show resolved Hide resolved
src/auth/oauth/test/oauth.test.ts Show resolved Hide resolved
src/auth/oauth/test/oauth.test.ts Outdated Show resolved Hide resolved
src/auth/oauth/test/oauth.test.ts Outdated Show resolved Hide resolved
src/auth/oauth/test/oauth.test.ts Show resolved Hide resolved
@Paulinakhew Paulinakhew requested a review from paulomarg August 23, 2021 19:46
@Paulinakhew Paulinakhew requested review from paulomarg, a team and mkevinosullivan August 25, 2021 20:06
Copy link
Contributor

@mkevinosullivan mkevinosullivan left a comment

Choose a reason for hiding this comment

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

LGTM - one small note

.github/ISSUE_TEMPLATE/BUG_REPORT.md Show resolved Hide resolved
@mllemango mllemango changed the base branch from main to v2 October 13, 2021 20:32
@mllemango mllemango merged commit c0c9b75 into v2 Oct 13, 2021
@mllemango mllemango deleted the delete-new-session-after-oauth branch October 13, 2021 20:32
mllemango added a commit that referenced this pull request Oct 27, 2021
Add docs reference to changelog
mllemango added a commit that referenced this pull request Oct 27, 2021
Add docs reference to changelog
mllemango added a commit that referenced this pull request Oct 27, 2021
Add docs reference to changelog
softdev66 added a commit to softdev66/shopify-api-node that referenced this pull request Aug 29, 2022
alsuper114 added a commit to alsuper114/shopify-api-node that referenced this pull request Nov 26, 2022
GoldStar919 added a commit to GoldStar919/shopify-api-js that referenced this pull request Mar 1, 2023
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants