91- Add GitHub Authentication- client side - Ebrahim #117

ebrahimbeiati · 2025-03-22T20:33:34Z

Implemented OAuth 2.0 GitHub authentication flow

bkarimii · 2025-03-29T10:33:49Z

The PR structure doesn't follow the project rules. please fix it and add description to your PR.

oliverlloyd

There's a lot of great work here, well done!

Would it be possible to update the description to explain what the changes are and why each one is needed. It would be really useful to have an overview for things

It would also be great to see some more comments in the code explaining what the different functions and steps are doing 🙏

api/api.js

oliverlloyd · 2025-04-07T15:08:20Z

web/src/components/AuthProvider.jsx

+		}
+	}, [userData]);
+
+	const contextValue = useMemo(() => ({ userData, setUserData }), [userData]);


Do we need to use useMemo here? This doesn't look like an expensive calculation

I use useMemo to ensure that the contextValue object passed to the Context Provider maintains the same reference between renders unless userData or setUserData change. This helps prevent unnecessary re-renders of components consuming the context, optimizing performance.

Did you get this answer from AI? :)

I don't see any benefit to useMemo here. Does it work if you remove it?

web/src/components/AuthProvider.jsx

oliverlloyd · 2025-04-07T15:13:23Z

web/src/pages/Upload.jsx

 				return false;
 			}
 			setFileList((prevList) => [...prevList, file]);
-			return true; // Allow the file to be added to the list


Was there a reason for deleting this comment?

This one :)

// Allow the file to be added to the list

I noticed it was removed and wondered why 🤷

oliverlloyd

Could you update the PR description please to explain what this solution is doing and why it is doing it? 🙏

oliverlloyd · 2025-04-12T10:19:15Z

web/src/pages/Login.jsx

 import { SubscriptionLogo } from "../components/SubscriptionLogo";
 import { ThemedButton } from "../components/ThemedButton";

+const CLIENT_ID = import.meta.env.VITE_GITHUB_CLIENT_ID;


Can you explain what this line is meant to be doing? I don't think this will work

oliverlloyd · 2025-04-12T10:20:27Z

web/src/pages/Login.jsx

+
+	const handleGitHubLogin = () => {
+		window.location.assign(
+			`https://github.com/login/oauth/authorize?client_id=${CLIENT_ID}`,


The client_id is public (it is used in the url so everyone can see it) so it does not need to be kept secret and can just be entered in the code here

oliverlloyd · 2025-04-12T10:24:51Z

api/api.js

Could we review the functions in this file to add logger.info and logger.error statements so that we have data in our logs about what is happening here

If we don't log errors or info about what is happening, we won't be able to debug problems.

Note. We should try to write detailed logs but we don't want to write any sensitve information to the log such as ids, passwords or email address

oliverlloyd · 2025-04-12T10:25:24Z

api/api.js

+		const userData = await response.json();
+		req.user = userData;
+		next();
+	} catch (error) {


Everywhere where we have an error we should capture this with logger.error

oliverlloyd · 2025-04-12T10:28:56Z

api/utils/config.cjs

+module.exports.CLIENT_SECRET = process.env.GITHUB_CLIENT_SECRET;
+module.exports.CLIENT_ID = process.env.GITHUB_CLIENT_ID;


I don't think this is the correct way to include these values. We already have an existing module.exports line

Better would be to have

github: { GITHUB_CLIENT_SECRET: process.env.GITHUB_CLIENT_SECRET, // etc. }

and add this to the extsing config object

oliverlloyd · 2025-04-12T10:29:34Z

web/src/App.jsx

+import AuthCallback from "./pages/AuthCallback.jsx";
 import { ConfirmationPage } from "./pages/ConfirmationPage.jsx";
+import Dashboard from "./pages/Dashboard.jsx";


These should also be named imports. It's good to keep things consistent 🙏

oliverlloyd · 2025-04-12T10:31:36Z

api/app.js

+import bodyParser from "body-parser";
+import cors from "cors";


What was the reason for including these?

It's always good to comment your own PRs with explanations for things so it's clear to reviewers why things area happening

oliverlloyd · 2025-04-12T10:34:40Z

web/src/components/AuthProvider.jsx

+		}
+	}, [userData]);
+
+	const contextValue = useMemo(() => ({ userData, setUserData }), [userData]);


Did you get this answer from AI? :)

I don't see any benefit to useMemo here. Does it work if you remove it?

textbook · 2025-04-12T13:35:57Z

Please don't "roll your own" security. Use existing, tested tools like Passport.

There's an example of this in use in CYF/tech-products-demo, where it's all handled server-side so the client can just have a vanilla link to start the process:

              <a href="/api/auth/login">Log In</a>

ebrahimbeiati added 3 commits March 20, 2025 23:32

github auth

8b6ce7c

git authentication

70e6941

Your commit message describing the changes

9078d48

bkarimii deployed to github-authentication - slack-dashboard-two PR #117 March 22, 2025 20:33 — with Render View deployment

bkarimii deployed to github-authentication - slack-dashboard-two PR #117 March 22, 2025 21:30 — with Render View deployment

Github auth - accessToken modify, design

82c6535

ebrahimbeiati force-pushed the github-authentication branch from 259dc8c to 82c6535 Compare March 24, 2025 02:41

bkarimii deployed to github-authentication - slack-dashboard-two PR #117 March 24, 2025 02:41 — with Render View deployment

bkarimii deployed to github-authentication - slack-dashboard-two PR #117 March 24, 2025 02:49 — with Render View deployment

test updated

d71fd14

ebrahimbeiati force-pushed the github-authentication branch from 3eb224b to d71fd14 Compare March 27, 2025 20:10

bkarimii deployed to github-authentication - slack-dashboard-two PR #117 March 27, 2025 20:10 — with Render View deployment

bkarimii deployed to github-authentication - slack-dashboard-two PR #117 March 27, 2025 20:25 — with Render View deployment

Merge branch 'main' into github-authentication

dc3153b

ebrahimbeiati force-pushed the github-authentication branch from 79c8630 to dc3153b Compare March 27, 2025 20:36

bkarimii deployed to github-authentication - slack-dashboard-two PR #117 March 27, 2025 20:36 — with Render View deployment

bkarimii deployed to github-authentication - slack-dashboard-two PR #117 March 27, 2025 20:47 — with Render View deployment

review the test

71c26c1

ebrahimbeiati force-pushed the github-authentication branch from 96b4c2a to 71c26c1 Compare March 27, 2025 20:59

bkarimii deployed to github-authentication - slack-dashboard-two PR #117 March 27, 2025 20:59 — with Render View deployment

oliverlloyd reviewed Apr 7, 2025

View reviewed changes

bkarimii deployed to github-authentication - slack-dashboard-two PR #117 April 9, 2025 19:13 — with Render View deployment

ebrahimbeiati force-pushed the github-authentication branch from d472e23 to 506b414 Compare April 9, 2025 19:16

bkarimii deployed to github-authentication - slack-dashboard-two PR #117 April 9, 2025 19:16 — with Render View deployment

review the test

817c5c4

ebrahimbeiati force-pushed the github-authentication branch from 506b414 to 817c5c4 Compare April 9, 2025 19:35

bkarimii deployed to github-authentication - slack-dashboard-two PR #117 April 9, 2025 19:35 — with Render View deployment

bkarimii changed the base branch from main to staging April 10, 2025 22:23

oliverlloyd reviewed Apr 12, 2025

View reviewed changes

ebrahimbeiati changed the title ~~Add GitHub Authentication~~ 91- Add GitHub Authentication- client side - Ebrahim Apr 27, 2025

		module.exports.CLIENT_SECRET = process.env.GITHUB_CLIENT_SECRET;
		module.exports.CLIENT_ID = process.env.GITHUB_CLIENT_ID;

		import bodyParser from "body-parser";
		import cors from "cors";

91- Add GitHub Authentication- client side - Ebrahim #117

Are you sure you want to change the base?

91- Add GitHub Authentication- client side - Ebrahim #117

Uh oh!

Conversation

ebrahimbeiati commented Mar 22, 2025

Uh oh!

bkarimii commented Mar 29, 2025

Uh oh!

oliverlloyd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oliverlloyd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

textbook commented Apr 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

textbook commented Apr 12, 2025 •

edited

Loading