Support allowed fields for public tables #172
Conversation
WalkthroughThis pull request introduces a comprehensive enhancement to the application's permission and field access control system. The changes span multiple files, focusing on implementing granular field-level access control. Key modifications include adding an Changes
Sequence DiagramsequenceDiagram
participant User
participant AuthService
participant Controller
participant Database
User->>AuthService: Request with permissions
AuthService->>AuthService: Check public/role access
AuthService->>AuthService: Filter allowed_fields
AuthService-->>Controller: Return authorized fields
Controller->>Database: Query with filtered fields
Database-->>Controller: Return restricted data
Controller-->>User: Respond with permitted fields
Possibly related PRs
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (20)
src/app.controller.post.ts (5)
49-49: InitializequeryFieldscloser to usage.
DeclaringqueryFieldsat this scope is fine, but consider initializing it closer to where it is actually manipulated to limit its lifetime and usage context, which can improve clarity.
72-73: Comment clarity.
The comment “If not public, perform auth” is valuable. Ensure all other relevant auth checks (e.g. role-based) are also clearly documented for maintainability.
74-81: Auth object usage.
Here you retrieve theauthobject with the same pattern aspublic_auth. If both checks are repeated across multiple methods, consider extracting them into a helper function to reduce duplication.
113-122: Flexible merging ofallowed_fields.
The intersection logic here ensures that when multiple permission sets apply, only the truly allowed fields remain. Consider using a set-based approach for clarity and to avoid accidental duplicates before filtering.
178-187: Appending role-basedallowed_fields.
Merging arrays via.push()and then filtering again for intersections is logically correct. You might consider a unified helper to streamline these repeated steps and reduce the chance of subtle bugs.src/app.controller.put.ts (6)
46-46: DeclarequeryFieldscontextually, if possible.
Like in the PostController, limiting the scope ofqueryFieldsmight improve maintainability and reduce the chance of undesired side effects.
69-71: Auth fallback note.
This comment clarifies fallback to standard auth if not public. Ensure the same patterns apply consistently across all controllers to avoid confusion.
71-81: Refactor repetitive auth call.
Much like in the PostController, consider centralizing this double-check pattern (public vs. private auth) into a shared utility if it appears throughout the codebase.
152-161: Merging allowed fields from role-based permissions.
The approach of first pushing then filtering for intersections is repeated. Consider extracting this pattern into a helper function (e.g.,mergeAllowedFields(baseFields, newAllowedFields)).
202-202: Re-initializequeryFieldsfor updateMany.
let queryFields = []duplicates earlier steps in the same file. If these segments grow, refactoring to a shared helper will reduce duplication.
347-356: Allowing fields from role-based permission for multiple items.
Duplicating logic forqueryFieldsintersection again. This is consistent but can become a future maintenance overhead. Consider centralizing.src/app.controller.get.ts (4)
65-65: InitializequeryFieldswith an empty array.
As in the Post/Put Controllers, confirm if a more local scope might suffice.
102-131: Applying role-based permission to the schema.
Filtering columns byallowed_fieldsis very powerful for restricting sensitive columns. Consider logging the final set of returned columns for debugging or compliance.
385-397: Standard auth fallback again.
Matches the pattern. Repeated code is consistent but consider centralizing the intersection logic.
398-434: Complex role-based filtering at scale.
Including relationships, field restrictions, and multi-level checks can grow complex. Break it into smaller helper functions or classes if maintainability becomes an issue.src/testing/auth.testing.service.ts (1)
Line range hint
52-64: Consider adding validation for allowed_fields format.Since
allowed_fieldsis expected to be a comma-separated string, it would be beneficial to add validation to ensure the correct format before creating the record.async createPublicTablesRecord(data: { table: string access_level: RolePermission allowed_fields?: string }): Promise<FindOneResponseObject> { + if (data.allowed_fields) { + const fields = data.allowed_fields.split(',') + if (!fields.every(field => field.trim().length > 0)) { + throw new Error('allowed_fields must be a comma-separated string of non-empty field names') + } + } const schema = await this.schema.getSchema({ table: LLANA_PUBLIC_TABLES, x_request_id: 'test' }) return (await this.query.perform(QueryPerform.CREATE, <DataSourceCreateOneOptions>{ schema, data, })) as FindOneResponseObject }src/utils/String.ts (1)
106-108: Add JSDoc documentation for the utility function.The function's purpose and parameters should be clearly documented.
/** * Convert a comma separated string to an array + * + * @param {string} string - A comma-separated string of field names + * @returns {string[]} An array of trimmed field names + * @throws {Error} If the input is not a string or contains empty field names */src/app.service.bootup.ts (2)
84-84: Update documentation to clarify allowed_fields format.The table documentation should specify the expected format and validation rules for the
allowed_fieldscolumn.- |`allowed_fields` | `string` | A comma separated list of fields that are restricted for this role | + |`allowed_fields` | `string` | A comma-separated list of field names that are restricted for this role. Field names must be non-empty and match the target table's schema. |
Line range hint
141-162: Consider adding example with allowed_fields in default public tables.The example auth configuration could demonstrate the usage of the new
allowed_fieldsfeature.const example_auth: any[] = [ { table: 'Employee', access_level: RolePermission.READ, + allowed_fields: 'name,department,position', // Example: Only allow access to non-sensitive fields }, ]src/app.controller.post.test.spec.ts (1)
657-659: Ensure error-handling coverage for post-deletion errorsCurrently, if
deleteCustomerfails, it will throw an exception which may terminate the overall cleanup process. Consider wrapping individual deletions in a try-catch to log and continue processing additional records.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/app.controller.get.test.spec.ts(2 hunks)src/app.controller.get.ts(5 hunks)src/app.controller.post.test.spec.ts(3 hunks)src/app.controller.post.ts(7 hunks)src/app.controller.put.test.spec.ts(2 hunks)src/app.controller.put.ts(9 hunks)src/app.service.bootup.ts(2 hunks)src/helpers/Authentication.ts(2 hunks)src/helpers/Roles.ts(4 hunks)src/testing/auth.testing.service.ts(1 hunks)src/types/auth.types.ts(1 hunks)src/utils/String.ts(1 hunks)
🔇 Additional comments (42)
src/app.controller.post.ts (7)
58-58: Check for potential data leaks when using public_auth.
Ensure that the logic granting public access aligns with your security model. If a table is improperly flagged as public, unauthorized data exposure could occur.
64-68: Intersection logic for queryFields.
Merging queryFields with public_auth.allowed_fields by checking if (!queryFields?.length) { ... } else { ... } ensures only intersected fields remain. This logic is correct for an intersection approach but watch out for duplicates if there are multiple merges across different checks.
82-85: 401 for both public and private ineligibility.
Returning 401 if neither public nor standard auth is valid is appropriate. Confirm that 401 is the correct response if your application also uses 403 for forbidden resources.
104-104: Additional check for roles.
Line 104 duplicates the logic seen in lines 82-85, but focuses on per-record checks. Verify that a record-level denial is also returning 401, and confirm that it aligns with your overall approach to unauthorized vs. not-found scenarios.
131-131: Pass queryFields to createOneRecord.
Making queryFields explicit in the creation logic is consistent with the introduced field-level access. Good job verifying you only pass allowed fields to the next layer.
174-174: Repeated record-level denial check.
Similar to line 104, ensure that 401 is consistently returned for unauthorized cases. Confirm that the corresponding error message doesn't conflict with other potential status codes.
196-196: Exposing limited fields in final creation.
Passing queryFields ensures minimal exposure of data in the final response. This pattern significantly helps manage field-level security.
src/app.controller.put.ts (9)
55-55: Public table check consistency.
Reiterating the same caution: verify that the table is truly intended to be public before trusting public_auth. Any misconfiguration here can lead to unintended exposure.
61-65: Intersection approach for public fields.
Using intersection logic with public_auth is consistent with the new field-level security model. Confirm that partial overlap of fields does not break the use cases for partial updates.
79-81: 401 status for invalid public and private auth.
Again, confirm that a 401 is appropriate for failed permission checks vs. 403 in your domain logic.
82-82: Ensure subsequent logic is unreachable if unauthorized.
Immediately returning after the 401 means no further code is executed. This is correct in principle. Good approach to short-circuiting the request.
148-148: Role-based check and public validation.
Here you replicate the pattern from earlier controllers. Ensuring both checks handle record updates consistently is key to preventing unauthorized changes.
173-175: Filtering fields on the response side.
Filtering the updated record by queryFields avoids exposing any disallowed fields. This is a solid approach to maintain data privacy.
211-211: Repeat of caution about public access.
As before, ensure your public table logic is thoroughly tested to avoid unintentional data exposure.
338-338: Record-level permission check for updates.
Similar to code in PostController, ensure that 401 is correct vs. 403. Also confirm that partial updates do not inadvertently skip permission checks.
374-376: Test coverage for updateMany.
It appears you rely heavily on the same patterns as updateById. Verify you have enough test coverage specifically for updateMany calls, especially with partial fields and multiple items.
src/app.controller.get.ts (10)
6-6: Removed ListTablesResponseObject import.
You replaced it with DataSourceSchema returns. Ensure that references to ListTablesResponseObject are fully removed throughout the codebase to avoid dead code.
58-58: Method signature updated to return DataSourceSchema.
Double-check that the front-end or other consumers of this method can handle the new return type.
74-87: Public auth logic for getSchema.
Similar intersection logic for gathering allowed fields. This is consistent. Ensure any schema-specific fields (like relationship definitions) are handled separately if needed.
89-101: Fallback to standard auth for getSchema.
Matches the pattern from the Post/Put controllers. Good consistency across the codebase.
132-135: Schema column filtering.
Trimming schema.columns ensures only allowed fields appear. If the table has columns automatically added in future migrations, confirm they are not unexpectedly exposed.
175-175: public_auth check for getById.
Good to unify the approach to reading or listing records with the approach to creation. Maintain strict consistency so that no route inadvertently bypasses checks.
181-188: Merging public allowed_fields with queryFields.
Same pattern as in other controllers. If your application eventually expands to combine multiple roles, ensure the intersection logic remains correct.
190-202: Fallback auth check for GET.
Nicely mirrors the approach used in CREATE/UPDATE flows. This consistency makes the code more predictable for maintainers.
203-239: Deep role-based filtering.
Here we see logic that merges restriction (where clauses) with allowed_fields. This is quite comprehensive. Test coverage, especially around complicated role restrictions, is crucial.
370-383: Handling of public_auth in list.
Repeating the logic from getById. Confirm that large datasets or paginated results do not degrade performance due to repeated intersection checks.
[performance]
src/app.controller.get.test.spec.ts (3)
350-369: Test: Public read with RolePermission.READ.
Successfully verifies that a public table record with READ permission yields an HTTP 200 instead of 401, confirming the new public access logic.
371-391: Test: Public read with limited allowed fields.
Ensures restricting columns on a publicly readable table. This coverage is good for verifying logic in public_auth.allowed_fields.
624-666: Test: Combine public view and role-based allowed fields.
This validated scenario ensures the intersection logic functions correctly with multiple permission sources. Good job covering corner cases where roles + public permissions overlap.
src/types/auth.types.ts (2)
54-54: New allowed_fields?: string[] property.
This optional property adds excellent granularity for field-level security. Confirm that other code dealing with these interfaces gracefully handles undefined vs. empty arrays.
54-54: allowed_fields in AuthTablePermissionSuccessResponse.
Similarly, this extension enhances role-based restrictions for table access. Make sure any logic merging fields from AuthRestrictionsResponse and AuthTablePermissionSuccessResponse is consistent.
src/testing/auth.testing.service.ts (1)
38-38: LGTM: Optional allowed_fields parameter added correctly.
The addition of the optional allowed_fields parameter to the createPublicTablesRecord method's data interface is well-structured and aligns with the schema changes.
src/app.service.bootup.ts (2)
123-131: LGTM: Schema definition for allowed_fields is correctly structured.
The column definition for allowed_fields in the LLANA_PUBLIC_TABLES schema is properly configured with appropriate nullability and type settings.
123-131: Verify schema consistency across related tables.
The allowed_fields column is added to both LLANA_PUBLIC_TABLES and LLANA_ROLES_TABLE. Let's verify the schema consistency.
Also applies to: 84-84
✅ Verification successful
Schema consistency verified for allowed_fields column
The allowed_fields column has been consistently defined with the same schema in both tables:
- In
LLANA_PUBLIC_TABLES: The column is defined as a string type, nullable, not required, and not a key field - In
LLANA_ROLES_TABLE: The column has the same exact definition with matching properties
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify schema consistency for allowed_fields column across tables
# Test: Check if allowed_fields column exists in both tables with same definition
ast-grep --pattern $'table: $_LLANA_PUBLIC_TABLES,
$$$
columns: [
$$$
{
field: "allowed_fields",
type: DataSourceColumnType.STRING,
nullable: true,
required: false,
$$$
},
$$$
]'
ast-grep --pattern $'table: $_LLANA_ROLES_TABLE,
$$$
columns: [
$$$
{
field: "allowed_fields",
type: DataSourceColumnType.STRING,
nullable: true,
required: false,
$$$
},
$$$
]'
Length of output: 461
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to find table definitions
rg -A 10 "LLANA_PUBLIC_TABLES" --type ts
rg -A 10 "LLANA_ROLES_TABLE" --type ts
# Also search for any allowed_fields column definitions
rg "allowed_fields.*DataSourceColumnType" --type ts
Length of output: 13808
Script:
#!/bin/bash
# Let's search for the schema definitions of both tables
rg -B 5 -A 20 "table: LLANA_PUBLIC_TABLES.*columns:" --type ts
rg -B 5 -A 20 "table: LLANA_ROLES_TABLE.*columns:" --type ts
Length of output: 127
Script:
#!/bin/bash
# Let's try a simpler pattern to find the schema definitions
rg -B 2 -A 30 "table:.*LLANA_PUBLIC_TABLES" --type ts
rg -B 2 -A 30 "table:.*LLANA_ROLES_TABLE" --type ts
Length of output: 10547
src/app.controller.post.test.spec.ts (2)
163-195: Well-structured test case for verifying field-level write permissions
These assertions correctly confirm that only the specified fields are returned when allowed fields are configured. This ensures that unauthorized fields remain undefined, strengthening confidence in your permission-based field filtering.
556-599: Comprehensive check for role vs. public table allowed fields
This test covers the scenario where both a public table record and a role specify allowed fields, ensuring that the returned fields align with the more permissive rules. Great for validating that the logic merges or restricts fields as expected.
src/app.controller.put.test.spec.ts (2)
274-308: Clear demonstration of allowed fields behavior for updates
Good job verifying that only the allowed fields are returned upon updating. This test nicely mirrors your POST tests for field-level permissions, ensuring consistency in application logic.
587-632: Validates combined role-based vs. public-table restrictions
This scenario is thoughtfully tested, confirming that even with an existing public table record, the final fields returned are limited to the intersection (or union) of the configured allowed fields. The test is thorough and ensures predictable outcomes when multiple permission sources exist.
src/helpers/Roles.ts (2)
12-12: Good import for a cleaner implementation
Switching to commaStringToArray makes the code simpler and avoids duplicating string parsing logic.
145-145: Validate empty or null inputs for allowed fields
If permission.allowed_fields is null or an empty string, ensure commaStringToArray handles it safely and returns a sensible default array. This prevents edge cases where an unexpected [''] array might arise.
Also applies to: 190-190, 227-227
✅ Verification successful
Let me generate additional shell scripts to check the implementation of commaStringToArray function.
Let me check the actual implementation of the commaStringToArray function.
The commaStringToArray function already handles empty/null inputs safely
The implementation in src/utils/String.ts shows that commaStringToArray properly handles null/empty inputs by returning an empty array []. The function also trims each field to prevent any whitespace issues. Here's the relevant implementation:
export function commaStringToArray(string: string): string[] {
if (!string) {
return []
}
return string.split(',').map(field => field.trim())
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check potential null or empty cases for allowed_fields usage across the codebase.
rg -A 5 --json $'allowed_fields' | jq .
Length of output: 65616
Script:
#!/bin/bash
# Search for commaStringToArray function implementation
ast-grep --pattern 'function commaStringToArray($_) {
$$$
}'
# Also search for any direct usage of this function
rg -A 2 'commaStringToArray\('
Length of output: 950
Script:
#!/bin/bash
# Get the full implementation of commaStringToArray function
cat src/utils/String.ts
Length of output: 2414
src/helpers/Authentication.ts (2)
82-82: Validate the transformation of allowed_fields.
Returning allowed_fields as an array is a great improvement, but confirm that all consumer code (e.g., front-end or downstream services) expects an array rather than a string. This ensures no regressions or type mismatches.
14-14: Ensure proper handling for empty or undefined strings when calling commaStringToArray.
While importing this utility is a good practice, please verify it handles edge cases like an empty string or an undefined allowed_fields.
Run the following script to check for all invocations of commaStringToArray to confirm they handle potential null, undefined, or empty arguments:
✅ Verification successful
Let me also check the implementation of this utility function:
commaStringToArray already handles empty/undefined strings properly
The implementation in src/utils/String.ts already includes proper handling for empty or undefined strings by returning an empty array:
if (!string) {
return []
}The function is used consistently in three locations:
src/helpers/Authentication.tssrc/helpers/Roles.ts(three occurrences)
All usages are for processing allowed_fields from database records or permissions, which is a safe context as undefined/empty values will result in an empty array.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for all invocations of commaStringToArray in the codebase
rg -A 3 "commaStringToArray("
Length of output: 120
Script:
#!/bin/bash
# Fix the regex pattern to search for commaStringToArray usage
rg -A 3 "commaStringToArray\("
Length of output: 1223
Script:
#!/bin/bash
# Find the implementation of commaStringToArray
ast-grep --pattern 'function commaStringToArray($_) { $$$ }'
Length of output: 62
Script:
#!/bin/bash
# Alternative search for the implementation
rg -A 5 "export (function |const )?commaStringToArray"
Length of output: 324
| /** | ||
| * Convert a comma separated string to an array | ||
| */ | ||
|
|
||
| export function commaStringToArray(string: string): string[] { | ||
| if (!string) { | ||
| return [] | ||
| } | ||
|
|
||
| return string.split(',').map(field => field.trim()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and error handling to commaStringToArray.
The function should handle edge cases and provide clear error messages.
export function commaStringToArray(string: string): string[] {
+ if (string === undefined || string === null) {
+ return []
+ }
+
+ if (typeof string !== 'string') {
+ throw new Error('Input must be a string')
+ }
+
if (!string) {
return []
}
- return string.split(',').map(field => field.trim())
+ const fields = string.split(',').map(field => field.trim())
+
+ if (!fields.every(field => field.length > 0)) {
+ throw new Error('Invalid format: empty field names are not allowed')
+ }
+
+ return fields
}📝 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.
| /** | |
| * Convert a comma separated string to an array | |
| */ | |
| export function commaStringToArray(string: string): string[] { | |
| if (!string) { | |
| return [] | |
| } | |
| return string.split(',').map(field => field.trim()) | |
| } | |
| /** | |
| * Convert a comma separated string to an array | |
| */ | |
| export function commaStringToArray(string: string): string[] { | |
| if (string === undefined || string === null) { | |
| return [] | |
| } | |
| if (typeof string !== 'string') { | |
| throw new Error('Input must be a string') | |
| } | |
| if (!string) { | |
| return [] | |
| } | |
| const fields = string.split(',').map(field => field.trim()) | |
| if (!fields.every(field => field.length > 0)) { | |
| throw new Error('Invalid format: empty field names are not allowed') | |
| } | |
| return fields | |
| } |
Summary by CodeRabbit
Release Notes
New Features
allowed_fieldsto restrict data access based on user permissionsImprovements
Testing
These changes improve data security and access control by providing more granular permission management.