feat: add restricted access on github auth #353

olzhik11 · 2025-01-28T14:04:59Z

Added access control on github auth through json file

Important

Added restricted access for GitHub authentication using a JSON file to control allowed emails.

Authentication:
- Added signIn callback in auth.ts to restrict GitHub access based on emails from allowed-emails.json.
- Introduced getEmailsConfig in server-utils.ts to read allowed emails from JSON.
Components:
- Updated GitHubSignInButton in github-signin.tsx to handle sign-in errors with toast notifications.
- Simplified EmailSignInButton in email-signin.tsx by removing unused props.
Features:
- Added check for GITHUB_AUTH in features.ts to ensure GitHub credentials are set.
Misc:
- Standardized import quotes across modified files.

^{This description was created by}^{for 855bacc. It will automatically update as commits are pushed.}

dinmukhamedm

Good job. I left a couple comments

dinmukhamedm · 2025-01-30T14:16:50Z

frontend/components/sign-in/github-signin.tsx

-import { signIn } from 'next-auth/react';
-import * as React from 'react';
+import { signIn } from "next-auth/react";
+import * as React from "react";


nit: redundant import

dinmukhamedm · 2025-01-30T14:18:36Z

frontend/components/sign-in/github-signin.tsx

+      setIsLoading(true);
+      await signIn("github", { callbackUrl });
+    } catch (error) {
+      console.error(error);


should we maybe do toast in addition to/instead of console.log?

dinmukhamedm · 2025-01-30T14:20:32Z

frontend/lib/server-utils.ts

+    const filePath = path.join(process.cwd(), "allowed-emails.json");
+
+    if (!fs.existsSync(filePath)) {
+      console.log("file not found :(");


nit: do we really need this? If this is not debug log I suggest we either remove it or make it more informative

dinmukhamedm · 2025-01-30T14:22:05Z

frontend/lib/server-utils.ts

+    const jsonData = JSON.parse(fileContent) as { emails: string[] };
+    return jsonData.emails;
+  } catch (e) {
+    console.error(e);


nit: let's make this a little more informative, e.g. Failed to read allowed-emails.json ${e}

dinmukhamedm · 2025-01-30T14:24:42Z

frontend/lib/server-utils.ts

+    const jsonData = JSON.parse(fileContent) as { emails: string[] };
+    return jsonData.emails;


Even though this may throw TS errors, I think it will silently return undefined for valid JSON files with invalid format. Please handle, by at least returning jsonData.emails ?? [] or better throw an error if jsonData.emails === undefined

dinmukhamedm · 2025-01-30T14:24:46Z

frontend/lib/server-utils.ts

+    const jsonData = JSON.parse(fileContent) as { emails: string[] };
+    return jsonData.emails;


Even though this may throw TS errors, I think it will silently return undefined for valid JSON files with invalid format. Please handle, by at least returning jsonData.emails ?? [] or better throw an error if jsonData.emails === undefined

we might also want to validate every email, but that might be overkill – up to you

I think email validation might be too much

olzhik11 · 2025-01-30T15:41:11Z

@dinmukhamedm yes agree with all comments, should have fixed them before, should be good now, for the emails part I agree we need fallback or error throw, I added error throw though if needed we can make fallback for empty list, meaning no user will be able to pass github signin

dinmukhamedm

LGTM

ellipsis-dev

❌ Changes requested. Reviewed everything up to 855bacc in 1 minute and 57 seconds

More details

Looked at 511 lines of code in 6 files
Skipped 0 files when reviewing.
Skipped posting 1 drafted comments based on config settings.

1. frontend/components/sign-in/email-signin.tsx:43

Draft comment:
The handleEnter prop is not defined in the Button component's props. Consider removing it or implementing it in the Button component.
Reason this comment was not posted:
Comment was on unchanged code.

Workflow ID: wflow_KfkpCB94KpD8oF5l

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

ellipsis-dev · 2025-01-31T10:11:03Z

frontend/lib/server-utils.ts

+    return false;
+  }
+
+  const fileContent = fs.readFileSync(filePath, "utf-8");


Consider using fs.promises.readFile instead of fs.readFileSync to avoid blocking the event loop in an async function.

@olzhik11 please have a look if that makes sense

ellipsis-dev · 2025-01-31T10:11:03Z

frontend/lib/server-utils.ts

+
+  const fileContent = fs.readFileSync(filePath, "utf-8");
+
+  const jsonData = JSON.parse(fileContent) as { emails?: string[] };


Consider adding a try-catch block around JSON.parse to handle potential JSON parsing errors gracefully.

@olzhik11 please address

@dinmukhamedm for this one then we won't be able to throw error if file doesn't match structure

@dinmukhamedm I added fallback instead, and throw on file parsing error

This reverts commit c4db1ae.

olzhik11 added 3 commits January 27, 2025 17:32

feat: add emails whitelist, add sign in check

e6c5437

feat: add file load logic

e0e928f

Merge branch 'dev' into feat/LAM-416

3fda153

dinmukhamedm reviewed Jan 30, 2025

View reviewed changes

olzhik11 added 2 commits January 31, 2025 00:28

feat: remove log, add toast, error description, fallback

d2c4815

feat: throw error on wrong file config

855bacc

dinmukhamedm previously approved these changes Jan 31, 2025

View reviewed changes

olzhik11 marked this pull request as ready for review January 31, 2025 10:08

ellipsis-dev bot reviewed Jan 31, 2025

View reviewed changes

feat: restore grpc files

c4db1ae

olzhik11 dismissed dinmukhamedm’s stale review via c4db1ae January 31, 2025 11:36

olzhik11 added 2 commits January 31, 2025 20:37

Revert "feat: restore grpc files"

ae7e78b

This reverts commit c4db1ae.

feat: replace with promise fileread, add trycatch, fallback

7ebd19e

dinmukhamedm approved these changes Jan 31, 2025

View reviewed changes

dinmukhamedm merged commit 74bc7f8 into lmnr-ai:dev Feb 1, 2025
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add restricted access on github auth #353

feat: add restricted access on github auth #353

olzhik11 commented Jan 28, 2025 •

edited by ellipsis-dev bot

Loading

dinmukhamedm left a comment

dinmukhamedm Jan 30, 2025

dinmukhamedm Jan 30, 2025

dinmukhamedm Jan 30, 2025

dinmukhamedm Jan 30, 2025

dinmukhamedm Jan 30, 2025

dinmukhamedm Jan 30, 2025

dinmukhamedm Jan 30, 2025

olzhik11 Jan 30, 2025

olzhik11 commented Jan 30, 2025

dinmukhamedm left a comment

ellipsis-dev bot left a comment

ellipsis-dev bot Jan 31, 2025

dinmukhamedm Jan 31, 2025

ellipsis-dev bot Jan 31, 2025

dinmukhamedm Jan 31, 2025

olzhik11 Jan 31, 2025

olzhik11 Jan 31, 2025

		const jsonData = JSON.parse(fileContent) as { emails: string[] };
		return jsonData.emails;


		const fileContent = fs.readFileSync(filePath, "utf-8");

		const jsonData = JSON.parse(fileContent) as { emails?: string[] };

feat: add restricted access on github auth #353

feat: add restricted access on github auth #353

Conversation

olzhik11 commented Jan 28, 2025 • edited by ellipsis-dev bot Loading

dinmukhamedm 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olzhik11 commented Jan 30, 2025

dinmukhamedm left a comment

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot Jan 31, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellipsis-dev bot Jan 31, 2025

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

olzhik11 commented Jan 28, 2025 •

edited by ellipsis-dev bot

Loading