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

Client-side signOut force reload if redirect to the same page. #1107

Closed
wants to merge 89 commits into from

Conversation

ThewBear
Copy link

@ThewBear ThewBear commented Jan 14, 2021

What:

This pr adds additional check in client-side signOut and forces reloading the page if final location is the same page.

Why:

Client-side signOut will not reload a page if it is the same page and url contains hash #. #603

How:

It checks if all of url fragments are equal except hash.

Checklist:

  • Documentation N/A
  • Tests N/A
  • Ready to be merged

balazsorban44 and others added 30 commits December 5, 2020 11:11
Vercel archived their now packages a while back, so you can use vercel env pull to pull in the .env
This is a simple typographical error changed accesed to accessed
https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-7769 reports a high-severity issue with the current version of nodemailer. This should be merged and released right away if possible.
the current routing for the Okta provider does not follow the standard
set by Okta, and as such doesn't allow for custom subdomains. this
update amends the routes to allow for customer subdomains, and also
aligns next-auth with Okta's documentation.
* chore(deps): upgrade "standard"

* style(lint): run lint fix

* fix(provider): optional chain Spotify provider profile img
* chore: use stale label, instead of wontfix

* chore: add link to issue explaining stalebot

* chore: fix typo in stalebot comment

* chore: run build GitHub Action on canary also

* chore: run build GitHub Actions on canary as well

* chore: add reproduction section to questions
* Fixed Reddit Authentication

* updated fix for build test

* updated buffer to avoid deprecation message

* Updated for passing tests
* update: deps

* fix: broken link

* fix: search upgrade change
* Include callbackUrl in newUser page

* Update src/server/routes/callback.js

Co-authored-by: Iain Collins <[email protected]>

* Update src/server/routes/callback.js

Co-authored-by: Iain Collins <[email protected]>

Co-authored-by: Iain Collins <[email protected]>
Co-authored-by: Nico Domino <[email protected]>
* Add support for Fauna DB

* Add integration tests

Co-authored-by: Nico Domino <[email protected]>
Co-authored-by: styxlab <[email protected]>
Co-authored-by: Balázs Orbán <[email protected]>
Bumps [next](https://github.com/vercel/next.js) from 9.5.3 to 9.5.4.
- [Release notes](https://github.com/vercel/next.js/releases)
- [Changelog](https://github.com/vercel/next.js/blob/canary/release.js)
- [Commits](vercel/next.js@v9.5.3...v9.5.4)

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Nico Domino <[email protected]>
* Add Bungie provider

* Use absolute URL for images

* Correct image URL and use consistent formatting

Co-authored-by: Nico Domino <[email protected]>
* add provider: Microsoft

* documentation

* support no tenant setup

* fix code style

* chore: rename Microsoft provider to AzureADB2C

* chore: alphabetical order in providers/index

* doc: add provider to FAQ
…xtauthjs#895)

* Update Slack to v2 authorize urls, option for additional authorize params
* acessTokenGetter + documentation
Bumps [highlight.js](https://github.com/highlightjs/highlight.js) from 9.18.1 to 9.18.5.
- [Release notes](https://github.com/highlightjs/highlight.js/releases)
- [Changelog](https://github.com/highlightjs/highlight.js/blob/9.18.5/CHANGES.md)
- [Commits](highlightjs/highlight.js@9.18.1...9.18.5)

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Balázs Orbán <[email protected]>
Co-authored-by: Nico Domino <[email protected]>
This allows us to check if the user is signed in when using JWTs

Part of nextauthjs#625
* chore: use stale label, instead of wontfix

* chore: add link to issue explaining stalebot

* chore: fix typo in stalebot comment

* chore: run build GitHub Action on canary also

* chore: run build GitHub Actions on canary as well

* chore: add reproduction section to questions

* feat(provider): Add Azure Active Directory B2C (nextauthjs#809)

* add provider: Microsoft

* documentation

* support no tenant setup

* fix code style

* chore: rename Microsoft provider to AzureADB2C

* chore: alphabetical order in providers/index

* Revert "feat(provider): Add Azure Active Directory B2C (nextauthjs#809)" (nextauthjs#919)

This reverts commit 6e6a24a.

* chore: add myself to the contributors list 🙈

* docs: fix incorrect references in cypress docs

* chore: add additional docs clarification

Co-authored-by: Balázs Orbán <[email protected]>
Co-authored-by: Vladimir Evdokimov <[email protected]>
* Display error if no [...nextauth].js found

fixes nextauthjs#647

* Log the error and describe it inside errors.md

Co-authored-by: Balázs Orbán <[email protected]>
balazsorban44 and others added 15 commits January 11, 2021 12:57
* feat(pages): add dark theme support

* docs: document theme option

* chore: remove ts-check from dev app

* style(pages): fix some text colors in dark mode
somehow the default export does not work in the dev app
…hjs#1094)

* Create reddit.md

* uncommented profile callback

* Update reddit.md

* fix lint issues

* added reddit provider

* added reddit provider

* Add Reddit Provider

For some reason a bunch of providers got deleted in the last commit

* Add Reddit Provider

* Add Reddit Provider
…uthjs#1087)

* added banner

* Changed banner image allignment

* changed location of banner again

* added to acknowledgement

* added to acknowledgement 1

* changed image size

* k

* l

* s

* s

* .

* added link to the banner in readme.md

* fixed image redirect

* fixed image allignment

* made changes in readme and index.js

* Changed the source of the banner image

* added banner to the footer of the site
@vercel
Copy link

vercel bot commented Jan 14, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/nextauthjs/next-auth/58s2lj06c
✅ Preview: https://next-auth-git-fork-thewbear-patch-1.nextauthjs.vercel.app

@vercel vercel bot temporarily deployed to Preview January 14, 2021 05:55 Inactive
@ThewBear ThewBear changed the title Reload if same page. Client-side signOut force reload if redirect to the same page. Jan 14, 2021
@vercel vercel bot temporarily deployed to Preview January 14, 2021 05:59 Inactive
@balazsorban44
Copy link
Member

Thanks for this PR!

I am not sure we would like to force reload the whole page or just make sure the session change triggers the useSession hook?

@ThewBear
Copy link
Author

I assume we would like to force reload the whole page for consistency with other url and following the documentation.

It reloads the page in the browser when complete.

https://next-auth.js.org/getting-started/client#signout

@balazsorban44
Copy link
Member

I wanted to send you to #781, but I see you recently commented on that as well. Hmm. we have to discuss this @iaincollins

@ThewBear
Copy link
Author

ThewBear commented Jan 14, 2021

I think #781 is a long-term feature but this PR solves a bug which produces inconsistent reloading when signing out.

@@ -287,7 +287,15 @@ export const signOut = async (args = {}) => {
const res = await fetch(`${baseUrl}/signout`, fetchOptions)
const data = await res.json()
_sendMessage({ event: 'session', data: { trigger: 'signout' } })
window.location = data.url ?? callbackUrl
const finalLocation = new URL(data.url ?? callbackUrl)
Copy link
Member

Choose a reason for hiding this comment

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

I wasn't sure myself if this would be a problem supporting IE11, or using it in Next.js would make sure that URL is polyfilled here?

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Member

@balazsorban44 balazsorban44 Jan 22, 2021

Choose a reason for hiding this comment

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

I'm aware, but will those apply for node_modules as well? I remember we had to transpile next-auth further down at work to be able to run in IE11. Could you please confirm that this would work in IE11 as well by trying it out?

If any of your dependencies includes these polyfills, they’ll be eliminated automatically from the production build to avoid duplication.

Sorry, just checked the link you sent. We do the same with fetch, so I guess URL WILL be polyfilled for us also. Thanks for the link!

window.location = data.url ?? callbackUrl
const finalLocation = new URL(data.url ?? callbackUrl)
window.location = finalLocation
if (
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand the need for this if statement. wouldn't setting window.location trigger a reload?

Copy link
Author

Choose a reason for hiding this comment

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

It won't trigger if the url is the same page and contains #.
Example:

Current Final window.location trigger reload
/ / yes
/ /# no
/# /# no
/ /other yes
/ /other# yes

Copy link
Member

@balazsorban44 balazsorban44 Feb 11, 2021

Choose a reason for hiding this comment

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

So I recently came across this at work and remembered your PR! Do you know if this is the intended browser behavior, or some kind of bug?

I can verify it very easily by opening the inspector on any page that has a hashtag in the url, writing window.location = "same as in the url with hashtag", it won't reload!

@balazsorban44
Copy link
Member

Closing this in favor of #1298. I do not wish you to fix the massive merge conflicts @ThewBear as it was our fault! I credited you in the new PR. Many thanks for discovering this! 🙏

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.