fix AR creation bug with incomplete naming schema - #5140
Conversation
tpurschke
left a comment
tpurschke
left a comment
There was a problem hiding this comment.
F1 — medium. convLength is computed above the areaIdString.Length >= FixedPartLength guard
so NetworkAreaPattern is now dereferenced unconditionally on a path that previously never touched it. A stored "networkAreaPattern": null (System.Text.Json happily assigns null to the non-nullable string) converts a safe return areaIdString into a NullReferenceException that tears down the Blazor circuit. Fix: move the convLength line inside the if.
F2 — medium. With fixedPartLength: 3 and networkAreaPattern: "NA12"
areas NA120100 and NA129900 both reduce to fixed part "AR". ProposeFreeAppRoleNumber then queries "AR%" for every area, so app roles from different areas share one number range and the ID carries no area information. Better handled by validating FixedPartLength >= NetworkAreaPattern.Length in SettingsModelling.razor instead of only clamping downstream.
F3 — low. The new test uses FixedPartLength = 0
where Remove(0, convLength) is a no-op under any clamping formula — it proves the crash is gone but not that min() is the right length. The case the ternary exists for (0 < FixedPartLength < NetworkAreaPattern.Length, e.g. FixedPartLength = 1, "NA" → "AR") is untested, as is the unchanged normal path.
Nothing blocking
the fix is a strict crash removal — convLength differs from NetworkAreaPattern.Length only in the case where the old code always threw, so no working configuration changes behaviour.
fixed |
F1 — medium — still partially open. Stored null naming patterns are only handled in one conversion path
Refs: F2 — medium — still open. Existing invalid fixed-part configurations still collapse area-specific App Role prefixesThe new Settings validation prevents future saves where Refs: Recommendation summaryPlease fix F1/F2 before merging. Both are about existing stored configurations, which is exactly where this bug fix needs to be robust. |
F4 — medium — editing existing App Roles is still effectively broken for installations that already stored an invalid area naming conventionWhen Refs: |
…-orchestrator into fix/modell-fixed-part-length
…rschke/firewall-orchestrator into fix/modell-fixed-part-length
Review findings
No findings are currently open. F1-F4 were verified against current PR head 28771c4. The changes normalize persisted naming conventions, reject unsafe settings saves, prevent unsafe App Role ID proposals, and suppress an existing-App-Role dialog when its area cannot be derived. The security pass found no new authentication, authorization, tenant-isolation, injection, secret-handling, or schema/upgrade issue. The UI changes have matching localized text, and the changed production paths have corresponding unit tests. RecommendationNo further fix is required for this review round. The remaining validation risk is that the full unit suite did not return a final summary within the environment runner window; the Debug solution build and the targeted modelling-handler tests passed. Review metadataStandard-depth review using the fwo-review-pr workflow. A reduced-tier sub-agent collected the diff, history, and mechanical guideline evidence; correctness, security, finding reconciliation, and ratings were verified on the primary agent. Usage quota was not observable, so the review used the configured proxy limits. GitHub review-thread retrieval returned no threads. |
…-orchestrator into fix/modell-fixed-part-length
…rschke/firewall-orchestrator into fix/modell-fixed-part-length
Review findingsReviewed at head
F4 — medium — still open (re-opened; the previous round reported it fixed)
public void DisplayAppRole(ModellingAppRole? appRole)
{
DisplayAppRoleMode = true; // NwObjHandling.cs:529
AddAppRoleMode = false;
HandleAppRole(appRole);
}
private void HandleAppRole(ModellingAppRole appRole)
{
EditAppRoleMode = false;
AppRoleHandler = new ModellingAppRoleHandler(...);
if (!AddAppRoleMode && !AppRoleHandler.AreaConversionValid)
{
return; // NwObjHandling.cs:545 — DisplayAppRoleMode is already true
}
EditAppRoleMode = true;
}
The new test Suggested fix: clear Refs: F5 — medium — new
Verified against the built assembly at this head, with So the proposed App Role ID carries a literal Suggested fix: when network areas are used, require equal pattern lengths ( Refs: F6 — medium — newCommit That is unrelated to the App Role naming fix, and merging it drops regression protection for an unattended git push/rebase path that runs in the middleware — including the shallow-clone, moved-remote, missing-committer-identity and conflict-keeps-the-file cases. Please restore the tests, or split the removal into its own PR with a stated reason, before merging. Refs: Verification performed
RecommendationFix before merging:
Nice to have: a test pinning the round trip Review metadataStandard-depth review following the |
|
3 participants
Uh oh!
There was an error while loading. Please reload this page.