feat(auth): LogicalModel scope for Method ACL and FieldRule - #259
Open
buke wants to merge 4 commits into
Open
Conversation
- Add LogicalModelName (and Method LogicalMethods) as a fifth exclusive scope so one rule covers per-app isomorphic inject models across host apps. - Wire CheckMethodAccess, FieldRule eval, and PermissionState ACL aggregation; register short names from platform inject bases; seed bootstrap grants and admin scope UI. Co-authored-by: Cursor <cursoragent@cursor.com>
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThis change adds registered logical models as authorization scopes. It supports logical method and field rules, evaluates them in ACL paths, exposes them in administration views, adds bootstrap permissions, and applies repository authorization bypasses to selected internal operations. ChangesLogical-model ACL support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant RoleMethodAccessFormView
participant RoleMethodAccess
participant evaluateRoleMethodAccess
participant PermissionStateACL
Admin->>RoleMethodAccessFormView: Select logical model and methods
RoleMethodAccessFormView->>RoleMethodAccess: Submit logical scope rule
evaluateRoleMethodAccess->>RoleMethodAccess: Query logical-model rules
RoleMethodAccess-->>evaluateRoleMethodAccess: Return matching method permissions
PermissionStateACL->>RoleMethodAccess: Load logical model and method data
PermissionStateACL-->>Admin: Expose effective permission state
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
modules/auth/web/views/access_rules_field_binding.test.ts (2)
37-46: 📐 Maintainability & Code Quality | 🔵 TrivialRun the required validation checks before merge.
Build the CLI and generate ignored embedded assets before installing modules. Then run the affected auth module typecheck and unit tests, plus applicable auth E2E tests. Confirm that application code remains compatible with embedded QuickJS.
As per coding guidelines, these checks are required for frontend modules and the auth module.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/auth/web/views/access_rules_field_binding.test.ts` around lines 37 - 46, Before merging changes to the access-rules form bindings tested by “PR-LM-4”, build the CLI and generate ignored embedded assets before installing modules. Then run the auth module typecheck, unit tests, applicable auth E2E tests, and the compatibility validation for embedded QuickJS.Source: Coding guidelines
37-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend this binding test to cover the new UI contracts.
The test checks the logical property names, but it does not verify
:allow-array="true"inmodules/auth/web/views/RoleMethodAccessFormView.vueor the newLogicalModelNamecolumns inmodules/auth/web/views/RoleMethodAccessListView.vueandmodules/auth/web/views/RoleFieldRuleListView.vue. Add assertions for these bindings. The array binding is part of the contract defined bymodules/auth/service/models/_logical_model_registry.ts:52-82.Suggested assertions
expect(methodForm).toContain('prop="LogicalMethods"'); + expect(methodForm).toContain(':allow-array="true"'); expect(methodForm).toContain('Logical Model (all host apps sharing that short name)'); + expect(viewSource('RoleMethodAccessListView.vue')).toContain('prop="LogicalModelName"'); + expect(viewSource('RoleFieldRuleListView.vue')).toContain('prop="LogicalModelName"');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/auth/web/views/access_rules_field_binding.test.ts` around lines 37 - 46, Extend the test case around viewSource in access_rules_field_binding.test.ts to assert RoleMethodAccessFormView.vue includes the :allow-array="true" binding, and assert RoleMethodAccessListView.vue and RoleFieldRuleListView.vue each include the new LogicalModelName column. Preserve the existing property and label assertions.modules/auth/service/models/_rule_scope_helpers.ts (1)
29-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
supportsLogicalModelis never read.
assertExclusiveScopebranches on the field keyLogicalModelNameat Line 145, not onspec.supportsLogicalModel. The flag is therefore unused metadata. Either use it in the normalization branch or drop it to avoid two sources of truth.♻️ Option: drive normalization from the flag
- if (f === 'LogicalModelName') { + if (f === 'LogicalModelName' && spec.supportsLogicalModel) { ids[f] = normalizeLogicalModelName((values as any)[f]); } else { ids[f] = normalizeRefId((values as any)[f]); }Also applies to: 145-149
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/auth/service/models/_rule_scope_helpers.ts` around lines 29 - 30, Resolve the duplicate source of truth between supportsLogicalModel and the LogicalModelName key check in assertExclusiveScope. Prefer using spec.supportsLogicalModel to drive the normalization branch, replacing the hardcoded field-key condition while preserving existing behavior; alternatively remove the unused flag and its related metadata.modules/auth/service/tests/permission_state_acl_source.test.ts (1)
270-277: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a logical rule without
LogicalMethods.The current test covers the method-restricted path only. The
methods == nullbranch setsallowAlland emits therpc:/<app>.<Model>/*wildcard. That branch is untested.💚 Suggested extra assertions
expect(allows.has('rpc:/auth.User/*')).toBe(false); + + (RoleMethodAccess as any).Search = async () => [ + { + RoleId: 'role_1', + MetaServiceId: null, + MetaModelId: null, + MetaApplicationId: null, + LogicalModelName: 'FieldDefault', + LogicalMethods: null, + Mode: 'allow', + Source: 'manual', + }, + ]; + const aggAll = await buildAclAggregation(['role_1'], { role_1: { global: true, companies: [] } }); + const allowsAll = aggAll.requiresAllowKeysByCompany.get('*') || new Set(); + expect(allowsAll.has('rpc:/auth.FieldDefault/*')).toBe(true); + expect(aggAll.companyGlobalAllow.has('*')).toBe(false);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/auth/service/tests/permission_state_acl_source.test.ts` around lines 270 - 277, Extend the aggregation test around buildAclAggregation to include a logical rule whose methods value is null or omitted. Assert that the resulting allows include the rpc:/<app>.<Model>/* wildcard for that rule, covering the allowAll branch while preserving the existing method-specific assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/auth/data/bootstrap.json`:
- Around line 76-104: Add a RoleRecordRule grant for the FieldDefault model to
the auth.role_base_user permissions in bootstrap.json, granting the required
write and create access used by FieldDefault.Set. Keep the existing
rma_base_user_field_default_logical and rfr_base_user_field_default_logical
grants unchanged.
In `@modules/auth/service/models/_user_permission_state_acl.ts`:
- Around line 194-199: Handle malformed LogicalMethods consistently per ACL
path: in modules/auth/service/models/_user_permission_state_acl.ts at lines
194-199, preserve the deny row and treat a normalization failure as model-wide;
in modules/auth/service/models/_user_method_access.ts at lines 160-169, catch
normalization errors per row and skip only that invalid rule so method checks do
not fail globally. Update the surrounding ACL projection and method-access
filtering logic without changing valid payload handling.
In `@modules/auth/service/models/role_method_access.ts`:
- Around line 176-195: Update the LogicalModelName handling in the role-method
validation flow to prevent stale LogicalMethods when the scope changes between
logical models. In the update path around touchesLogicalName, detect a changed
non-empty LogicalModelName and either clear LogicalMethods when it is omitted or
require a replacement whitelist in the same payload, while preserving existing
behavior for unchanged scope and non-logical rows.
---
Nitpick comments:
In `@modules/auth/service/models/_rule_scope_helpers.ts`:
- Around line 29-30: Resolve the duplicate source of truth between
supportsLogicalModel and the LogicalModelName key check in assertExclusiveScope.
Prefer using spec.supportsLogicalModel to drive the normalization branch,
replacing the hardcoded field-key condition while preserving existing behavior;
alternatively remove the unused flag and its related metadata.
In `@modules/auth/service/tests/permission_state_acl_source.test.ts`:
- Around line 270-277: Extend the aggregation test around buildAclAggregation to
include a logical rule whose methods value is null or omitted. Assert that the
resulting allows include the rpc:/<app>.<Model>/* wildcard for that rule,
covering the allowAll branch while preserving the existing method-specific
assertions.
In `@modules/auth/web/views/access_rules_field_binding.test.ts`:
- Around line 37-46: Before merging changes to the access-rules form bindings
tested by “PR-LM-4”, build the CLI and generate ignored embedded assets before
installing modules. Then run the auth module typecheck, unit tests, applicable
auth E2E tests, and the compatibility validation for embedded QuickJS.
- Around line 37-46: Extend the test case around viewSource in
access_rules_field_binding.test.ts to assert RoleMethodAccessFormView.vue
includes the :allow-array="true" binding, and assert
RoleMethodAccessListView.vue and RoleFieldRuleListView.vue each include the new
LogicalModelName column. Preserve the existing property and label assertions.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 293fff57-2078-406c-bfaa-2e74eb5a9390
📒 Files selected for processing (25)
modules/auth/data/bootstrap.jsonmodules/auth/service/models/_logical_model_registry.tsmodules/auth/service/models/_rule_scope_helpers.tsmodules/auth/service/models/_user_field_rule_eval.tsmodules/auth/service/models/_user_lifecycle_auth.tsmodules/auth/service/models/_user_method_access.tsmodules/auth/service/models/_user_permission_state_acl.tsmodules/auth/service/models/role_field_rule.tsmodules/auth/service/models/role_method_access.tsmodules/auth/service/models/user.tsmodules/auth/service/tests/field_rule.test.tsmodules/auth/service/tests/logical_model_registry.test.tsmodules/auth/service/tests/method_access_eval_observability.test.tsmodules/auth/service/tests/permission_state_acl_source.test.tsmodules/auth/service/tests/rule_scope_helpers.test.tsmodules/auth/web/views/RoleFieldRuleFormView.vuemodules/auth/web/views/RoleFieldRuleListView.vuemodules/auth/web/views/RoleMethodAccessFormView.vuemodules/auth/web/views/RoleMethodAccessListView.vuemodules/auth/web/views/access_rules_field_binding.test.tsmodules/core/service/orm/model/app_setting_base_model.tsmodules/core/service/orm/model/field_default_base_model.tsmodules/core/service/orm/model/logical_model_registry.test.tsmodules/core/service/orm/model/logical_model_registry.tsmodules/core/service/orm/model/translation_term_base_model.ts
Comment on lines
+76
to
+104
| "name": "rma_base_user_field_default_logical", | ||
| "model": "RoleMethodAccess", | ||
| "values": { | ||
| "RoleId": { | ||
| "ref": "auth.role_terminology_editor" | ||
| "ref": "auth.role_base_user" | ||
| }, | ||
| "MetaApplicationId": null, | ||
| "MetaModelId": null, | ||
| "MetaServiceId": { | ||
| "serviceRef": "auth.TranslationTerm/Update" | ||
| }, | ||
| "MetaServiceId": null, | ||
| "LogicalModelName": "FieldDefault", | ||
| "LogicalMethods": ["Get", "Set"], | ||
| "Mode": "allow" | ||
| } | ||
| }, | ||
| { | ||
| "name": "rma_terminology_editor_tt_count", | ||
| "model": "RoleMethodAccess", | ||
| "name": "rfr_base_user_field_default_logical", | ||
| "model": "RoleFieldRule", | ||
| "values": { | ||
| "RoleId": { | ||
| "ref": "auth.role_terminology_editor" | ||
| "ref": "auth.role_base_user" | ||
| }, | ||
| "MetaApplicationId": null, | ||
| "MetaModelId": null, | ||
| "MetaServiceId": { | ||
| "serviceRef": "auth.TranslationTerm/Count" | ||
| "MetaFieldId": null, | ||
| "LogicalModelName": "FieldDefault", | ||
| "PermRead": "allow", | ||
| "PermWrite": "allow" | ||
| } | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect FieldDefault write path and record-rule requirements.
fd -t f 'field_default'
rg -nP --type=ts -C6 '\bSet\s*\(' -g '*field_default*' | head -60
rg -nP --type=ts -C3 'withRepositoryAuthzRuleBypass|withPermissionGraphBypass' -g '*field_default*' | head -40Repository: choysum-dev/choysum
Length of output: 708
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- candidate file outlines ---'
ast-grep outline modules/core/service/orm/model/field_default_base_model.ts
ast-grep outline modules/core/service/orm/model/field_default_lookup.ts
ast-grep outline modules/core/service/orm/model/field_default_resolve.ts
printf '%s\n' '--- FieldDefault definitions and Set callers ---'
rg -n -C8 'class FieldDefault|FieldDefault|LogicalModelName|RoleRecordRule|record.?rule|Set\s*\(' \
modules/core/service/orm/model modules/auth modules/core/service/orm \
-g '*.ts' -g '*.json' | head -240
printf '%s\n' '--- bootstrap seed context ---'
cat -n modules/auth/data/bootstrap.json | sed -n '1,180p'Repository: choysum-dev/choysum
Length of output: 31697
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- FieldDefault implementation ---'
cat -n modules/core/service/orm/model/field_default_base_model.ts | sed -n '210,390p'
printf '%s\n' '--- authorization symbols ---'
rg -n -C5 'RoleRecordRule|recordRule|RecordRule|withRepositoryAuthzRuleBypass|withPermissionGraphBypass|LogicalModelName' \
modules/auth modules/core/service modules/base -g '*.ts' -g '*.json' | head -420
printf '%s\n' '--- bootstrap record rules ---'
rg -n -C12 '"model": "RoleRecordRule"|RoleRecordRule|role_base_user.*record|r.*base_user.*record' \
modules/auth/data/bootstrap.jsonRepository: choysum-dev/choysum
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- concrete FieldDefault model declarations and callers ---'
rg -n -C8 'FieldDefaultBaseModel|`@Model`\(.FieldDefault.|\.Set\(' \
modules -g '*.ts' | head -320
printf '%s\n' '--- authorization bypass definitions and call sites ---'
rg -n -C10 'function withRepositoryAuthzRuleBypass|const withRepositoryAuthzRuleBypass|export .*withRepositoryAuthzRuleBypass|withRepositoryAuthzRuleBypass' \
modules -g '*.ts'
printf '%s\n' '--- logical ACL and record-rule evaluation ---'
rg -n -C12 'logicalMethodsAllow|LogicalModelName|recordRuleMode|PermCreate|PermWrite|record rule' \
modules/auth/service modules/core/service -g '*.ts' | head -500
printf '%s\n' '--- FieldDefault-specific tests ---'
rg -n -C12 'FieldDefault|field default|Set\(' \
modules -g '*field_default*.test.ts' -g '*authz*.test.ts' | head -420Repository: choysum-dev/choysum
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- authz implementation files ---'
fd -t f -i 'authz' modules/core modules/auth | head -80
printf '%s\n' '--- repository authz implementation ---'
for f in $(fd -t f -i 'authz' modules/core modules/auth | head -20); do
echo "### $f"
rg -n -C8 'export|bypass|RecordRule|FieldRule|PermCreate|PermWrite|allow|deny' "$f" | head -180
done
printf '%s\n' '--- CRUD authorization hooks ---'
rg -n -C10 'getRepositoryRecordRuleBypassDepth|getRepositoryFieldRuleBypassDepth|assert.*(Record|Field)|check.*(Record|Field)|PermCreate|PermWrite|recordRuleMode' \
modules/core/service/orm -g '*.ts' | head -500
printf '%s\n' '--- complete base-user record-rule seed blocks ---'
sed -n '300,460p' modules/auth/data/bootstrap.jsonRepository: choysum-dev/choysum
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- BaseModel CRUD outline ---'
ast-grep outline modules/core/service/orm/model/model.ts
printf '%s\n' '--- CRUD implementation and authz options ---'
rg -n -C14 'static async (Create|UpdateById|Search)|async (Create|UpdateById|Search)|recordRule|fieldRule|withRepositoryAuthzRuleBypass|PermCreate|PermWrite' \
modules/core/service/orm/model/model.ts modules/core/service/orm/repository -g '*.ts' | head -600
printf '%s\n' '--- concrete auth evaluator entry points ---'
fd -t f '_user_record_rule_eval|_user_field_rule_eval|_user_method_access' modules/auth
for f in $(fd -t f '_user_record_rule_eval|_user_field_rule_eval|_user_method_access' modules/auth); do
echo "### $f"
ast-grep outline "$f"
doneRepository: choysum-dev/choysum
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
import json
import re
from pathlib import Path
seed_path = Path("modules/auth/data/bootstrap.json")
source_path = Path("modules/core/service/orm/model/field_default_base_model.ts")
seed = json.loads(seed_path.read_text())
records = seed["records"]
base_role = next(r for r in records if r["name"] == "role_base_user")
method = next(r for r in records if r["name"] == "rma_base_user_field_default_logical")
field = next(r for r in records if r["name"] == "rfr_base_user_field_default_logical")
record_rules = [
r for r in records
if r["model"] == "RoleRecordRule"
and r.get("values", {}).get("RoleId", {}).get("ref") == "auth.role_base_user"
]
print("base role:", base_role["values"]["Code"])
print("method scope/methods/mode:",
method["values"]["LogicalModelName"],
method["values"]["LogicalMethods"],
method["values"]["Mode"])
print("field scope/read/write:",
field["values"]["LogicalModelName"],
field["values"]["PermRead"],
field["values"]["PermWrite"])
print("base-user record rules:")
for rule in record_rules:
v = rule["values"]
print({
"name": rule["name"],
"application": v.get("MetaApplicationId"),
"model": v.get("MetaModelId"),
"read": v.get("PermRead"),
"write": v.get("PermWrite"),
"create": v.get("PermCreate"),
"delete": v.get("PermDelete"),
})
source = source_path.read_text()
match = re.search(
r"static async Set\(.*?\n \}\n\n /\*\*\n \* Read the exact-scope default",
source,
re.S,
)
if not match:
raise SystemExit("Set body not found")
set_body = match.group(0)
print("Set uses Create:", bool(re.search(r"\(this as any\)\.Create\s*\(", set_body)))
print("Set uses UpdateById:", bool(re.search(r"\(this as any\)\.UpdateById\s*\(", set_body)))
print("Set wraps a repository authz bypass:",
bool(re.search(r"withRepositoryAuthzRuleBypass|withRepositoryRecordRuleBypass", set_body)))
print("GetEffective wraps a repository authz bypass:",
"withRepositoryAuthzRuleBypass" in source[source.find("static async GetEffective"):source.find("static async Unset")])
PYRepository: choysum-dev/choysum
Length of output: 1581
Add a RoleRecordRule grant for FieldDefault. FieldDefault.Set calls Search, UpdateById, and Create without a record-rule bypass. The base-user rules do not grant write or create access for this model, so the existing method and field grants are insufficient.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/auth/data/bootstrap.json` around lines 76 - 104, Add a RoleRecordRule
grant for the FieldDefault model to the auth.role_base_user permissions in
bootstrap.json, granting the required write and create access used by
FieldDefault.Set. Keep the existing rma_base_user_field_default_logical and
rfr_base_user_field_default_logical grants unchanged.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 25 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="modules/auth/service/models/_user_permission_state_acl.ts">
<violation number="1" location="modules/auth/service/models/_user_permission_state_acl.ts:192">
P3: LogicalModel aggregation now re-filters the full model catalog for each matching access row, which can add noticeable latency on large role/model sets. Caching models by logical short name once before (or during first use in) the loop would avoid repeated full scans.</violation>
<violation number="2" location="modules/auth/service/models/_user_permission_state_acl.ts:192">
P2: The new LogicalModel scope resolves its target models by bare short name across *all* apps (`getAllModels().filter(m => m.name === logicalName)`), and the runtime path matches the same way (`LogicalModelName = modelName`). This is fine for the intended isomorphic inject models, but it means a grant for a logical short name also matches any custom/unrelated model in another app that happens to share that `Name`, and when `LogicalMethods` is empty it yields an 'all methods' `allowAll`/`denyAll` for that whole surface. Consider validating that each matched model is actually one of the registered logical inject models (e.g. against the registry from `_logical_model_registry`) for the resolved host apps before applying the grant, so a naming collision can't turn into an over-granted ACL.</violation>
</file>
<file name="modules/auth/service/models/_rule_scope_helpers.ts">
<violation number="1" location="modules/auth/service/models/_rule_scope_helpers.ts:37">
P2: Adding `LogicalModelName` to the method/field `fields` arrays means the update-time 'must provide ... together' check now demands all four keys for any scope-touching update. Existing callers performing a meta-scope update with only the original three refs will now get a hard error because `LogicalModelName` is missing. This is a behavior-breaking change to the update contract for the existing meta scopes. If intentional, callers must be updated to always send `LogicalModelName: null`; otherwise consider only requiring `LogicalModelName` when it is actually being used (e.g. when `LogicalMethods` is present).</violation>
</file>
<file name="modules/auth/web/views/access_rules_field_binding.test.ts">
<violation number="1" location="modules/auth/web/views/access_rules_field_binding.test.ts:37">
P3: The new LogicalModel-scope test only greps the view source for static prop names and exact user-facing help copy, so it never verifies the actual behavior this PR introduces — the exclusive scoping where choosing a LogicalModel clears Meta* and vice-versa. As written, the test would still pass if the @Onchange exclusivity handlers were removed, and it breaks on any harmless wording change to the _t() help text. Consider exercising the binding behavior (e.g. setting LogicalModelName and asserting MetaServiceId/MetaModelId/MetaApplicationId clear, and the reverse), and matching on stable identifiers rather than exact help copy so the test guards the feature's real contract.</violation>
</file>
<file name="modules/auth/service/models/_logical_model_registry.ts">
<violation number="1" location="modules/auth/service/models/_logical_model_registry.ts:92">
P2: The read/eval helper `logicalMethodsAllow` calls `normalizeLogicalMethods`, which throws on malformed or non-array `LogicalMethods` values. In the hot method-access filter (`_user_method_access.ts`), this call is not wrapped in try/catch, so a single corrupt legacy row could abort ACL evaluation for the request. The parallel ACL aggregation path (`_user_permission_state_acl.ts`) already guards the same normalization with try/catch and `continue`, so the read path should be defensive too.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| .trim() | ||
| .toLowerCase(); | ||
| if (!want) return false; | ||
| const list = normalizeLogicalMethods(methods); |
There was a problem hiding this comment.
P2: The read/eval helper logicalMethodsAllow calls normalizeLogicalMethods, which throws on malformed or non-array LogicalMethods values. In the hot method-access filter (_user_method_access.ts), this call is not wrapped in try/catch, so a single corrupt legacy row could abort ACL evaluation for the request. The parallel ACL aggregation path (_user_permission_state_acl.ts) already guards the same normalization with try/catch and continue, so the read path should be defensive too.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/auth/service/models/_logical_model_registry.ts, line 92:
<comment>The read/eval helper `logicalMethodsAllow` calls `normalizeLogicalMethods`, which throws on malformed or non-array `LogicalMethods` values. In the hot method-access filter (`_user_method_access.ts`), this call is not wrapped in try/catch, so a single corrupt legacy row could abort ACL evaluation for the request. The parallel ACL aggregation path (`_user_permission_state_acl.ts`) already guards the same normalization with try/catch and `continue`, so the read path should be defensive too.</comment>
<file context>
@@ -0,0 +1,95 @@
+ .trim()
+ .toLowerCase();
+ if (!want) return false;
+ const list = normalizeLogicalMethods(methods);
+ if (list == null) return true;
+ return list.some(m => m.toLowerCase() === want);
</file context>
| if (!sid && !mid && !aid) { | ||
| // LogicalModel scope: all host apps whose @Model short name matches. | ||
| if (!sid && !mid && !aid && logicalName) { | ||
| const models = (await getAllModels()).filter(m => m.name === logicalName); |
There was a problem hiding this comment.
P2: The new LogicalModel scope resolves its target models by bare short name across all apps (getAllModels().filter(m => m.name === logicalName)), and the runtime path matches the same way (LogicalModelName = modelName). This is fine for the intended isomorphic inject models, but it means a grant for a logical short name also matches any custom/unrelated model in another app that happens to share that Name, and when LogicalMethods is empty it yields an 'all methods' allowAll/denyAll for that whole surface. Consider validating that each matched model is actually one of the registered logical inject models (e.g. against the registry from _logical_model_registry) for the resolved host apps before applying the grant, so a naming collision can't turn into an over-granted ACL.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/auth/service/models/_user_permission_state_acl.ts, line 192:
<comment>The new LogicalModel scope resolves its target models by bare short name across *all* apps (`getAllModels().filter(m => m.name === logicalName)`), and the runtime path matches the same way (`LogicalModelName = modelName`). This is fine for the intended isomorphic inject models, but it means a grant for a logical short name also matches any custom/unrelated model in another app that happens to share that `Name`, and when `LogicalMethods` is empty it yields an 'all methods' `allowAll`/`denyAll` for that whole surface. Consider validating that each matched model is actually one of the registered logical inject models (e.g. against the registry from `_logical_model_registry`) for the resolved host apps before applying the grant, so a naming collision can't turn into an over-granted ACL.</comment>
<file context>
@@ -182,11 +183,40 @@ export async function buildAclAggregation(
- if (!sid && !mid && !aid) {
+ // LogicalModel scope: all host apps whose @Model short name matches.
+ if (!sid && !mid && !aid && logicalName) {
+ const models = (await getAllModels()).filter(m => m.name === logicalName);
+ if (models.length === 0) continue;
+ let methods: string[] | null;
</file context>
| modelName: 'RoleMethodAccess', | ||
| fields: ['MetaServiceId', 'MetaModelId', 'MetaApplicationId'], | ||
| shapesLabel: 'service/model/application/global', | ||
| fields: ['MetaServiceId', 'MetaModelId', 'MetaApplicationId', 'LogicalModelName'], |
There was a problem hiding this comment.
P2: Adding LogicalModelName to the method/field fields arrays means the update-time 'must provide ... together' check now demands all four keys for any scope-touching update. Existing callers performing a meta-scope update with only the original three refs will now get a hard error because LogicalModelName is missing. This is a behavior-breaking change to the update contract for the existing meta scopes. If intentional, callers must be updated to always send LogicalModelName: null; otherwise consider only requiring LogicalModelName when it is actually being used (e.g. when LogicalMethods is present).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/auth/service/models/_rule_scope_helpers.ts, line 37:
<comment>Adding `LogicalModelName` to the method/field `fields` arrays means the update-time 'must provide ... together' check now demands all four keys for any scope-touching update. Existing callers performing a meta-scope update with only the original three refs will now get a hard error because `LogicalModelName` is missing. This is a behavior-breaking change to the update contract for the existing meta scopes. If intentional, callers must be updated to always send `LogicalModelName: null`; otherwise consider only requiring `LogicalModelName` when it is actually being used (e.g. when `LogicalMethods` is present).</comment>
<file context>
@@ -15,36 +17,48 @@ export type RuleScopeProfile = 'method' | 'record' | 'field' | 'ui';
modelName: 'RoleMethodAccess',
- fields: ['MetaServiceId', 'MetaModelId', 'MetaApplicationId'],
- shapesLabel: 'service/model/application/global',
+ fields: ['MetaServiceId', 'MetaModelId', 'MetaApplicationId', 'LogicalModelName'],
+ shapesLabel: 'service/model/application/logical_model/global',
alwaysValidateOnCreate: false,
</file context>
| if (!sid && !mid && !aid) { | ||
| // LogicalModel scope: all host apps whose @Model short name matches. | ||
| if (!sid && !mid && !aid && logicalName) { | ||
| const models = (await getAllModels()).filter(m => m.name === logicalName); |
There was a problem hiding this comment.
P3: LogicalModel aggregation now re-filters the full model catalog for each matching access row, which can add noticeable latency on large role/model sets. Caching models by logical short name once before (or during first use in) the loop would avoid repeated full scans.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/auth/service/models/_user_permission_state_acl.ts, line 192:
<comment>LogicalModel aggregation now re-filters the full model catalog for each matching access row, which can add noticeable latency on large role/model sets. Caching models by logical short name once before (or during first use in) the loop would avoid repeated full scans.</comment>
<file context>
@@ -182,11 +183,40 @@ export async function buildAclAggregation(
- if (!sid && !mid && !aid) {
+ // LogicalModel scope: all host apps whose @Model short name matches.
+ if (!sid && !mid && !aid && logicalName) {
+ const models = (await getAllModels()).filter(m => m.name === logicalName);
+ if (models.length === 0) continue;
+ let methods: string[] | null;
</file context>
| } | ||
| }); | ||
|
|
||
| it('exposes LogicalModel scope on Method Access and Field Rule forms (PR-LM-4)', () => { |
There was a problem hiding this comment.
P3: The new LogicalModel-scope test only greps the view source for static prop names and exact user-facing help copy, so it never verifies the actual behavior this PR introduces — the exclusive scoping where choosing a LogicalModel clears Meta* and vice-versa. As written, the test would still pass if the @onchange exclusivity handlers were removed, and it breaks on any harmless wording change to the _t() help text. Consider exercising the binding behavior (e.g. setting LogicalModelName and asserting MetaServiceId/MetaModelId/MetaApplicationId clear, and the reverse), and matching on stable identifiers rather than exact help copy so the test guards the feature's real contract.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/auth/web/views/access_rules_field_binding.test.ts, line 37:
<comment>The new LogicalModel-scope test only greps the view source for static prop names and exact user-facing help copy, so it never verifies the actual behavior this PR introduces — the exclusive scoping where choosing a LogicalModel clears Meta* and vice-versa. As written, the test would still pass if the @Onchange exclusivity handlers were removed, and it breaks on any harmless wording change to the _t() help text. Consider exercising the binding behavior (e.g. setting LogicalModelName and asserting MetaServiceId/MetaModelId/MetaApplicationId clear, and the reverse), and matching on stable identifiers rather than exact help copy so the test guards the feature's real contract.</comment>
<file context>
@@ -33,4 +33,15 @@ describe('Access Rules admin field binding (PR-C-5)', () => {
}
});
+
+ it('exposes LogicalModel scope on Method Access and Field Rule forms (PR-LM-4)', () => {
+ const methodForm = viewSource('RoleMethodAccessFormView.vue');
+ expect(methodForm).toContain('prop="LogicalModelName"');
</file context>
- Reuse auth's loginAsE2EAdmin helper so Login submit waits for nprogress and Login RPC, matching meta and avoiding stuck /web/login redirects. Co-authored-by: Cursor <cursoragent@cursor.com>
- Fail closed on malformed LogicalMethods (deny keeps/model-wide; allow skips) and reject non-string whitelist entries. - Clear stale LogicalMethods when LogicalModelName changes; allow methods-only updates; drop unused supportsLogicalModel. - Bypass RecordRule/FieldRule on FieldDefault store Get/Set/Unset paths so preset Logical Method grants work without O(apps) RR seeds. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
2 issues found across 10 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="modules/auth/service/models/role_method_access.ts">
<violation number="1" location="modules/auth/service/models/role_method_access.ts:193">
P1: An update that includes `LogicalModelName` but omits `LogicalMethods` now clears the whitelist to `null` (interpreted as all methods), which can silently broaden an allow ACL. A safer behavior is to require an explicit `LogicalMethods` payload when `LogicalModelName` is touched, instead of widening permissions implicitly.</violation>
</file>
<file name="modules/auth/web/views/access_rules_field_binding.test.ts">
<violation number="1" location="modules/auth/web/views/access_rules_field_binding.test.ts:41">
P3: The update drops two still-passing assertions that verified the user-facing scope explanations ('Logical Model (all host apps sharing that short name)' / 'Logical Model (all host apps / all business fields on that short name)') on the Method Access and Field Rule forms. Both strings remain in the templates, so this removes real regression coverage for UX wording with no replacement. Consider keeping substring assertions for the logical-model hints while adding the new allow-array/list-view checks.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| (values as any).LogicalMethods = null; | ||
| } else if (mode === 'update' && !touchesMethods) { | ||
| // Logical name changed/re-set without a new whitelist → drop stale methods for the prior model. | ||
| (values as any).LogicalMethods = null; |
There was a problem hiding this comment.
P1: An update that includes LogicalModelName but omits LogicalMethods now clears the whitelist to null (interpreted as all methods), which can silently broaden an allow ACL. A safer behavior is to require an explicit LogicalMethods payload when LogicalModelName is touched, instead of widening permissions implicitly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/auth/service/models/role_method_access.ts, line 193:
<comment>An update that includes `LogicalModelName` but omits `LogicalMethods` now clears the whitelist to `null` (interpreted as all methods), which can silently broaden an allow ACL. A safer behavior is to require an explicit `LogicalMethods` payload when `LogicalModelName` is touched, instead of widening permissions implicitly.</comment>
<file context>
@@ -188,11 +188,13 @@ export default class RoleMethodAccess extends BaseModel {
(values as any).LogicalMethods = null;
+ } else if (mode === 'update' && !touchesMethods) {
+ // Logical name changed/re-set without a new whitelist → drop stale methods for the prior model.
+ (values as any).LogicalMethods = null;
}
- } else if (mode === 'update' && touchesMethods && (values as any).LogicalMethods != null) {
</file context>
Suggested change
| (values as any).LogicalMethods = null; | |
| throw new Error('invalid RoleMethodAccess: LogicalMethods must be provided when LogicalModelName is updated'); |
| const methodForm = viewSource('RoleMethodAccessFormView.vue'); | ||
| expect(methodForm).toContain('prop="LogicalModelName"'); | ||
| expect(methodForm).toContain('prop="LogicalMethods"'); | ||
| expect(methodForm).toContain(':allow-array="true"'); |
There was a problem hiding this comment.
P3: The update drops two still-passing assertions that verified the user-facing scope explanations ('Logical Model (all host apps sharing that short name)' / 'Logical Model (all host apps / all business fields on that short name)') on the Method Access and Field Rule forms. Both strings remain in the templates, so this removes real regression coverage for UX wording with no replacement. Consider keeping substring assertions for the logical-model hints while adding the new allow-array/list-view checks.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/auth/web/views/access_rules_field_binding.test.ts, line 41:
<comment>The update drops two still-passing assertions that verified the user-facing scope explanations ('Logical Model (all host apps sharing that short name)' / 'Logical Model (all host apps / all business fields on that short name)') on the Method Access and Field Rule forms. Both strings remain in the templates, so this removes real regression coverage for UX wording with no replacement. Consider keeping substring assertions for the logical-model hints while adding the new allow-array/list-view checks.</comment>
<file context>
@@ -38,10 +38,20 @@ describe('Access Rules admin field binding (PR-C-5)', () => {
expect(methodForm).toContain('prop="LogicalModelName"');
expect(methodForm).toContain('prop="LogicalMethods"');
- expect(methodForm).toContain('Logical Model (all host apps sharing that short name)');
+ expect(methodForm).toContain(':allow-array="true"');
const fieldForm = viewSource('RoleFieldRuleFormView.vue');
</file context>
- Cover LogicalMethods normalize/Onchange/FieldsGet, malformed fail-closed paths, and Logical FieldRule eval. - Exercise remaining registry and method-access branch edges Codecov reported as partials. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="modules/auth/service/tests/logical_model_acl_coverage.test.ts">
<violation number="1" location="modules/auth/service/tests/logical_model_acl_coverage.test.ts:367">
P3: The empty-modelName case in this evaluateFieldRules test only asserts `Array.isArray(outEmptyName.denyReadFields)`. Since `denyReadFields` is an array on essentially every successful result of this function, the assertion is vacuous and would pass even if the empty-name branch it claims to cover was removed or broken. The test comment says it exercises the `input.modelName || ''` path on `modelNameWant`, but the stubbed `MetaModel.Search` still returns a valid model, so nothing about the empty-name handling is actually distinguished. Consider asserting the concrete outcome — for the stubbed fields and an empty RoleFieldRule rule set, `denyReadFields` should equal the full sorted non-system field list (e.g. `['Model', 'Value']`) — so the test genuinely verifies the empty logical name matches no rule.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| modelFullName: 'auth.', | ||
| roleIds: ['r1'], | ||
| }); | ||
| expect(Array.isArray(outEmptyName.denyReadFields)).toBe(true); |
There was a problem hiding this comment.
P3: The empty-modelName case in this evaluateFieldRules test only asserts Array.isArray(outEmptyName.denyReadFields). Since denyReadFields is an array on essentially every successful result of this function, the assertion is vacuous and would pass even if the empty-name branch it claims to cover was removed or broken. The test comment says it exercises the input.modelName || '' path on modelNameWant, but the stubbed MetaModel.Search still returns a valid model, so nothing about the empty-name handling is actually distinguished. Consider asserting the concrete outcome — for the stubbed fields and an empty RoleFieldRule rule set, denyReadFields should equal the full sorted non-system field list (e.g. ['Model', 'Value']) — so the test genuinely verifies the empty logical name matches no rule.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/auth/service/tests/logical_model_acl_coverage.test.ts, line 367:
<comment>The empty-modelName case in this evaluateFieldRules test only asserts `Array.isArray(outEmptyName.denyReadFields)`. Since `denyReadFields` is an array on essentially every successful result of this function, the assertion is vacuous and would pass even if the empty-name branch it claims to cover was removed or broken. The test comment says it exercises the `input.modelName || ''` path on `modelNameWant`, but the stubbed `MetaModel.Search` still returns a valid model, so nothing about the empty-name handling is actually distinguished. Consider asserting the concrete outcome — for the stubbed fields and an empty RoleFieldRule rule set, `denyReadFields` should equal the full sorted non-system field list (e.g. `['Model', 'Value']`) — so the test genuinely verifies the empty logical name matches no rule.</comment>
<file context>
@@ -0,0 +1,374 @@
+ modelFullName: 'auth.',
+ roleIds: ['r1'],
+ });
+ expect(Array.isArray(outEmptyName.denyReadFields)).toBe(true);
+ } finally {
+ (MetaModel as any).Search = origModel;
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Summary
RoleMethodAccess/RoleFieldRuleso one grant covers per-app isomorphic inject models (TranslationTerm/FieldDefault/AppSetting) across all host apps.CheckMethodAccess, FieldRule, PermissionState ACL aggregation), core base-class self-registration for logical names, bootstrap preset grants, and admin Method/Field scope UI (selection + Onchange exclusivity).withRepositoryAuthzRuleBypass(e.g. LoginAppSetting.Get); do not use Logical grants as data-plane sudo.Test plan
./choysum test unit auth --be./choysum test unit auth --fe./choysum test unit core --be(logical model registry)terminology.editor/base.user/sys.adminterminology.editor,Search/Updateonbase.TranslationTerm(non-auth host) is allowed;GetTranslationsstill works via internal path without user Logical grantMade with Cursor
Summary by cubic
Adds a LogicalModel scope to
RoleMethodAccessandRoleFieldRuleso one rule covers per‑app inject models (e.g.TranslationTerm,FieldDefault,AppSetting) across all host apps. Updates admin UX, ACL evaluation, and seeds; rebuild the DB or reinstall modules to apply new grants.New Features
LogicalModelNameas a fifth, exclusive scope on Method and Field rules; for methods, optionalLogicalMethodswhitelists RPCs (null/empty = all).AppSetting,FieldDefault,TranslationTerm.LogicalMethods.LogicalMethods); Onchange enforces exclusive scopes; FieldsGet returns Logical Model selection.terminology.editor(TranslationTerm),base.user(FieldDefault), andsys.admin(AppSetting).Bug Fixes
AppSetting.GetandFieldDefaultstoreGet/Set/UnsetusewithRepositoryAuthzRuleBypassso logical grants work without per‑app Record/Field Rule seeds.loginAsE2EAdminto await nprogress and Login RPC, preventing auth init races.Written for commit 87ab23f. Summary will update on new commits.
PR Type
Enhancement
Description
Go core (outside modules/): No changes in this PR.
TypeScript modules: Added LogicalModel scope to RoleMethodAccess and RoleFieldRule.
Registered AppSetting, FieldDefault, and TranslationTerm in core logical model registry.
Updated runtime evaluation, ACL aggregation, bootstrap grants, and admin views.
Verified SPDX headers on all 4 new source files; expanded unit test coverage.
File Walkthrough
13 files
Create process-local registry for logical model namesRegister AppSetting as a logical model short nameRegister FieldDefault as a logical model short nameRegister TranslationTerm as a logical model short nameProvide auth helpers for logical model normalization and matchingSupport LogicalModelName in exclusive scope validationEvaluate LogicalModel scoped rules in field rule evaluationIncorporate LogicalModel scope into method access resolutionAggregate LogicalModel ACL grants into permission stateAdd LogicalModelName field and Onchange clear handlersAdd LogicalModelName and LogicalMethods fields with payloadnormalizationExpose LogicalModel selection in Field Rule form viewExpose LogicalModel and LogicalMethods in Method Access form view1 files
Update seed bootstrap grants to use LogicalModel scope6 files
Add tests for core logical model registry self-registrationAdd unit tests for auth logical model registry helpersUpdate scope helper tests for LogicalModel shapesAdd test for LogicalModel scope in ACL aggregationAdd method access evaluation test for logical methods whitelistVerify LogicalModel field bindings in admin Vue views5 files
Summary by CodeRabbit