Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

the new useSession should use basePath settings for redirect #2316

Closed
Naddiseo opened this issue Jul 5, 2021 · 6 comments
Closed

the new useSession should use basePath settings for redirect #2316

Naddiseo opened this issue Jul 5, 2021 · 6 comments
Assignees
Labels
enhancement New feature or request good first issue Good issue to take for first time contributors

Comments

@Naddiseo
Copy link
Contributor

Naddiseo commented Jul 5, 2021

Description 🐜

Follow-up for #2236 , as I was browsing the code changes I noticed that the redirect in useSession doesn't use the basePath settings that are set from NEXTAUTH_URL. We use a slightly different url paths in our project (/api/v1/auth/...) so we rely on everything being parsed from from NEXTAUTH_URL

EDIT: I'm also providing basePath to the SessionProvider to match the NEXTAUTH_URL

Is this a bug in your own project?

No

How to reproduce ☕️

  • Change NEXTAUTH_URL to have a different path to usual: eg: NEXTAUTH_URL=http://localhost:3000/api/v1/auth
  • Go to an authentication required page, the redirect will redirect to an invalid/404 page

Screenshots / Logs 📽

No response

Environment 🖥

next-auth@next

Contributing 🙌🏽

No, I am afraid I cannot help regarding this

@Naddiseo Naddiseo added the bug Something isn't working label Jul 5, 2021
@0ubbe 0ubbe self-assigned this Jul 5, 2021
@balazsorban44
Copy link
Member

balazsorban44 commented Jul 5, 2021

So I'm fairly sure that this is also the current behavior, and that this issue is a duplicate of #1713, but at least related.

UPDATE, on second read, I now know what you mean. You could easily use a custom `onUnauthenticated method though, if you saw rest of the PR.

UPDATE2: We could probably utilize the _apiBaseUrl though. Feel free to open a PR! 😊

@balazsorban44 balazsorban44 added good first issue Good issue to take for first time contributors enhancement New feature or request and removed bug Something isn't working labels Jul 5, 2021
@mahieyin-rahmun
Copy link
Contributor

mahieyin-rahmun commented Jul 6, 2021

I tested this out at my local machine. Let me know if I got something wrong, but here is what I understood:

Assuming OP's description, this is sort of his folder structure:

.
├── pages
│   ├── api
│   │   └── v1
│   │       ├── auth
│   │       │   └── [...nextauth].js
... other files

and, he has set NEXTAUTH_URL in his .env file to be:

NEXTAUTH_URL=http://localhost:3000/api/v1

Now, with the way environment variables work, they won't be available client-side. If we take a look at the parseUrl() implementation, which is what _apiBaseUrl() relies on:

// Default values
const defaultHost = 'http://localhost:3000'
const defaultPath = '/api/auth'

if (!url) { url = `${defaultHost}${defaultPath}` }

This right here is the problem. The hardcoded values should instead refer to the environment variable. But, since we do not have an environment variable access client-side, initially I thought

"maybe we can use window.location.pathname as the value of defaultPath",

but this will break other parts of the application that rely on _apiBaseUrl(), e.g. the following snippet will break, since it tries to ping http://session

// We should always update if we don't have a client session yet
// or if there are events from other tabs/windows
if (storageEvent || __NEXTAUTH._session === undefined) {
  __NEXTAUTH._lastSync = _now()
  __NEXTAUTH._session = await getSession({
    broadcast: !storageEvent,
  })
  setSession(__NEXTAUTH._session)
  return
}

So, the only real solutions would be:

  1. Use a prop in Provider to pass the client-side baseUrl
  2. Use NEXT_PUBLIC_ prefix to declare a second variable in .env.local file

Then, parseUrl() needs to use process.env.NEXT_PUBLIC_NEXTAUTH_URL if the url parameter it got is undefined, because then it is on the client side.

@Naddiseo
Copy link
Contributor Author

Naddiseo commented Jul 6, 2021

@mahieyin-rahmun, Sorry, I also missed in my description that I'm also setting basePath in the Provider. I'll edit my original to reflect that.

I do agree that the "best" way of fixing this is to use NEXT_PUBLIC_NEXTAUTH_URL though, and your PR would accomplish what I'm looking for! I think there are a few other bugs asking for NEXT_PUBLIC_NEXTAUTH_URL

@mahieyin-rahmun
Copy link
Contributor

@mahieyin-rahmun, Sorry, I also missed in my description that I'm also setting basePath in the Provider. I'll edit my original to reflect that.

I see. Even if you do that, as you pointed out, the current signin flow doesn't make use of the passed prop. Whether we go with solution 1 or 2, a refactor of the core client module is needed to make this work 😅

I believe if the core team agrees to introduce the new environment variable, the necessary refactors could be done. But, that would be something that should only go live with v4, as it would be a big breaking change, and several parts of the documentation will require review and hence change.

@stale stale bot added the stale Did not receive any activity for 60 days label Sep 4, 2021
@nextauthjs nextauthjs deleted a comment from stale bot Sep 4, 2021
@stale stale bot closed this as completed Sep 11, 2021
@nextauthjs nextauthjs deleted a comment from stale bot Sep 11, 2021
@balazsorban44 balazsorban44 reopened this Sep 11, 2021
@stale stale bot closed this as completed Sep 18, 2021
@nextauthjs nextauthjs deleted a comment from stale bot Sep 18, 2021
@balazsorban44 balazsorban44 removed the stale Did not receive any activity for 60 days label Sep 18, 2021
@balazsorban44 balazsorban44 reopened this Sep 18, 2021
@stale stale bot added the stale Did not receive any activity for 60 days label Nov 18, 2021
@nextauthjs nextauthjs deleted a comment from stale bot Nov 18, 2021
@stale stale bot removed the stale Did not receive any activity for 60 days label Nov 18, 2021
@lenzi-e
Copy link

lenzi-e commented Dec 17, 2021

Are there any updates on this? I believe I'm running into the same issue and am very blocked. I need my next-auth REST calls to use /app/api/auth instead of /api/auth. I have the basePath and baseUrl set in the SessionProvider. I also have the NEXTAUTH_URL set properly. But the redirects after login are still going to: /api/auth.

@Naddiseo
Copy link
Contributor Author

@lenzi-erickson Possibly setting the pages: {} config in your backend? You may also need to duplicate the env variable and create a NEXT_PUBLIC_NEXTAUTH_URL. It depends on your setup. I've been dynamically setting the NEXTAUTH_URL depending on the referrer header (after matching against a whitelist), and also pulling the "next-auth.callback-url" cookie to verify the redirect go where I want them to.

@nextauthjs nextauthjs locked and limited conversation to collaborators Feb 12, 2022
@balazsorban44 balazsorban44 converted this issue into discussion #3943 Feb 12, 2022

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
enhancement New feature or request good first issue Good issue to take for first time contributors
Projects
None yet
Development

No branches or pull requests

5 participants