Added access tokens to GraphQL request headers, updated middleware to handle authorization #60

jupiter007 · 2024-06-24T16:29:31Z

Issue: #55

Added JWT token to headers for graphql requests on both the client- and server-side components. Updated lib/graphql/client.ts and lib/graphql/apollo-wrapper.tsx
Added an example client-side component called 'client-graphql-test' to show how apollo client hooks can work on client side using the 'apollo-wrapper'.
Updated middleware.ts for authorization and redirection and updated middleware spec
Added a 'get-token' endpoint to get the token from the cookie and added unit test
Updated auth.ts to use 'jose' instead of 'jsonwebtoken' for token verification due to errors related to the edge runtime not support Node.js 'crypto' module.
Added utils:
-- utils/authLink.ts - Creates 'authLink' for the creation of the client instances
-- utils/cookiesUtil.ts - Contains a deleteCookie method
-- utils/getAuthTokenServer.ts - Gets JWT token from cookie

To Test authentication and authorization:

Run development branch of dmsp_backend_prototype. (Make sure the JWT secret is the same for backend as frontend)
Run this frontend branch and go to /signup. Signup with a new user.
Go to /login and sign in with new user. Notice the new 'dmspt' httpOnly cookie that was added
Navigate to /dmps/10.48321/D1SP4H. There are currently errors getting data for the page, but you should be able to acces that page.
Delete the 'dmspt' cookie and try navigating to /dmps/10.48321/D1SP4H. You should be redirected to /login page

To Test Token in Header for Client Side:

Navigate to /client-graphql-test and look at your Network tab and filter for graphql. You should see the token in the Authorization header

…sing client component and graphql hooks

…est-headers

… the get-token route

… and added some comments to the client-side-graphql component

jupiter007 · 2024-06-24T16:32:43Z

lib/server/auth.ts

-    // Secret is hardcoded for now
-    // TODO: Need to figure out where the secret will be stored
-    const secret = Buffer.from('Zn8Q5tyZ/G1MHltc4F/gTkVJMlrbKiZt', 'base64');
+/**


Changed to using 'jose' package to verify the token. This is because the "Edge runtime environment" does not include the 'crypto' Node.js core module, and jsonwebtoken relies on this module for cryptographic operations.

The 'jose' package is designed to work without this, using standard APIs like Web Cryptography API which is supported in browsers.

…be returned, even if null

andrewebdev · 2024-06-28T11:00:12Z

__tests__/middleware.spec.ts


-        expect(redirectSpy).toHaveBeenCalledWith(`${process.env.NEXT_PUBLIC_BASE_URL}/login`);
+        expect(redirectSpy).toHaveBeenCalledWith('http://localhost:3000/login');


Just wondering if there is a reason to hard-code the localhost url here? Keeping it as an environment variable might be preferable, especially since every dev might have their environment set up differently.

Ah, reading further into the code, looks like all of this is inside the context of a test and mocking the request/response. If this is the case then it's understandable to use a fixed url.

andrewebdev · 2024-06-28T11:00:57Z

__tests__/middleware.spec.ts

-        expect(request.cookies.delete).toHaveBeenCalledWith('dmspt');
-        expect(redirectSpy).toHaveBeenCalledWith(`${process.env.NEXT_PUBLIC_BASE_URL}/login`);
+        expect(deleteCookie).toHaveBeenCalled();
+        expect(redirectSpy).toHaveBeenCalledWith('http://localhost:3000/login');


Same as previous comment on hard-coded url. There are more in this document, but I'm not going to flag each individual one :)

Yes, @andrewebdev as you mentioned above, I mocked the request and passed in the URL so that I could check the redirect url.

andrewebdev · 2024-06-28T11:15:57Z

app/api/get-token/__tests__/route.spec.ts

+        (getAuthTokenServer as jest.Mock).mockRejectedValue(new Error('Test error'));
+
+        const response = await GET();
+        expect(response.status).toEqual(405);


405: Method not Allowed doesn't seem to be the correct error code for this case. The test doesn't seem to check if the correct method (GET, POST etc.) is used.

Further down, the error message also reads as a Internal Server Error which should be a 500 code. So it feels like things aren't matching up here.

Thanks @andrewebdev. The 500 error code totally makes sense to me since it's on the server side. I think I was copying and pasting and forgot to change that one.

andrewebdev · 2024-06-28T11:24:22Z

app/api/get-token/route.ts

+
+    } catch (err) {
+        console.error('Error fetching token:', err);
+        return NextResponse.json({ error: 'Internal Server Error' }, { status: 405 });


From the use case here it seems like 500 should be the correct code to use.

What would be the cases where a token cannot be found? We probably need to handle each of those cases with different error responses.

If the token cannot be found cause a fault on our side, then 500 would be the correct code. But if it cannot be found cause the user isn't signed in, then 401 is probably the correct code.

But most likely the status 405 here is not the right one, since the method is technically allowed, so long as you have a token.

I'm still getting used to JS, so not sure if this is a good pattern or not, but would adding a try catch block within the getAuthTokenServer() function that throws a more specific error that includes a code based on the what happened (like there was no token, the token is invalid/expired, etc.). Then check if there is a code already on the error, if so use it, otherwise it's a 500?

Thanks @andrewebdev. I will change it to statusCode of 500.

andrewebdev · 2024-06-28T11:42:21Z

lib/server/auth.ts

+ * @returns 
+ */
+export function getJwtSecretKey(): Uint8Array {
+    const secret = process.env.JWT_SECRET


Just a side note: We should probably set up a mechanism somewhere to easily rotate secret keys if needed. It's good practice to rotate them every so often to improve site security.

@briri , we should probably create an issue for this so we are reminded of it at a later stage?

My thought was to store this and probably other variables we have yet to identify in AWS Secrets manager which you can configure to rotate on a schedule.

I created issue 153 in the infrastructure repo to track the SecretsManager work

I created issue 153 in the infrastructure repo to track the SecretsManager work

Thanks @andrewebdev and @briri. Using AWS Secrets Manager is a great idea. I'll add a placeholder function for now.

I'll also create a ticket to use Secrets Manager in the NextJS app.

I created this ticket to implement Secrets Manager on the frontend: #62

@briri should I create another ticket for setting up AWS Secrets Manager?

briri

this all looks good to me

briri · 2024-06-28T15:00:08Z

app/api/get-token/route.ts

+
+    } catch (err) {
+        console.error('Error fetching token:', err);
+        return NextResponse.json({ error: 'Internal Server Error' }, { status: 405 });


I'm still getting used to JS, so not sure if this is a good pattern or not, but would adding a try catch block within the getAuthTokenServer() function that throws a more specific error that includes a code based on the what happened (like there was no token, the token is invalid/expired, etc.). Then check if there is a code already on the error, if so use it, otherwise it's a 500?

briri · 2024-06-28T15:01:29Z

app/api/get-token/route.ts

+    }
+};
+
+export const POST = () => {


these are great. I like that we explicitly block them

@briri That's a good question about adding a try-catch in getAuthTokenServer(). I thought about it but wasn't sure it was necessary because it is always returning a value or null, and I wasn't sure of the likelihood of any error happening when just getting the cookie itself.

I added a try-catch to the GET request for any possible issues that might occur outside of the getAuthTokenServer though.

briri · 2024-06-28T15:08:38Z

lib/server/auth.ts

+ * @returns 
+ */
+export function getJwtSecretKey(): Uint8Array {
+    const secret = process.env.JWT_SECRET


I created issue 153 in the infrastructure repo to track the SecretsManager work

briri · 2024-06-28T15:13:29Z

utils/authLink.ts

+        let token;
+
+        try {
+            //Server side


Thanks for noting the different contexts here. I personally am going to need these clues so I know whether something is happening server or client 🙃

… and revised error status code type to 500 for get-token

jupiter007 · 2024-06-28T15:58:05Z

@andrewebdev and @briri thanks so much for your code reviews. I really value your input. I've made the changes you requested so far. Please let me know if there is anything else you recommend that I update or add. I'm sure we'll discover that we need more changes as we progress with the project as well.

jupiter007 · 2024-07-01T16:18:08Z

Hi @briri and @andrewebdev, I consolidated my es lint configs into .eslintrc.json and fixed some linting issues that weren't caught with "npm lint". For now, I added "npx eslint ." to the "lint" script until I can figure out how to capture all directories.

I also updated some package versions.

jupiter007 added 7 commits June 13, 2024 15:54

added access token to header in client.ts and added an example page u…

7d56860

…sing client component and graphql hooks

Merge branch 'development' into feature/authorization-in-graphql-requ…

af0adc0

…est-headers

addressed linting errors

78118ce

Updated middleware.ts and the associated unit test

cb99eea

added some comments and cleaned up some auth code

e1685b9

fixed linting errors in the setCookie test, and added a unit test for…

a636da7

… the get-token route

removed unused jsonwebtoken package, fixed some test description text…

729b31f

… and added some comments to the client-side-graphql component

jupiter007 commented Jun 24, 2024

View reviewed changes

jupiter007 requested review from briri, andrewebdev and fraserclark June 24, 2024 16:33

jupiter007 added 2 commits June 25, 2024 11:44

made small update to get-token endpoint to because token will always …

6da3ebb

…be returned, even if null

added missing 'await' to verifyJwtToken call

e4e2a11

andrewebdev reviewed Jun 28, 2024

View reviewed changes

briri approved these changes Jun 28, 2024

View reviewed changes

Responded to PR comments: added a placeholder function to get secret,…

c7c0a2c

… and revised error status code type to 500 for get-token

fixed linting issues and updated versions of some packages

c841add

forgot to remove @types/cookie from package.json

be00e0c

jupiter007 merged commit 31e3b0e into development Jul 1, 2024

jupiter007 deleted the feature/authorization-in-graphql-request-headers branch August 5, 2024 15:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added access tokens to GraphQL request headers, updated middleware to handle authorization #60

Added access tokens to GraphQL request headers, updated middleware to handle authorization #60

jupiter007 commented Jun 24, 2024 •

edited

Loading

jupiter007 Jun 24, 2024 •

edited

Loading

andrewebdev Jun 28, 2024

andrewebdev Jun 28, 2024

andrewebdev Jun 28, 2024

jupiter007 Jun 28, 2024

andrewebdev Jun 28, 2024

jupiter007 Jun 28, 2024

andrewebdev Jun 28, 2024

briri Jun 28, 2024

jupiter007 Jun 28, 2024

andrewebdev Jun 28, 2024

briri Jun 28, 2024 •

edited

Loading

briri Jun 28, 2024

jupiter007 Jun 28, 2024

jupiter007 Jun 28, 2024

jupiter007 Jun 28, 2024

briri left a comment

briri Jun 28, 2024

briri Jun 28, 2024

jupiter007 Jun 28, 2024

briri Jun 28, 2024

briri Jun 28, 2024

jupiter007 commented Jun 28, 2024

jupiter007 commented Jul 1, 2024


		expect(redirectSpy).toHaveBeenCalledWith(`${process.env.NEXT_PUBLIC_BASE_URL}/login`);
		expect(redirectSpy).toHaveBeenCalledWith('http://localhost:3000/login');

Added access tokens to GraphQL request headers, updated middleware to handle authorization #60

Added access tokens to GraphQL request headers, updated middleware to handle authorization #60

Conversation

jupiter007 commented Jun 24, 2024 • edited Loading

jupiter007 Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briri Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jupiter007 commented Jun 28, 2024

jupiter007 commented Jul 1, 2024

jupiter007 commented Jun 24, 2024 •

edited

Loading

jupiter007 Jun 24, 2024 •

edited

Loading

briri Jun 28, 2024 •

edited

Loading