[RFC] Page Authentication and Authorization

[Acrylic125]

Page Authentication and Authorization (Auth)

Based off PR: #120

As of me writing this, pages are exposed to the public, meaning they are not protected.

Preface

There are many ways to implement this such as using middleware to do the auth protection. However, this can get tedious and prone to issues as every new page we create, we have to manually hardcode in the pages that requires the necessary protection.

Proposed Solution

In the above proposed PR, a component called AuthGuard does the auth protection on a Page level, removing the need for a middleware.

How does it work?

A component called AuthGuard is mounted on the App level (_app.jsx). We pass in a prop, authGuard, which determines whether the requester user is allowed to access a page.

In the page that we want to protect, we can bind a boolean or a function that returns a boolean, to the Page component.

Example

users.jsx

const Page = () => <div>Users</div>;

Page.authGuard = async (user) => {
    const supabase = createBrowserSupabaseClient();
    const {data, error} = await supabase.rpc('is_sysadmin', {
      user_id: user.id,
    });
    if (error) throw error;
    return data;
};

The above example will check if the user is an admin. If the user is an admin, then they can access the users page.

[Eravar1]

We still need a similar check for non-admin users to access protected pages like the marketplace I think

[Acrylic125]

that can be done like so:

Page.authGuard = true;

By marking it as true, there is no predicate to check against but it still requires the user to be authed.

[Axiver]

As for middlewares being tedious to implement as every page needs to be hardcoded in, I don't think that's necessarily true?

According to their doumentation, we can include/exclude entire routes that the middleware will run on. https://nextjs.org/docs/advanced-features/middleware

So for example, to get the middleware to run on, and in turn protect every page in the admin app, all we would need to do would be

export const config = {
  matcher: '/:path*',
}

in our middleware file.

We can also exclude certain pages like /login and /register by doing the following:

export async function middleware(req, ev) {
  const { pathname } = req.nextUrl;

  if (pathname === "/login" || pathname === "/register") {
    return NextResponse.next();
  }

 ...
}

I think it would be better to handle authentication checks in middlewares because that way, the check would occur before the page is served to the user.

[Acrylic125]

As for middlewares being tedious to implement as every page needs to be hardcoded in, I don't think that's necessarily true?

According to their doumentation, we can include/exclude entire routes that the middleware will run on. https://nextjs.org/docs/advanced-features/middleware

So for example, to get the middleware to run on, and in turn protect every page in the admin app, all we would need to do would be

export const config = {
  matcher: '/:path*',
}

in our middleware file.

We can also exclude certain pages like /login and /register by doing the following:

export async function middleware(req, ev) {
  const { pathname } = req.nextUrl;

  if (pathname === "/login" || pathname === "/register") {
    return NextResponse.next();
  }

 ...
}

I think it would be better to handle authentication checks in middlewares because that way, the check would occur before the page is served to the user.

Then how would you propose we do authorization? There will be many different rules. For instance, some pages should only allow admins, some allow admins + the user who has access to the resource, some are strictly restricted to 2 users (i.e. chats).

As of now, the way I see it is to shove everything in this 1 middleware file OR control it via getServerSideProps. Both will work but we have to consider the following:

1. Do we want to have a custom loader as the user is being loaded in? What sort of feedback response do we want to give to the user?
2. How easy is it to work with your proposed solution? Say if we rely on getServerSideProps. What sort of utility or abstraction do we want to write so that we can reduce boilerplate? Keep in mind, with getServerSideProps, we need to return a redirect and that can get super tedious so it will be best if we write our own function to wrap getServerSideProps.
3. Are the features we are using soon to be deprecated? With NextJS 13, there is a shift away from getServerSideProps. Yes, our project isnt using it now but does that mean we wont ever migrate?

So I am not saying doing it on the server side is bad, just that the way I see it being done on the server side have some issues we should iron out.

[Acrylic125]

Also this solution was suggested by @kKar1503 maybe he can weigh in on this?

[Acrylic125]

I should also address some issues with this solution so we can best consider how we want to approach this.

1. Complexity. This implementation has not been easy to implement and it is likely I forgot to address an edge case when developing this POC.
2. We have the ability to do custom loaders but it may not be necessary.
3. There is already a layer of abstraction made so component authentication but this has been proven to be good enough for all our use case.

Maybe @Axiver you can highlight more issues to do with this approach.

[kKar1503]

Is there a reason we're doing it with props instead of context? I'm sure the one I was sharing utilised context. I can draw out the logic from my project and showcase a PET project of the use case, give me till end of Friday, I can showcase the sample

[Acrylic125]

Hm. What will the context provide? The session? The auth predicate?

[Acrylic125]

If it's the session: Supabase already provides us with the session provider through useSupabaseClient(). To me, I dont see a need to reprovide the session.

If it's the authGuard prop: I dont think there is a need to share this property to the other components other than the AuthGuard component.

[Axiver]

Then how would you propose we do authorization? There will be many different rules. For instance, some pages should only allow admins, some allow admins + the user who has access to the resource, some are strictly restricted to 2 users (i.e. chats).

As of now, the way I see it is to shove everything in this 1 middleware file OR control it via getServerSideProps. Both will work but we have to consider the following:

1. Do we want to have a custom loader as the user is being loaded in? What sort of feedback response do we want to give to the user?
2. How easy is it to work with your proposed solution? Say if we rely on getServerSideProps. What sort of utility or abstraction do we want to write so that we can reduce boilerplate? Keep in mind, with getServerSideProps, we need to return a redirect and that can get super tedious so it will be best if we write our own function to wrap getServerSideProps.
3. Are the features we are using soon to be deprecated? With NextJS 13, there is a shift away from getServerSideProps. Yes, our project isnt using it now but does that mean we wont ever migrate?

So I am not saying doing it on the server side is bad, just that the way I see it being done on the server side have some issues we should iron out.

Authorisation Rules

For authorisation, I only see 2 different rules:

1. All pages on the admin project should be admin-only
2. All pages on the marketplace project should be logged-in users only

I came to this conclusion because:

• There are no scenarios where admins & users have access to the same resource (none of the admin pages should be accessible by the average user)
• There are no scenarios where only 2 users have access to a specific functionality, not even chats, because chats should be rendered in a single route, where the view changes depending on the user that is viewing it (???)
  • Like unless you're telling me there will be a chat.jsx per every chat between two users that exists in the system, I don't see how this is a concern

Therefore, only 1 middleware file is required per app.

getServerSideProps

We do not need to use getServerSideProps because Supabase has a very nice createMiddlewareSupabaseClient that we can use.

As getServerSideProps will not be used, there is no risk of any deprecation.

Proposed Solution

So how does this all tie together?

Following the sample middleware implementation from Supabase's documentation, we can see that createMiddlewareSupabaseClient can be used to create a Supabase client lol.

Using this Supabase client we can retrieve the user's session object, which we can then do checks against.

Example

middleware.js

// This function can be marked `async` if using `await` inside
export async function middleware(req) {
  // We need to create a response and hand it to the supabase client to be able to modify the response headers.
  const res = NextResponse.next();

  // Create authenticated Supabase Client.
  const supabase = createMiddlewareSupabaseClient({ req, res });

  // Check if we have a session
  const {
    data: { session },
  } = await supabase.auth.getSession();

  // Check if user is authenticated
  if (session) {
    // User is authenticated, check if the user is an admin
    const { data, error } = await supabase.rpc('is_sysadmin', {
      userid: session.user.id,
    });

    if (error) throw error;

    if (data) {
      // The user is an admin, forward request to protected route.
      return res;
    }
  }

  // Auth condition not met, redirect to home page.
  const redirectUrl = req.nextUrl.clone();
  redirectUrl.pathname = '/login';
  return NextResponse.redirect(redirectUrl);
}

A complete solution with route inclusion & exclusion can be found in #122

[Acrylic125]

Hmmm not entirely true. I am not sure about the chat implementation detail but regardless, we cannot guarantee the 2 rules you mentioned strictly applies to all future use cases. This pose a scalability risk as we have to constantly update the middleware file which can get tedious. The reason why I brought up the use of getServerSideProps is due to it being an alternative to adding it all in the middleware file. That way, page files take control in specifying the auth rules of the page.

Anyways, I was looking through your sample and it doesnt seem to do authorization. Maybe update it to include how you think we can do authorization with the middleware approach? So maybe do an admin protection for /users and lets see how it goes.

There are other issues. Yes we can use matchers however they are best suited for pages that are part of a subpath like /auth or /admin-protected. That said, in this project, if we need to refactor pages, we will cause a giant cascade of issues if we do decide to move pages into subpaths as there are already Link elements being used so refactoring is not going to be fun.

If we do decide to hardcode the routes using if statements, again its tedious.

[Axiver]

Hmmm not entirely true. I am not sure about the chat implementation detail but regardless, we cannot guarantee the 2 rules you mentioned strictly applies to all future use cases. This pose a scalability risk as we have to constantly update the middleware file which can get tedious. The reason why I brought up the use of getServerSideProps is due to it being an alternative to adding it all in the middleware file. That way, page files take control in specifying the auth rules of the page.

Anyways, I was looking through your sample and it doesnt seem to do authorization. Maybe update it to include how you think we can do authorization with the middleware approach? So maybe do an admin protection for /users and lets see how it goes.

There are other issues. Yes we can use matchers however they are best suited for pages that are part of a subpath like /auth or /admin-protected. That said, in this project, if we need to refactor pages, we will cause a giant cascade of issues if we do decide to move pages into subpaths as there are already Link elements being used so refactoring is not going to be fun.

If we do decide to hardcode the routes using if statements, again its tedious.

Uhhh my sample code does do authorisation, more specifically these few lines:

// Create authenticated Supabase Client.
const supabase = createMiddlewareSupabaseClient({ req, res });

// Check if we have a session
const {
  data: { session },
} = await supabase.auth.getSession();

// Check if user is authenticated
if (session) {
  // User is authenticated, check if the user is an admin
  const { data, error } = await supabase.rpc('is_sysadmin', {
    userid: session.user.id,
  });

  if (error) throw error;

  if (data) {
    // The user is an admin, forward request to protected route.
    return res;
  }
}

You can checkout b-feat/middlewares/xavier and play around with it if you want. That branch has every admin route protected.

Regarding matchers, as we want the middleware to protect almost every route, it can be used to exclude routes using regex instead (see NextJS documentation on negative lookaheads)

In the sample code in #122, you can observe the following:

export const config = {
  matcher: '/((?!_next|api/auth).*)(.+)',
};

This matcher config excludes the following routes:

• /_next
• /api
• /auth
• Any file in /public

This means that the middleware will run on every page in the admin project. We only want to unprotect 2 routes, namely /login and /register, so that can just be done using if statements. which is the recommended way to do it aside from using the matcher config, as per NextJS documentation

There is absolutely no need to refactor any pages if we use middlewares.

[Acrylic125]

Right then here comes a problem. Now it is simple access control for the admin pages. We can safely assume all pages in the admin side will require the user to be an admin.

But we cannot say the same for the main side of the project. As mentioned previously, we cannot guarantee what future rules is to be enforced. Like for example, just now I thought of this, is there an edit product page? If so the authorization rule is to enforce users and users of the same company to edit the same product. So as you can see we are probably dealing with use cases which we will encounter that we dont know now.

In your middleware solution, you are proposing to add if checks based on routes AND add the rule accordingly.

If so, clearly this 1 middleware is doing a lot and can get super overwhelming and unmaintainable quickly.

Do you have a different way to do this? Maybe come up with some abstraction to better handle this to avoid a massive function.

[Acrylic125]

Ill address any input on this matter in the morning 😃

[Axiver]

Ahh I see what's going on now.

You're thinking based on the idea that any and all redirects out of a certain page should be handled by only the middleware, but I'm thinking more on the lines of "the middleware should be used for general authorisation".

Clarifications

Here's what I mean by this:

• The middleware's sole job should be only to restrict the user from accessing routes. But it's job isn't to restrict users from accessing a variation of a route (e.g. /edit-listing?id=3)
  • So like in my example code, the middleware prevents the user from accessing basically the entire admin application if they are not logged in as an admin user
  • Same should be done for the main marketplace application
• The prevention of authenticated users from accessing something they do not have access to, like /edit-listing?id=3 should be done on the page-level, so this is where we agree.

Justification for middleware

The reason we want the middleware to perform general authorisation is because we don't want non-authenticated users to know about the existence (or non-existence) of any page if they are not logged in, and the middleware prevents that.

Not only that, protecting admin-only or user-only routes with only the AuthGuard makes it tedious and repetitive, both of which I know are qualities that you hate. This is because if we were to protect, let's say admin-only pages, the following sample code you provided would need to be included in every page file:

Page.authGuard = async (user) => {
    const supabase = createBrowserSupabaseClient();
    const {data, error} = await supabase.rpc('is_sysadmin', {
      user_id: user.id,
    });
    if (error) throw error;
    return data;
};

Limitations of the current AuthGuard implementation

In my initial reply (which has now been deleted), I mentioned that we could use AuthGuard (on top of the middleware) to redirect the user away from something like, a listing edit page that edits a listing that they are not allowed to edit (edit-listing?id=3).

But after much thinking, I realised that even something like that (which I assume is your intended use for AuthGuard, alongside protecting routes from general user/admin authorisation) would not be easily possible with the way AuthGuard is currently implemented.

Example scenario

To set the scene, let's assume that we have a route called edit-listing which allows users to edit a particular listing. To be able to function properly, the page would need to do the following:

1. The user's id would need to be obtained
2. The listing id would need to be obtained from the url
3. We would need to attempt to retrieve the listing details from Supabase
4. No listing retrieved = no access / doesn’t exist

Now, say we have a listing with id of 3, and that only the users with id 1 and 2 should be allowed to edit it, because they are the only two users under the company that created said listing.

Another user with id 4 is trying to do some tomfoolery and decides to access /edit-listing?id=3. To prevent access and redirect the user away from the page, the following would need to be done within AuthGuard:

1. The user's id would need to be obtained
2. The listing id would need to be obtained from the url
3. We would need to attempt to retrieve the listing details from Supabase
4. No listing retrieved = no access / doesn’t exist

You'd suddenly notice that AuthGuard needs to perform the exact same operations and requests that would already be done by the page.

Instead of performing the operations and requests twice, why not just have the page do the redirect, instead of having a wrapper component, AuthGuard, do it?

It would be a simple few lines of code that can be added after the 4 example operations provided were already complete:

// Redirect the user if no listing was retrieved
if (!isLoading && (!queryData.data || queryData.data.length === 0)) {
  // No listing was retrieved
  router.push('/listings');
}

Because remember, no listing would be retrieved in the first place due to RLS.

Additional limitations

By the way, we probably don't want onFailAuthGuard to always redirect the user back to the login page like it currently does. If AuthGuard were used in the provided example scenario, when onFailAuthGuard is invoked, redirecting the user back to the login page is odd because the user should already be logged in. I think onFailAuthGuard should be a prop that can be passed in by the page if we are to proceed with AuthGuard so that the page can specify where it wants the user to be redirected to.

[Acrylic125]

The middleware's sole job should be only to restrict the user from accessing routes. But it's job isn't to restrict users from accessing a variation of a route (e.g. /edit-listing?id=3)

* So like in my example code, the middleware prevents the user from accessing basically the entire admin application if they are not logged in as an admin user

* Same should be done for the main marketplace application

Hm it seems this form of control should be handled the same way as they are classed under 'Authorization'. Furthermore, this 2 sources of control is prone to bugs as there will be another 'layer' to be concerned about. Why would this be an issue? Think of it like using RLS. The risk there is that we have 1 more thing to worry about. We might miss THAT IT IS ENABLED WITH NO POLICIES :)

The reason we want the middleware to perform general authorisation is because we don't want non-authenticated users to know about the existence (or non-existence) of any page if they are not logged in, and the middleware prevents that.

Hm... wdym by "don't want non-authenticated users to know about the existence"? Cus AuthGuard does that. I am guessing you want to prevent the files from being sent to the user from all the imports so as to reduce payload. To that I say, ye fair enough.

Not only that, protecting admin-only or user-only routes with only the AuthGuard makes it tedious and repetitive, both of which I know are qualities that you hate.

You know me too well 😄 The way I think this can be solved is by creating these functions in some api package or something so we can reuse.

Instead of performing the operations and requests twice, why not just have the page do the redirect, instead of having a wrapper component, AuthGuard, do it?

Hm I still do not see where and how it would need to be done 'twice' (At least on the page level). Lets say we have an RPC, does_user_belong_to_company. We are not doing the restriction on the page directly say, when the user submits the edit. The RLS policy associated with the edit is controlling whether the user can edit the given listing.

Restrictions of calls to supabase should EITHER be handled via RLS or an API EP we make. Both cases will cause the access control of the action to be handled somewhere else other than the page.

The access control of the initial page load is thus controlled by us which can be done with 1 call.

So why do we need a wrapper? Well the answer is one of my favorite phrases, SEPARATION OF CONCERNS!!!!!!!!

The idea is to break up the responsibility of auth away from the page directly so as to avoid confusion and to help with debugging since we will have a central place to do access control.

[kKar1503]

@Axiver @Acrylic125 The designed that I shared in class uses the approach that can be demonstrated in the following repo: https://github.com/kKar1503/nextjs-context-auth This mainly uses React Context, and Guard Components to conditional render its nested children. The perks of this design is its maintainability and extensibility.

Because it utilizes custom PageProps like the guestGuard and aclAbilities declared in each page component to define its own authentication and authorization, it makes the overall design cleaner and reduce the need of even like a "router file" which contains the necessary authentication and authorization information.

It makes things each to maintain overall because all the 3 Guards -- auth/AclGuard.tsx, auth/AuthGuard.tsx and auth/GuestGuard.tsx -- are designed to have "Single Responsibility", so logic can be readjusted as things go.

This repo only apply simple auth design using a simple API for login and localStorage for user authentication related stuff, but it'll not be any difficult to adopt into the Supabase design.

[Acrylic125]

@Axiver @Acrylic125 The designed that I shared in class uses the approach that can be demonstrated in the following repo: https://github.com/kKar1503/nextjs-context-auth This mainly uses React Context, and Guard Components to conditional render its nested children. The perks of this design is its maintainability and extensibility.

Because it utilizes custom PageProps like the guestGuard and aclAbilities declared in each page component to define its own authentication and authorization, it makes the overall design cleaner and reduce the need of even like a "router file" which contains the necessary authentication and authorization information.

It makes things each to maintain overall because all the 3 Guards -- auth/AclGuard.tsx, auth/AuthGuard.tsx and auth/GuestGuard.tsx -- are designed to have "Single Responsibility", so logic can be readjusted as things go.

This repo only apply simple auth design using a simple API for login and localStorage for user authentication related stuff, but it'll not be any difficult to adopt into the Supabase design.

Thanks! Really appreciate you using TS for this else I would lose my mind with wondering what props accepts what 😆 .

I got the chance to look through the sample project and generally feel the design you are proposing is a little complicated.

My main issue is with the Guard component.

const Guard = ({ children, authGuard, guestGuard }: GuardProps) => {
  console.log('guard', { authGuard, guestGuard });
  if (guestGuard) {
    return <GuestGuard fallback={<Loader />}>{children}</GuestGuard>;
  } else if (!guestGuard && !authGuard) {
    return <>{children}</>;
  } else {
    return <AuthGuard fallback={<Loader />}>{children}</AuthGuard>;
  }
};

From my understanding, I am guessing we would add in a component to handle each possible case of auth like to check if the client belongs to a particular company. In such cases, we would make a new guard component for that check like <CompanyGuard />.

However, what confuses me a more is the responsibility of the AclGuard. Why does it not belong to the Guard component in '_app.jsx'? What differentiates it from the other guards?

A flaw I see with the current design is that it uses the idea of 'inheritance' for guards. In the file, auth/GuestGuard.tsx, I noticed the comments,

// There's only 1 drawback in this GuestGuard design.
// It is that if you would like some pages that allow both guests and logged in user.
// This design would not support it. Since guestGuard now only supports boolean.
// If you would like to explore a more flexible version that allow more options, you
// may opt for a design that allow 3 values that allow the following behaviour:
//   • Guest Only
//   • Logged In Only
//   • Both

I am theorizing this may be heavily restrictive when we need to introduce authorization rules like whether the user belongs to a company, user is the requested user resource, etc... The moment we introduce new props and conditions to this component to do authorization, we expand the responsibility of this component. Of course, new components can be made specific for pages like User is logged in + User belongs to company. However, this risks having us to duplicate code just to have a guard component do something similar.

On another note, if we do decide to adopt this design, we can do away with AuthContext as supabase auth helpers for NextJS already provides us with a session provider.

[Acrylic125]

@Axiver @kKar1503 @Eravar1 In this thread, we have covered 3 possible strategies, mainly,

• The RFC solution (Single component to control user access).
• Using a middleware + (possibly using getServerSideProps or some way to control access on a page level)
• Guard components to implement auth strategies.

in the interest of time, let's come to an agreement on which strategy we should use by Friday?

If we still cant come to one, we can maybe gather more input from the others.

[kKar1503]

@Acrylic125 one main reason of why AclGuard is being nested within is because of dependency.

AclGuard is dependent on AuthGuard's verification, since it kinda require the "role" information from after the AuthGuard has verified that user is correctly being authenticated.

Since after all, if the AuthGuard level failed, we can prevent the need to render the children components.

[kKar1503]

From my understanding, I am guessing we would add in a component to handle each possible case of auth like to check if the client belongs to a particular company. In such cases, we would make a new guard component for that check like .

Nope, this is when the AclGuard comes more in handy, especially when used together with the @casl/ability library. This allow u to set different levels of ability, having a level beyond just the usual "admin" vs "other user".

Even if you may find that being overkill, the next method of doing such verification is at the AuthContext level storing the Company information, which is 100% no wrong as well.

[kKar1503]

I am theorizing this may be heavily restrictive when we need to introduce authorization rules like whether the user belongs to a company, user is the requested user resource, etc... The moment we introduce new props and conditions to this component to do authorization, we expand the responsibility of this component. Of course, new components can be made specific for pages like User is logged in + User belongs to company. However, this risks having us to duplicate code just to have a guard component do something similar.

No, not at all. I can show u the additional use case of combining the AclComponent with another use case that I was working with for today's work.

[Acrylic125]

132

An implementation of Auth Guard as suggested by @kKar1503

For future reference:

• This can be migrated easily to NextJS 13 and can be done on the server side, thereby covering the benefits of using a middleware.
• Possible support for dynamic CASL abilities.

[Acrylic125]

I shall be closing this issue. Thanks everyone!