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

feat(#797, #878): set baseURL via environment variables and improve internal url detection #913

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

zoey-kaiser
Copy link
Member

@zoey-kaiser zoey-kaiser commented Sep 18, 2024

πŸ”— Linked issue

closes #797, #878

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have added tests (if possible).
  • I have updated the documentation accordingly.

Copy link

pkg-pr-new bot commented Sep 18, 2024

Open in Stackblitz

pnpm add https://pkg.pr.new/@sidebase/nuxt-auth@913

commit: 1cd7381

@sidebase sidebase deleted a comment from pkg-pr-new bot Sep 18, 2024
@zoey-kaiser
Copy link
Member Author

zoey-kaiser commented Sep 18, 2024

Testing auth.baseURL and AUTH_ORIGIN:

Only setting auth.baseURL

  • auth.baseURL not set:
    • Requests made to /api/auth
  • auth.baseURL set to http://localhost:8080
    • Requests made to http://localhost:8080
  • auth.baseURL set to http://localhost:8080/auth
    • Requests made to http://localhost:8080/auth

Combination of auth.baseURL and AUTH_ORIGIN

  • auth.baseURL not set, AUTH_ORIGIN not set:
    • Requests made to /api/auth
  • auth.baseURL set to http://localhost:8080, AUTH_ORIGIN not set:
    • Requests made to http://localhost:8080
  • auth.baseURL set to http://localhost:8080, AUTH_ORIGIN set to http://localhost:8081:
    • Requests made to http://localhost:8081
  • auth.baseURL set to http://localhost:8080, AUTH_ORIGIN set to http://localhost:8081/auth:
    • Requests made to http://localhost:8081/auth

Testing setting internal vs external URLs:

  • Internal auth.baseURL:
    • auth.baseURL set to /auth
    • AUTH_ORIGIN not set
    • auth.provider.endpoints.signIn.path set to /login
    • Request made to: /auth/login
  • Internal AUTH_ORIGIN:
    • auth.baseURL not set
    • AUTH_ORIGIN set to /auth
    • auth.provider.endpoints.signIn.path set to /login
    • Request made to: /auth/login
  • External auth.baseURL:
    • auth.baseURL set to http://localhost:8080/auth
    • AUTH_ORIGIN not set
    • auth.provider.endpoints.signIn.path set to /login
    • Request made to: http://localhost:8080/auth/login
  • External AUTH_ORIGIN:
    • auth.baseURL not set
    • AUTH_ORIGIN set to http://localhost:8080/auth
    • auth.provider.endpoints.signIn.path set to /login
    • Request made to: http://localhost:8080/auth/login

Note: I tested that the internal URLs actually request internal URLs, but setting the path to a non existent route and ensuring I receive the Vue Router warning

Copy link
Collaborator

@phoenix-ru phoenix-ru left a comment

Choose a reason for hiding this comment

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

In general I understand the intent and very like the testing comment you left. However, there are several points:

  1. What happens to users who already use baseURL as e.g. http://localhost:3000? In my understanding, requests are now made to this URL without adding /api/auth? This is a breaking change and needs to have its own minor release + docs if we settle on it.
  2. What happens to internal $fetch calls? Remember that they have a limitation - they only work when path starts with a /. And here are two facts that break it:
  3. When using authjs provider in which these calls are the most relevant, you need to set AUTH_ORIGIN or baseURL to a fully-specified URL (i.e. protocol, hostname, etc.);
  4. Calling a fully-specified URL using $fetch invokes external fetch, meaning that a real HTTP call is made from the server to itself - and it has a relatively high cost, bottlenecking performance.

Any implementation that doesn't provide backwards compatibility with at least these two points is therefore highly discouraged :/
I'd like for us to get involved in an RFC discussion and come up with a spec of how calls are made depending on what variable is set. For example, we know for sure:

  • authjs provider is the same Nuxt server and therefore should always prefer internal calls, regardless of the AUTH_ORIGIN (only taking into account pathname);
  • local provider is isomorphic (with lean towards external backends, it seems), and origin needs to be taken literally - if it is set, we assume all calls are external.
    With these in mind, it might even make sense to disconnect the implementations of two providers into separate URL utils.

@@ -8,7 +8,7 @@ export default defineNuxtConfig({
globalAppMiddleware: {
isEnabled: true
},
baseURL: `http://localhost:${process.env.PORT || 3000}`
baseURL: `http://localhost:${process.env.PORT || 3000}/api/auth`
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like a breaking change

@zoey-kaiser
Copy link
Member Author

zoey-kaiser commented Sep 19, 2024

This is a breaking change and needs to have its own minor release + docs if we settle on it.

It is! I had forgotten to add the label. However, I think these changes are desperately needed as the current logic makes no sense and limits how you can use it. E.g. I am working on a project where the external API routes are as follows:

  • sign in: localhost:8080/auth/login
  • get session: localhost:8080/account/me

Adding /api/auth if no path is provided, makes the URLs listed above incompatible with the local provider, as there is no base path I can provide.

If I provide localhost:8080 as baseURL it gets transformed to: localhost:8080/api/auth which does not make sense IMO.

What happens to internal $fetch calls? Remember that they have a limitation - they only work when path starts with a /. And here are two facts that break it:

This logic has been removed, in favour of checking if origin was provided. If you are using an internal API within your Nuxt Application, I do not expect the origin to be passed (as in all our our examples this is also not the case). If no origin is provided, the fetch requests are made internally, if one is provided they are handled as external URLs.

Example Nuxt configuration that currently breaks
export default defineNuxtConfig({
  modules: ['@sidebase/nuxt-auth'],
  runtimeConfig: {
    public: {
      api: {
        baseURL: 'http://localhost:8080'
      }
    }
  },
  auth: {
    originEnvKey: 'NUXT_PUBLIC_API_BASE_URL',
    baseURL: 'http://localhost:8080',
    provider: {
      type: 'local',
      endpoints: {
        signIn: { path: '/auth/login', method: 'post' },
        signUp: { path: '/auth/register', method: 'post' },
        signOut: { path: '/auth/logout', method: 'post' },
        getSession: { path: '/accounts/me', method: 'get' }
      },
      pages: {
        login: '/auth/sign-in'
      },
      token: {
        signInResponseTokenPointer: '/token',
        maxAgeInSeconds: 60 * 60 * 24
      },
      session: {
        dataType: {
          id: 'string',
          name: 'string',
          email: 'string',
          createdAt: 'number',
          members: '{ id: number, email: string, roles: string[] }[]'
        },
      }
    },
    sessionRefresh: {
      enableOnWindowFocus: true,
      enablePeriodically: 5000
    },
    globalAppMiddleware: {
      isEnabled: true
    }
  },
})

@zoey-kaiser zoey-kaiser added the breaking-change A change will changes that require at least a minor release. label Sep 19, 2024
src/module.ts Outdated
Comment on lines 111 to 118
let baseURL = userOptions.baseURL ?? '/api/auth'
if (userOptions.originEnvKey) {
const envFromRuntimeConfig = extractFromRuntimeConfig(nuxt.options.runtimeConfig, userOptions.originEnvKey)
const envOrigin = envFromRuntimeConfig ?? process.env[userOptions.originEnvKey]
if (envOrigin) {
baseURL = envOrigin
}
}
Copy link
Collaborator

@phoenix-ru phoenix-ru Oct 10, 2024

Choose a reason for hiding this comment

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

This doesn't actually work the way you'd expect. It will set variables during build time. Because we are doing additional extractFromRuntimeConfig later, it gets quite convoluted

If you do not set AUTH_ORIGIN during build but set baseURL: 'http://localhost:3000/api/auth', and then during runtime set AUTH_ORIGIN=http://localhost:3001/other, some requests are made to localhost:3000/api/auth, while others are made to locahost:3001/api/auth

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change A change will changes that require at least a minor release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC: Improve the handing of baseURL to be more intuitive
2 participants