BioDrop icon indicating copy to clipboard operation
BioDrop copied to clipboard

[FEATURE] Move premium user check to middleware

Open dmitrykulakovfrontend opened this issue 1 year ago • 13 comments

Is this a unique feature?

  • [X] I have checked "open" AND "closed" issues and this is not a duplicate

Is your feature request related to a problem/unavailable functionality? Please describe.

For /pages/account/statistics we are using "getServerSideProps" redirection instead of using middleware.js. The middleware ends code duplication and reduces possibility of introducing errors.

Proposed Solution

I'm proposing we consolidate authentication checking using only middleware.js.

Screenshots

image

Do you want to work on this issue?

Yes

If you want to work on this issue...

I would add in middleware.js a code that would look at current user route and if it is /account/statistics and user account type is not premium, it would redirect him

dmitrykulakovfrontend avatar Oct 26 '23 19:10 dmitrykulakovfrontend

To reduce notifications, issues are locked until they are https://github.com/EddieHubCommunity/BioDrop/labels/%F0%9F%8F%81%20status%3A%20ready%20for%20dev and to be assigned. You can learn more in our contributing guide https://github.com/EddieHubCommunity/BioDrop/blob/main/CONTRIBUTING.md

github-actions[bot] avatar Oct 26 '23 19:10 github-actions[bot]

The issue has been unlocked and is now ready for dev. If you would like to work on this issue, you can comment to have it assigned to you. You can learn more in our contributing guide https://github.com/EddieHubCommunity/BioDrop/blob/main/CONTRIBUTING.md

github-actions[bot] avatar Oct 26 '23 19:10 github-actions[bot]

This will clean up the code. I am not sure how possible it is to get the session in the middleware to do the check though - so we might have some challenges

ℹ️ eddiejaoude has some opened assigned issues: 🔧View assigned issues

eddiejaoude avatar Oct 26 '23 19:10 eddiejaoude

Upon investigation there are 3 different path forward that have been investigated so far. Each of them having there own pros and cons.

Next.js "middleware.js" runs into a separate light weight runtime environment called Edge.

"The Edge Runtime offers lower latency, higher scalability, and better security than Node.js, but it has some limitations, such as not supporting native Node.js APIs."

There is an ongoing discussion for middleware.js to allow Node.js runtime: https://github.com/vercel/next.js/discussions/46722

dmitrykulakovfrontend avatar Nov 03 '23 18:11 dmitrykulakovfrontend

Proposal 1

Adding accountType on the JWT Token

pages/api/[...next-auth].js

async jwt({ token, account, profile }) {
      // Persist the OAuth access_token and or the user id to the token right after signin
      if (account) {
        token.accessToken = account.access_token;
        token.id = profile.id;
        token.username = profile.login;
      }
      // we added this code
      const user = await User.findOne({ _id: token.sub });
      if (user) {
        token.accountType = user.type;
      } else {
        token.accountType = "free";
      }
      return token;
    },

middleware.js

We use the accountType property from the session(JWT Token) object to create a logic within the middleware.js to ascertain whether the user is a free or premium user.

  const session = await getToken({
    req: req,
    secret: process.env.NEXTAUTH_SECRET,
  });

console.log(session.accountType) // "Premium" "Free"

kbventures avatar Nov 03 '23 19:11 kbventures

The advantage of this solution is that it leverages existing code (next-auth), but a drawback is that we would need to reset the JWT Secret, resulting in the logout of all users.

dmitrykulakovfrontend avatar Nov 03 '23 19:11 dmitrykulakovfrontend

Solution 2

As a work around to being unable to make MongoDB calls from the middleware.js file:

middleware.js

Make a request to Next.js API end point and include in the header an authenticated cookie(required by next-auth).:

const res = await fetch("http://localhost:3000/api/getSession", {
method: 'GET',
headers: {
"cookie": req.headers.get("cookie")
}
});
const serverSession = await res.json();

pages/api/getSession.js

import { authOptions } from "./auth/[...nextauth]";


export default async function handler(req, res) {
if (req.method != "GET") {
return res
.status(400)
.json({ error: "Invalid request: GET request required" });
}
const session = await getServerSession(req, res, authOptions);


return res.status(200).json(session);
}

CON: Making an API and database requests could be an additional resource burden. PROS: No need to change the JWT Secret key.

kbventures avatar Nov 03 '23 19:11 kbventures

Thank you for the different solutions, I will go through in more details tomorrow. Please note: we have the session and the mongodb connection in the middleware already here...

https://github.com/EddieHubCommunity/BioDrop/blob/b11b991cbc56f20066c694763f0e4e92597fcfe9/middleware.js#L31-L34

eddiejaoude avatar Nov 03 '23 19:11 eddiejaoude

Thank you for the different solutions, I will go through in more details tomorrow. Please note: we have the session and the mongodb connection in the middleware already here...

https://github.com/EddieHubCommunity/BioDrop/blob/b11b991cbc56f20066c694763f0e4e92597fcfe9/middleware.js#L31-L34

But according to the next auth documentation: https://next-auth.js.org/tutorials/securing-pages-and-api-routes#using-gettoken

getToken function is being used to get jwt token information, which doesn't contains accountType property

dmitrykulakovfrontend avatar Nov 03 '23 19:11 dmitrykulakovfrontend

getToken function is being used to get jwt token information, which doesn't contains accountType property

We add this to the session here

https://github.com/EddieHubCommunity/BioDrop/blob/b257fc2a05b20edcadec124c09e8ba27f826a898/pages/api/auth/%5B...nextauth%5D.js#L74

eddiejaoude avatar Nov 06 '23 23:11 eddiejaoude

getToken function is being used to get jwt token information, which doesn't contains accountType property

We add this to the session here

https://github.com/EddieHubCommunity/BioDrop/blob/b257fc2a05b20edcadec124c09e8ba27f826a898/pages/api/auth/%5B...nextauth%5D.js#L74

You right, but there is a difference between session and jwt token, session is being stored in the database (to which we don't have access due to edge runtime), and jwt is being stored in user cookie, here an example of what jwt actually looks like:

const session = await getToken({
    req: req,
    secret: process.env.NEXTAUTH_SECRET,
  });
  console.log(session)
  /*
    {
      name: 'Dmitry Kulakov',
      email: '[email protected]',
      picture: 'https://avatars.githubusercontent.com/u/36533957?v=4',
      sub: '6542306a8e45c14d99cb27ea',
      accessToken: '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',
      id: 36533957,
      username: 'dmitrykulakovfrontend',
      iat: 1699335414,
      exp: 1701927414,
      jti: '74aa1844-6e30-403e-bab7-d7e611884d20'
    }
  */

This is the reason why we were suggesting Solution #1, which would basically do the same thing like session.accountType = user.type; but with jwt token instead. But because that would create a difference between users with new jwt tokens and old we would need to sign out everyone and make sure that everyone has same jwt, by changing the JWT Secret.

I think it is some kind of missunderstanding naming result of getToken as session

dmitrykulakovfrontend avatar Nov 07 '23 05:11 dmitrykulakovfrontend

Oh I see, now I think I understand better, thank you for explaining 👍

ℹ️ eddiejaoude has some opened assigned issues: 🔧View assigned issues

eddiejaoude avatar Nov 07 '23 16:11 eddiejaoude

The current naming convention is confusing and it will be remedied in the solution we propose. (session is actually JWT token).

const session = await getToken({
    req: req,
    secret: process.env.NEXTAUTH_SECRET,
  });

  // if no session reject request
  if (!session) {
    if (reqPathName.startsWith("/api")) {
      return NextResponse.json({}, { status: 401 });
    }
    return NextResponse.redirect(new URL("/auth/signin", req.url));
  }

session should be renamed to something resembling JWT Token.

kbventures avatar Nov 07 '23 17:11 kbventures