first user management#83
Open
tangshixiang wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the authentication UX by adding role-aware post-login redirects on the client and simplifying middleware behavior for public auth routes.
Changes:
- Add client-side redirect for already-authenticated users visiting
/login, with an admin fallback route (/admin/users). - Introduce a
resolvePostLoginPathhelper to decide where to redirect after successful login. - Remove middleware’s “already authed user on public auth pages” redirect logic.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/app/login/page.tsx | Adds auth-user detection and redirects after login / when already logged in. |
| middleware.ts | Removes redirect behavior for authenticated users on public auth paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+21
to
+37
| useEffect(() => { | ||
| if (isLoading || !user) { | ||
| return; | ||
| } | ||
|
|
||
| const next = new URLSearchParams(window.location.search).get("next"); | ||
| const fallback = user.role === "admin" ? "/admin/users" : "/"; | ||
| router.replace(next && next !== "/" ? next : fallback); | ||
| router.refresh(); | ||
| }, [isLoading, router, user]); | ||
|
|
||
| function resolvePostLoginPath(role: "admin" | "user"): string { | ||
| const next = new URLSearchParams(window.location.search).get("next"); | ||
| if (next && next !== "/") { | ||
| return next; | ||
| } | ||
| return role === "admin" ? "/admin/users" : "/"; |
Comment on lines
+26
to
+37
| const next = new URLSearchParams(window.location.search).get("next"); | ||
| const fallback = user.role === "admin" ? "/admin/users" : "/"; | ||
| router.replace(next && next !== "/" ? next : fallback); | ||
| router.refresh(); | ||
| }, [isLoading, router, user]); | ||
|
|
||
| function resolvePostLoginPath(role: "admin" | "user"): string { | ||
| const next = new URLSearchParams(window.location.search).get("next"); | ||
| if (next && next !== "/") { | ||
| return next; | ||
| } | ||
| return role === "admin" ? "/admin/users" : "/"; |
| @@ -76,9 +76,6 @@ export async function middleware(request: NextRequest) { | |||
| const { pathname } = request.nextUrl; | |||
|
|
|||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Full review comments:
This GET path ignores ownerUserId entirely and never verifies access to workspaceId, so any authenticated user can enumerate every global skill and can read another user's workspace skills by calling /api/skills?workspaceId=. Because skill rows include prompts and tool allowlists, this leaks private configuration even though the write
paths now enforce ownership.
Allowing owner_user_id IS NULL here makes every pre-auth skill globally accessible through requireSkillAccess. The new migration adds owner_user_id without backfilling existing rows, so on an upgraded database any signed-in user can GET/PATCH/DELETE legacy skills that were previously part of someone else's workspace or account.
When a database already contains workspaces from before this migration, those rows have owner_user_id = NULL. For a non-admin opening one of those folders, this branch neither claims the existing row nor rejects it, so the handler falls through and inserts a second workspace for the same folderPath. That duplicates workspace records and strands the old
row's notes/sessions/links on the original ID.
If /login is opened with a crafted next such as https://evil.example or //evil.example, this helper returns it verbatim and both the submit flow and the already-signed-in effect pass it to router.replace. That creates an open redirect from the auth page; next should be validated to stay on-site before using it.