Feat/role authorization#39
Conversation
… scoped to org_id and created_by
📝 WalkthroughWalkthroughAdds a Role model and middleware; extends repository interfaces, SQL queries, and Postgres stores with org_id+created_by scoped methods; updates services to branch on Role and accept createdBy; propagates userID and role through HTTP handlers and source flows; frontend adds Zitadel admin lookup and UserDisplayName. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant HTTP as HTTP Route
participant MW as Role Middleware
participant Svc as Service
participant Repo as Store/Repository
participant DB as PostgreSQL
Client->>HTTP: Request (auth + X-User-ID)
HTTP->>MW: Execute RoleAuthorization
MW-->>HTTP: ctx.locals: userID, role
HTTP->>Svc: Details(id, orgID, userID, role)
alt role == Admin
Svc->>Repo: Details(id, orgID)
else role != Admin
Svc->>Repo: DetailsByOrgAndCreator(id, orgID, createdBy=userID)
end
Repo->>DB: SELECT ... WHERE id=$1 AND org_id=$2[, created_by=$3]
DB-->>Repo: row / no rows
Repo-->>Svc: model / ErrRecordNotFound
Svc-->>HTTP: model / error
HTTP-->>Client: 200 / 404 / 500
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @elliot14A's task —— View job Pull Request ReviewI've completed my review of the role authorization implementation. Here are my findings: Todo List:
🔍 Review SummaryThis PR implements role-based authorization with Admin/Member roles, adding scope-based access control for projects and datasets. While the implementation is functional, there are several critical security concerns that need to be addressed. 🚨 Critical Security Issues1. Hardcoded Role Assignment -
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| OrgID: pgtype.Text{String: orgID, Valid: true}, | ||
| CreatedBy: pgtype.Text{String: createdBy, Valid: true}, | ||
| Column3: query, | ||
| Limit: int32(pagination.Limit), |
Check failure
Code scanning / CodeQL
Incorrect conversion between integer types High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
To fix this issue, ensure that before casting pagination.Limit (and similarly pagination.Offset, if relevant) to int32, the value is checked to be within the safe range for int32 (0 to math.MaxInt32, as negative or zero limits for pagination are not sensible). If the value does not pass this check, default to a safe, sensible maximum or minimum value (e.g., a documented upper bound such as 1000, or the previous safe default like 10).
To implement this:
- In
server/infrastructure/postgres/store/projects/list.go, lines 16 and 17, before passingint32(pagination.Limit)andint32(pagination.Offset)to the query parameters, perform an explicit bounds check onpagination.Limitandpagination.Offset. - Add an appropriate import for
"math"to usemath.MaxInt32. - Consider also applying this to other places in this file where untrusted ints are cast to
int32for database queries, for defense-in-depth.
| @@ -2,6 +2,7 @@ | ||
|
|
||
| import ( | ||
| "context" | ||
| "math" | ||
|
|
||
| "github.com/factly/gopie/domain/models" | ||
| "github.com/factly/gopie/infrastructure/postgres/gen" | ||
| @@ -10,11 +11,23 @@ | ||
| ) | ||
|
|
||
| func (s *PostgresProjectStore) SearchProject(ctx context.Context, query string, pagination models.Pagination, orgID string) (*models.PaginationView[*models.SearchProjectsResults], error) { | ||
| limit := pagination.Limit | ||
| offset := pagination.Offset | ||
| if limit < 0 { | ||
| limit = 0 | ||
| } else if limit > math.MaxInt32 { | ||
| limit = math.MaxInt32 | ||
| } | ||
| if offset < 0 { | ||
| offset = 0 | ||
| } else if offset > math.MaxInt32 { | ||
| offset = math.MaxInt32 | ||
| } | ||
| ps, err := s.q.SearchProjects(ctx, gen.SearchProjectsParams{ | ||
| OrgID: pgtype.Text{String: orgID, Valid: true}, | ||
| Column2: query, | ||
| Limit: int32(pagination.Limit), | ||
| Offset: int32(pagination.Offset), | ||
| Limit: int32(limit), | ||
| Offset: int32(offset), | ||
| }) | ||
| if err != nil { | ||
| s.logger.Error("Error fetching projects", zap.Error(err)) | ||
| @@ -101,12 +111,24 @@ | ||
| } | ||
|
|
||
| func (s *PostgresProjectStore) SearchProjectByOrgAndCreator(ctx context.Context, query string, pagination models.Pagination, orgID, createdBy string) (*models.PaginationView[*models.SearchProjectsResults], error) { | ||
| limit := pagination.Limit | ||
| offset := pagination.Offset | ||
| if limit < 0 { | ||
| limit = 0 | ||
| } else if limit > math.MaxInt32 { | ||
| limit = math.MaxInt32 | ||
| } | ||
| if offset < 0 { | ||
| offset = 0 | ||
| } else if offset > math.MaxInt32 { | ||
| offset = math.MaxInt32 | ||
| } | ||
| ps, err := s.q.SearchProjectsByOrgAndCreator(ctx, gen.SearchProjectsByOrgAndCreatorParams{ | ||
| OrgID: pgtype.Text{String: orgID, Valid: true}, | ||
| CreatedBy: pgtype.Text{String: createdBy, Valid: true}, | ||
| Column3: query, | ||
| Limit: int32(pagination.Limit), | ||
| Offset: int32(pagination.Offset), | ||
| Limit: int32(limit), | ||
| Offset: int32(offset), | ||
| }) | ||
| if err != nil { | ||
| s.logger.Error("Error searching projects by org and creator", zap.Error(err)) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
server/interfaces/http/routes/source/s3/create.go (1)
155-157: MissinguserIDandrolein cleanup object initialization.The
cleanupstruct is initialized with onlytableName, but thecleanupResourcesfunction at line 56 callsh.datasetSvc.Delete(rc.datasetID, rc.orgID, rc.userID, rc.role). SinceuserIDandroleare never assigned to the cleanup object, the delete call will use zero values, potentially causing authorization failures during error cleanup.Apply this diff to include userID and role:
// Initialize cleanup resource object cleanup := resourceCleanup{ tableName: res.TableName, + userID: userID, + role: role, }server/interfaces/http/routes/api/projects/datasets/update.go (1)
66-70: Incorrect error message: "deleting" should be "updating".The error message references "deleting" but this handler is for updating a dataset.
return ctx.Status(fiber.StatusInternalServerError).JSON(fiber.Map{ "error": err.Error(), - "message": "Error deleting dataset", + "message": "Error updating dataset", "code": fiber.StatusInternalServerError, })server/application/services/store.go (5)
107-114: Inconsistency: Missing role-based access control.Unlike
ProjectService.List(lines 61-72), this method doesn't implement role-based filtering. Non-admin users could potentially list datasets outside their authorization scope. AddorgID,createdBy, androleparameters with branching logic similar toProjectService.List.Apply this diff to add role-based access control:
-func (service *DatasetService) List(projectID string, limit, page int) (*models.PaginationView[*models.Dataset], error) { +func (service *DatasetService) List(projectID string, limit, page int, orgID, createdBy string, role models.Role) (*models.PaginationView[*models.Dataset], error) { pagination := models.NewPagination() if limit != 0 { pagination.Limit = limit } pagination.Offset = (page - 1) * limit - return service.datasetRepo.List(context.Background(), projectID, pagination) + + if role == models.Admin { + return service.datasetRepo.List(context.Background(), projectID, pagination) + } + return service.datasetRepo.ListByOrgAndCreator(context.Background(), orgID, createdBy) }
163-165: Authorization bypass vulnerability: GetDatasetByID exposes datasets to unauthorized access.The HTTP handler at
GET /datasets/{datasetID}callsGetDatasetByIDwithout extracting or validating user role, organization, or creator information. This allows any client to retrieve any dataset by ID, bypassing authorization entirely.A properly authorized
Detailsmethod exists in the same service (lines 96-101) that implements role-based access control: admins can access any dataset within their org, while non-admins are restricted toDetailsByOrgAndCreator. The public HTTP endpoint should either be removed or refactored to use the authorized Details method with proper context extraction (orgID, userID, role).
40-42: Add role-based authorization toUpdatemethod.The
Updatemethod lacks the role-based access control thatDetailsandDeletemethods implement. It accepts no authorization parameters and performs no ownership validation, allowing any authenticated user to modify any project if they know the project ID. AddorgID,createdBy, androleparameters with conditional logic to check project ownership for non-admin users, similar to theDeleteandDetailsimplementations. Also extract and pass the user's role from the HTTP context to the service method.
123-125: Add role-based access control to DatasetService.Update method.The
Updatemethod lacks role-based authorization checks present inDetailsandDelete. The refresh.go handler callsUpdatewithout verifying authorization throughDetailsfirst, allowing non-admin users to modify datasets outside their scope.Update the signature to accept
orgID,createdBy, androleparameters, then implement conditional logic similar toDetailsandDelete:
- Admins: call repo's
Updatewith orgID- Non-admins: call repo's
UpdateByOrgAndCreatorwith orgID and createdByApply the same pattern to
ProjectService.Updatefor consistency.
35-37: MovegetByIDhandler out ofRoutes()or apply role-based authorization to prevent unauthorized project access.The
getByIDhandler inInternalRoutes()bypasses authorization entirely. Since bothRoutes()andInternalRoutes()register to the same path (GET /:projectID), the unprotectedgetByIDhandler overrides the protecteddetailshandler. This allows any request to/v1/api/projects/{id}to bypass organization and role-based access controls by directly querying by ID without scoping toorgIDorcreatedBy. TheDetailsmethod correctly demonstrates the required pattern: acceptorgID,createdBy, androleparameters, then apply role-based filtering (Detailsfor admins,DetailsByOrgAndCreatorfor others). Either movegetByIDto a completely separate, clearly internal endpoint path, or rewrite it to use theDetailsmethod with proper authorization context extraction.
🧹 Nitpick comments (5)
server/interfaces/http/routes/api/projects/delete.go (1)
23-24: Consider defensive type assertions to prevent panics.The direct type assertions will panic if the middleware fails to set these values or if they're of the wrong type. Consider using two-value type assertions for robustness:
-userID := ctx.Locals(middleware.UserCtxKey).(string) -role := ctx.Locals(middleware.RoleCtxKey).(models.Role) +userID, ok := ctx.Locals(middleware.UserCtxKey).(string) +if !ok || userID == "" { + return ctx.Status(fiber.StatusUnauthorized).JSON(fiber.Map{ + "error": "User not authenticated", + "code": fiber.StatusUnauthorized, + }) +} +role, ok := ctx.Locals(middleware.RoleCtxKey).(models.Role) +if !ok { + return ctx.Status(fiber.StatusInternalServerError).JSON(fiber.Map{ + "error": "Invalid role context", + "code": fiber.StatusInternalServerError, + }) +}This pattern should be applied consistently across all route handlers that extract userID and role from context.
server/domain/models/role.go (1)
1-8: LGTM!The Role model is clean and straightforward. The string-based type provides flexibility for future extensibility.
For future enhancements, consider adding helper methods:
func (r Role) IsAdmin() bool { return r == Admin } func (r Role) IsValid() bool { return r == Member || r == Admin }server/interfaces/http/routes/api/datasets/get-column-descriptions.go (1)
25-26: Consider defensive type assertions to prevent panics.Same concern as in server/interfaces/http/routes/api/projects/delete.go: direct type assertions will panic if middleware fails to set these values.
Apply the same defensive pattern:
-userID := ctx.Locals(middleware.UserCtxKey).(string) -role := ctx.Locals(middleware.RoleCtxKey).(models.Role) +userID, ok := ctx.Locals(middleware.UserCtxKey).(string) +if !ok || userID == "" { + return ctx.Status(fiber.StatusUnauthorized).JSON(fiber.Map{ + "error": "User not authenticated", + "code": fiber.StatusUnauthorized, + }) +} +role, ok := ctx.Locals(middleware.RoleCtxKey).(models.Role) +if !ok { + return ctx.Status(fiber.StatusInternalServerError).JSON(fiber.Map{ + "error": "Invalid role context", + "code": fiber.StatusInternalServerError, + }) +}server/infrastructure/postgres/store/datasets/details.go (1)
148-185: Suggest extracting common mapping logic to reduce duplication.The
DetailsByOrgAndCreatormethod duplicates significant logic fromDetails(lines 17-53) andGetDatasetByID(lines 114-146). All three methods share identical column unmarshaling and model mapping code.Consider extracting a helper method:
func (s *PgDatasetStore) mapDatasetRow(d gen.Dataset) (*models.Dataset, error) { columns := make([]map[string]any, 0) err := json.Unmarshal([]byte(d.Columns), &columns) if err != nil { s.logger.Error("Error unmarshaling columns", zap.Error(err)) return nil, err } return &models.Dataset{ ID: d.ID, Name: d.Name, Alias: d.Alias.String, Description: d.Description.String, RowCount: int(d.RowCount.Int32), Size: int(d.Size.Int64), FilePath: d.FilePath, CreatedAt: time.Time(d.CreatedAt.Time), CreatedBy: d.CreatedBy.String, UpdatedAt: time.Time(d.UpdatedAt.Time), UpdatedBy: d.UpdatedBy.String, Columns: columns, OrgID: d.OrgID.String, CustomPrompt: d.CustomPrompt.String, Source: d.Source, }, nil }Then simplify all three methods:
func (s *PgDatasetStore) DetailsByOrgAndCreator(ctx context.Context, datasetID, orgID, createdBy string) (*models.Dataset, error) { d, err := s.q.GetDatasetByOrgAndCreator(ctx, gen.GetDatasetByOrgAndCreatorParams{ ID: datasetID, OrgID: pgtype.Text{String: orgID, Valid: true}, CreatedBy: pgtype.Text{String: createdBy, Valid: true}, }) if err != nil { s.logger.Error("Error fetching dataset by org and creator", zap.Error(err)) if errors.Is(err, pgx.ErrNoRows) { return nil, domain.ErrRecordNotFound } return nil, err } return s.mapDatasetRow(d) }server/infrastructure/postgres/sql/queries/datasets.sql (1)
69-78: New creator-scoped queries look correct.The new queries properly filter by
org_idandcreated_byfor role-based access control. Consider adding a composite index on(org_id, created_by)if these queries are expected to run frequently to optimize performance.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (12)
server/infrastructure/postgres/gen/apikeys.sql.gois excluded by!**/gen/**server/infrastructure/postgres/gen/chats.sql.gois excluded by!**/gen/**server/infrastructure/postgres/gen/database_sources.sql.gois excluded by!**/gen/**server/infrastructure/postgres/gen/dataset_summary.sql.gois excluded by!**/gen/**server/infrastructure/postgres/gen/datasets.sql.gois excluded by!**/gen/**server/infrastructure/postgres/gen/db.gois excluded by!**/gen/**server/infrastructure/postgres/gen/downloads.sql.gois excluded by!**/gen/**server/infrastructure/postgres/gen/failed_dataset_uploads.sql.gois excluded by!**/gen/**server/infrastructure/postgres/gen/models.gois excluded by!**/gen/**server/infrastructure/postgres/gen/project_datasets.sql.gois excluded by!**/gen/**server/infrastructure/postgres/gen/projects.sql.gois excluded by!**/gen/**server/infrastructure/postgres/gen/querier.gois excluded by!**/gen/**
📒 Files selected for processing (26)
server/application/repositories/store.go(2 hunks)server/application/services/store.go(5 hunks)server/domain/models/role.go(1 hunks)server/infrastructure/postgres/sql/queries/datasets.sql(1 hunks)server/infrastructure/postgres/sql/queries/projects.sql(1 hunks)server/infrastructure/postgres/store/datasets/delete.go(1 hunks)server/infrastructure/postgres/store/datasets/details.go(1 hunks)server/infrastructure/postgres/store/datasets/list.go(2 hunks)server/infrastructure/postgres/store/projects/delete.go(1 hunks)server/infrastructure/postgres/store/projects/details.go(1 hunks)server/infrastructure/postgres/store/projects/list.go(1 hunks)server/interfaces/http/middleware/role.go(1 hunks)server/interfaces/http/middleware/validate.go(2 hunks)server/interfaces/http/routes/api/datasets/details.go(2 hunks)server/interfaces/http/routes/api/datasets/get-column-descriptions.go(1 hunks)server/interfaces/http/routes/api/datasets/update-column-descriptions.go(2 hunks)server/interfaces/http/routes/api/projects/datasets/delete.go(2 hunks)server/interfaces/http/routes/api/projects/datasets/details.go(2 hunks)server/interfaces/http/routes/api/projects/datasets/update.go(2 hunks)server/interfaces/http/routes/api/projects/delete.go(2 hunks)server/interfaces/http/routes/api/projects/details.go(2 hunks)server/interfaces/http/routes/api/projects/list.go(2 hunks)server/interfaces/http/routes/source/database/create.go(5 hunks)server/interfaces/http/routes/source/database/refresh.go(2 hunks)server/interfaces/http/routes/source/s3/create.go(4 hunks)server/interfaces/http/serve.go(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-07-29T09:36:39.908Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-29T09:36:39.908Z
Learning: Applies to server/**/*.go : Use Fiber as the web framework in the Go backend server
Applied to files:
server/interfaces/http/routes/api/datasets/details.goserver/interfaces/http/routes/api/projects/list.goserver/interfaces/http/routes/api/projects/datasets/details.goserver/interfaces/http/routes/api/projects/delete.goserver/interfaces/http/routes/api/projects/details.goserver/interfaces/http/middleware/validate.go
📚 Learning: 2025-07-29T09:36:39.908Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-29T09:36:39.908Z
Learning: Applies to server/**/* : Organize Go backend server code into Domain, Infrastructure, Interfaces, and Application layers
Applied to files:
server/interfaces/http/routes/api/datasets/details.goserver/interfaces/http/routes/api/projects/list.goserver/interfaces/http/routes/api/projects/datasets/details.goserver/interfaces/http/routes/api/projects/delete.goserver/interfaces/http/routes/api/projects/details.goserver/interfaces/http/middleware/validate.go
📚 Learning: 2025-07-29T09:36:39.908Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-29T09:36:39.908Z
Learning: Applies to server/**/*.go : Integrate Zitadel authentication with JWT in the Go backend server
Applied to files:
server/interfaces/http/routes/api/datasets/details.goserver/interfaces/http/serve.goserver/interfaces/http/routes/api/projects/datasets/details.goserver/interfaces/http/middleware/validate.go
📚 Learning: 2025-07-29T09:36:39.908Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-29T09:36:39.908Z
Learning: Applies to server/**/*.go : Store dataset files in S3-compatible storage (MinIO) in the Go backend server
Applied to files:
server/infrastructure/postgres/store/datasets/list.go
📚 Learning: 2025-07-29T09:36:39.908Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-29T09:36:39.908Z
Learning: Applies to server/**/*.go : Integrate DuckDB for OLAP queries in the Go backend server
Applied to files:
server/infrastructure/postgres/store/datasets/list.go
📚 Learning: 2025-07-29T09:36:39.908Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-29T09:36:39.908Z
Learning: Applies to server/**/*.go : Use PostgreSQL with SQLC for type-safe queries in the Go backend server
Applied to files:
server/infrastructure/postgres/store/datasets/list.go
📚 Learning: 2025-07-29T09:36:39.908Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-29T09:36:39.908Z
Learning: Applies to server/infrastructure/postgres/migrations/* : Run database migrations in the Go backend server using `goose up`
Applied to files:
server/infrastructure/postgres/store/datasets/list.go
🧬 Code graph analysis (23)
server/infrastructure/postgres/store/projects/details.go (2)
server/infrastructure/postgres/gen/models.go (1)
Project(195-205)server/infrastructure/postgres/gen/projects.sql.go (1)
GetProjectByOrgAndCreatorParams(205-209)
server/interfaces/http/middleware/role.go (1)
server/domain/models/role.go (1)
Member(6-6)
server/interfaces/http/routes/source/database/refresh.go (2)
server/interfaces/http/middleware/role.go (1)
RoleCtxKey(8-8)server/domain/models/role.go (1)
Role(3-3)
server/interfaces/http/routes/api/datasets/get-column-descriptions.go (2)
server/interfaces/http/middleware/role.go (1)
RoleCtxKey(8-8)server/domain/models/role.go (1)
Role(3-3)
server/infrastructure/postgres/store/datasets/details.go (2)
server/infrastructure/postgres/gen/models.go (1)
Dataset(149-165)server/infrastructure/postgres/gen/datasets.sql.go (1)
GetDatasetByOrgAndCreatorParams(207-211)
server/interfaces/http/routes/api/datasets/details.go (2)
server/interfaces/http/middleware/role.go (1)
RoleCtxKey(8-8)server/domain/models/role.go (1)
Role(3-3)
server/interfaces/http/routes/api/projects/list.go (3)
server/interfaces/http/middleware/role.go (1)
RoleCtxKey(8-8)server/domain/models/role.go (1)
Role(3-3)server/domain/pkg/utils.go (1)
ParseLimitAndPage(15-35)
server/interfaces/http/serve.go (1)
server/interfaces/http/middleware/role.go (1)
RoleAuthorization(10-15)
server/interfaces/http/routes/api/projects/datasets/details.go (2)
server/interfaces/http/middleware/role.go (1)
RoleCtxKey(8-8)server/domain/models/role.go (1)
Role(3-3)
server/infrastructure/postgres/store/projects/delete.go (1)
server/infrastructure/postgres/gen/projects.sql.go (1)
DeleteProjectByOrgAndCreatorParams(113-117)
server/infrastructure/postgres/store/datasets/delete.go (1)
server/infrastructure/postgres/gen/datasets.sql.go (1)
DeleteDatasetByOrgAndCreatorParams(101-105)
server/infrastructure/postgres/store/datasets/list.go (3)
server/infrastructure/postgres/store/datasets/datasets.go (1)
PgDatasetStore(10-14)server/infrastructure/postgres/gen/models.go (1)
Dataset(149-165)server/infrastructure/postgres/gen/datasets.sql.go (3)
SearchDatasetsParams(388-393)ListDatasetsByOrgAndCreatorParams(329-332)SearchDatasetsByOrgAndCreatorParams(455-461)
server/interfaces/http/routes/api/datasets/update-column-descriptions.go (2)
server/interfaces/http/middleware/role.go (1)
RoleCtxKey(8-8)server/domain/models/role.go (1)
Role(3-3)
server/interfaces/http/routes/api/projects/datasets/delete.go (2)
server/interfaces/http/middleware/role.go (1)
RoleCtxKey(8-8)server/domain/models/role.go (1)
Role(3-3)
server/infrastructure/postgres/store/projects/list.go (1)
server/infrastructure/postgres/gen/projects.sql.go (2)
ListProjectsByOrgAndCreatorParams(341-344)SearchProjectsByOrgAndCreatorParams(507-513)
server/interfaces/http/routes/source/s3/create.go (2)
server/domain/models/role.go (1)
Role(3-3)server/interfaces/http/middleware/role.go (1)
RoleCtxKey(8-8)
server/interfaces/http/routes/api/projects/delete.go (2)
server/interfaces/http/middleware/role.go (1)
RoleCtxKey(8-8)server/domain/models/role.go (1)
Role(3-3)
server/interfaces/http/routes/source/database/create.go (2)
server/domain/models/role.go (1)
Role(3-3)server/interfaces/http/middleware/role.go (1)
RoleCtxKey(8-8)
server/application/services/store.go (3)
server/domain/models/role.go (2)
Role(3-3)Admin(7-7)server/infrastructure/postgres/gen/models.go (2)
Project(195-205)Dataset(149-165)server/domain/models/dataset.go (1)
Dataset(7-37)
server/interfaces/http/routes/api/projects/datasets/update.go (2)
server/interfaces/http/middleware/role.go (1)
RoleCtxKey(8-8)server/domain/models/role.go (1)
Role(3-3)
server/application/repositories/store.go (2)
server/infrastructure/postgres/gen/models.go (2)
Project(195-205)Dataset(149-165)server/domain/models/dataset.go (1)
Dataset(7-37)
server/interfaces/http/routes/api/projects/details.go (2)
server/interfaces/http/middleware/role.go (1)
RoleCtxKey(8-8)server/domain/models/role.go (1)
Role(3-3)
server/interfaces/http/middleware/validate.go (3)
server/application/services/store.go (1)
ProjectService(12-14)server/interfaces/http/middleware/role.go (1)
RoleCtxKey(8-8)server/domain/models/role.go (1)
Role(3-3)
🪛 GitHub Check: CodeQL
server/infrastructure/postgres/store/projects/list.go
[failure] 108-108: Incorrect conversion between integer types
Incorrect conversion of an integer with architecture-dependent bit size from strconv.Atoi to a lower bit size type int32 without an upper bound check.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
🔇 Additional comments (30)
server/infrastructure/postgres/sql/queries/projects.sql (5)
80-82: LGTM!The addition of
org_idfiltering strengthens access control by ensuring projects are scoped to the organization.
84-92: LGTM!The query correctly implements org and creator-scoped project retrieval with dataset aggregation, consistent with the existing
GetProjectpattern.
94-102: LGTM!The listing query correctly filters by both organization and creator, maintaining consistency with the authorization model.
104-105: LGTM!The deletion query correctly enforces all three authorization checks (id, org_id, created_by).
107-126: LGTM!The search query correctly extends the existing
SearchProjectspattern with creator-scoped filtering while maintaining relevance-based ordering.server/interfaces/http/routes/api/projects/delete.go (1)
26-26: LGTM!The service call correctly propagates all authorization parameters (projectID, orgID, userID, role) for role-based access control.
server/interfaces/http/routes/api/datasets/get-column-descriptions.go (1)
29-29: LGTM!The Details call correctly includes userID and role for authorization-aware dataset retrieval.
server/infrastructure/postgres/store/projects/delete.go (1)
29-43: LGTM!The
DeleteByOrgAndCreatormethod correctly implements creator-scoped deletion with proper error handling and domain error translation.server/infrastructure/postgres/store/datasets/delete.go (1)
65-79: LGTM!The new
DeleteByOrgAndCreatormethod follows the established pattern from the existingDeletemethod, with consistent error handling and logging.server/interfaces/http/routes/source/database/refresh.go (2)
36-36: LGTM!Role context retrieval is consistent with the pattern used across other handlers in this PR.
59-59: LGTM!The project details call correctly propagates userID and role for access control.
server/infrastructure/postgres/store/projects/details.go (1)
113-138: LGTM!The new
DetailsByOrgAndCreatormethod follows the established pattern from the existingDetailsmethod, with consistent error handling, logging, and field mapping.server/interfaces/http/routes/api/datasets/update-column-descriptions.go (2)
32-33: LGTM!User context and role retrieval is consistent with the pattern used in other dataset handlers.
55-55: LGTM!The Details call correctly propagates userID and role for role-based access control.
server/interfaces/http/routes/api/projects/details.go (1)
26-26: LGTM!The service call correctly propagates userID and role for role-based access control.
server/interfaces/http/routes/api/projects/datasets/update.go (1)
56-56: LGTM!The Details call correctly includes userID and role for role-based access control.
server/interfaces/http/routes/source/database/create.go (3)
19-29: LGTM!The
resourceCleanupstruct correctly includesuserIDandrolefields, and these are properly initialized and propagated through the cleanup flow.Also applies to: 173-177
41-46: LGTM!The
cleanupResourcesfunction correctly uses the stored userID and role when callingDelete, ensuring consistent role-based access control during cleanup operations.
125-125: LGTM!Project existence check correctly includes userID and role for role-based access control.
server/interfaces/http/routes/api/projects/list.go (1)
34-34: LGTM!The List call correctly includes userID and role for role-based filtering.
server/infrastructure/postgres/sql/queries/datasets.sql (1)
80-96: Search query is well-structured with proper prioritization.The
SearchDatasetsByOrgAndCreatorquery correctly mirrors the existingSearchDatasetspattern with the additionalcreated_byfilter. The ordering logic prioritizes alias prefix matches, then name prefix, then partial name matches.server/infrastructure/postgres/store/projects/list.go (1)
74-101: ListByOrgAndCreator implementation looks correct.The method properly maps database results to domain models and handles the org+creator scoping appropriately.
server/infrastructure/postgres/store/datasets/list.go (1)
195-230: ListByOrgAndCreator implementation is correct.The method properly filters by org and creator, and correctly maps all dataset fields including JSON column deserialization.
server/application/services/store.go (5)
28-33: LGTM! Role-based access control correctly implemented.The branching logic appropriately restricts non-admin users to resources they created within their organization, while admins have organization-wide access.
45-50: LGTM! Role-based deletion correctly implemented.The branching logic appropriately restricts deletion permissions based on user role and ownership.
61-72: LGTM! Role-based listing correctly implemented.The implementation appropriately scopes search results based on user role, ensuring non-admin users only see projects they created within their organization.
96-101: LGTM! Role-based access control correctly implemented.The branching logic appropriately restricts dataset access based on user role and ownership.
116-121: LGTM! Role-based deletion correctly implemented.The branching logic appropriately restricts deletion permissions based on user role and ownership.
server/application/repositories/store.go (2)
40-44: LGTM! Clean interface additions for creator-scoped operations.The new methods provide appropriate repository-layer support for role-based access control. Naming is clear, signatures are consistent, and the comment documents the scoping intent well.
73-77: LGTM! Consistent interface additions for creator-scoped operations.The new methods mirror the pattern established in
ProjectStoreRepository, providing good consistency across dataset and project operations. The comment clearly documents the scoping intent.
| ListAllDatasets(ctx context.Context) ([]*models.Dataset, error) | ||
| ListALlDatasetsFromProject(ctx context.Context, projectID string) ([]*models.Dataset, error) | ||
| GetProjectForDataset(ctx context.Context, datasetID string) (string, error) | ||
| SearchDatasets(ctx context.Context, query string, pagination models.Pagination, orgID string) (*models.PaginationView[*models.Dataset], error) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for implementations and usages of SearchDatasets
echo "=== Searching for SearchDatasets implementations ==="
ast-grep --pattern $'func ($_ *$_) SearchDatasets($$$) ($$$) {
$$$
}'
echo ""
echo "=== Searching for SearchDatasets calls/usage ==="
rg -nP '\bSearchDatasets\s*\(' --type=go -C3
echo ""
echo "=== Additional grep for SearchDatasets references ==="
rg -n "SearchDatasets" --type=go -C2Repository: factly/gopie
Length of output: 16545
🏁 Script executed:
#!/bin/bash
# Check what methods DatasetService.List calls
# Find DatasetService.List implementation
rg -n "func.*List" server/application/services/ --type=go -C5 | grep -A10 "Dataset"
# Search more specifically for DatasetService
fd "dataset.*service" server/application/services/ --type=go
# Try to find the service implementation file
find server/application/services -name "*dataset*" -type fRepository: factly/gopie
Length of output: 5279
🏁 Script executed:
#!/bin/bash
# Get the full DatasetService.List implementation
sed -n '107,130p' server/application/services/store.goRepository: factly/gopie
Length of output: 1067
Remove unused SearchDatasets method or document its intended use.
This method is added to the interface and fully implemented in the store layer, but is never called. DatasetService.List uses the existing List method instead, and no other code invokes SearchDatasets. Either remove it if not needed, or document that it's reserved for future use.
🤖 Prompt for AI Agents
In server/application/repositories/store.go around line 71, the SearchDatasets
method was added to the repository interface but is never called; either remove
it from the interface and all concrete implementations (and delete the
corresponding store implementation) or keep it but add a clear doc comment
stating it's reserved for future use (or deprecated) so callers/reviewers know
it's intentionally unused; after making the change, update any compile errors,
run tests, and remove related dead imports or unused variables.
| func (s *PgDatasetStore) SearchDatasets(ctx context.Context, query string, pagination models.Pagination, orgID string) (*models.PaginationView[*models.Dataset], error) { | ||
| ds, err := s.q.SearchDatasets(ctx, gen.SearchDatasetsParams{ | ||
| OrgID: pgtype.Text{String: orgID, Valid: true}, | ||
| Limit: int32(pagination.Limit), | ||
| Offset: int32(pagination.Offset), | ||
| }) | ||
| if err != nil { | ||
| s.logger.Error("Error searching datasets", zap.Error(err)) | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Missing query parameter in SearchDatasets call.
The query parameter is accepted by the function but not passed to gen.SearchDatasetsParams. According to the SearchDatasetsParams struct in the relevant snippets, it expects a Concat field for the search term. This means the search will not filter by the query string.
ds, err := s.q.SearchDatasets(ctx, gen.SearchDatasetsParams{
OrgID: pgtype.Text{String: orgID, Valid: true},
+ Concat: query,
Limit: int32(pagination.Limit),
Offset: int32(pagination.Offset),
})🤖 Prompt for AI Agents
In server/infrastructure/postgres/store/datasets/list.go around lines 152 to
161, the SearchDatasets call omits the query parameter so searches aren't
filtered; add the Concat field to gen.SearchDatasetsParams and pass the incoming
query as a pgtype.Text with Valid set to true (e.g., Concat: pgtype.Text{String:
query, Valid: true}) so the query string is forwarded to the DB query.
| // Note: This count may not be accurate for filtered results | ||
| // You might want to add a separate count query for search results | ||
| count := len(datasets) | ||
|
|
||
| paginationView := models.NewPaginationView(pagination.Offset, pagination.Limit, count, datasets) | ||
| return &paginationView, nil |
There was a problem hiding this comment.
Pagination count is incorrect for search results.
Using len(datasets) returns only the count of the current page, not the total matching results. This breaks pagination - users cannot navigate beyond the first page correctly.
Add a dedicated count query for search results, similar to how List uses GetProjectDatasetsCount:
-- name: GetSearchDatasetsCount :one
SELECT COUNT(*) FROM datasets
WHERE org_id = $1 AND (name ILIKE concat('%', $2, '%') OR description ILIKE concat('%', $2, '%') OR alias ILIKE concat('%', $2, '%'));🤖 Prompt for AI Agents
In server/infrastructure/postgres/store/datasets/list.go around lines 187 to
192, the pagination count is set to len(datasets) which only reflects the
current page; replace this with a dedicated total-count query for the search
filters (like GetSearchDatasetsCount used for List/GetProjectDatasetsCount). Run
the count query with the same org ID and search term/filters you applied to
fetch datasets, handle the returned total (and any error) before constructing
the PaginationView, and pass that total into NewPaginationView instead of
len(datasets).
| func (s *PgDatasetStore) SearchDatasetsByOrgAndCreator(ctx context.Context, query string, pagination models.Pagination, orgID, createdBy string) (*models.PaginationView[*models.Dataset], error) { | ||
| ds, err := s.q.SearchDatasetsByOrgAndCreator(ctx, gen.SearchDatasetsByOrgAndCreatorParams{ | ||
| OrgID: pgtype.Text{String: orgID, Valid: true}, | ||
| CreatedBy: pgtype.Text{String: createdBy, Valid: true}, | ||
| Limit: int32(pagination.Limit), | ||
| Offset: int32(pagination.Offset), | ||
| }) | ||
| if err != nil { | ||
| s.logger.Error("Error searching datasets by org and creator", zap.Error(err)) | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Missing query parameter in SearchDatasetsByOrgAndCreator call.
Same issue as SearchDatasets - the query parameter is not passed to the generated query params. The Concat field should receive the search term.
ds, err := s.q.SearchDatasetsByOrgAndCreator(ctx, gen.SearchDatasetsByOrgAndCreatorParams{
OrgID: pgtype.Text{String: orgID, Valid: true},
CreatedBy: pgtype.Text{String: createdBy, Valid: true},
+ Concat: query,
Limit: int32(pagination.Limit),
Offset: int32(pagination.Offset),
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *PgDatasetStore) SearchDatasetsByOrgAndCreator(ctx context.Context, query string, pagination models.Pagination, orgID, createdBy string) (*models.PaginationView[*models.Dataset], error) { | |
| ds, err := s.q.SearchDatasetsByOrgAndCreator(ctx, gen.SearchDatasetsByOrgAndCreatorParams{ | |
| OrgID: pgtype.Text{String: orgID, Valid: true}, | |
| CreatedBy: pgtype.Text{String: createdBy, Valid: true}, | |
| Limit: int32(pagination.Limit), | |
| Offset: int32(pagination.Offset), | |
| }) | |
| if err != nil { | |
| s.logger.Error("Error searching datasets by org and creator", zap.Error(err)) | |
| return nil, err | |
| } | |
| func (s *PgDatasetStore) SearchDatasetsByOrgAndCreator(ctx context.Context, query string, pagination models.Pagination, orgID, createdBy string) (*models.PaginationView[*models.Dataset], error) { | |
| ds, err := s.q.SearchDatasetsByOrgAndCreator(ctx, gen.SearchDatasetsByOrgAndCreatorParams{ | |
| OrgID: pgtype.Text{String: orgID, Valid: true}, | |
| CreatedBy: pgtype.Text{String: createdBy, Valid: true}, | |
| Concat: query, | |
| Limit: int32(pagination.Limit), | |
| Offset: int32(pagination.Offset), | |
| }) | |
| if err != nil { | |
| s.logger.Error("Error searching datasets by org and creator", zap.Error(err)) | |
| return nil, err | |
| } |
🤖 Prompt for AI Agents
In server/infrastructure/postgres/store/datasets/list.go around lines 232 to
242, the generated query params for SearchDatasetsByOrgAndCreator omit the
search term; add the missing Concat (or Query) field and set it to the provided
query value (e.g., Concat: pgtype.Text{String: query, Valid: true}) when calling
s.q.SearchDatasetsByOrgAndCreator so the search term is passed through, then run
tests/build to verify.
| // Note: This count may not be accurate for filtered results | ||
| // You might want to add a separate count query for search results | ||
| count := len(datasets) | ||
|
|
||
| paginationView := models.NewPaginationView(pagination.Offset, pagination.Limit, count, datasets) | ||
| return &paginationView, nil |
There was a problem hiding this comment.
Same pagination count issue as SearchDatasets.
The count calculation using len(datasets) is incorrect for pagination metadata. This needs a dedicated count query filtered by org_id, created_by, and the search term.
🤖 Prompt for AI Agents
In server/infrastructure/postgres/store/datasets/list.go around lines 268-273,
the pagination count is currently computed with len(datasets) which is wrong;
replace that with a dedicated COUNT query using the same filters (org_id,
created_by and any search term/visibility constraints) but without LIMIT/OFFSET.
Execute a SELECT COUNT(*) FROM ... WHERE <same filter expressions> (or use the
ORM/count helper), scan the result into the count variable, handle any DB
errors, and then pass that accurate count into models.NewPaginationView before
returning.
| userID := ctx.Locals(middleware.UserCtxKey).(string) | ||
| role := ctx.Locals(middleware.RoleCtxKey).(models.Role) |
There was a problem hiding this comment.
Unsafe type assertions may cause runtime panics.
Direct type assertions without checking for nil or type mismatch will panic if the middleware fails to set these context values. Consider adding defensive checks or using the two-value assertion form.
- userID := ctx.Locals(middleware.UserCtxKey).(string)
- role := ctx.Locals(middleware.RoleCtxKey).(models.Role)
+ userID, ok := ctx.Locals(middleware.UserCtxKey).(string)
+ if !ok || userID == "" {
+ return ctx.Status(fiber.StatusUnauthorized).JSON(fiber.Map{
+ "error": "Unauthorized",
+ "message": "User context not found",
+ "code": fiber.StatusUnauthorized,
+ })
+ }
+ role, ok := ctx.Locals(middleware.RoleCtxKey).(models.Role)
+ if !ok {
+ return ctx.Status(fiber.StatusUnauthorized).JSON(fiber.Map{
+ "error": "Unauthorized",
+ "message": "Role context not found",
+ "code": fiber.StatusUnauthorized,
+ })
+ }🤖 Prompt for AI Agents
In server/interfaces/http/routes/api/projects/details.go around lines 23-24, the
code uses direct type assertions for ctx.Locals(...).(string) and .(models.Role)
which can panic if the middleware did not set these values; change to the
two-value form (val, ok := ctx.Locals(...).(T)), check ok (and nil) for both
userID and role, and handle the failure path by returning an appropriate HTTP
error (e.g., 401/400) or logging and aborting the request instead of letting the
panic occur.
| userID := ctx.Locals(middleware.UserCtxKey).(string) | ||
| role := ctx.Locals(middleware.RoleCtxKey).(models.Role) |
There was a problem hiding this comment.
Unsafe type assertions may cause runtime panics.
Same issue as other handlers — direct type assertions will panic if context values are missing.
- userID := ctx.Locals(middleware.UserCtxKey).(string)
- role := ctx.Locals(middleware.RoleCtxKey).(models.Role)
+ userID, ok := ctx.Locals(middleware.UserCtxKey).(string)
+ if !ok || userID == "" {
+ return ctx.Status(fiber.StatusUnauthorized).JSON(fiber.Map{
+ "error": "Unauthorized",
+ "message": "User context not found",
+ "code": fiber.StatusUnauthorized,
+ })
+ }
+ role, ok := ctx.Locals(middleware.RoleCtxKey).(models.Role)
+ if !ok {
+ return ctx.Status(fiber.StatusUnauthorized).JSON(fiber.Map{
+ "error": "Unauthorized",
+ "message": "Role context not found",
+ "code": fiber.StatusUnauthorized,
+ })
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| userID := ctx.Locals(middleware.UserCtxKey).(string) | |
| role := ctx.Locals(middleware.RoleCtxKey).(models.Role) | |
| userID, ok := ctx.Locals(middleware.UserCtxKey).(string) | |
| if !ok || userID == "" { | |
| return ctx.Status(fiber.StatusUnauthorized).JSON(fiber.Map{ | |
| "error": "Unauthorized", | |
| "message": "User context not found", | |
| "code": fiber.StatusUnauthorized, | |
| }) | |
| } | |
| role, ok := ctx.Locals(middleware.RoleCtxKey).(models.Role) | |
| if !ok { | |
| return ctx.Status(fiber.StatusUnauthorized).JSON(fiber.Map{ | |
| "error": "Unauthorized", | |
| "message": "Role context not found", | |
| "code": fiber.StatusUnauthorized, | |
| }) | |
| } |
🤖 Prompt for AI Agents
In server/interfaces/http/routes/api/projects/list.go around lines 26-27, the
code uses direct type assertions on ctx.Locals which can panic if values are
absent; replace them with safe comma-ok checks (e.g. val, ok :=
ctx.Locals(middleware.UserCtxKey).(string) and role, ok2 :=
ctx.Locals(middleware.RoleCtxKey).(models.Role)), and handle the !ok cases by
logging and returning an appropriate HTTP error (400/401/500 as appropriate)
instead of allowing a panic so the handler fails gracefully when context values
are missing.
| func (h *httpHandler) create(ctx *fiber.Ctx) error { | ||
| orgID := ctx.Locals(middleware.OrganizationCtxKey).(string) | ||
| userID := ctx.Locals(middleware.UserCtxKey).(string) | ||
| role := ctx.Locals(middleware.RoleCtxKey).(models.Role) |
There was a problem hiding this comment.
Unsafe type assertion may cause runtime panic.
Same issue as other handlers — missing safety check for the role context value.
- role := ctx.Locals(middleware.RoleCtxKey).(models.Role)
+ role, ok := ctx.Locals(middleware.RoleCtxKey).(models.Role)
+ if !ok {
+ return ctx.Status(fiber.StatusUnauthorized).JSON(fiber.Map{
+ "error": "Unauthorized",
+ "message": "Role context not found",
+ "code": fiber.StatusUnauthorized,
+ })
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| role := ctx.Locals(middleware.RoleCtxKey).(models.Role) | |
| role, ok := ctx.Locals(middleware.RoleCtxKey).(models.Role) | |
| if !ok { | |
| return ctx.Status(fiber.StatusUnauthorized).JSON(fiber.Map{ | |
| "error": "Unauthorized", | |
| "message": "Role context not found", | |
| "code": fiber.StatusUnauthorized, | |
| }) | |
| } |
🤖 Prompt for AI Agents
In server/interfaces/http/routes/source/database/create.go around line 102, the
code uses an unsafe type assertion for the role (role :=
ctx.Locals(middleware.RoleCtxKey).(models.Role)); change this to a safe
extraction: retrieve ctx.Locals(middleware.RoleCtxKey) into a variable, perform
a type assertion with the "value, ok := ..."(models.Role) pattern, and if ok is
false handle it by logging and returning an appropriate HTTP error (e.g.,
401/500) instead of letting a panic occur; ensure any early return matches
existing handler error handling conventions.
|
Claude finished @elliot14A's task —— View job Pull Request ReviewI've completed my review of the role authorization implementation. Here are my findings: Todo List:
🔍 Review SummaryThis PR implements role-based authorization with Admin/Member roles, adding scope-based access control for projects and datasets. While the implementation is functional, there are several critical security concerns that need to be addressed. 🚨 Critical Security Issues1. Role Authorization Logic Vulnerability -
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/interfaces/http/middleware/role.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-29T09:36:39.908Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-29T09:36:39.908Z
Learning: Applies to server/**/*.go : Integrate Zitadel authentication with JWT in the Go backend server
Applied to files:
server/interfaces/http/middleware/role.go
🧬 Code graph analysis (1)
server/interfaces/http/middleware/role.go (2)
server/infrastructure/zitadel/interceptor.go (1)
ZitadelInterceptor(15-15)server/domain/models/role.go (2)
Member(6-6)Role(3-3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
🔇 Additional comments (1)
server/interfaces/http/middleware/role.go (1)
19-38: Fix unchecked type assertions and simplify role selection logic.This code segment has multiple critical issues:
- Unchecked type assertions (lines 22, 24) will panic if the claim structure differs from expected. Zitadel claim formats can vary.
- Confusing role selection logic (lines 27-33): Setting
orgsRoletwice with overlapping conditions makes the precedence unclear. The intent appears to be "prefer non-Member roles," but the implementation is convoluted.Apply this diff to add safety and clarify logic:
orgsRole := "" claimScope := fmt.Sprintf("urn:zitadel:iam:org:project:%s:roles", viper.GetString("zitadel_project_id")) if claimValue, ok := authCtx.Claims[claimScope].(map[string]any); ok { for role, orgs := range claimValue { if orgsMap, ok := orgs.(map[string]any); ok { - for key := range orgsMap { - if key == orgID { - if orgsRole == "" { - orgsRole = role - } - - if orgsRole != "" && role != string(models.Member) { - orgsRole = role - } + if _, exists := orgsMap[orgID]; exists { + // Prefer non-Member roles; Admin takes precedence + if orgsRole == "" || role != string(models.Member) { + orgsRole = role } } } } } + + // Default to Member if no role found + if orgsRole == "" { + orgsRole = string(models.Member) + }Likely an incorrect or invalid review comment.
| authCtx := zitadel.ZitadelInterceptor.Context(c.Context()) | ||
| orgID := c.Locals(OrganizationCtxKey).(string) |
There was a problem hiding this comment.
Add nil checks and safe type assertions to prevent runtime panics.
The middleware has two critical safety issues:
authCtx(line 16) can benilwhen no authentication context exists, but line 22 accessesauthCtx.Claimswithout checking, causing a nil pointer dereference.- The type assertion
orgID := c.Locals(OrganizationCtxKey).(string)(line 17) will panic if the value is not a string or isnil.
Apply this diff to add safety checks:
func RoleAuthorization() fiber.Handler {
return func(c *fiber.Ctx) error {
authCtx := zitadel.ZitadelInterceptor.Context(c.Context())
- orgID := c.Locals(OrganizationCtxKey).(string)
+ if authCtx == nil {
+ c.Locals(RoleCtxKey, models.Member)
+ return c.Next()
+ }
+
+ orgID, ok := c.Locals(OrganizationCtxKey).(string)
+ if !ok || orgID == "" {
+ c.Locals(RoleCtxKey, models.Member)
+ return c.Next()
+ }
orgsRole := ""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| authCtx := zitadel.ZitadelInterceptor.Context(c.Context()) | |
| orgID := c.Locals(OrganizationCtxKey).(string) | |
| func RoleAuthorization() fiber.Handler { | |
| return func(c *fiber.Ctx) error { | |
| authCtx := zitadel.ZitadelInterceptor.Context(c.Context()) | |
| if authCtx == nil { | |
| c.Locals(RoleCtxKey, models.Member) | |
| return c.Next() | |
| } | |
| orgID, ok := c.Locals(OrganizationCtxKey).(string) | |
| if !ok || orgID == "" { | |
| c.Locals(RoleCtxKey, models.Member) | |
| return c.Next() | |
| } | |
| orgsRole := "" |
🤖 Prompt for AI Agents
In server/interfaces/http/middleware/role.go around lines 16 to 17, the code
assumes zitadel.ZitadelInterceptor.Context(c.Context()) and
c.Locals(OrganizationCtxKey) are non-nil and of the expected type; add explicit
nil and type-safety checks: verify authCtx is non-nil before accessing
authCtx.Claims and handle the missing auth context path (return/abort with a
401/appropriate error or skip role checks), and replace the direct assertion
orgID := c.Locals(OrganizationCtxKey).(string) with a safe assertion like v, ok
:= c.Locals(OrganizationCtxKey).(string) and handle !ok or empty string (abort
with a 400/unauthorized or fallback) to avoid panics.
| } | ||
| } | ||
|
|
||
| c.Locals(RoleCtxKey, models.Role(orgsRole)) |
There was a problem hiding this comment.
Validate role value before storing in context.
The orgsRole string is converted to models.Role without validation. If the Zitadel claims contain an unexpected role value or if extraction fails, this could store an invalid role (including empty string) that downstream handlers won't recognize.
Apply this diff to validate the role:
+ // Validate role is a known value
+ role := models.Role(orgsRole)
+ if role != models.Member && role != models.Admin {
+ role = models.Member
+ }
- c.Locals(RoleCtxKey, models.Role(orgsRole))
+ c.Locals(RoleCtxKey, role)
return c.Next()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| c.Locals(RoleCtxKey, models.Role(orgsRole)) | |
| // Validate role is a known value | |
| role := models.Role(orgsRole) | |
| if role != models.Member && role != models.Admin { | |
| role = models.Member | |
| } | |
| c.Locals(RoleCtxKey, role) | |
| return c.Next() |
🤖 Prompt for AI Agents
In server/interfaces/http/middleware/role.go around line 40, the code stores
models.Role(orgsRole) into context without validating orgsRole; change this to
first validate that orgsRole is non-empty and matches one of the known Role
values (use the models.Role constants or a whitelist of allowed strings), and
only call c.Locals(RoleCtxKey, models.Role(...)) when the value is valid; if the
value is missing or invalid, handle it explicitly (return an appropriate HTTP
error or skip setting the role and log the issue) so downstream handlers never
receive an unknown/empty role.
|
Claude finished @elliot14A's task —— View job Pull Request ReviewI've completed my review of this role authorization implementation. Here are my findings: Todo List:
🔍 Review SummaryThis PR implements role-based authorization with Admin/Member roles, adding scope-based access control for projects and datasets. While the implementation is functional, there are several critical security concerns that need to be addressed. 🚨 Critical Security Issues1. Role Determination Logic Vulnerability -
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
server/infrastructure/postgres/store/datasets/update.go (2)
44-45: Handle json.Unmarshal errors to prevent silent data corruption.The error from
json.Unmarshalis being silently ignored. If unmarshaling fails (e.g., due to corrupted data or format changes), thecolumnsslice will remain empty, potentially masking data integrity issues.🔎 Proposed fix
columns := make([]map[string]any, 0) - _ = json.Unmarshal(columnsBytes, &columns) + if err := json.Unmarshal(columnsBytes, &columns); err != nil { + s.logger.Error("Error unmarshaling columns", zap.Error(err)) + return nil, err + }
92-93: Handle json.Unmarshal errors to prevent silent data corruption.The error from
json.Unmarshalis being silently ignored. If unmarshaling fails (e.g., due to corrupted data or format changes), thecolumnsslice will remain empty, potentially masking data integrity issues. This is the same issue present in the non-transactionalUpdatemethod.🔎 Proposed fix
columns := make([]map[string]any, 0) - _ = json.Unmarshal(columnsBytes, &columns) + if err := json.Unmarshal(columnsBytes, &columns); err != nil { + s.logger.Error("Error unmarshaling columns with tx", zap.Error(err)) + return nil, err + }web/src/lib/auth/zitadel-client.ts (1)
150-159: Add validation for required environment variables.The constructor reads environment variables without validating they exist. If
ZITADEL_ADMIN_PATor other required variables are missing, this will silently create an instance withundefinedvalues, leading to runtime failures when methods are called.🔎 Proposed fix to validate environment variables
constructor() { this.authority = process.env.ZITADEL_AUTHORITY!; this.clientId = process.env.ZITADEL_CLIENT_ID!; this.pat = process.env.ZITADEL_PAT!; this.adminPat = process.env.ZITADEL_ADMIN_PAT!; this.idpId = process.env.ZITADEL_IDP_ID!; this.redirectUri = process.env.ZITADEL_REDIRECT_URI!; this.serviceUserId = process.env.ZITADEL_SERVICE_USER_ID!; this.projectId = process.env.ZITADEL_PROJECT_ID!; + + const requiredVars = { + ZITADEL_AUTHORITY: this.authority, + ZITADEL_CLIENT_ID: this.clientId, + ZITADEL_PAT: this.pat, + ZITADEL_ADMIN_PAT: this.adminPat, + ZITADEL_IDP_ID: this.idpId, + ZITADEL_REDIRECT_URI: this.redirectUri, + ZITADEL_SERVICE_USER_ID: this.serviceUserId, + ZITADEL_PROJECT_ID: this.projectId, + }; + + const missing = Object.entries(requiredVars) + .filter(([_, value]) => !value) + .map(([key]) => key); + + if (missing.length > 0) { + throw new Error(`Missing required environment variables: ${missing.join(', ')}`); + } }
🧹 Nitpick comments (2)
web/src/lib/queries/user/get-user-detail.ts (2)
3-20: Consider consolidating duplicate UserDetail interface.The
UserDetailinterface here duplicatesZitadelUserDetaildefined inweb/src/lib/auth/zitadel-client.ts(lines 75-91). The structures are nearly identical, which creates maintenance burden if either needs updates.🔎 Proposed consolidation approach
Option 1: Export and reuse ZitadelUserDetail
In
web/src/lib/auth/zitadel-client.ts, the interface is already exported. Import and reuse it here:+import { ZitadelUserDetail } from "@/lib/auth/zitadel-client"; - -interface UserDetail { - id: string; - human?: { - profile: { - givenName?: string; - familyName?: string; - nickName?: string; - displayName?: string; - preferredLanguage?: string; - gender?: string; - avatarUrl?: string; - }; - email: { - email: string; - isVerified: boolean; - } - }; -} async function fetchUserDetail({ userId, -}: GetUserDetailVariables): Promise<{ user: UserDetail }> { +}: GetUserDetailVariables): Promise<{ user: ZitadelUserDetail }> { // ... } export const useUserDetail = createQuery< - { user: UserDetail }, + { user: ZitadelUserDetail }, GetUserDetailVariables, Error >({ // ... });Option 2: Create a shared types file
If the auth client types shouldn't be imported directly, create
web/src/types/user.ts:export interface UserDetail { id: string; human?: { profile: { givenName?: string; familyName?: string; nickName?: string; displayName?: string; preferredLanguage?: string; gender?: string; avatarUrl?: string; }; email: { email: string; isVerified: boolean; } }; }Then import from both files.
26-40: Refine error handling to preserve error details.The catch block wraps the error but loses the original error object's type and properties. This makes debugging more difficult and loses useful information like HTTP status codes.
🔎 Proposed improvement
async function fetchUserDetail({ userId, }: GetUserDetailVariables): Promise<{ user: UserDetail }> { - try { - const response = await fetch(`/api/users/${userId}`); + const response = await fetch(`/api/users/${userId}`); - if (!response.ok) { - throw new Error(`Failed to fetch user: ${response.status}`); - } + if (!response.ok) { + const errorText = await response.text().catch(() => 'Unknown error'); + throw new Error( + `Failed to fetch user ${userId}: ${response.status} ${response.statusText} - ${errorText}` + ); + } - return await response.json(); - } catch (error) { - throw new Error("Failed to fetch user information: " + error); - } + return await response.json(); }This approach:
- Removes the redundant try-catch (fetch already throws on network errors)
- Provides more detailed error messages including status text
- Safely handles error response body parsing
- Allows React Query to handle the thrown error directly
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docker-compose-auth.yamlserver/infrastructure/postgres/store/datasets/update.goserver/interfaces/http/serve.goweb/src/app/api/users/[userId]/route.tsweb/src/components/dataset/dataset-header.tsxweb/src/components/project/inline-project-editor.tsxweb/src/components/user/user-display-name.tsxweb/src/lib/auth/zitadel-client.tsweb/src/lib/queries/user/get-user-detail.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- server/interfaces/http/serve.go
🧰 Additional context used
📓 Path-based instructions (4)
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.{ts,tsx}: Use Next.js 15 with App Router and React 19 for the web frontend framework
Use TypeScript as the language for the web frontend
Generate and use types from API responses for type safety
Use dynamic imports and code splitting for optimal performance
Load DuckDB WASM on-demand and use web workers for client-side SQL execution
Files:
web/src/app/api/users/[userId]/route.tsweb/src/components/project/inline-project-editor.tsxweb/src/components/user/user-display-name.tsxweb/src/lib/queries/user/get-user-detail.tsweb/src/components/dataset/dataset-header.tsxweb/src/lib/auth/zitadel-client.ts
web/src/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
Protect routes using the ProtectedRoute wrapper HOC for authentication-required pages
Files:
web/src/app/api/users/[userId]/route.ts
web/src/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/components/**/*.{ts,tsx}: Use TailwindCSS with Radix UI components for styling instead of custom CSS
Use React Hook Form with Zod validation for form handling
Use pre-configured queries and mutations from lib/queries and lib/mutations for API calls
Wrap features in error boundaries to handle errors gracefully
Always handle loading and error states in UI components
Use Next.js Image component for image optimization
Files:
web/src/components/project/inline-project-editor.tsxweb/src/components/user/user-display-name.tsxweb/src/components/dataset/dataset-header.tsx
web/src/lib/{queries,mutations}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
Use React Query (TanStack Query) for server state management
Files:
web/src/lib/queries/user/get-user-detail.ts
🧠 Learnings (10)
📚 Learning: 2025-07-29T09:36:39.908Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-29T09:36:39.908Z
Learning: Applies to web/**/*.{js,jsx,ts,tsx} : Use Zitadel for authentication in the Next.js frontend
Applied to files:
web/src/app/api/users/[userId]/route.tsweb/src/lib/auth/zitadel-client.ts
📚 Learning: 2025-12-02T11:48:50.411Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: web/CLAUDE.md:0-0
Timestamp: 2025-12-02T11:48:50.411Z
Learning: Applies to web/src/app/**/*.{ts,tsx} : Protect routes using the ProtectedRoute wrapper HOC for authentication-required pages
Applied to files:
web/src/app/api/users/[userId]/route.ts
📚 Learning: 2025-12-02T11:48:50.411Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: web/CLAUDE.md:0-0
Timestamp: 2025-12-02T11:48:50.411Z
Learning: Applies to web/src/lib/{queries,mutations}/**/*.{ts,tsx} : Use React Query (TanStack Query) for server state management
Applied to files:
web/src/lib/queries/user/get-user-detail.ts
📚 Learning: 2025-12-02T11:48:50.411Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: web/CLAUDE.md:0-0
Timestamp: 2025-12-02T11:48:50.411Z
Learning: Applies to web/src/components/**/*.{ts,tsx} : Use pre-configured queries and mutations from lib/queries and lib/mutations for API calls
Applied to files:
web/src/lib/queries/user/get-user-detail.ts
📚 Learning: 2025-12-02T11:48:50.411Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: web/CLAUDE.md:0-0
Timestamp: 2025-12-02T11:48:50.411Z
Learning: Applies to web/src/**/*.{ts,tsx} : Generate and use types from API responses for type safety
Applied to files:
web/src/lib/queries/user/get-user-detail.ts
📚 Learning: 2025-12-02T11:48:50.411Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: web/CLAUDE.md:0-0
Timestamp: 2025-12-02T11:48:50.411Z
Learning: Applies to web/src/components/dataset/**upload**.{ts,tsx} : Use Uppy for dataset file uploads in upload components
Applied to files:
web/src/components/dataset/dataset-header.tsx
📚 Learning: 2025-12-02T11:48:50.411Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: web/CLAUDE.md:0-0
Timestamp: 2025-12-02T11:48:50.411Z
Learning: When authentication is enabled, configure Zitadel OAuth variables: ZITADEL_AUTHORITY, ZITADEL_CLIENT_ID, ZITADEL_CLIENT_SECRET, ZITADEL_PROJECT_ID, ZITADEL_PAT, and ZITADEL_IDP_ID
Applied to files:
web/src/lib/auth/zitadel-client.ts
📚 Learning: 2025-07-29T09:36:39.908Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-29T09:36:39.908Z
Learning: Applies to docker-compose-noauth.yaml : Use `docker-compose-noauth.yaml` for no-auth development setup
Applied to files:
docker-compose-auth.yaml
📚 Learning: 2025-07-29T09:36:39.908Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-29T09:36:39.908Z
Learning: Applies to docker-compose*.yaml : Use Docker Compose for full stack and integration testing
Applied to files:
docker-compose-auth.yaml
📚 Learning: 2025-12-02T11:48:34.955Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: chat-server/CLAUDE.md:0-0
Timestamp: 2025-12-02T11:48:34.955Z
Learning: Applies to chat-server/**/docker-compose.yaml : Use docker-compose for orchestrating service dependencies in development
Applied to files:
docker-compose-auth.yaml
🧬 Code graph analysis (4)
web/src/app/api/users/[userId]/route.ts (1)
web/src/lib/auth/zitadel-client.ts (1)
zitadelClient(747-747)
web/src/components/project/inline-project-editor.tsx (1)
web/src/components/user/user-display-name.tsx (1)
UserDisplayName(12-29)
web/src/components/user/user-display-name.tsx (1)
web/src/lib/queries/user/get-user-detail.ts (1)
useUserDetail(42-49)
web/src/components/dataset/dataset-header.tsx (1)
web/src/components/user/user-display-name.tsx (1)
UserDisplayName(12-29)
🔇 Additional comments (5)
server/infrastructure/postgres/store/datasets/update.go (1)
83-84: Verify UpdatedBy and OrgID are non-empty before marking Valid.Similar to the non-transactional
Updatemethod, bothUpdatedByandOrgIDare unconditionally marked asValid: true, which means empty strings will be written to the database if these fields are empty inupdateDatasetParams.web/src/lib/auth/zitadel-client.ts (1)
741-744: LGTM! Admin context appropriately used for user details.The
getUserByIdmethod correctly uses the admin PAT (viaisAdmin: true) to fetch user details, which is necessary for retrieving information about other users.web/src/components/dataset/dataset-header.tsx (1)
661-674: LGTM! Consistent user display implementation.The UserDisplayName component provides a consistent way to display user information across the application. Using it for both created_by and updated_by fields improves maintainability.
web/src/components/project/inline-project-editor.tsx (1)
249-252: LGTM! Consistent user display implementation.Using the UserDisplayName component here maintains consistency with other parts of the application.
web/src/components/user/user-display-name.tsx (1)
12-29: LGTM! Clean component structure with appropriate loading state.The component correctly handles:
- Loading state with a Skeleton placeholder
- Fallback chain: displayName → fallback prop → userId
- Conditional query enabling
| /usr/bin/mc anonymous set private myminio/gopie && | ||
| /usr/bin/mc anonymous set private myminio/gopie/visualizations && | ||
| /usr/bin/mc anonymous set private myminio/downloads |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | head -20Repository: factly/gopie
Length of output: 562
🏁 Script executed:
find . -name "docker-compose*.yaml" -type fRepository: factly/gopie
Length of output: 206
🏁 Script executed:
rg -nP --type=go '\bhttp.*s3.*amazonaws\.com\b' -A 3 -B 3Repository: factly/gopie
Length of output: 38
🏁 Script executed:
rg -nP --type=go 'Presign|PreSign|presign' -A 3Repository: factly/gopie
Length of output: 7601
🏁 Script executed:
rg -l "minio|s3\." | head -20Repository: factly/gopie
Length of output: 664
🏁 Script executed:
sed -n '130,160p' docker-compose-auth.yamlRepository: factly/gopie
Length of output: 1079
🏁 Script executed:
rg -nP 'visualizations' --type=go -A 2 -B 2 | head -40Repository: factly/gopie
Length of output: 38
🏁 Script executed:
rg -nP 'visualization.*presign|presign.*visualization' --type=go -A 3Repository: factly/gopie
Length of output: 38
🏁 Script executed:
# Check if visualization assets are served through the application or directly
rg -nP 'visualization' --type=ts --type=tsx -A 2 | head -40Repository: factly/gopie
Length of output: 83
🏁 Script executed:
# Check for any direct bucket access patterns in web frontend
rg -nP 's3\.|minio|bucket' --type=ts --type=tsx | head -20Repository: factly/gopie
Length of output: 83
🏁 Script executed:
rg -nP 'visualization' --type=go -B 2 -A 2Repository: factly/gopie
Length of output: 38
🏁 Script executed:
# Search for API endpoints that serve visualizations
rg -nP 'visualization|visual' --type=go | grep -i 'route\|handler\|endpoint' | head -20Repository: factly/gopie
Length of output: 38
🏁 Script executed:
# Check web frontend for visualization components/API calls
fd -e tsx -e ts | xargs rg -l 'visualization' | head -10Repository: factly/gopie
Length of output: 454
🏁 Script executed:
# Check the actual visualization endpoint implementation
rg -nP 'GET.*visualization|visualization.*endpoint' --type=go -A 5Repository: factly/gopie
Length of output: 38
🏁 Script executed:
# Look for any bucket-specific logic
rg -nP 'gopie|downloads|visualizations' --type=go | grep -i 'bucket\|key\|path' | head -20Repository: factly/gopie
Length of output: 2137
🏁 Script executed:
# Search for visualization routes/handlers more broadly
rg -nP 'visualization' --type=go | head -30Repository: factly/gopie
Length of output: 38
🏁 Script executed:
# Check if there's an API endpoint that handles visualization retrieval
rg -nP 'func.*Handler|func.*Route|func.*Endpoint' --type=go -A 3 | grep -i -B 3 -A 3 'visualization\|visual'Repository: factly/gopie
Length of output: 38
🏁 Script executed:
# Look at the visualization results component to understand the API call
cat web/src/components/chat/visualization-results.tsxRepository: factly/gopie
Length of output: 6841
🏁 Script executed:
# Check server HTTP routes configuration
fd -e go -path '*/http/*' -o -path '*/routes/*' | head -10Repository: factly/gopie
Length of output: 224
🏁 Script executed:
rg -nP 'visualization' --type=go -B 3 -A 3 | head -60Repository: factly/gopie
Length of output: 38
🏁 Script executed:
# Search for where visualization paths/URLs are generated in the chat/download service
rg -nP 'path.*visualization|visualization.*path|gopie/visualization' --type=go -B 2 -A 2Repository: factly/gopie
Length of output: 38
🏁 Script executed:
# Check the download service to see how it returns visualization paths
cat server/application/services/download.go | head -100Repository: factly/gopie
Length of output: 2969
🏁 Script executed:
# Search for where visualization objects are stored or referenced
rg -nP '"gopie/visualization|myminio/gopie' --type=goRepository: factly/gopie
Length of output: 38
🏁 Script executed:
# Continue reading the download service
cat server/application/services/download.go | sed -n '100,250p'Repository: factly/gopie
Length of output: 2721
🏁 Script executed:
# Look for where visualization files are generated or stored
rg -nP 'visualization.*\.json|vega.*spec' --type=go -B 2 -A 2Repository: factly/gopie
Length of output: 38
🏁 Script executed:
# Check the chat routes to see how visualization responses are built
rg -nP 'Visualization' --type=go -B 3 -A 5 | head -80Repository: factly/gopie
Length of output: 38
🏁 Script executed:
# Search for Chat service and visualization path generation
find server -name "*.go" -type f | xargs grep -l "visualization" | head -5Repository: factly/gopie
Length of output: 38
🏁 Script executed:
# Look for chat service or any service that returns visualization paths
rg -nP 'type.*Chat|type.*Visualization' --type=go -A 10 | head -60Repository: factly/gopie
Length of output: 4501
🏁 Script executed:
# Search for where paths are constructed and returned to the frontend
rg -nP 'Visualizations|visualizations' --type=go -B 5 -A 5 | head -80Repository: factly/gopie
Length of output: 38
🏁 Script executed:
# Look for any mentions of gopie bucket path construction
rg -nP 'gopie/|downloads/' --type=go | grep -v 'test\|mock' | head -20Repository: factly/gopie
Length of output: 1835
🏁 Script executed:
# Look at chat message model to see how visualizations are included
cat server/domain/models/chat.goRepository: factly/gopie
Length of output: 3432
🏁 Script executed:
# Check the AI agent response model for visualization paths
cat server/domain/models/aiagent.goRepository: factly/gopie
Length of output: 500
🏁 Script executed:
# Search for where visualization paths/URLs are constructed
rg -nP 'path|url' server/domain/models/chat.go -iRepository: factly/gopie
Length of output: 38
🏁 Script executed:
# Check the visualization results component to see what format it expects
rg -nP 'paths\[' web/src/components/chat/visualization-results.tsx -B 5 -A 5Repository: factly/gopie
Length of output: 448
🏁 Script executed:
# Search for where visualization paths are constructed and sent to frontend
rg -nP 'visualization' --type=go | grep -i 'path\|url\|s3\|bucket' -A 2 -B 2Repository: factly/gopie
Length of output: 38
🏁 Script executed:
# Look at the AI agent implementation to see how it returns visualizations
find server -name "*.go" -path "*/ai*" -o -path "*/agent*" | head -10Repository: factly/gopie
Length of output: 462
🏁 Script executed:
# Search for where response is built with visualization data
rg -nP 'response.*visualization|visualization.*response' --type=go -A 5 -B 5Repository: factly/gopie
Length of output: 38
🏁 Script executed:
# Check chat-server for visualization handling
find chat-server -name "*.py" | xargs grep -l "visualization" | head -5Repository: factly/gopie
Length of output: 298
🏁 Script executed:
# Check the result_paths tool to see how visualization paths are constructed
cat chat-server/app/tool_utils/tools/result_paths.pyRepository: factly/gopie
Length of output: 1663
🏁 Script executed:
# Check run_python_code to see if it generates visualization paths
cat chat-server/app/tool_utils/tools/run_python_code.py | head -100Repository: factly/gopie
Length of output: 2297
🏁 Script executed:
# Search for S3 bucket references in chat-server
rg -nP 'gopie|visualizations|downloads' chat-server --type=py | head -30Repository: factly/gopie
Length of output: 4542
🏁 Script executed:
# Check S3 configuration in chat-server
rg -nP 's3|bucket|minio' chat-server --type=py -i | head -40Repository: factly/gopie
Length of output: 4771
🏁 Script executed:
# View the upload_visualization_result_data function
sed -n '187,237p' chat-server/app/workflow/graph/visualize_data_graph/utils.pyRepository: factly/gopie
Length of output: 2094
🏁 Script executed:
# Check if S3 URLs returned are pre-signed or direct public URLs
rg -nP 'external_s3_host|s3_path' chat-server/app/workflow/graph/visualize_data_graph/utils.py -B 2 -A 2Repository: factly/gopie
Length of output: 1100
🏁 Script executed:
# Check the bucket configuration and if it uses public or private ACLs
rg -nP 'S3_BUCKET|bucket_name' chat-server --type=pyRepository: factly/gopie
Length of output: 677
🏁 Script executed:
# Look for any pre-signed URL generation in chat-server
rg -nP 'presign|pre_sign|generate_presigned' chat-server --type=py -iRepository: factly/gopie
Length of output: 38
🏁 Script executed:
# Check if visualization URLs are ever pre-signed in chat-server
rg -nP 'presign|pre.sign|generate.*url|sign.*url' chat-server --type=py -iRepository: factly/gopie
Length of output: 38
🏁 Script executed:
# Check the full flow of how visualization paths reach the frontend
cat chat-server/app/workflow/graph/visualize_data_graph/node/process_results.py | head -150Repository: factly/gopie
Length of output: 5467
🏁 Script executed:
# Verify the bucket names used for visualizations vs downloads
rg -nP 'S3_BUCKET|gopie.*bucket|visualization.*bucket' chat-server --type=py -iRepository: factly/gopie
Length of output: 210
🏁 Script executed:
# Check if there's any middleware or authentication for S3 access
rg -nP 'middleware|auth|access.*control' chat-server --type=py -i | grep -i s3Repository: factly/gopie
Length of output: 38
🏁 Script executed:
# Check what bucket is used for visualizations
rg -nP 'S3_BUCKET.*gopie|bucket.*visualization' chat-server --type=py -A 2 -B 2Repository: factly/gopie
Length of output: 38
🏁 Script executed:
# Check the docker-compose file to confirm bucket names
cat docker-compose-auth.yaml | grep -A 5 'createbuckets'Repository: factly/gopie
Length of output: 170
🏁 Script executed:
# Confirm visualization bucket path setup
grep -n 'gopie/visualizations\|visualizations/' chat-server/app/workflow/graph/visualize_data_graph/utils.pyRepository: factly/gopie
Length of output: 211
🏁 Script executed:
# Check how S3_BUCKET is set in docker-compose
grep -n 'S3_BUCKET' docker-compose-auth.yamlRepository: factly/gopie
Length of output: 38
🏁 Script executed:
# Check the config files to see default bucket values
grep -n 'gopie\|bucket' config-auth.env.exampleRepository: factly/gopie
Length of output: 466
🏁 Script executed:
cat config-auth.env.example | grep -i bucketRepository: factly/gopie
Length of output: 141
Visualization access will fail with private bucket ACLs—pre-signed URLs required.
Changing bucket ACLs to private breaks visualization delivery. The downloads service correctly uses pre-signed URLs, but visualizations return direct S3 URLs (http://minio:9000/gopie/visualizations/{id}.json) that the frontend fetches directly. With private buckets, these direct fetches will fail with 403 Forbidden.
Update the visualization upload path (chat-server/app/workflow/graph/visualize_data_graph/utils.py line 227) to generate pre-signed URLs instead of direct bucket URLs, matching the approach used for downloads.
🤖 Prompt for AI Agents
In docker-compose-auth.yaml lines 147-149 we set the buckets to private which
makes direct S3 URLs for visualizations 403; update
chat-server/app/workflow/graph/visualize_data_graph/utils.py (around line 227)
so the function that builds the visualization URL returns a pre-signed GET URL
(using the same MinIO/S3 client call and expiration logic used by the downloads
service) instead of a direct http://minio:9000/... path; ensure the pre-signed
URL generation includes the correct bucket/object path, expiry, and any required
credentials so the frontend can fetch the visualization JSON via the presigned
link.
| UpdatedBy: pgtype.Text{String: updateDatasetParams.UpdatedBy, Valid: true}, | ||
| OrgID: pgtype.Text{String: updateDatasetParams.OrgID, Valid: true}, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's examine the update.go file to understand the context
cat -n server/infrastructure/postgres/store/datasets/update.goRepository: factly/gopie
Length of output: 5009
🏁 Script executed:
#!/bin/bash
# Search for the UpdateDatasetParams struct definition
rg -B 5 -A 15 'type UpdateDatasetParams struct' --type goRepository: factly/gopie
Length of output: 3417
🏁 Script executed:
#!/bin/bash
# Search for where this Update function is called and if there's validation
rg -B 10 -A 5 'func.*Update.*UpdateDatasetParams' --type goRepository: factly/gopie
Length of output: 5884
🏁 Script executed:
#!/bin/bash
# Check for validation logic in handler or service layers
find . -type f -name "*.go" | xargs grep -l "UpdateDatasetParams" | head -20Repository: factly/gopie
Length of output: 506
🏁 Script executed:
#!/bin/bash
# Check the HTTP handler for dataset update
cat -n server/interfaces/http/routes/api/projects/datasets/update.goRepository: factly/gopie
Length of output: 4346
🏁 Script executed:
#!/bin/bash
# Look for SQL schema or migrations to see database constraints
find . -type f \( -name "*.sql" -o -name "*migration*" \) | head -20Repository: factly/gopie
Length of output: 1740
🏁 Script executed:
#!/bin/bash
# Search for any validation middleware or input validation
rg -B 3 -A 3 'UpdateDatasetParams' server/interfaces/http/ --type goRepository: factly/gopie
Length of output: 1782
Apply consistent validation for UpdatedBy and OrgID fields.
Both UpdatedBy and OrgID are unconditionally marked as Valid: true, inconsistent with how other optional Text fields are handled (e.g., Alias, Description, CustomPrompt check for non-empty values). Validate that these fields are non-empty before marking Valid, matching the established pattern in this function.
🤖 Prompt for AI Agents
In server/infrastructure/postgres/store/datasets/update.go around lines 35 to
36, UpdatedBy and OrgID are being set with pgtype.Text{String: ..., Valid: true}
unconditionally; make them follow the same non-empty validation pattern used for
Alias/Description/CustomPrompt by setting Valid to true only when the
corresponding string is non-empty (e.g., Valid: updateDatasetParams.UpdatedBy !=
"" and Valid: updateDatasetParams.OrgID != "") so empty values are represented
as invalid/null in the DB update.
| export async function GET( | ||
| request: NextRequest, | ||
| { params }: { params: Promise<{ userId: string }> } | ||
| ) { | ||
| try { | ||
| const { userId } = await params; | ||
|
|
||
| if (!userId) { | ||
| return NextResponse.json( | ||
| { error: "User ID is required" }, | ||
| { status: 400 } | ||
| ); | ||
| } | ||
|
|
||
| // Get user details from Zitadel | ||
| const user = await zitadelClient.getUserById(userId); | ||
|
|
||
| if (!user) { | ||
| return NextResponse.json({ error: "User not found" }, { status: 404 }); | ||
| } | ||
|
|
||
| // Only "user" field in response needs to be passed. | ||
| return NextResponse.json({ user }); | ||
| } catch (error) { | ||
| console.error("Error fetching user info:", error); | ||
| return NextResponse.json( | ||
| { error: "Failed to fetch user information" }, | ||
| { status: 500 } | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
CRITICAL: Missing authentication and authorization checks.
This API route exposes user details without verifying:
- Whether the requester is authenticated
- Whether the requester is authorized to view the requested user's information
- Rate limiting to prevent enumeration attacks
Any unauthenticated user can fetch any user's profile information (name, email, avatar) by calling /api/users/{userId}.
🔎 Proposed fix to add authentication
import { NextRequest, NextResponse } from "next/server";
import { zitadelClient } from "@/lib/auth/zitadel-client";
+import { getServerSession } from "@/lib/auth/session"; // Adjust import path as needed
export async function GET(
request: NextRequest,
{ params }: { params: Promise<{ userId: string }> }
) {
try {
+ // Verify the user is authenticated
+ const session = await getServerSession();
+ if (!session?.user) {
+ return NextResponse.json(
+ { error: "Unauthorized" },
+ { status: 401 }
+ );
+ }
+
const { userId } = await params;
if (!userId) {
return NextResponse.json(
{ error: "User ID is required" },
{ status: 400 }
);
}
// Get user details from Zitadel
const user = await zitadelClient.getUserById(userId);
if (!user) {
return NextResponse.json({ error: "User not found" }, { status: 404 });
}
// Only "user" field in response needs to be passed.
return NextResponse.json({ user });
} catch (error) {
console.error("Error fetching user info:", error);
return NextResponse.json(
{ error: "Failed to fetch user information" },
{ status: 500 }
);
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In web/src/app/api/users/[userId]/route.ts around lines 4 to 34, the route
currently returns user data without authentication, authorization, or
rate-limiting checks; update the handler to (1) authenticate the requester
(validate session or bearer token and reject with 401 if missing/invalid), (2)
authorize access (allow if requester is the same userId or has admin/scope,
otherwise return 403), (3) enforce rate limiting to prevent enumeration (apply
per-IP or per-user limits), and (4) only return a minimal safe user object (no
sensitive fields) after these checks; keep existing error handling and ensure
proper status codes for each failure case.
|
Claude finished @elliot14A's task —— View job Pull Request ReviewI've completed my review of this role authorization implementation. Here are my findings: Todo List:
🔍 Review SummaryThis PR implements role-based authorization with Admin/Member roles, adding scope-based access control for projects and datasets. While the implementation is functional, there are several critical security concerns that need to be addressed. 🚨 Critical Security Issues1. Role Determination Logic Vulnerability -
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
server/Dockerfile.dev (2)
8-9: Pin goose to a specific version for reproducibility.Using
@latestcan lead to non-reproducible builds. Consider pinning to a specific version like Air is pinned on line 6.Proposed fix
# Install goose migration tool -RUN go install github.com/pressly/goose/v3/cmd/goose@latest +RUN go install github.com/pressly/goose/v3/cmd/goose@v3.24.1
16-43: Consider using a COPY instead of inline echo for maintainability.The inline
.air.tomlgeneration works but is harder to read and maintain. An alternative approach is to maintain a separate.air.tomlfile and COPY it. However, this is acceptable for a dev-only Dockerfile.docker-compose.yaml (1)
56-68: Development credentials in compose file are acceptable, but consider env_file consistency.The
gopie-migrateservice uses inline credentials while other services useenv_file. For consistency, consider moving these to the env file as well. The static analysis tool flagged the embedded credentials, but this is expected for local development.Proposed fix for consistency
gopie-migrate: build: context: ./server dockerfile: Dockerfile.migrate - environment: - - GOOSE_DBSTRING=postgres://postgres:postgres@postgres:5432/gopie?sslmode=disable - - GOOSE_DRIVER=postgres - - GOOSE_MIGRATION_DIR=migrations + env_file: + - ./config.env depends_on: postgres: condition: service_healthyThen add to
config.env:GOOSE_DBSTRING=postgres://postgres:postgres@postgres:5432/gopie?sslmode=disable GOOSE_DRIVER=postgres GOOSE_MIGRATION_DIR=migrationsREADME.md (2)
25-27: Add language specifier to fenced code block.Per markdown best practices, fenced code blocks should specify a language. For
/etc/hostsentries, usetextorplaintext.Proposed fix
-``` +```text 127.0.0.1 127.0.0.1.zitadel.local</details> --- `9-9`: **Consider using proper markdown headings instead of bold text.** Static analysis flagged several instances of bold text being used as pseudo-headings. While functional, using proper heading levels (`####`) improves document structure and accessibility. <details> <summary>Example fix for one instance</summary> ```diff -**1. Prepare Environment Variables** +#### 1. Prepare Environment VariablesAlso applies to: 18-18, 22-22, 29-29, 34-34
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.mddocker-compose.yamlserver/.env.exampleserver/Dockerfile.dev
💤 Files with no reviewable changes (1)
- server/.env.example
🧰 Additional context used
🧠 Learnings (12)
📚 Learning: 2025-07-29T09:36:39.908Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-29T09:36:39.908Z
Learning: Applies to server/main.go : Use `go run main.go serve` to start the Go backend server development server
Applied to files:
server/Dockerfile.devREADME.md
📚 Learning: 2025-07-29T09:36:39.908Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-29T09:36:39.908Z
Learning: Applies to server/go.mod : Run `go mod tidy` to install and tidy dependencies in the Go backend server
Applied to files:
server/Dockerfile.dev
📚 Learning: 2025-07-29T09:36:39.908Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-29T09:36:39.908Z
Learning: Applies to server/**/* : Generate Swagger docs in the Go backend server using `swag init`
Applied to files:
server/Dockerfile.dev
📚 Learning: 2025-07-29T09:36:39.908Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-29T09:36:39.908Z
Learning: Applies to server/**/*.go : Integrate DuckDB for OLAP queries in the Go backend server
Applied to files:
server/Dockerfile.dev
📚 Learning: 2025-07-29T09:36:39.908Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-29T09:36:39.908Z
Learning: Applies to server/go.mod : Use `go mod tidy` to install dependencies in the Go backend server
Applied to files:
server/Dockerfile.dev
📚 Learning: 2025-07-29T09:36:39.908Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-29T09:36:39.908Z
Learning: Applies to server/infrastructure/postgres/migrations/* : Run database migrations in the Go backend server using `goose up`
Applied to files:
docker-compose.yaml
📚 Learning: 2025-12-02T11:48:34.955Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: chat-server/CLAUDE.md:0-0
Timestamp: 2025-12-02T11:48:34.955Z
Learning: Applies to chat-server/**/docker-compose.yaml : Use docker-compose for orchestrating service dependencies in development
Applied to files:
docker-compose.yaml
📚 Learning: 2025-07-29T09:36:39.908Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-29T09:36:39.908Z
Learning: Applies to docker-compose*.yaml : Use `cd chat-server && docker-compose up` to run the chat server in Docker
Applied to files:
docker-compose.yaml
📚 Learning: 2025-07-29T09:36:39.908Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-29T09:36:39.908Z
Learning: Applies to docker-compose-noauth.yaml : Use `docker-compose-noauth.yaml` for no-auth development setup
Applied to files:
docker-compose.yaml
📚 Learning: 2025-07-29T09:36:39.908Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-29T09:36:39.908Z
Learning: Applies to docker-compose*.yaml : Use `docker-compose up` to start the full stack for development
Applied to files:
docker-compose.yaml
📚 Learning: 2025-07-29T09:36:39.908Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-29T09:36:39.908Z
Learning: Applies to docker-compose*.yaml : Use Docker Compose for full stack and integration testing
Applied to files:
docker-compose.yaml
📚 Learning: 2025-07-29T09:36:39.908Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-29T09:36:39.908Z
Learning: Applies to server/**/*.go : Integrate Zitadel authentication with JWT in the Go backend server
Applied to files:
README.md
🪛 Checkov (3.2.334)
docker-compose.yaml
[medium] 61-62: Basic Auth Credentials
(CKV_SECRET_4)
🪛 markdownlint-cli2 (0.18.1)
README.md
9-9: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
18-18: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
22-22: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
29-29: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
34-34: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
40-40: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
59-59: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (6)
server/Dockerfile.dev (2)
52-67: LGTM!Non-root user setup with UID/GID 1001 and proper directory ownership is well-structured for development security.
1-1: The golang:1.24.5 base image is available and valid. Go 1.24 was released on February 11, 2025, and continues to receive patch updates (latest is 1.24.11 as of December 2, 2025). No changes needed.Likely an incorrect or invalid review comment.
docker-compose.yaml (3)
85-89: LGTM!The pattern of starting as root to fix volume permissions, then dropping to the non-root
gopieuser viasu, is appropriate for handling Docker volume ownership issues.
135-139: LGTM!The bucket creation now includes the
downloadsbucket and sets public access for visualizations, which aligns with the expected functionality.
160-161: The included compose file exists and is correctly referenced.The
docker-compose-auth.yamlfile exists in thechat-serverdirectory, and the include path is correctly configured.README.md (1)
5-77: Good addition of comprehensive Getting Started documentation.The documentation clearly explains both authentication and non-authentication setup flows, with detailed Zitadel configuration steps that align with the PR's role-based authorization features.
| * Open **Default Settings** (Instance Settings) from the top right corner. | ||
| * Click the **+** icon (top right) to add a manager. | ||
| * Select the **Service User** created above. | ||
| * Add the manager role **IAM OWNER** (or "I AM LOGIN CLIENT"). |
There was a problem hiding this comment.
Typo: "I AM LOGIN CLIENT" should be "IAM LOGIN CLIENT".
The role name appears to have a typo. Based on Zitadel's IAM roles, it should likely be IAM_LOGIN_CLIENT or IAM LOGIN CLIENT without the space in "I AM".
Proposed fix
- * Add the manager role **IAM OWNER** (or "I AM LOGIN CLIENT").
+ * Add the manager role **IAM_OWNER** (or "IAM_LOGIN_CLIENT").📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * Add the manager role **IAM OWNER** (or "I AM LOGIN CLIENT"). | |
| * Add the manager role **IAM_OWNER** (or "IAM_LOGIN_CLIENT"). |
🤖 Prompt for AI Agents
In README.md at line 70, the role string contains a typo "I AM LOGIN CLIENT";
replace it with the correct form "IAM LOGIN CLIENT" (or, if you prefer
constant-style naming, "IAM_LOGIN_CLIENT") so the role name no longer has the
unintended space between I and AM.
|
Claude finished @elliot14A's task —— View job Pull Request ReviewI've completed my review of this role authorization implementation. Here are my findings: Todo List:
🔍 Review SummaryThis PR implements role-based authorization with Admin/Member roles, adding scope-based access control for projects and datasets. While the implementation is functional, there are several critical security concerns that need to be addressed. 🚨 Critical Security Issues1. Role Determination Logic Vulnerability -
|
|
Claude Code is working… I'll analyze this and get back to you. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
server/interfaces/http/middleware/role.go (2)
16-17: Critical: Add nil checks and safe type assertions to prevent runtime panics (unaddressed from previous review).The middleware still lacks critical safety checks that will cause panics in production:
- Line 16:
authCtxcan benilwhen no authentication context exists, but line 22 accessesauthCtx.Claimswithout checking, causing a nil pointer dereference.- Line 17: The type assertion
orgID := c.Locals(OrganizationCtxKey).(string)will panic if the value is not a string or isnil.These issues were previously flagged but remain unaddressed.
🔎 Proposed fix to add safety checks
func RoleAuthorization() fiber.Handler { return func(c *fiber.Ctx) error { authCtx := zitadel.ZitadelInterceptor.Context(c.Context()) - orgID := c.Locals(OrganizationCtxKey).(string) + if authCtx == nil { + c.Locals(RoleCtxKey, models.Member) + return c.Next() + } + + orgID, ok := c.Locals(OrganizationCtxKey).(string) + if !ok || orgID == "" { + c.Locals(RoleCtxKey, models.Member) + return c.Next() + } orgsRole := ""
38-39: Validate role value before storing in context (unaddressed from previous review).The
orgsRolestring is converted tomodels.Rolewithout validation. If the Zitadel claims don't contain a role for the user's organization or if the role value is unexpected, this will store an invalid role (including empty string"") that downstream handlers won't recognize, breaking authorization.This issue was previously flagged but remains unaddressed.
🔎 Proposed fix to validate and default role
+ // Validate role is a known value + role := models.Role(orgsRole) + if role != models.Member && role != models.Admin { + role = models.Member + } - c.Locals(RoleCtxKey, models.Role(orgsRole)) + c.Locals(RoleCtxKey, role) return c.Next()
🧹 Nitpick comments (3)
web/src/lib/queries/user/get-user-detail.ts (2)
26-40: Consider preserving the original error for better debugging.On line 38, wrapping the error with string concatenation loses the original stack trace and error properties, making debugging harder.
🔎 Suggested improvement to preserve error context
async function fetchUserDetail({ userId, }: GetUserDetailVariables): Promise<{ user: UserDetail }> { - try { - const response = await fetch(`/api/users/${userId}`); + const response = await fetch(`/api/users/${userId}`); - if (!response.ok) { - throw new Error(`Failed to fetch user: ${response.status}`); - } + if (!response.ok) { + throw new Error(`Failed to fetch user: ${response.status}`); + } - return await response.json(); - } catch (error) { - throw new Error("Failed to fetch user information: " + error); - } + return await response.json(); }This preserves the original error stack trace. If you need to add context, wrap without converting to string:
} catch (error) { throw new Error("Failed to fetch user information", { cause: error }); }
42-61: Consider using a queryKey function instead of middleware.The middleware approach (lines 49-60) successfully addresses the cache collision issue from the previous review, but it adds complexity. React Query Kit typically supports a simpler pattern where queryKey is a function that receives variables directly.
🔎 Simpler alternative using queryKey function
export const useUserDetail = createQuery< { user: UserDetail }, GetUserDetailVariables, Error >({ - queryKey: ["user-detail"], + queryKey: (variables) => ["user-detail", variables.userId], fetcher: fetchUserDetail, - use: [ - (next) => (options, queryClient) => { - const { variables } = options; - return next( - { - ...options, - queryKey: ["user-detail", variables?.userId], - }, - queryClient - ); - }, - ], });This achieves the same result (unique cache keys per user) with less code and better readability. If your version of react-query-kit doesn't support this pattern, the current middleware approach is a valid workaround.
server/interfaces/http/middleware/role.go (1)
22-36: Consider refactoring nested role extraction logic for clarity.The role extraction logic has 4 levels of nesting and the role selection logic (lines 27-31) could be more explicit. While functionally correct, this complexity makes the code harder to maintain and test.
💡 Suggested refactor to improve readability
claimScope := fmt.Sprintf("urn:zitadel:iam:org:project:%s:roles", viper.GetString("zitadel_project_id")) - if claimValue, ok := authCtx.Claims[claimScope].(map[string]any); ok { - for role, orgs := range claimValue { - if orgsMap, ok := orgs.(map[string]any); ok { - for key := range orgsMap { - if key == orgID { - if orgsRole == "" { - orgsRole = role - } else if role == string(models.Admin) { - orgsRole = role - } - } - } - } - } - } + orgsRole = extractRoleForOrg(authCtx.Claims, claimScope, orgID) + +func extractRoleForOrg(claims map[string]any, claimScope, orgID string) string { + claimValue, ok := claims[claimScope].(map[string]any) + if !ok { + return "" + } + + foundRole := "" + for role, orgs := range claimValue { + orgsMap, ok := orgs.(map[string]any) + if !ok { + continue + } + if _, exists := orgsMap[orgID]; exists { + // Prefer Admin over any other role + if role == string(models.Admin) { + return role + } + if foundRole == "" { + foundRole = role + } + } + } + return foundRole +}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/interfaces/http/middleware/role.goweb/src/lib/queries/user/get-user-detail.ts
🧰 Additional context used
📓 Path-based instructions (2)
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.{ts,tsx}: Use Next.js 15 with App Router and React 19 for the web frontend framework
Use TypeScript as the language for the web frontend
Generate and use types from API responses for type safety
Use dynamic imports and code splitting for optimal performance
Load DuckDB WASM on-demand and use web workers for client-side SQL execution
Files:
web/src/lib/queries/user/get-user-detail.ts
web/src/lib/{queries,mutations}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
Use React Query (TanStack Query) for server state management
Files:
web/src/lib/queries/user/get-user-detail.ts
🧠 Learnings (4)
📚 Learning: 2025-07-29T09:36:39.908Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-29T09:36:39.908Z
Learning: Applies to server/**/*.go : Integrate Zitadel authentication with JWT in the Go backend server
Applied to files:
server/interfaces/http/middleware/role.go
📚 Learning: 2025-12-02T11:48:50.411Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: web/CLAUDE.md:0-0
Timestamp: 2025-12-02T11:48:50.411Z
Learning: Applies to web/src/lib/{queries,mutations}/**/*.{ts,tsx} : Use React Query (TanStack Query) for server state management
Applied to files:
web/src/lib/queries/user/get-user-detail.ts
📚 Learning: 2025-12-02T11:48:50.411Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: web/CLAUDE.md:0-0
Timestamp: 2025-12-02T11:48:50.411Z
Learning: Applies to web/src/components/**/*.{ts,tsx} : Use pre-configured queries and mutations from lib/queries and lib/mutations for API calls
Applied to files:
web/src/lib/queries/user/get-user-detail.ts
📚 Learning: 2025-12-02T11:48:50.411Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: web/CLAUDE.md:0-0
Timestamp: 2025-12-02T11:48:50.411Z
Learning: Applies to web/src/**/*.{ts,tsx} : Generate and use types from API responses for type safety
Applied to files:
web/src/lib/queries/user/get-user-detail.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
|
|
||
| orgsRole := "" | ||
|
|
||
| claimScope := fmt.Sprintf("urn:zitadel:iam:org:project:%s:roles", viper.GetString("zitadel_project_id")) |
There was a problem hiding this comment.
Validate Zitadel project ID configuration.
If zitadel_project_id is not configured or empty, the claim scope will be malformed ("urn:zitadel:iam:org:project::roles"), causing role extraction to silently fail. This will result in users receiving an empty/invalid role.
🔎 Proposed fix to validate configuration
orgsRole := ""
+ projectID := viper.GetString("zitadel_project_id")
+ if projectID == "" {
+ // Log error and default to Member
+ c.Locals(RoleCtxKey, models.Member)
+ return c.Next()
+ }
- claimScope := fmt.Sprintf("urn:zitadel:iam:org:project:%s:roles", viper.GetString("zitadel_project_id"))
+ claimScope := fmt.Sprintf("urn:zitadel:iam:org:project:%s:roles", projectID)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| claimScope := fmt.Sprintf("urn:zitadel:iam:org:project:%s:roles", viper.GetString("zitadel_project_id")) | |
| orgsRole := "" | |
| projectID := viper.GetString("zitadel_project_id") | |
| if projectID == "" { | |
| // Log error and default to Member | |
| c.Locals(RoleCtxKey, models.Member) | |
| return c.Next() | |
| } | |
| claimScope := fmt.Sprintf("urn:zitadel:iam:org:project:%s:roles", projectID) |
🤖 Prompt for AI Agents
In server/interfaces/http/middleware/role.go around line 21, the claimScope is
built from viper.GetString("zitadel_project_id") without validation which yields
a malformed scope when the config is empty; validate that zitadel_project_id is
present and non-empty before constructing claimScope and fail fast: if empty,
log a clear error and return an initialization/fatal error (or cause middleware
to reject requests with a 5xx and explanatory log) so role extraction cannot
silently continue with a bad scope; update the code path to perform this check
and handle the missing configuration accordingly.
|
Claude finished @elliot14A's task —— View job I've completed my review of this role authorization implementation. Here are my findings: Todo List:
🔍 Review SummaryThis PR implements role-based authorization with Admin/Member roles, adding scope-based access control for projects and datasets. While the implementation is functional, there are several critical security concerns that need to be addressed. 🚨 Critical Security Issues1. Role Determination Logic Vulnerability -
|
| ProjectID: projectID, | ||
| OrgID: pgtype.Text{String: orgID, Valid: true}, | ||
| CreatedBy: pgtype.Text{String: createdBy, Valid: true}, | ||
| Limit: int32(pagination.Limit), |
Check failure
Code scanning / CodeQL
Incorrect conversion between integer types High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
In general, to fix this type of issue you must ensure that any value parsed from a string into an architecture‑dependent int is either (a) parsed directly into the correct bit size (strconv.ParseInt with bitSize=32 when you later need int32), or (b) checked against the appropriate min/max bounds before conversion to a smaller integer type. Since the rest of the code uses int for pagination and the DB/gen layer expects int32, the least invasive fix is to clamp the user‑provided limit and derived Offset to a safe range before they are used in the query.
The best targeted fix, without changing existing APIs, is:
-
In
ParseLimitAndPage(server/domain/pkg/utils.go), enforce minimum and maximum values forlimitandpage. Use constants that (a) fit withinint32and (b) are reasonable for pagination. For example:- default limit: 10 (already present),
- minimum limit: 1,
- maximum limit: e.g. 10,000 (well below
math.MaxInt32), - minimum page: 1.
After parsing, clamplimitandpageto these bounds.
-
Optionally, add a defensive clamp in
DatasetService.List(server/application/services/store.go) just before usinglimitto computepagination.Limitandpagination.Offset, to ensure even non‑HTTP callers of the service can’t pass unreasonable values. This also guarantees(page-1)*limitstays within a range where casting toint32is safe. -
With those clamps in place,
pagination.Limitandpagination.Offsetcan be safely converted toint32inListByProjectAndRole, because they will always be non‑negative and well belowmath.MaxInt32. No change is required inlist_by_project_and_role.goitself; its cast becomes safe due to the upstream bounds check.
All required functionality remains the same from a client perspective (pagination still works; if an excessively large limit is requested, it is bounded to a safe maximum). We only modify the logic in ParseLimitAndPage and DatasetService.List, both within the snippets you provided, and we don’t need any new imports.
| @@ -109,10 +109,21 @@ | ||
|
|
||
| func (service *DatasetService) List(projectID string, role models.Role, orgID, createdBy string, limit, page int) (*models.PaginationView[*models.Dataset], error) { | ||
| pagination := models.NewPagination() | ||
| if limit != 0 { | ||
| pagination.Limit = limit | ||
|
|
||
| // Defensive bounds: keep values reasonable and within int32 range | ||
| if limit <= 0 { | ||
| limit = pagination.Limit | ||
| } | ||
| if limit > 10000 { | ||
| limit = 10000 | ||
| } | ||
| if page <= 0 { | ||
| page = 1 | ||
| } | ||
|
|
||
| pagination.Limit = limit | ||
| pagination.Offset = (page - 1) * limit | ||
|
|
||
| return service.datasetRepo.ListByProjectAndRole(context.Background(), projectID, orgID, createdBy, role, pagination) | ||
| } | ||
|
|
| @@ -13,6 +13,7 @@ | ||
| ) | ||
|
|
||
| func ParseLimitAndPage(limitStr, pageStr string) (int, int) { | ||
| // defaults | ||
| limit := 10 | ||
| page := 1 | ||
| var err error | ||
| @@ -31,6 +32,16 @@ | ||
| } | ||
| } | ||
|
|
||
| // enforce sane bounds that are safely within int32 | ||
| if limit <= 0 { | ||
| limit = 10 | ||
| } else if limit > 10000 { | ||
| limit = 10000 | ||
| } | ||
| if page <= 0 { | ||
| page = 1 | ||
| } | ||
|
|
||
| return limit, page | ||
| } | ||
|
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
server/interfaces/http/routes/api/projects/datasets/update.go (1)
36-36: Unsafe type assertion can cause runtime panic.This issue was previously flagged. The direct type assertion will panic if role context is missing.
server/application/repositories/store.go (1)
72-72:SearchDatasetsmethod appears unused.This was previously flagged. The method is implemented but
DatasetService.ListusesListByProjectAndRoleinstead.
🧹 Nitpick comments (5)
server/infrastructure/postgres/store/datasets/list_by_project_and_role.go (2)
36-36: Silent error on JSON unmarshal may hide data corruption.Silently discarding the unmarshal error (
_ = json.Unmarshal(...)) could mask data integrity issues. Consider logging the error at debug level to aid troubleshooting.🔎 Proposed fix
columns := make([]map[string]any, 0) -_ = json.Unmarshal([]byte(d.Columns), &columns) +if err := json.Unmarshal([]byte(d.Columns), &columns); err != nil { + s.logger.Debug("Error unmarshaling columns", zap.String("dataset_id", d.ID), zap.Error(err)) +}
57-69: Inefficient count retrieval - consider using a dedicated COUNT query.Re-querying all datasets with a limit of 1,000,000 to count results is inefficient and could cause performance issues with large datasets. A dedicated
COUNT(*)query would be more efficient and scalable.🔎 Recommended approach
Add a count query like
CountDatasetsByProjectAndCreatorin the SQL layer:-- name: CountDatasetsByProjectAndCreator :one SELECT COUNT(*) FROM datasets WHERE project_id = $1 AND org_id = $2 AND created_by = $3;Then use it:
count, err := s.q.CountDatasetsByProjectAndCreator(ctx, gen.CountDatasetsByProjectAndCreatorParams{ ProjectID: projectID, OrgID: pgtype.Text{String: orgID, Valid: true}, CreatedBy: pgtype.Text{String: createdBy, Valid: true}, })server/infrastructure/postgres/store/datasets/update_by_org_and_creator.go (1)
44-45: Consider usingd.Columnsfrom DB result for consistency.Currently, the return value uses
columnsBytes(the input that was just written) rather thand.Columns(the database result). While functionally equivalent in most cases, using the DB result would be more consistent with other store methods and would reflect any database-level transformations.🔎 Proposed fix
columns := make([]map[string]any, 0) -_ = json.Unmarshal(columnsBytes, &columns) +_ = json.Unmarshal(d.Columns, &columns)server/infrastructure/postgres/sql/queries/projects.sql (1)
94-102: Missing pagination could cause performance issues.
ListProjectsByOrgAndCreatorreturns all projects withoutLIMIT/OFFSET, unlikeSearchProjectsByOrgAndCreator(lines 107-126) which includes pagination. For users with many projects, this could return unbounded result sets.Consider adding pagination parameters:
Proposed fix
-- name: ListProjectsByOrgAndCreator :many SELECT p.*, count(pd.dataset_id) as dataset_count FROM projects p LEFT JOIN project_datasets pd ON p.id = pd.project_id WHERE p.org_id = $1 AND p.created_by = $2 GROUP BY p.id -ORDER BY p.created_at DESC; +ORDER BY p.created_at DESC +LIMIT $3 OFFSET $4;server/infrastructure/postgres/sql/queries/datasets.sql (1)
72-75: Missing pagination could cause performance issues.Same issue as
ListProjectsByOrgAndCreator- this query returns all datasets without bounds. Consider addingLIMIT/OFFSETfor consistency withSearchDatasetsByOrgAndCreator.Proposed fix
-- name: ListDatasetsByOrgAndCreator :many select * from datasets where org_id = $1 and created_by = $2 -order by created_at desc; +order by created_at desc +limit $3 offset $4;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
server/infrastructure/postgres/gen/datasets.sql.gois excluded by!**/gen/**server/infrastructure/postgres/gen/projects.sql.gois excluded by!**/gen/**server/infrastructure/postgres/gen/querier.gois excluded by!**/gen/**
📒 Files selected for processing (13)
server/application/repositories/store.goserver/application/services/store.goserver/domain/cmd/reindex_schemas.goserver/infrastructure/postgres/sql/queries/datasets.sqlserver/infrastructure/postgres/sql/queries/projects.sqlserver/infrastructure/postgres/store/datasets/list_by_project_and_role.goserver/infrastructure/postgres/store/datasets/update_by_org_and_creator.goserver/infrastructure/postgres/store/projects/update_by_org_and_creator.goserver/interfaces/http/routes/api/projects/datasets/list.goserver/interfaces/http/routes/api/projects/datasets/update.goserver/interfaces/http/routes/api/projects/openapi.goserver/interfaces/http/routes/api/projects/update.goserver/interfaces/http/routes/source/database/refresh.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-29T09:36:39.908Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-29T09:36:39.908Z
Learning: Applies to server/**/*.go : Store dataset files in S3-compatible storage (MinIO) in the Go backend server
Applied to files:
server/infrastructure/postgres/store/datasets/update_by_org_and_creator.go
📚 Learning: 2025-07-29T09:36:39.908Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-29T09:36:39.908Z
Learning: Applies to server/**/*.go : Use Fiber as the web framework in the Go backend server
Applied to files:
server/interfaces/http/routes/api/projects/datasets/list.go
📚 Learning: 2025-07-29T09:36:39.908Z
Learnt from: CR
Repo: factly/gopie PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-29T09:36:39.908Z
Learning: Applies to server/**/* : Organize Go backend server code into Domain, Infrastructure, Interfaces, and Application layers
Applied to files:
server/interfaces/http/routes/api/projects/datasets/list.go
🧬 Code graph analysis (8)
server/infrastructure/postgres/store/projects/update_by_org_and_creator.go (2)
server/infrastructure/postgres/gen/projects.sql.go (2)
UpdateProjectParams(576-583)UpdateProjectByOrgAndCreatorParams(620-628)server/infrastructure/postgres/gen/models.go (1)
Project(195-205)
server/infrastructure/postgres/store/datasets/update_by_org_and_creator.go (2)
server/infrastructure/postgres/gen/datasets.sql.go (2)
UpdateDatasetParams(578-589)UpdateDatasetByOrgAndCreatorParams(640-652)server/infrastructure/postgres/gen/models.go (1)
Dataset(149-165)
server/interfaces/http/routes/api/projects/update.go (3)
server/interfaces/http/middleware/role.go (1)
RoleCtxKey(12-12)server/domain/models/role.go (1)
Role(3-3)server/infrastructure/postgres/gen/projects.sql.go (1)
UpdateProjectParams(576-583)
server/interfaces/http/routes/api/projects/datasets/list.go (3)
server/interfaces/http/middleware/role.go (1)
RoleCtxKey(12-12)server/domain/models/role.go (1)
Role(3-3)server/domain/pkg/utils.go (1)
ParseLimitAndPage(15-35)
server/interfaces/http/routes/api/projects/datasets/update.go (3)
server/interfaces/http/middleware/role.go (1)
RoleCtxKey(12-12)server/domain/models/role.go (1)
Role(3-3)server/infrastructure/postgres/gen/datasets.sql.go (1)
UpdateDatasetParams(578-589)
server/interfaces/http/routes/source/database/refresh.go (3)
server/interfaces/http/middleware/role.go (1)
RoleCtxKey(12-12)server/domain/models/role.go (1)
Role(3-3)server/domain/models/dataset.go (1)
UpdateDatasetParams(112-129)
server/application/services/store.go (5)
server/domain/models/role.go (2)
Role(3-3)Admin(7-7)server/infrastructure/postgres/gen/models.go (2)
Project(195-205)Dataset(149-165)server/infrastructure/postgres/gen/projects.sql.go (1)
UpdateProjectParams(576-583)server/domain/models/dataset.go (2)
Dataset(7-37)UpdateDatasetParams(112-129)server/infrastructure/postgres/gen/datasets.sql.go (1)
UpdateDatasetParams(578-589)
server/interfaces/http/routes/api/projects/openapi.go (2)
server/interfaces/http/middleware/role.go (1)
RoleCtxKey(12-12)server/domain/models/role.go (1)
Role(3-3)
🪛 GitHub Check: CodeQL
server/infrastructure/postgres/store/datasets/list_by_project_and_role.go
[failure] 24-24: Incorrect conversion between integer types
Incorrect conversion of an integer with architecture-dependent bit size from strconv.Atoi to a lower bit size type int32 without an upper bound check.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: claude-review
- GitHub Check: Analyze (go)
🔇 Additional comments (29)
server/interfaces/http/routes/source/database/refresh.go (3)
34-36: LGTM - Role context extraction follows existing patterns.The addition of
roleextraction from context aligns with the existingorgIDanduserIDextraction pattern and correctly propagates the role to downstream service calls.
213-218: TheUpdatemethod signature correctly includes theroleparameter as the second argument.The calling code at lines 213-218 matches the
DatasetService.Updatesignature defined inserver/application/services/store.go(line 126), which takes(id string, role models.Role, params *models.UpdateDatasetParams). No issues found.
59-59: No action required—theDetailsmethod signature correctly accepts all parameters.The method
ProjectService.Details(id, orgID, createdBy string, role models.Role)is properly defined to accept theuserID(passed ascreatedBy),orgID, androleparameters. The implementation handles these parameters correctly, using the role to conditionally route to different repository methods.server/infrastructure/postgres/store/datasets/list_by_project_and_role.go (1)
13-17: LGTM - Admin role delegation is appropriate.Admins correctly bypass the creator filter and use the existing
Listmethod that returns all datasets in the project.server/interfaces/http/routes/api/projects/update.go (2)
37-37: LGTM - Role extraction added correctly.The role is properly extracted from context and will be used to enforce authorization in the update flow.
57-63: LGTM - Update call correctly includes role for authorization.The
roleparameter is now passed to the project service'sUpdatemethod, enabling role-based access control for project updates.server/interfaces/http/routes/api/projects/openapi.go (2)
15-18: LGTM - Context extraction for role-based authorization.The extraction of
role,orgID, anduserIDfrom the request context enables proper authorization filtering when listing datasets for OpenAPI generation.
27-27: LGTM - Dataset listing now respects role-based access control.The updated
Listcall signature correctly passes role and user context, ensuring non-admin users only see datasets they created in the OpenAPI specification.server/interfaces/http/routes/api/projects/datasets/list.go (3)
26-28: LGTM - Role-based context extraction added.The handler now extracts
orgID,userID, androlefrom the request context, following the established pattern in other handlers.
34-34: LGTM - List call correctly propagates role-based parameters.The dataset listing now respects role-based access control by passing the user's role and identity to the service layer.
61-89: ThelistAllDatasetsendpoint is intentionally on an internal server without role-based filtering.The method is registered in
InternalRoutes, which runs on a separate internal server (serveInternalfunction) with no authentication middleware applied, unlike the public routes that include Zitadel authorization and role-based filtering. This architectural separation is intentional: internal APIs are isolated from external requests. If this endpoint should be exposed publicly, migrate it to the regularRoutesfunction and implement role-based filtering like the publiclistmethod. Consider adding a code comment explaining why the internal endpoint skips role extraction.server/infrastructure/postgres/store/projects/update_by_org_and_creator.go (1)
12-38: LGTM - Clean implementation of org/creator-scoped update.The method correctly:
- Builds the update params with proper
pgtypefield mappings- Handles
CustomPromptvalidity based on non-empty string- Maps the database result back to
models.Project- Logs errors appropriately
server/infrastructure/postgres/store/datasets/update_by_org_and_creator.go (2)
14-24: LGTM - Proper input validation and marshaling.The method correctly:
- Marshals columns to JSON bytes
- Parses and validates the dataset ID as a UUID
- Logs errors for both operations
26-42: LGTM - Database update with proper error handling.The update call correctly maps all fields with appropriate
pgtypewrappers andValidflags based on field presence.server/infrastructure/postgres/sql/queries/projects.sql (4)
80-82: LGTM!Query correctly filters by
org_idand matches project IDs usingANY()with proper type casting.
84-92: LGTM!Query correctly fetches project details with dataset aggregation, filtering by
id,org_id, andcreated_by.
104-126: LGTM!Delete and search queries correctly implement org+creator scoping with appropriate filtering and pagination.
128-136: LGTM!Update query correctly scopes by
id,org_id, andcreated_bywith COALESCE for optional field updates.server/infrastructure/postgres/sql/queries/datasets.sql (3)
65-70: LGTM!Formatting adjustment and new
GetDatasetByOrgAndCreatorquery look correct.
77-96: LGTM!Delete and search queries correctly implement org+creator scoping with appropriate filtering.
98-117: LGTM!Update and project-scoped list queries are correctly implemented with proper filtering and pagination.
server/interfaces/http/routes/api/projects/datasets/update.go (1)
56-83: LGTM!Service calls correctly propagate
userIDandrolefor role-based access control.server/application/services/store.go (5)
28-32: LGTM!Role-based routing correctly branches between admin (org-wide access) and member (creator-scoped access) for project details.
48-74: LGTM!Role-based routing for delete and list operations correctly uses
createdByparameter passed explicitly by the caller.
99-103: LGTM!DatasetService.Details correctly implements role-based routing.
110-116: LGTM!DatasetService.List delegates to
ListByProjectAndRolewhich handles role-based filtering internally.
119-123: LGTM!DatasetService.Delete correctly implements role-based routing with explicit
createdBy.server/application/repositories/store.go (2)
39-45: LGTM!New org+creator scoped methods for
ProjectStoreRepositoryare well-structured with clear naming and consistent signatures.
74-80: LGTM!New org+creator scoped methods for
DatasetStoreRepositoryprovide comprehensive coverage for role-based access control, including theListByProjectAndRolemethod that handles role-aware filtering.
| func (service *ProjectService) Update(projectID string, role models.Role, params *models.UpdateProjectParams) (*models.Project, error) { | ||
| if role == models.Admin { | ||
| return service.projectRepo.Update(context.Background(), projectID, params) | ||
| } | ||
| return service.projectRepo.UpdateByOrgAndCreator(context.Background(), projectID, params.OrgID, params.UpdatedBy, params) | ||
| } |
There was a problem hiding this comment.
Bug: Using UpdatedBy as createdBy will fail to match records.
UpdateByOrgAndCreator is called with params.UpdatedBy as the createdBy argument. However, UpdatedBy is the current user performing the update, not the original creator. This will fail to find records when the updater is different from the creator.
The caller should pass the actual createdBy value (available from params or fetched separately).
Proposed fix
The method signature should accept createdBy explicitly:
-func (service *ProjectService) Update(projectID string, role models.Role, params *models.UpdateProjectParams) (*models.Project, error) {
+func (service *ProjectService) Update(projectID string, role models.Role, createdBy string, params *models.UpdateProjectParams) (*models.Project, error) {
if role == models.Admin {
return service.projectRepo.Update(context.Background(), projectID, params)
}
- return service.projectRepo.UpdateByOrgAndCreator(context.Background(), projectID, params.OrgID, params.UpdatedBy, params)
+ return service.projectRepo.UpdateByOrgAndCreator(context.Background(), projectID, params.OrgID, createdBy, params)
}Then update callers to pass the createdBy from the existing record (fetched via Details).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In server/application/services/store.go around lines 40 to 45, the call to
UpdateByOrgAndCreator incorrectly uses params.UpdatedBy as the createdBy
argument which will not match when the updater differs from the original
creator; change the Update method and its callers to accept and pass the actual
createdBy (not UpdatedBy): update the ProjectService.Update signature (and any
interface) to accept a createdBy string (or fetch the existing project via
service.Details(projectID) inside Update to obtain the original CreatedBy) and
pass that createdBy into UpdateByOrgAndCreator along with params.OrgID and
params; then update all call sites to supply the real createdBy (by fetching the
record before updating) so UpdateByOrgAndCreator can match records correctly.
| func (service *DatasetService) Update(id string, role models.Role, params *models.UpdateDatasetParams) (*models.Dataset, error) { | ||
| if role == models.Admin { | ||
| return service.datasetRepo.Update(context.Background(), id, params) | ||
| } | ||
| return service.datasetRepo.UpdateByOrgAndCreator(context.Background(), id, params.OrgID, params.UpdatedBy, params) | ||
| } |
There was a problem hiding this comment.
Bug: Same UpdatedBy/createdBy mismatch as in ProjectService.Update.
Line 130 passes params.UpdatedBy as the createdBy argument, which will fail when the updater differs from the original creator.
Proposed fix
-func (service *DatasetService) Update(id string, role models.Role, params *models.UpdateDatasetParams) (*models.Dataset, error) {
+func (service *DatasetService) Update(id string, role models.Role, createdBy string, params *models.UpdateDatasetParams) (*models.Dataset, error) {
if role == models.Admin {
return service.datasetRepo.Update(context.Background(), id, params)
}
- return service.datasetRepo.UpdateByOrgAndCreator(context.Background(), id, params.OrgID, params.UpdatedBy, params)
+ return service.datasetRepo.UpdateByOrgAndCreator(context.Background(), id, params.OrgID, createdBy, params)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (service *DatasetService) Update(id string, role models.Role, params *models.UpdateDatasetParams) (*models.Dataset, error) { | |
| if role == models.Admin { | |
| return service.datasetRepo.Update(context.Background(), id, params) | |
| } | |
| return service.datasetRepo.UpdateByOrgAndCreator(context.Background(), id, params.OrgID, params.UpdatedBy, params) | |
| } | |
| func (service *DatasetService) Update(id string, role models.Role, createdBy string, params *models.UpdateDatasetParams) (*models.Dataset, error) { | |
| if role == models.Admin { | |
| return service.datasetRepo.Update(context.Background(), id, params) | |
| } | |
| return service.datasetRepo.UpdateByOrgAndCreator(context.Background(), id, params.OrgID, createdBy, params) | |
| } |
🤖 Prompt for AI Agents
In server/application/services/store.go around lines 126 to 131, the call to
UpdateByOrgAndCreator incorrectly passes params.UpdatedBy as the creator
argument, causing failures when the updater is different from the original
creator; change that call to pass the original creator (params.CreatedBy) as the
createdBy argument (i.e., replace params.UpdatedBy with params.CreatedBy) so the
repository filter uses the actual creator, not the updater.
| // TODO: fix this later | ||
| datasets, err := datasetService.List(project.ID, "", "", "", limit, page) |
There was a problem hiding this comment.
Type mismatch: passing empty string where models.Role is expected.
The List method signature expects models.Role as the second parameter, but an empty string "" is passed. This may cause issues depending on how the role is used downstream (e.g., comparing with models.Admin).
For a CLI reindexing command that should process all datasets regardless of ownership, consider passing models.Admin to bypass creator filtering:
🔎 Proposed fix
// TODO: fix this later
-datasets, err := datasetService.List(project.ID, "", "", "", limit, page)
+datasets, err := datasetService.List(project.ID, models.Admin, "", "", limit, page)Would you like me to help implement a proper solution that either:
- Uses
models.Adminrole to list all datasets, or - Adds a dedicated
ListAllmethod for CLI/internal use cases?
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // TODO: fix this later | |
| datasets, err := datasetService.List(project.ID, "", "", "", limit, page) | |
| // TODO: fix this later | |
| datasets, err := datasetService.List(project.ID, models.Admin, "", "", limit, page) |
🤖 Prompt for AI Agents
In server/domain/cmd/reindex_schemas.go around lines 107 to 108, the call to
datasetService.List passes an empty string for the role parameter which expects
models.Role; change the call to pass models.Admin (or otherwise the appropriate
models.Role constant) so the CLI reindex command lists all datasets regardless
of owner, and ensure the models package is imported/qualified where needed.
| Limit: int32(pagination.Limit), | ||
| Offset: int32(pagination.Offset), |
There was a problem hiding this comment.
Integer conversion may overflow on 32-bit systems.
Converting pagination.Limit and pagination.Offset from int to int32 without bounds checking can cause overflow on systems where int is 64-bit. As flagged by CodeQL, this could lead to unexpected behavior with large pagination values.
🔎 Proposed fix with bounds checking
+func safeInt32(v int) int32 {
+ if v > math.MaxInt32 {
+ return math.MaxInt32
+ }
+ if v < math.MinInt32 {
+ return math.MinInt32
+ }
+ return int32(v)
+}
+
ds, err := s.q.ListDatasetsByProjectAndCreator(ctx, gen.ListDatasetsByProjectAndCreatorParams{
ProjectID: projectID,
OrgID: pgtype.Text{String: orgID, Valid: true},
CreatedBy: pgtype.Text{String: createdBy, Valid: true},
- Limit: int32(pagination.Limit),
- Offset: int32(pagination.Offset),
+ Limit: safeInt32(pagination.Limit),
+ Offset: safeInt32(pagination.Offset),
})🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 24-24: Incorrect conversion between integer types
Incorrect conversion of an integer with architecture-dependent bit size from strconv.Atoi to a lower bit size type int32 without an upper bound check.
🤖 Prompt for AI Agents
In server/infrastructure/postgres/store/datasets/list_by_project_and_role.go
around lines 24-25, converting pagination.Limit and pagination.Offset directly
to int32 can overflow; validate that both values are within the int32 range
before casting and handle out-of-range values (either return a clear error or
clamp them to math.MaxInt32/math.MinInt32 as your API policy dictates).
Implement checks using constants from math (math.MaxInt32 and math.MinInt32) or
explicit bounds (<= 1<<31-1 and >= -1<<31), return an error to the caller if
values are invalid (preferred) or clamp to safe limits, then perform the int32
cast only after those checks. Ensure tests cover large and negative pagination
inputs.
|
Claude finished @elliot14A's task —— View job I'll analyze this and get back to you. |
3 participants
Summary by CodeRabbit
New Features
Other
✏️ Tip: You can customize this high-level summary in your review settings.