awinogrodzki / next-firebase-auth-edge

Next.js Firebase Authentication for Edge and Node.js runtimes. Compatible with latest Next.js features.
https://next-firebase-auth-edge-docs.vercel.app/
MIT License
499 stars 43 forks source link

Migration to v0.4.0 #26

Closed awinogrodzki closed 1 year ago

awinogrodzki commented 1 year ago

In recent v0.4.0 update, there has been some important optimisations in how we save, fetch and verify tokens.

Due to the nature of caching in Next.js, some may have experienced errors when running getTokens in long-running Node.js processes. This has now been fixed.

What's more important, it's no longer recommended to run getTokens in middleware.ts. Please use authentication middleware function.

getTokens does not allow to update cookies once token is refreshed, forcing the server to call firebase servers on each request after initial idToken is expired, usually after one hour. This could be addressed in previous versions by re-calling /api/login on client side, which was sub-optimal.

Using authentication function saves a number of network roundtrips and improves overall response times.

You can still use getTokens function inside server components. The function has been optimised to work in pair with authentication middleware, which should further improve response times.

For advanced use cases, getAuthenticatedResponse and getErrorResponse options have been introduced. Please take a look at authentication middleware function options

awinogrodzki commented 1 year ago

@seanaguinaga @steve-marmalade I am mentioning you here as you're probably be interested in this update :)

seanaguinaga commented 1 year ago

Wow this is awesome!

Thank you for mentioning me - excited to upgrade 😎

And thank for all the excellent work!

(The dynamic import methods you've been using since the beginning are a pattern I've been stealing too)

steve-marmalade commented 1 year ago

Thanks for the update @awinogrodzki :rocket:

steve-marmalade commented 1 year ago

Hi @awinogrodzki , I had a chance to look at the new authentication middleware function.

One question for you: how would you suggest I incorporate this function with other logic in the middleware? For example, previously I would call getTokens and, based on the response and the intended destination, I could make some custom decisions about where to redirect the user. It seems with redirectOptions some of that is now built into this library, but AFAICT it (understandably) doesn't handle complex cases (where I e.g. might need to fetch additional user-data before redirecting).

Given that you've nicely laid out the library with utility functions that are composed in authentication, maybe the natural recommendation would be for the end-user to use those functions directly in middleware.

Wdyt?

awinogrodzki commented 1 year ago

@steve-marmalade for cases you mentioned you can use getAuthenticatedResponse function described in options section. Whatever response you return there, it would be returned by middleware.

There might be scenarios not covered by authentication. In such cases I would suggest to create custom authentication function based on the one in library: https://github.com/awinogrodzki/next-firebase-auth-edge/blob/main/src/next/middleware.ts#L95

awinogrodzki commented 1 year ago

I think I see your point now. getAuthenticatedResponse is a synchronous function, so you can access tokens and modify final response, but it won't let you wait for any asynchronous action such as data load.

What if I make getAuthenticatedResponse asynchronous? Do you think it'll cover all of your use-cases?

steve-marmalade commented 1 year ago

Yes, I think that would work. So concretely what I'd be doing is moving most of the business logic that's currently in my middleware into a function that I provide to getAuthenticatedResponse? Seems like a reasonable place to start, although I will just caveat that this new approach is more monolithic so there may be edge cases that come up where more flexibility is needed. Will have to try it out and see!

awinogrodzki commented 1 year ago

Yes, basically. I also felt that way and tried to come up with more "functional" approach, but this was the neatest way I could abstract away all the repetitive logic. Think of getAuthenticatedResponse as a nested middleware decorated with authentication details.

I was thinking on an alternative approach where function would return both NextResponse and Tokens (id + decoded token), as such:

// middleware body
const { tokens, response } = await authenticate(request, options);

if (tokens === null) {
  return NextResponse.redirect('/login');
}

const data = await doSomethingWithTokens(tokens);

// response is instance of NextResponse possibly decorated by `Set-Cookie` headers containing new or refreshed credentials

if (data.shouldDecorateResponse) {
   return NextResponse.redirect('/special-url', { headers: response.headers }); // User has to handle this every time they want to decorate a response
}

return response;

Users would have to remember to copy returned response headers every time they update the response.

getAuthenticatedResponse is a tiny abstraction that inverses this problem. Now user has to provide a response, which will be later decorated by us with specific headers. You could just do:

getAuthenticatedResponse(tokens: Tokens): Promise<NextResponse> {
  const data = await doSomethingWithTokens(tokens);

  if (data.shouldDecorateResponse) {
    return NextResponse.redirect('/special-url');
  }

  return NextResponse.next();
}

Please note that above example is not yet implemented (will be in an hour or so).

Also, I created authentication function in such manner, that it would be easy to copy and implement by user, which should give you "ultimate" control over the flow. :)

Let me know what you think. It may be that you have some different idea in mind.

awinogrodzki commented 1 year ago

Async getAuthenticatedResponse and getErrorResponse are now available in next-firebase-auth-edge@0.4.2

steve-marmalade commented 1 year ago

Thanks for all of the context @awinogrodzki, your reasoning makes sense to me.

If I want to access the request within getAuthenticatedResponse, should I be writing that function as a closure, or do you think the request is something that this library should be passing into getAuthenticatedResponse?

steve-marmalade commented 1 year ago

What do you think about allowing users to provide a redirect function, either instead of or in addition to the redirectOptions? I am finding that I could use more control over that building that URL, and so providing my own function would be convenient.

awinogrodzki commented 1 year ago

Thanks for all of the context @awinogrodzki, your reasoning makes sense to me.

If I want to access the request within getAuthenticatedResponse, should I be writing that function as a closure, or do you think the request is something that this library should be passing into getAuthenticatedResponse?

In Next.js middleware request should not be modified, so in order to avoid ambiguities I decided not to pass request as a parameter limiting user to writing this function as a closure. Of course, you can still extract it, by doing something like this:

async function getAuthenticatedResponse(request: NextRequest, tokens: Tokens) {
  // your logic
  return NextResponse.next();
}
export async function middleware(request: NextRequest) {
 return authentication(request, {
   // ...other options
   getAuthenticatedResponse: tokens => getAuthenticatedResponse(request, tokens)
 })
}

I am now starting to rethink this. The question is – what is the possible use case where you would need request details as well as tokens? Is it common enough?

awinogrodzki commented 1 year ago

What do you think about allowing users to provide a redirect function, either instead of or in addition to the redirectOptions? I am finding that I could use more control over that building that URL, and so providing my own function would be convenient.

You got a point here. I didn't think about it at first, but redirect function would be much more friendly solution to this. I will allow both solutions, just to keep backwards compatibility (at least until 1.0.0) :-) Thank you @steve-marmalade!

steve-marmalade commented 1 year ago

Thanks for the responses.

The question is – what is the possible use case where you would need request details as well as tokens?

My immediate use-case is redirecting conditionally based on the requested URL and the current state of the user (given by tokens). Concretely, I am currently bypassing isTokenValid and handling all redirect logic in getAuthenticatedResponse because there is more than one state a user can be in (so a global notion of "validity" doesn't make sense for this use case), and we need to handle them separately. e.g. some pages are unauthenticated, some require email verification, some require other user properties to be completed, etc.

An ambitious approach (just as a thought experiment) would be to support a waterfall of request paths to match, and each set of paths has its own isTokenValid, redirectFunction, and getAuthenticatedResponse function. In this case, I probably wouldn't need the request object in getAuthenticatedResponse.

awinogrodzki commented 1 year ago

In 0.4.3 I introduced getUnauthenticatedResponse. It's basically redirectFunction you have mentioned, but matches naming convention of the other functions. In readme you can find a redirect example:

getUnauthenticatedResponse: async () => {
      if (request.nextUrl.pathname === "/login") {
        return NextResponse.next();
      }

      // Redirect to /login?redirect=/prev-path when request is unauthenticated
      const url = request.nextUrl.clone();
      url.pathname = "/login";
      url.search = `redirect=${request.nextUrl.pathname}${url.search}`;
      return NextResponse.redirect(url);
}

I'll definitely work on naming and descriptions before 1.0.0 release. I am not sure if it's descriptive enough. Maybe you have some naming suggestions?

steve-marmalade commented 1 year ago

getUnauthenticatedResponse seems like a reasonable name to me. Will test it out and report back, thanks!

awinogrodzki commented 1 year ago

Thanks for the responses.

The question is – what is the possible use case where you would need request details as well as tokens?

My immediate use-case is redirecting conditionally based on the requested URL and the current state of the user (given by tokens). Concretely, I am currently bypassing isTokenValid and handling all redirect logic in getAuthenticatedResponse because there is more than one state a user can be in (so a global notion of "validity" doesn't make sense for this use case), and we need to handle them separately. e.g. some pages are unauthenticated, some require email verification, some require other user properties to be completed, etc.

An ambitious approach (just as a thought experiment) would be to support a waterfall of request paths to match, and each set of paths has its own isTokenValid, redirectFunction, and getAuthenticatedResponse function. In this case, I probably wouldn't need the request object in getAuthenticatedResponse.

Thanks for the detailed description. I think I now can imagine your use case. isTokenValid is synchronous, and it does receive only DecodedIdToken. We don't have access to idToken string at this point, as isTokenValid is supposed to fast check the token data against specific permissions.

getAuthenticatedResponse is the only place to achieve what you're describing, but it has limitations.

The limitation comes from the fact that each of the options: isTokenValid, getAuthenticatedResponse, getErrorResponse and getUnauthenticatedResponse is called separately in context of every request. It's a challenge, but it would be nice to achieve ultimate separation of different authentication contexts. What comes to my mind is Strategy pattern. We could have something like (please don't hesitate to suggest improvements, I am thinking fast here):

interface AuthenticationStrategy {
  isTokenValid: (token: DecodedIdToken) => boolean;
  getAuthenticatedResponse: (tokens: Tokens) => Promise<NextResponse>;
  getUnauthenticatedResponse: () => Promise<NextResponse>;
  getErrorResponse: (error: unknown) => Promise<NextResponse>;
}

function getAnonymousStrategy(request: NextRequest): AuthenticationStrategy {
  return {
    isTokenValid: (token) => true,
    getErrorResponse: async (error) => NextResponse.next(),
    getAuthenticatedResponse: async () => NextResponse.next(),
    getUnauthenticatedResponse: async () => NextResponse.next()
  };
}

function getEmailStrategy(request: NextRequest): AuthenticationStrategy {
  return {
    isTokenValid: (token) => token?.email_verified ?? false,
    getErrorResponse: async (error) => NextResponse.redirect("/email-login"),
    getAuthenticatedResponse: async () => NextResponse.next(),
    getUnauthenticatedResponse: async () =>
      request.nextUrl.pathname !== "/email-login"
        ? NextResponse.redirect("/email-login")
        : NextResponse.next(),
  };
}

function getPermissionStrategy(
  request: NextRequest,
  permissions: string[]
): AuthenticationStrategy {
  return {
    isTokenValid: (token) => {
      const customClaims = filterStandardClaims(token);

      return permissions.every((permission) =>
        customClaims.has_permissions?.includes(permission)
      );
    },
    getErrorResponse: async (error) => NextResponse.next({ status: 401 }),
    getAuthenticatedResponse: async (tokens) => {
      if (await checkPaymentStatus(tokens)) {
        return NextResponse.next();
      }

      return NextResponse.redirect("/billing");
    },
    getUnauthenticatedResponse: async () => NextResponse.next({ status: 401 }),
  };
}

function getStrategy(request: NextRequest): AuthenticationStrategy {
  if (ANONYMOUS_URLS.includes(request.nextUrl.pathname)) {
    return getAnonymousStrategy(request);
  }

  if (EMAIL_URLS.includes(request.nextUrl.pathname)) {
    return getEmailStrategy(request);
  }

  if (ADMIN_URLS.includes(request.nextUrl.pathname)) {
    return getPermissionStrategy(request, ["view", "read", "write"]);
  }

  return getPermissionStrategy(request, ["view"]);
}

export async function middleware(request: NextRequest) {
  const strategy = getStrategy(request);
  return authentication(request, {
    loginPath: "/api/login",
    logoutPath: "/api/logout",
    apiKey: serverConfig.firebaseApiKey,
    cookieName: "AuthToken",
    cookieSignatureKeys: ["secret1", "secret2"],
    cookieSerializeOptions: {
      path: "/",
      httpOnly: true,
      secure: false, // Set this to true on HTTPS environments
      sameSite: "strict" as const,
      maxAge: 12 * 60 * 60 * 24 * 1000, // twelve days
    },
    serviceAccount: serverConfig.serviceAccount,
    // Strategy
    getUnauthenticatedResponse: strategy.getUnauthenticatedResponse,
    getAuthenticatedResponse: strategy.getAuthenticatedResponse,
    getErrorResponse: strategy.getErrorResponse,
    isTokenValid: strategy.isTokenValid
  });
}
awinogrodzki commented 1 year ago

Also, probably getAuthenticatedResponse is not the best name, since in your case it sometimes redirects users that are not in fact authenticated. By Authenticated I meant has valid token attached to the request, but the function itself can throw errors and redirect to wherever we want. :-)

steve-marmalade commented 1 year ago

Your Strategy pattern is exactly the kind of thing I was getting at with the suggestion of a "waterfall" of paths, each with their own suite of authentication functions. Such an API would definitely allow me to use things like isTokenValid and getUnauthenticatedResponse more regularly (instead of putting everything into getAuthenticatedResponse).

However, there is still one edge case that's not covered in your initial implementation, which is allowing multiple strategies for a single route. Consider the case where, for a given route, we first run getEmailStrategy but, if it's not valid, we want to run a different strategy (i.e. do they need to verify their account or are they a new user?). So this would be like specifying another strategy as the getUnauthenticatedResponse. Not sure on the design (perhaps some kind of StrategyPipeline :joy: ), just trying to flesh out the use case.

I am not a big stickler on the names, although I see your point. It could be something as direct as handleValidToken, handleInvalidToken and handleError.

FWIW I don't think I'm blocked on migrating to 0.4.3 at the moment, since I believe your latest update gives me what I need to do all of the logic in getAuthenticatedResponse and getUnauthenticatedResponse. But your Strategy idea might still be an overall design worth working towards.