Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: backend admin page logic was broken #9616

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from
Open

fix: backend admin page logic was broken #9616

wants to merge 8 commits into from
+41 −31

Conversation

ntindle
Copy link
Member

@ntindle ntindle commented Mar 11, 2025

We're building out admin utilities so we need to bring back the /admin route with RBAC. This PR goes through re-enabling that to work with the latest changes

Changes 🏗️

  • Adds back removed logic
  • Refactors the role checks to fix minor bug for admin page and more importantly clarify
  • Updates routes to the latest

Checklist 📋

For code changes:

  • I have clearly listed my changes in the PR description
  • I have made a test plan
  • I have tested my changes according to the test plan:
    • Test with admin and authenticated user roles
    • Test with logged out user role
    • For the above check the all the existing routes + new ones in the middleware.ts

@ntindle ntindle requested a review from a team as a code owner March 11, 2025 21:06
@ntindle ntindle requested review from Swiftyos and aarushik93 and removed request for a team March 11, 2025 21:06
@qodo-merge-pro Qodo Merge Pro
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Duplicate Route

The '/onboarding' route appears twice in the PROTECTED_PAGES array (lines 8 and 12), which could cause confusion or unexpected behavior in route protection logic.

 
"/onboarding",
"/profile",
"/library",
"/monitoring",
"/onboarding",
Improper Server Action

The 'use server' directive is placed after a console.log statement, which is incorrect. It should be at the top of the function to properly mark it as a server action.

 
console.log("withRoleAccess called:", allowedRoles);
("use server");

@netlify Netlify
Copy link

netlify bot commented Mar 11, 2025 •
edited
Loading

✅ Deploy Preview for auto-gpt-docs-dev canceled.

Name Link
🔨 Latest commit fefd48a
🔍 Latest deploy log https://app.netlify.com/sites/auto-gpt-docs-dev/deploys/67d24fefa30c2d00088fd5a0

@github-actions github-actions bot added platform/frontend AutoGPT Platform - Front end platform/backend AutoGPT Platform - Back end labels Mar 11, 2025
@deepsource-io deepsource.io
Copy link

deepsource-io bot commented Mar 11, 2025 •
edited
Loading

Here's the code health analysis summary for commits 02618e1..fefd48a. View details on DeepSource ↗.

Analysis Summary

AnalyzerStatusSummaryLink
DeepSource JavaScript LogoJavaScript✅ Success
❗ 3 occurences introduced
🎯 9 occurences resolved
View Check ↗
DeepSource Python LogoPython✅ SuccessView Check ↗
💡 If you’re a repository administrator, you can configure the quality gates from the settings.

@netlify Netlify
Copy link

netlify bot commented Mar 11, 2025 •
edited
Loading

✅ Deploy Preview for auto-gpt-docs canceled.

Name Link
🔨 Latest commit fefd48a
🔍 Latest deploy log https://app.netlify.com/sites/auto-gpt-docs/deploys/67d24fefb871f00009836352

@ntindle ntindle requested review from Pwuts, Bentlybro, andrewhooker2 and majdyz and removed request for Swiftyos and aarushik93 March 12, 2025 05:21
majdyz
majdyz previously approved these changes Mar 13, 2025
View reviewed changes
autogpt_platform/frontend/src/lib/supabase/middleware.ts
if (!user) {
// Check if the user is trying to access either a protected page or an admin page
const isAttemptingProtectedPage = PROTECTED_PAGES.some((page) =>
request.nextUrl.pathname.startsWith(page),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we trim the initial slashes ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think so? why would we vs not?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@majdyz majdyz majdyz approved these changes

@Pwuts Pwuts Awaiting requested review from Pwuts

@Bentlybro Bentlybro Awaiting requested review from Bentlybro

@andrewhooker2 andrewhooker2 Awaiting requested review from andrewhooker2

Assignees
No one assigned
Labels
platform/backend AutoGPT Platform - Back end platform/frontend AutoGPT Platform - Front end Review effort 2/5 size/m
Projects
AutoGPT development kanban
Status: 👍🏼 Mergeable
Frontend
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ntindle @majdyz