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

Node API feedbacks and potential improvements. #526

Open
cyrilchapon opened this issue Mar 27, 2023 · 1 comment
Open

Node API feedbacks and potential improvements. #526

cyrilchapon opened this issue Mar 27, 2023 · 1 comment

Comments

@cyrilchapon
Copy link

cyrilchapon commented Mar 27, 2023

Hi,

New to Supertokens, I'm implementing a backend API with Auth, and was sort of surprised of the code architectural / API choices for supertokens-node. I'll try to make it clear and objective, but this is inherently opiniated and discussion-centric.


To explain myself, I'll come from this homemade example featuring email/pwd and session management :

import Supertokens from 'supertokens-node'
import { middleware as stMiddleware } from 'supertokens-node/framework/express/index.js'
import express from 'express'
import cors from 'cors'

Supertokens.init({
  framework: 'express',
  // ...
  appInfo: {
    apiBasePath: '/auth',
    // ...
  },
  recipeList: [
    EmailPassword.init(),
    Session.init({
      errorHandlers: {
        onUnauthorised: async (msg, req, res) => {
          const errorPayload: ApiSupertokensErrorPayloadDTO = {
            type: 'supertokens',
            code: 401,
            message: 'unauthorised',
            hint: msg,
          } as ApiSupertokensErrorPayloadDTO

          res.setStatusCode(401)
          res.sendJSONResponse(errorPayload)
        },
      },
    }),
  ],
})

app.use(
  cors({
    allowedHeaders: ['content-type', ...Supertokens.getAllCORSHeaders()],
    credentials: true,
  }),
)

const app = express()
app.use(stMiddleware())

This design appear strange to me for several reasons. Here in no particular order :

Why exposing that global import variable, in the first place ?

import Supertokens from 'supertokens-node'
import { middleware } from 'supertokens-node/framework/express/index.js'

Supertokens.init()
const expressSt = middleware()

// VERSUS

const st = new Supertokens()
const expressSt = middleware(st)

The first one :

  • is magically mutating a global variable in init()
  • and using it magically from stMiddleware() under the hood
  • is very hard to behavior-predict from a consumer point of view

The second one :

  • is way cleaner, less error-prone, magic-less
  • is pretty much more standard
  • is perfectly predictable

User story : As a (dumb) developer, I want to expose 2 authentications backed by 2 separate Supertokens core on a single backend API.
User story : As a developer, I want to create a supertokens.provider.ts file in which I managed my Supertokens instanciation momentum (for example, to orchestrate in a dependency injection pipeline)
User story : As a developer, I want to manage the garbage collection of objects inside my code; and avoid impurities and mutations as much as possible.

Why do I need to tell the used framework on the core instance ?

(probably linked to first concern)

Supertokens.init({
  framework: 'express', // ???
})
const expressSt = middleware() // This is actually the express middleware

The fact we pass the backend framework seems to be a witness something should not be done right inside the core Supertokens.

User story : As a (dumb) developer, I want to run both an Express and a Koa servers with the same Supertokens instance

Why responses customization is on the core instance ?

Supertokens.init({
  // ...
  recipeList: [
    Session.init({
      errorHandlers: {
        onUnauthorised: async (msg, req, res) => {
          const errorPayload: ApiSupertokensErrorPayloadDTO = {
            type: 'supertokens',
            code: 401,
            message: 'unauthorised',
            hint: msg,
          } as ApiSupertokensErrorPayloadDTO

          res.setStatusCode(401)
          res.sendJSONResponse(errorPayload)
        },
      },
    }),
  ],
})

const verifySessionMiddleware = verifySession(/* Why not here ? */)

Supertokens instance has nothing to do with express in the first place; verifySession has, which should hold the responses management logic.

User story : As a developer, I want to understand what I'm doing. When instanciating a verifySession middleware, I'd expect the responses to be part of its configuration. When instanciating a Supertokens core instance, I wouldn't expect the responses to be part of its configuration.

Why Session recipe declaration on the Supertokens core instance in the first place ?

Supertokens.init({
  // ...
  recipeList: [
    Session.init(), // Why do I need this ?
  ],
})

// Isn't that enough to say "I want to manage sessions" ?
const verifySessionMiddleware = verifySession()

Strange magic route attachment

When reading this :

Supertokens.init({
  appInfo: {
    apiBasePath: '/auth',
  },
})

const stExpress = middleware()
app.use(stExpress)

It's barely clear the auth routes will be attached to /auth. Now separate Supertokens.init() in a different file, it leads to :

import Supertokens from 'supertokens-node'
import { initSupertokens } from './st.js'

initSupertokens()

const stExpress = middleware()
// Where is that mounted ?
app.use(stExpress)

User story : as a developer, I want the rest of my team to understand my code when they read it. Adding app.use(stExpress) means "I add the stExpress middleware for all routes at the root ('/')"

GetSession magic res behavior

Calling

import Session from 'supertokens-node/recipe/session'

app.use('/example', (req, res, next) => {
  const session = await Session.getSession(req, res)
})

Magically calls res.send; while we basically expect a thrown error.

User story : as a developer, sometimes I want to entirely manage the error handling (error typings exposure, OpenAPI error definitions, ...) on my route and create my own middleware.


Some ideas :

  1. Having a monorepo with
  • @supertokens-node/core
  • @supertokens-node/express
  • @supertokens-node/koa

With clear separation of concerns : the core would be a supertokens rest API client; the express would be the wrapper with different levels of abstractions.

  1. Provide a non-magic API
import Supertokens from '@supertokens-node/core'
import SupertokensExpress from '@supertokens-node/express'

const st = new Supertokens({
  // core options
  // auth recipes
  // security
  // everything related to Supertokens-core
  appInfo: {
    apiBasePath: '/auth'
  }
})

const stExpress = new SupertokensExpress(st) // Pass Supertokens instance here

app.use('/auth', stExpress.middleware({
  // Express specific auth related options
}))

app.use('/protected', stExpress.verifySession({
  // responses customization...
}))

app.get('/protected/example', (req, res) => { res.json('ok') })
@rishabhpoddar
Copy link
Member

Hi @cyrilchapon thanks for the extensive feedback.

Why exposing that global import variable, in the first place ?

  • It only makes sense to have one supertokens instance on the backend per app. Therefore a singleton pattern works well.
  • Creating an instance of supertokens and passing that around will require you to keep that instance in the global scope anyway, since you will probably require to use it in several files in your API code.
  • "is very hard to behavior-predict from a consumer point of view" -> why so? What makes the behaviour unpredictable?

That being said, the user stories you pointed out are all valid, but rare. We are open to making this change if we see these user stories enough, by real projects.

Why do I need to tell the used framework on the core instance ?

  • SuperTokens' middleware and verifySession function reads from the request and writes to the response object directly. In order to do this, we need to know which framework the req / res objects are coming from.
  • The user story you mentioned here is quite rare - we haven't seen this type of request / usage. That being said, not using the singleton pattern will solve for this (as you mentioned), but it would still require you to pass in the framework being used.

Why responses customization is on the core instance ?

The supertokens.init on the backend has very little to do with the supertokens core. When you do supertokens.init, you inform the sdk about configs that it needs to use when recipe functions are called or verifySession middleware is called.

Why Session recipe declaration on the Supertokens core instance in the first place ?

You are not declaring the session recipe in the core instance. You are telling the SDK that this recipe is to be enabled for use with these set of configurations.

Strange magic route attachment

This is a fair point that adding the middleware like app.use(stExpress) would have the expectation that it adds it to the / route and not /auth route. However, the backend sdk would need to know about the basePath being used anyway cause it requires that info when attaching the refresh token to the request cookies. So you would need to pass in the apiBasePath somehow.

GetSession magic res behaviour

GetSession does not do res.send. It does throw an error which is then handled by the supertokens errorHandler (that you added to your app), which in turn does res.send. If you like, you can catch the error from getSession and it handle it however you like.


Finally, the idea you suggested at the end looks really good. We shall think about this, but in our experience, in reality, it usually ends up making the dev ex more complex with all the different parts of what the sdk does.

Please feel free to further argue with my comments :)

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

No branches or pull requests

2 participants