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.

Sign up for GitHub

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

Jump to bottom

fix(aws-amplify): createKeyValueStorageFromCookieStorageAdapter misses default path and secure values #13508

Merged
merged 7 commits into from
Jul 19, 2024
Merged

fix(aws-amplify): createKeyValueStorageFromCookieStorageAdapter misses default path and secure values #13508

merged 7 commits into from
Jul 19, 2024

Conversation

HuiSF
Copy link
Member

@HuiSF HuiSF commented Jun 18, 2024 •
edited
Loading

Description of changes

Added default path: '/' and secure: true to the cookies setting options from the server side.

Issue #, if available

#13509

Description of how you validated changes

  1. E2E tests
  2. Manual testing with a Next.js sample app

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

cshfang
cshfang previously approved these changes Jun 18, 2024
View reviewed changes
ashika112
ashika112 previously approved these changes Jun 18, 2024
View reviewed changes
jimblanc
jimblanc suggested changes Jun 18, 2024
View reviewed changes
Copy link
Contributor

@jimblanc jimblanc left a comment

Choose a reason for hiding this comment

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

Change looks good to me, but as this is auth related please hold off until we have sign-off.

@HuiSF HuiSF dismissed stale reviews from ashika112 and cshfang via 08d2b44 June 18, 2024 17:41
cshfang
cshfang previously approved these changes Jun 18, 2024
View reviewed changes
israx
israx previously approved these changes Jun 18, 2024
View reviewed changes
ashika112
ashika112 previously approved these changes Jun 18, 2024
View reviewed changes
@HuiSF HuiSF marked this pull request as ready for review June 19, 2024 16:22
@HuiSF HuiSF requested review from ukhan-amazon, haverchuck and a team as code owners June 19, 2024 16:22
@HuiSF HuiSF dismissed stale reviews from ashika112, israx, and cshfang via c9f6884 July 5, 2024 21:14
@HuiSF HuiSF requested a review from a team as a code owner July 5, 2024 21:14
@HuiSF HuiSF force-pushed the hui/fix/aws-amplify/adapter-cookie-settings branch from c9f6884 to c2a7429 Compare July 9, 2024 15:34
@haverchuck
Copy link
Contributor

Have we tested this for both a) apps that were spun up before the fix, with tokens already persisted, and b) new apps?

@HuiSF
Copy link
Member Author

HuiSF commented Jul 18, 2024 •
edited
Loading

Have we tested this for both a) apps that were spun up before the fix, with tokens already persisted, and b) new apps?

@haverchuck here's the manual testing setup:

Set 1

  1. A next.js sample app with the page router installed the latest version of aws-amplify and @aws-amplify/adapter-nextjs
  2. access route /fetch-auth-session/test with expired access token, server-side refreshed tokens are set back in the browser cookie store with /fetch-auth-session path (wrong cookie persisted)
  3. Swap the aws-amplify and @aws-amplify/adapter-nextjs with a version that contains this PR fix
  4. Wait till the tokens set in step 2 expired, and refresh the page, the originally cookie item set with path /fetch-auth-session got removed, and the newly refreshed tokens are set in the cookie store with / path.

Set 2

  1. A next.js sample with the page router installed a version of aws-amplify and @aws-amplify/adapter-nextjs that contains this PR fix
  2. access route /fetch-auth-session/test
  3. Wait till the access token expires, then refresh the page, the newly refreshed tokens are set in the cookie store with / path.

@HuiSF HuiSF merged commit 60a559f into main Jul 19, 2024
30 checks passed
@HuiSF HuiSF deleted the hui/fix/aws-amplify/adapter-cookie-settings branch July 19, 2024 23:37
jjarvisp added a commit to jjarvisp/amplify-js that referenced this pull request Jul 23, 2024
@jjarvisp
Squashed commit of the following:
563dd8f 
commit 381ca11
Author: ashika112 <[email protected]>
Date:   Tue Jul 23 13:01:22 2024 -0700

    update bundle size

commit df890f1
Merge: 043d913 b473ce3
Author: ashika112 <[email protected]>
Date:   Tue Jul 23 12:52:15 2024 -0700

    Merge branch 'main' of github.com:aws-amplify/amplify-js into feat/multi-bucket

commit b473ce3
Author: Ashwin Kumar <[email protected]>
Date:   Tue Jul 23 10:13:25 2024 -0700

    fix(rtn-web-browser): signInWithRedirect needs to be called twice on Android

    * fix(rtn-web-browser): signInWithRedirect needs to be called twice on Android

    * chore: add code owner

    * address feedback

    * update codeowners

    * update codeowners

    * Update .github/CODEOWNERS

    ---------

    Co-authored-by: Ashwin Kumar <[email protected]>

commit 3d70792
Author: israx <[email protected]>
Date:   Tue Jul 23 08:19:21 2024 -0400

    fix(storage): omit subPathStrategy when prefix is defined (aws-amplify#13618)

    * fix(storage): omit subPathStrategy when prefix is defined (aws-amplify#13606)

    * fix: omit subPathStrategy on prefix

    * chore: fix build

    * chore: address feedback

    * chore: omit subpathStrategy from options

    * chore: add unit tests

    * chore: update tests

    * chore: fix test

    * chore: move subpathstrategy to service options

    * chore: update comment

    * chore: fix type

commit 9fb5988
Merge: 60a559f 4c7e6ed
Author: AllanZhengYP <[email protected]>
Date:   Mon Jul 22 17:30:32 2024 -0700

    chore: Merge release into main (aws-amplify#13623)

commit 4c7e6ed
Author: aws-amplify-bot <[email protected]>
Date:   Mon Jul 22 22:38:34 2024 +0000

    chore(release): Update API docs [skip release]

commit 5c372cc
Author: aws-amplify-bot <[email protected]>
Date:   Mon Jul 22 22:37:14 2024 +0000

    chore(release): Publish [skip release]

     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - [email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - [email protected]

commit 4efe9be
Author: aws-amplify-bot <[email protected]>
Date:   Mon Jul 22 22:37:01 2024 +0000

    chore(release): Set core metadata [skip release]

commit 9cc959a
Merge: 02abc66 60a559f
Author: AllanZhengYP <[email protected]>
Date:   Mon Jul 22 15:01:44 2024 -0700

    release(required): Amplify JS release (aws-amplify#13622)

commit 60a559f
Author: Hui Zhao <[email protected]>
Date:   Fri Jul 19 16:36:55 2024 -0700

    fix(aws-amplify): createKeyValueStorageFromCookieStorageAdapter misses default path and secure values (aws-amplify#13508)

    * fix(aws-amplify): createKeyValueStorageFromCookieStorageAdapter misses default path and secure values

    * Ensure Path is being serialized

    * Delete cookie without path attr before setting it

commit aa7ae18
Merge: 60885b7 02abc66
Author: AllanZhengYP <[email protected]>
Date:   Fri Jul 19 10:16:51 2024 -0700

    chore: Merge release into main (aws-amplify#13616)

commit 02abc66
Author: aws-amplify-bot <[email protected]>
Date:   Fri Jul 19 01:38:05 2024 +0000

    chore(release): Update API docs [skip release]

commit cae6274
Author: aws-amplify-bot <[email protected]>
Date:   Fri Jul 19 01:36:39 2024 +0000

    chore(release): Publish [skip release]

     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - [email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - [email protected]

commit 71683a4
Author: aws-amplify-bot <[email protected]>
Date:   Fri Jul 19 01:36:24 2024 +0000

    chore(release): Set core metadata [skip release]

commit 880dfdc
Merge: dc2fc7b 60885b7
Author: AllanZhengYP <[email protected]>
Date:   Thu Jul 18 17:03:14 2024 -0700

    release: Amplify JS release (aws-amplify#13612)

commit 60885b7
Author: israx <[email protected]>
Date:   Thu Jul 18 17:28:30 2024 -0400

    Revert "fix(storage): omit subPathStrategy when prefix is defined (aws-amplify#13614)

    Revert "fix(storage): omit subPathStrategy when prefix is defined (aws-amplify#13606)"

    This reverts commit d3b9546.

commit d3b9546
Author: israx <[email protected]>
Date:   Thu Jul 18 14:31:27 2024 -0400

    fix(storage): omit subPathStrategy when prefix is defined (aws-amplify#13606)

    * fix: omit subPathStrategy on prefix

    * chore: fix build

    * chore: address feedback

commit af46e39
Author: Hui Zhao <[email protected]>
Date:   Wed Jul 17 12:15:58 2024 -0700

    fix(rtn-push-notification): wrong completion handler key (aws-amplify#13604)

commit 1cc40e3
Merge: e3a10ca 6f3e0d8
Author: Jim Blanchard <[email protected]>
Date:   Thu Jul 11 09:41:22 2024 -0500

    chore: Merge release into main (aws-amplify#13570)

commit 6f3e0d8
Merge: dc2fc7b e3a10ca
Author: Jim Blanchard <[email protected]>
Date:   Thu Jul 11 09:28:45 2024 -0500

    Merge branch 'main' into temp/release-merge-1720476144

commit e3a10ca
Author: James Jarvis <[email protected]>
Date:   Tue Jul 9 16:30:02 2024 -0400

    downgrade react-native-get-random-values from >=1.9.0 to >=1.8.0 (aws-amplify#13573)

    The purpose of this change is to downgrade the
    react-native-get-random-values peer dependency from >=1.9.0 to
    >=1.8.0.Expo 50 utilizes 1.8.0 as a dependency and amplify-js dependency
    of 1.9.0 produced installation friction when getting started via the
    recommended workflow. Dropping the peer dep version will remove
    installation friction while having no impact on current library
    functionality.

commit dc2fc7b
Author: aws-amplify-bot <[email protected]>
Date:   Mon Jul 8 22:05:06 2024 +0000

    chore(release): Update API docs [skip release]

commit 884f77a
Author: aws-amplify-bot <[email protected]>
Date:   Mon Jul 8 22:03:39 2024 +0000

    chore(release): Publish [skip release]

     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - [email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - @aws-amplify/[email protected]
     - [email protected]

commit cf6fe30
Author: aws-amplify-bot <[email protected]>
Date:   Mon Jul 8 22:03:24 2024 +0000

    chore(release): Set core metadata [skip release]

commit 1d353e7
Merge: 8cc3fd5 0a07393
Author: Ashwin Kumar <[email protected]>
Date:   Mon Jul 8 14:14:12 2024 -0700

    release: Amplify JS release (aws-amplify#13569)

commit 0a07393
Author: israx <[email protected]>
Date:   Mon Jul 8 11:17:57 2024 -0400

    feat(storage): enable storage Next JS app (aws-amplify#13555)

    feat: enable storage next js app

commit 95ba49c
Author: israx <[email protected]>
Date:   Mon Jul 8 11:06:49 2024 -0400

    feat(storage): add delimiter support to list API (aws-amplify#13517)

    * feat: add types

    * feat: enable delimiter

    * chore: add unit tests

    * chore: bump bundle size

    * chore: add tsdocs

    * chore: address feedback

    * chore: address feedback

    * chore: address feedback

commit 0f5f4cb
Author: Rami Elwan <[email protected]>
Date:   Mon Jul 1 21:42:54 2024 +0200

    fix(datastore): handle case when getting auth info for expired token users (aws-amplify#13536)

    fix: handle case when getting auth info for expired token users

    Co-authored-by: Aaron S <[email protected]>

commit b6de5f9
Author: Hui Zhao <[email protected]>
Date:   Wed Jun 26 11:23:35 2024 -0700

    chore: enabling linting on __tests__ for the react-native packages (aws-amplify#13545)

    * chore: enabling linting on __tests__ for the react-native packages

    * chore(rtn-push-notification): run yarn lint:fix

    * chore(rtn-push-notification): manual fix linter errors

commit 2ef7c59
Author: Ashwin Kumar <[email protected]>
Date:   Tue Jun 25 18:00:35 2024 -0700

    chore: update PRs checklist to also add/update E2E tests (aws-amplify#13543)

    * chore: update PRs checklist to also add/update E2E tests

    * Update .github/PULL_REQUEST_TEMPLATE.md

    Co-authored-by: Hui Zhao <[email protected]>

    * address feedback

    ---------

    Co-authored-by: Ashwin Kumar <[email protected]>
    Co-authored-by: Hui Zhao <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ashika112 ashika112 ashika112 approved these changes

@jimblanc jimblanc jimblanc requested changes

@haverchuck haverchuck haverchuck approved these changes

@AllanZhengYP AllanZhengYP AllanZhengYP approved these changes

@cshfang cshfang cshfang approved these changes

@israx israx israx left review comments

@ukhan-amazon ukhan-amazon Awaiting requested review from ukhan-amazon

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@HuiSF @haverchuck @jimblanc @cshfang @AllanZhengYP @israx @ashika112