feat(rls): implement flexible RLS modes (public-read vs private)#80
feat(rls): implement flexible RLS modes (public-read vs private)#80yash-pouranik merged 3 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds read-side row-level security: introduces Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant PublicAPI as Public API
participant AuthMW as authorizeReadOperation
participant DataCtrl as data.controller
participant DB as Database
Client->>PublicAPI: GET /:collectionName or /:collectionName/:id
PublicAPI->>AuthMW: resolvePublicAuthContext -> evaluate RLS
AuthMW->>AuthMW: if secret key -> set rlsFilter = {}
alt collection missing
AuthMW->>Client: 404 { error: 'Collection not found' }
else RLS disabled or public-read
AuthMW->>AuthMW: set rlsFilter = {}
AuthMW->>PublicAPI: next()
else private + no auth
AuthMW->>Client: 401 { error: 'Authentication required' }
else private + auth
AuthMW->>AuthMW: set rlsFilter = { ownerField: authUser.userId }
AuthMW->>PublicAPI: next()
end
PublicAPI->>DataCtrl: handler called with req.rlsFilter
DataCtrl->>DataCtrl: build baseFilter from req.rlsFilter
DataCtrl->>DB: Model.find(baseFilter) / Model.findOne({_id, ...baseFilter})
DB->>DataCtrl: return documents
DataCtrl->>Client: res.json(results)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
This PR introduces flexible Row-Level Security (RLS) read modes across the backend, common model schema, and dashboard UI—so collections can be configured as public-read (anyone can read) or private (owner-only reads), while keeping legacy owner-write-only compatible.
Changes:
- Added RLS mode support (
public-read,private, legacyowner-write-only) in the Project collection schema and dashboard API defaults/validation. - Enforced read-side RLS via a new
authorizeReadOperationmiddleware and appliedreq.rlsFilterto public-api read controllers. - Updated dashboard UI + docs/examples to reflect the new modes, and added unit tests for read authorization + controller filtering.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates RLS docs to mention read scoping by mode. |
| packages/common/src/models/Project.js | Expands rls.mode enum and sets default to public-read. |
| examples/social-demo/README.md | Updates demo config to use public-read. |
| examples/social-demo/COLLECTIONS_GUIDE.md | Updates demo RLS settings to public-read. |
| docs/rls-implementation-roadmap.md | Revises roadmap to include public-read and private behaviors. |
| docs/introduction.md | Documents read behavior and legacy mapping. |
| docs/database.md | Adds RLS read mode explanation to database API docs. |
| apps/web-dashboard/src/pages/Database.jsx | Adds UI selector for RLS access mode and legacy mode normalization. |
| apps/public-api/src/routes/data.js | Adds resolvePublicAuthContext + authorizeReadOperation to GET routes. |
| apps/public-api/src/middlewares/authorizeWriteOperation.js | Updates write middleware to accept new/legacy RLS modes. |
| apps/public-api/src/middlewares/authorizeReadOperation.js | New middleware that sets req.rlsFilter based on RLS mode and auth. |
| apps/public-api/src/controllers/data.controller.js | Applies req.rlsFilter to list/detail reads. |
| apps/public-api/src/tests/data.controller.read.test.js | Tests that controllers pass rlsFilter into queries. |
| apps/public-api/src/tests/authorizeReadOperation.test.js | Tests read middleware behavior across key roles and modes. |
| apps/dashboard-api/src/controllers/project.controller.js | Updates default RLS mode and validates allowed modes on update. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const features = new QueryEngine(Model.find(baseFilter), req.query) | ||
| .filter() |
There was a problem hiding this comment.
getAllData applies req.rlsFilter by seeding the query with Model.find(baseFilter), but QueryEngine.filter() later calls this.query.find(mongoQuery) (packages/common/src/utils/queryEngine.js:31). In Mongoose, later .find() conditions can override earlier ones for the same field, so a client could pass a query param matching the owner field (e.g. ?userId=other) and bypass the RLS owner constraint in private mode.
To make the RLS constraint non-bypassable, combine RLS and user filters with an $and, or apply/enforce the RLS filter after QueryEngine.filter() in a way that cannot be overridden (and consider stripping the ownerField from user-supplied query params for private collections).
| const features = new QueryEngine(Model.find(baseFilter), req.query) | |
| .filter() | |
| const features = new QueryEngine(Model.find(), req.query) | |
| .filter(); | |
| if (Object.keys(baseFilter).length > 0) { | |
| features.query = features.query.and([baseFilter]); | |
| } | |
| features |
|
|
||
| const doc = await Model.findById(id).lean(); | ||
| const baseFilter = req.rlsFilter && typeof req.rlsFilter === 'object' ? req.rlsFilter : {}; | ||
| const doc = await Model.findOne({ _id: id, ...baseFilter }).lean(); |
There was a problem hiding this comment.
getSingleDoc builds the query as { _id: id, ...baseFilter }. If baseFilter ever contains _id (e.g. when rls.ownerField === '_id' for the users collection in private mode), the spread will overwrite the path parameter id, causing the request to ignore /:id and potentially return the wrong document.
Prefer composing filters without key overwrites (e.g., { ...baseFilter, _id: id } or an explicit $and of the id constraint and baseFilter).
| const doc = await Model.findOne({ _id: id, ...baseFilter }).lean(); | |
| const doc = await Model.findOne({ ...baseFilter, _id: id }).lean(); |
| <span>Enable RLS for publishable-key writes</span> | ||
| </label> |
There was a problem hiding this comment.
The checkbox label still says “Enable RLS for publishable-key writes”, but this dialog now also configures read access via the new Access Mode selector (e.g. private restricts reads). This wording is now misleading for users and can cause misconfiguration.
Update the label/help text to reflect that enabling RLS may also affect read access depending on the selected mode (or conditionally explain read behavior when rlsMode === 'private').
| <span>Enable RLS for publishable-key writes</span> | |
| </label> | |
| <span>Enable RLS access rules for publishable-key requests</span> | |
| </label> | |
| <p className="rls-help-text"> | |
| {rlsMode === "private" | |
| ? "When enabled, publishable-key access is restricted to the owner for both reads and writes." | |
| : "When enabled, publishable-key writes are restricted to the owner. Reads remain available according to the selected access mode."} | |
| </p> |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements read-side Row-Level Security (RLS) by introducing public-read and private modes, supported by a new authorizeReadOperation middleware and updates to the data controller. The changes also include dashboard UI enhancements and documentation updates. A significant logic error was noted in the getSingleDoc controller where the RLS filter might overwrite the requested document ID if the owner field is _id, potentially leading to incorrect data retrieval; a suggestion was provided to use an $and operator to prevent this.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
apps/public-api/src/controllers/data.controller.js (1)
144-145: Prefer$andwhen combining_idwithreq.rlsFilter.Object spread gives
req.rlsFilterprecedence over the path id. If any legacy collection still uses_idas itsownerField,GET /:idcan return the caller's own document instead of the requested one.♻️ Suggested refactor
- const doc = await Model.findOne({ _id: id, ...baseFilter }).lean(); + const doc = await Model.findOne({ $and: [{ _id: id }, baseFilter] }).lean();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/public-api/src/controllers/data.controller.js` around lines 144 - 145, Replace the object-spread merge with an $and query to ensure the requested id cannot be overridden by req.rlsFilter: when building the query for Model.findOne use a conjunctive $and array containing { _id: id } and the computed baseFilter (derived from req.rlsFilter) instead of spreading baseFilter into the same object; update the code that computes baseFilter and the Model.findOne call (look for baseFilter, req.rlsFilter, Model.findOne, id) to construct and pass { $and: [{ _id: id }, baseFilter] }.apps/dashboard-api/src/controllers/project.controller.js (1)
1308-1311: Normalize legacyowner-write-onlybefore saving it.This still persists the legacy string as-is. Since
apps/web-dashboard/src/pages/Database.jsx, Lines 789-801 only exposespublic-readandprivate, normalizing here keeps backward-compatible input support without surfacing a third stored mode back to clients.♻️ Suggested refactor
- const validMode = mode || collection?.rls?.mode || 'public-read'; + const requestedMode = mode || collection?.rls?.mode || 'public-read'; const allowedModes = new Set(['public-read', 'private', 'owner-write-only']); - if (!allowedModes.has(validMode)) { + if (!allowedModes.has(requestedMode)) { return res.status(400).json({ error: "Unsupported RLS mode. Allowed: public-read, private, owner-write-only (legacy)." }); } + const validMode = requestedMode === 'owner-write-only' ? 'public-read' : requestedMode;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard-api/src/controllers/project.controller.js` around lines 1308 - 1311, The controller accepts the legacy RLS string "owner-write-only" and persists it unchanged; normalize legacy values before saving so clients only see "public-read" or "private". Update the logic around validMode (and where allowedModes is used) to map "owner-write-only" to "private" (or another canonical value) before validation/response and before saving to the DB, ensuring the rest of the codepath works with the normalized value instead of the legacy string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/public-api/src/__tests__/data.controller.read.test.js`:
- Around line 82-91: The controller's getSingleDoc currently merges
req.rlsFilter into the findOne query which can overwrite the route _id when
ownerField === '_id'; change the query construction in getSingleDoc to preserve
both predicates by using an $and clause when base/RLS filter exists (e.g., if
baseFilter non-empty use { $and: [{ _id: id }, baseFilter ] } else { _id: id }),
and update the test to expect mockFindOne to have been called with that
$and-shaped query instead of a merged object; refer to getSingleDoc and any
baseFilter/rlsFilter/ownerField variables to locate and modify the logic and the
assertion.
In `@apps/public-api/src/controllers/data.controller.js`:
- Around line 105-107: The current approach seeds Model.find(baseFilter) but
QueryEngine (packages/common/src/utils/queryEngine.js) re-calls
.find(mongoQuery) using raw req.query which allows req.rlsFilter to be bypassed;
modify QueryEngine (or its usage) so the request-level RLS predicate is merged
into and enforced on the final mongoQuery before any .find is executed:
specifically ensure req.rlsFilter (baseFilter) is combined with parsed req.query
filters (e.g., deep merge or $and wrap) inside QueryEngine (or pass baseFilter
into QueryEngine and have QueryEngine.mergeBaseFilter/constructor merge it) so
that Model.find is only ever called with the combined query and no subsequent
.find replaces it, preserving the owner predicate.
In `@docs/rls-implementation-roadmap.md`:
- Around line 42-44: The roadmap wording is inconsistent: V1 now includes read
behavior for `public-read` and `private` but later sections still call the
change "write-only" and claim "no extra overhead on read routes." Update those
later sections (the parts describing auth-context changes and performance
impact) to reflect that reads are part of V1: remove or rephrase any
"write-only" language, change "no extra overhead on read routes" to accurately
state the overhead/behavior for reads under `public-read` and `private`, and
ensure the auth-context explanation applies to read paths as well so the
document consistently describes current V1 behavior for `public-read` and
`private`.
In `@examples/social-demo/COLLECTIONS_GUIDE.md`:
- Around line 165-169: The RLS settings declare ownerField: userId for the
follows collection but the follows schema only defines followerId and
followingId, so writes will fail ownership checks; update the follows schema to
include a userId field (or change ownerField to followerId) to match the RLS
config, e.g., add a userId property to the follows schema or set ownerField to
"followerId" and ensure any write code uses that field consistently (refer to
the follows schema, ownerField, followerId, and followingId when making the
change).
In `@examples/social-demo/README.md`:
- Around line 140-145: The posts schema in the README is missing the userId
field required by the RLS config's ownerField: userId; update the posts schema
example (the object labelled "posts" or the "posts" schema block) to include a
"userId" property (type String, required true) alongside the existing
authorId/authorUsername/authorDisplayName fields so documents created by clients
will contain the owner field RLS relies on.
In `@README.md`:
- Around line 99-103: The "Read any collection" row in the Key Behavior table
must be split into two rows to match the README steps: create one row for
public-read indicating pk_live reads require no Authorization header (public
access), and a separate row for private indicating pk_live reads require a valid
JWT in the Authorization: Bearer <token> header and that urBackend enforces the
JWT's userId matches the document's owner field (owner field like `userId`);
also ensure the note about `owner-write-only` being treated as `public-read`
remains consistent and update any entrypoint/docs text that still states pk_live
reads are unauthenticated for all modes.
---
Nitpick comments:
In `@apps/dashboard-api/src/controllers/project.controller.js`:
- Around line 1308-1311: The controller accepts the legacy RLS string
"owner-write-only" and persists it unchanged; normalize legacy values before
saving so clients only see "public-read" or "private". Update the logic around
validMode (and where allowedModes is used) to map "owner-write-only" to
"private" (or another canonical value) before validation/response and before
saving to the DB, ensuring the rest of the codepath works with the normalized
value instead of the legacy string.
In `@apps/public-api/src/controllers/data.controller.js`:
- Around line 144-145: Replace the object-spread merge with an $and query to
ensure the requested id cannot be overridden by req.rlsFilter: when building the
query for Model.findOne use a conjunctive $and array containing { _id: id } and
the computed baseFilter (derived from req.rlsFilter) instead of spreading
baseFilter into the same object; update the code that computes baseFilter and
the Model.findOne call (look for baseFilter, req.rlsFilter, Model.findOne, id)
to construct and pass { $and: [{ _id: id }, baseFilter] }.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d7d53f2b-838a-4008-a9e1-8d515c89db0d
📒 Files selected for processing (15)
README.mdapps/dashboard-api/src/controllers/project.controller.jsapps/public-api/src/__tests__/authorizeReadOperation.test.jsapps/public-api/src/__tests__/data.controller.read.test.jsapps/public-api/src/controllers/data.controller.jsapps/public-api/src/middlewares/authorizeReadOperation.jsapps/public-api/src/middlewares/authorizeWriteOperation.jsapps/public-api/src/routes/data.jsapps/web-dashboard/src/pages/Database.jsxdocs/database.mddocs/introduction.mddocs/rls-implementation-roadmap.mdexamples/social-demo/COLLECTIONS_GUIDE.mdexamples/social-demo/README.mdpackages/common/src/models/Project.js
| test('getSingleDoc applies rlsFilter to findOne()', async () => { | ||
| const req = makeReq({ rlsFilter: { userId: 'user_1' } }); | ||
| const res = makeRes(); | ||
|
|
||
| await getSingleDoc(req, res); | ||
|
|
||
| expect(mockFindOne).toHaveBeenCalledWith({ | ||
| _id: '507f1f77bcf86cd799439011', | ||
| userId: 'user_1', | ||
| }); |
There was a problem hiding this comment.
Preserve both the route id and the RLS constraint in detail reads.
This test currently locks in the findOne({ _id: id, ...req.rlsFilter }) shape. When ownerField === '_id', that merge rewrites the requested id, so /.../:otherId can return the caller's own document instead of 404. Please fix the controller query to keep both predicates and update this assertion to cover the _id case.
Possible update once the controller preserves both predicates
- expect(mockFindOne).toHaveBeenCalledWith({
- _id: '507f1f77bcf86cd799439011',
- userId: 'user_1',
- });
+ expect(mockFindOne).toHaveBeenCalledWith({
+ $and: [
+ { _id: '507f1f77bcf86cd799439011' },
+ { userId: 'user_1' },
+ ],
+ });const query = Object.keys(baseFilter).length
? { $and: [{ _id: id }, baseFilter] }
: { _id: id };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/public-api/src/__tests__/data.controller.read.test.js` around lines 82 -
91, The controller's getSingleDoc currently merges req.rlsFilter into the
findOne query which can overwrite the route _id when ownerField === '_id';
change the query construction in getSingleDoc to preserve both predicates by
using an $and clause when base/RLS filter exists (e.g., if baseFilter non-empty
use { $and: [{ _id: id }, baseFilter ] } else { _id: id }), and update the test
to expect mockFindOne to have been called with that $and-shaped query instead of
a merged object; refer to getSingleDoc and any baseFilter/rlsFilter/ownerField
variables to locate and modify the logic and the assertion.
| Read behavior in V1: | ||
| - Keep existing behavior unchanged initially (to reduce risk). | ||
| - `public-read` keeps existing behavior for reads. | ||
| - `private` filters reads by owner. |
There was a problem hiding this comment.
Update the remaining V1 write-only wording.
These bullets now make reads part of V1, but later sections still describe auth-context changes and performance impact as write-only / “no extra overhead on read routes.” Please align those sections so the roadmap matches the implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/rls-implementation-roadmap.md` around lines 42 - 44, The roadmap wording
is inconsistent: V1 now includes read behavior for `public-read` and `private`
but later sections still call the change "write-only" and claim "no extra
overhead on read routes." Update those later sections (the parts describing
auth-context changes and performance impact) to reflect that reads are part of
V1: remove or rephrase any "write-only" language, change "no extra overhead on
read routes" to accurately state the overhead/behavior for reads under
`public-read` and `private`, and ensure the auth-context explanation applies to
read paths as well so the document consistently describes current V1 behavior
for `public-read` and `private`.
| **RLS settings for `follows` (required):** | ||
| - enabled: `true` | ||
| - mode: `owner-write-only` | ||
| - mode: `public-read` | ||
| - ownerField: `userId` | ||
| - requireAuthForWrite: `true` |
There was a problem hiding this comment.
follows still omits the configured owner field.
The guide now sets ownerField: userId, but the follows schema above only defines followerId and followingId. If readers follow this document as written, follow/unfollow writes will fail RLS ownership checks.
📝 Suggested doc fix
Field Name | Type | Required | Index
--------------|---------|----------|-------
+userId | String | Yes | Yes
followerId | String | Yes | Yes
followingId | String | Yes | Yes
createdAt | Date | No | Date.now-- [ ] `follows` collection created with 3 fields
+- [ ] `follows` collection created with 4 fields🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/social-demo/COLLECTIONS_GUIDE.md` around lines 165 - 169, The RLS
settings declare ownerField: userId for the follows collection but the follows
schema only defines followerId and followingId, so writes will fail ownership
checks; update the follows schema to include a userId field (or change
ownerField to followerId) to match the RLS config, e.g., add a userId property
to the follows schema or set ownerField to "followerId" and ensure any write
code uses that field consistently (refer to the follows schema, ownerField,
followerId, and followingId when making the change).
| For each writable collection (`posts`, `comments`, `likes`, `follows`, `profiles`): | ||
|
|
||
| - `enabled: true` | ||
| - `mode: owner-write-only` | ||
| - `mode: public-read` | ||
| - `ownerField: userId` | ||
| - `requireAuthForWrite: true` |
There was a problem hiding this comment.
Add userId to the posts schema example.
This block now tells readers to use ownerField: userId, but the posts schema earlier in this README still omits userId. Following the setup verbatim leaves post documents without the field RLS writes depend on.
📝 Suggested doc fix
**2. `posts`**
```json
{
+ "userId": { "type": "String", "required": true },
"authorId": { "type": "String", "required": true },
"authorUsername": { "type": "String", "required": true },
"authorDisplayName": { "type": "String" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/social-demo/README.md` around lines 140 - 145, The posts schema in
the README is missing the userId field required by the RLS config's ownerField:
userId; update the posts schema example (the object labelled "posts" or the
"posts" schema block) to include a "userId" property (type String, required
true) alongside the existing authorId/authorUsername/authorDisplayName fields so
documents created by clients will contain the owner field RLS relies on.
| 1. Enable RLS for a collection in the Dashboard and choose a mode. | ||
| 2. Use `public-read` for content anyone can view, or `private` for owner-only access. (`owner-write-only` is treated as `public-read` for legacy projects.) | ||
| 3. Choose the **owner field** — the document field that stores the authenticated user's ID (e.g., `userId`). | ||
| 4. The client must send a valid user JWT in the `Authorization: Bearer <token>` header for `pk_live` writes and for `private` reads. | ||
| 5. urBackend enforces that the JWT's `userId` matches the document's owner field. |
There was a problem hiding this comment.
The README still describes all pk_live reads as unauthenticated.
These new steps correctly introduce private read auth, but the “Key Behavior” table above still says Read any collection with pk_live needs no token. Please split that row into public-read and private so the entrypoint docs stay consistent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 99 - 103, The "Read any collection" row in the Key
Behavior table must be split into two rows to match the README steps: create one
row for public-read indicating pk_live reads require no Authorization header
(public access), and a separate row for private indicating pk_live reads require
a valid JWT in the Authorization: Bearer <token> header and that urBackend
enforces the JWT's userId matches the document's owner field (owner field like
`userId`); also ensure the note about `owner-write-only` being treated as
`public-read` remains consistent and update any entrypoint/docs text that still
states pk_live reads are unauthenticated for all modes.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
@copilot apply changes based on the comments in this thread |
…pdate dashboard label Agent-Logs-Url: https://github.com/yash-pouranik/urBackend/sessions/91669e19-a2de-410f-a997-63b1a2f4f61d Co-authored-by: yash-pouranik <172860064+yash-pouranik@users.noreply.github.com>
yash-pouranik
temporarily deployed
to
feature/flexible-rls-modes - urBackend-frankfrut PR #80
Render
Destroyed
Applied all three review fixes in commit
Tests updated to assert the new |
|
@coderabbitai Can we erge this? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 18553
🔴 Blocking: CI check still runningThe CodeQL Advanced security analysis ( 🔴 Blocking: Unresolved review comments (6 actionable)My earlier review flagged several issues that haven't been addressed yet. The most critical one is: 1.
|
getAllData: apply RLS filter afterQueryEngine.filter()using.and([baseFilter])to prevent client bypass via query paramsgetSingleDoc: already uses$andapproach (non-bypassable, no key overwrite risk)rlsModedata.controller.read.test.jsto match new mock structure and assertions