refactor(grpc): name product arg consistently in policy interceptors#116
Merged
Conversation
The shared checkAuthorization parameter was renamed sub->product in the user-flow product-isolation work, but the gRPC unary/stream interceptors still named their local "sub" while passing it into the product slot. Rename the local to product and clarify PolicyConfig/SubResolver docs so the resolved value's role is explicit: it is the product identifier (e.g. "midaz") that becomes the M2M subject "admin/<product>-editor-role" and the normal-user isolation key. No behavior change — the same value is forwarded; verified against a real M2M call (admin/<product>-editor-role emitted identically to v2.8.0). X-Lerian-Ref: 0x1
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe gRPC auth middleware now resolves and forwards a product identifier in unary and streaming interceptors. Normal-user authorization also now fails when the JWT ChangesgRPC authorization product switch
Normal-user authorization validation
Possibly related PRs
✨ Finishing Touches✨ Simplify code
Comment |
A normal-user token missing the "sub" claim previously produced the subject "<owner>/" and was sent to the auth service as a degenerate, identity-less principal. Reject it with 401 (mirroring the existing missing-owner guard) so malformed tokens fail closed. Also pin two auth-contract edges flagged in review: - normal-user without "sub" is rejected; - an empty product preserves prior behavior (subject unchanged, no product forwarded) — the gate-by-presence contract. X-Lerian-Ref: 0x1
|
🎉 This PR is included in version 2.9.0-beta.2 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
1 task
7 tasks
|
🎉 This PR is included in version 2.9.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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
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.
Pull Request Type
Summary
Clarity-only follow-up to the user-flow product-isolation change. When
checkAuthorization's second parameter was renamedsub→product, the HTTPAuthorizecaller was updated, but the gRPC unary/stream interceptors kept naming their local variablesubwhile passing it into theproductslot. This made the gRPC call sites read as if they were passing a subject when they are actually passing the product identifier.This PR renames the gRPC local to
productand clarifies thePolicyConfig.SubResolverdoc so the resolved value's role is explicit.What it is NOT
No behavior change. The resolved value was already the product identifier (e.g.
midaz) — the same thing the HTTP path passes. For M2M it becomes the subjectadmin/<product>-editor-role; for normal-user it is forwarded as the isolation key.The public field
PolicyConfig.SubResolverandSubFromMetadataare kept (no exported API rename) to avoid breaking consumers.Why it's safe
NewGRPCAuthUnaryPolicy/PolicyConfigAPI is not used by any service in production (only referenced in planned docs), so there is no live path to regress.Authorize, which uses the same positional slot — unchanged.type=application) hitting the local access manager: the new code emitsadmin/plugin-crm-editor-role/admin/onboarding-editor-role/admin/transaction-editor-role— byte-identical to v2.8.0.Checklist
Notes
Targets
develop. Should land before promotingv2.9.0to stable (PR #115).Update — also closes the remaining CodeRabbit threads
This PR now additionally addresses the two other review findings from the release diff:
sub(Security thread): a normal-user JWT without asubclaim previously produced"<owner>/"and was sent as an identity-less principal. It is now rejected with 401, mirroring the existing missing-owner guard. Non-breaking — every legitimate user token hassub.TestCheckAuthorization_MissingSubClaim(rejects no-sub) andTestCheckAuthorization_NormalUser_EmptyProduct_NotForwarded(empty product preserves prior behavior, no product forwarded — gate-by-presence).All tests green.