ECOPROJECT-4368 | feat: Add authz for group endpoint#1097
Open
tupyy wants to merge 1 commit into
Open
Conversation
tupyy
requested review from
AvielSegev and
ronenav
and removed request for
a team
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (19)
📝 WalkthroughWalkthroughAdded explicit 403 responses in the OpenAPI and regenerated client/server artifacts; introduced AccountsServicer and admin-group YAML parsing/Initialize; added AuthzAccountsService enforcing admin checks; wired admin-group initialization and conditional authz wrapping at server startup; updated handlers and tests to surface 403 flows. ChangesOpenAPI / Generated API
Accounts service, parsing, and authz
Handlers & Tests
Server wiring, config, and deployment
Sequence Diagram(s)sequenceDiagram
participant Server
participant Config
participant Parser
participant AccountsSvc
participant Store
Server->>Config: read MIGRATION_PLANNER_ADMIN_GROUP_FILE
alt AdminGroupFile set
Server->>Parser: ParseAdminGroupFile(path)
Parser->>Parser: read YAML, validate -> AdminGroup
Parser-->>Server: AdminGroup
Server->>AccountsSvc: Initialize(ctx, AdminGroup)
AccountsSvc->>Store: Begin tx, delete existing admin groups/members
loop members
AccountsSvc->>Store: create member + group links
end
AccountsSvc->>Store: commit tx
AccountsSvc-->>Server: success
end
sequenceDiagram
participant Client
participant Handler
participant AuthzAccountsService
participant InnerAccountsService
participant Store
Client->>Handler: ListGroups request
Handler->>AuthzAccountsService: ListGroups(ctx)
AuthzAccountsService->>AuthzAccountsService: requireAdmin(ctx)
alt user is admin
AuthzAccountsService->>InnerAccountsService: ListGroups(ctx)
InnerAccountsService->>Store: query groups
Store-->>InnerAccountsService: groups
InnerAccountsService-->>AuthzAccountsService: groups
AuthzAccountsService-->>Handler: groups
Handler-->>Client: 200 OK
else user not admin
AuthzAccountsService-->>Handler: ErrForbidden
Handler-->>Client: 403 Forbidden
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/service/accounts.go (1)
3-13:⚠️ Potential issue | 🟡 MinorNormalize admin group values before validation.
Whitespace-only
name,username, or🛡️ Proposed validation tightening
import ( "context" "errors" "fmt" "os" + "strings" "github.com/google/uuid" "github.com/kubev2v/migration-planner/internal/auth" "github.com/kubev2v/migration-planner/internal/store" "github.com/kubev2v/migration-planner/internal/store/model" @@ if err := yaml.Unmarshal(data, &ag); err != nil { return nil, fmt.Errorf("parsing admin group file: %w", err) } + ag.Name = strings.TrimSpace(ag.Name) if ag.Name == "" { return nil, fmt.Errorf("admin group file: name is required") } if len(ag.Members) == 0 { return nil, fmt.Errorf("admin group file: at least one member is required") } - for i, m := range ag.Members { - if m.Username == "" { + for i := range ag.Members { + ag.Members[i].Username = strings.TrimSpace(ag.Members[i].Username) + ag.Members[i].Email = strings.TrimSpace(ag.Members[i].Email) + if ag.Members[i].Username == "" { return nil, fmt.Errorf("admin group file: member[%d] username is required", i) } - if m.Email == "" { + if ag.Members[i].Email == "" { return nil, fmt.Errorf("admin group file: member[%d] email is required", i) } }As per coding guidelines, “Input validation is mandatory.”
Also applies to: 41-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/accounts.go` around lines 3 - 13, Trim whitespace from admin group fields before performing required-field validation: in internal/service/accounts.go update the validation path that checks admin group "name", "username", and "email" so each value is normalized with strings.TrimSpace(...) prior to the existing non-empty checks (and add the strings import). Apply this normalization immediately after YAML/unmarshal or when constructing the admin group object so whitespace-only values fail validation the same as empty values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/handlers/v1alpha1/accounts_test.go`:
- Line 43: Update the tests in internal/handlers/v1alpha1/accounts_test.go to
exercise handler-level 403 mappings by replacing or augmenting the injected
service used to construct handlers.NewServiceHandler: instead of
service.NewAccountsService(s) inject an authorization-wrapped service
(service.NewAuthzAccountsService(...)) with a non-admin token context, or mock
the accounts service to return service.ErrForbidden for the group/member
methods; add test cases that call the handlers' ListGroups, CreateGroup,
GetGroup, UpdateGroup, DeleteGroup, ListGroupMembers, CreateGroupMember,
UpdateGroupMember, and RemoveGroupMember endpoints and assert they produce HTTP
403 responses when the underlying service returns ErrForbidden or the authz
wrapper denies access.
In `@internal/service/accounts_test.go`:
- Around line 502-556: Add validation for whitespace-only fields in
ParseAdminGroupFile: trim string fields before checking emptiness (e.g., ensure
ag.Name, each member.Username and member.Email are checked as
strings.TrimSpace(value) == "") and return the same error messages used for
empty strings. Also extend the tests in internal/service/accounts_test.go by
adding parallel cases to the existing ones that write whitespace-only values
(e.g., "name: \" \"", "username: \" \"", "email: \" \"", and "members: [ ]"
as appropriate) and assert the same error substrings; use the same test helpers
and filenames as the current tests to keep structure consistent.
- Around line 400-427: Add a new test (e.g., "replaces existing admin groups on
re-initialize when multiple match") that seeds two AdminGroup entries with the
same Name (using service.AdminGroup and service.AdminGroupMember) before calling
svc.Initialize(context.TODO(), ag2); after Initialize call use
svc.ListGroups(context.TODO(), nil) and assert there is a single group with the
new members and that no stale members from either of the original groups remain
(check group count, members length, and that members contain only the new
usernames like "bob" and "carol"). Ensure the test mirrors the structure of the
existing "replaces existing admin group on re-initialize" but creates two
initial groups to trigger removal of all matching stale groups.
In `@internal/service/accounts.go`:
- Around line 140-174: The Initialize method currently deletes the existing
admin group then creates a new group and members with separate store calls
(ListGroups, DeleteGroup, CreateGroup, CreateMember), which can leave startup in
a partial state and bypass member authz side effects; change this to a single
atomic operation in the accounts store (e.g., add a transactional method like
ReplaceAdminGroup or CreateOrReplaceAdminGroup) that performs the delete, create
group, and create-member (using the normal CreateMember/authz path) inside one
DB transaction and rolls back on any error, then call that new transactional
method from Initialize and propagate wrapped errors.
---
Outside diff comments:
In `@internal/service/accounts.go`:
- Around line 3-13: Trim whitespace from admin group fields before performing
required-field validation: in internal/service/accounts.go update the validation
path that checks admin group "name", "username", and "email" so each value is
normalized with strings.TrimSpace(...) prior to the existing non-empty checks
(and add the strings import). Apply this normalization immediately after
YAML/unmarshal or when constructing the admin group object so whitespace-only
values fail validation the same as empty values.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fd7894a8-4d18-491e-ab46-d81692ae2541
📒 Files selected for processing (13)
api/v1alpha1/openapi.yamlgo.modinternal/api/server/server.gen.gointernal/api_server/server.gointernal/config/config.gointernal/handlers/v1alpha1/accounts.gointernal/handlers/v1alpha1/accounts_test.gointernal/handlers/v1alpha1/partner_test.gointernal/handlers/v1alpha1/source.gointernal/service/accounts.gointernal/service/accounts_test.gointernal/service/authz_accounts.gointernal/service/partner_test.go
tupyy
force-pushed
the
auhtz-groups
branch
4 times, most recently
from
6bb8322 to
f282531
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
internal/service/accounts.go (1)
155-185:⚠️ Potential issue | 🟠 Major
Initializestill bypasses authz relationship bookkeeping for admin members (and leaves orphans on delete).Two consistency gaps with the regular
CreateMember/RemoveGroupMemberpaths (see lines 278‑284 and 354‑359 in this same file):
- Lines 161‑165: deleting a pre‑existing admin group via
s.store.Accounts().DeleteGroupdoes not issue aWithout(...)WriteRelationshipsfor its previous members, so stalememberReBAC tuples remain under that (now‑deleted) org resource.- Lines 177‑185:
s.store.Accounts().CreateMemberis called directly on the store, skipping the service‑levelCreateMemberwhich also writes theMemberRelationtuple vias.store.Authz().WriteRelationships. Newly bootstrapped admins therefore have no org membership relationship in the authz backend.
requireAdminusesGetIdentity(group Kind == "admin") so login still works, but any ReBAC check keyed onorg/member(user)for admins (e.g., future cross‑resource rules, consistency of the authz store) will be wrong. Since all writes here already run inside the transaction context, both fixes are cheap.🔒 Proposed fix
for _, g := range groups { + // Collect members first so we can drop their authz tuples after deletion. + oldMembers, err := s.store.Accounts().ListMembers(ctx, store.NewMemberQueryFilter().ByGroupID(g.ID)) + if err != nil { + return fmt.Errorf("listing members of admin group %s: %w", g.ID, err) + } if err := s.store.Accounts().DeleteGroup(ctx, g.ID); err != nil { return fmt.Errorf("deleting existing admin group %s: %w", g.ID, err) } + if len(oldMembers) > 0 { + b := store.NewRelationshipBuilder() + for _, om := range oldMembers { + b = b.Without(model.NewOrgResource(g.ID.String()), model.MemberRelation, model.NewUserSubject(om.Username)) + } + if err := s.store.Authz().WriteRelationships(ctx, b.Build()); err != nil { + return fmt.Errorf("removing authz relations for old admin group %s: %w", g.ID, err) + } + } } @@ for _, m := range adminGroup.Members { if _, err := s.store.Accounts().CreateMember(ctx, model.Member{ Username: m.Username, Email: m.Email, GroupID: group.ID, }); err != nil { return fmt.Errorf("adding admin member %s: %w", m.Username, err) } + updates := store.NewRelationshipBuilder(). + With(model.NewOrgResource(group.ID.String()), model.MemberRelation, model.NewUserSubject(m.Username)). + Build() + if err := s.store.Authz().WriteRelationships(ctx, updates); err != nil { + return fmt.Errorf("writing admin member authz relation %s: %w", m.Username, err) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/accounts.go` around lines 155 - 185, When initializing admin groups, ensure authz relationship bookkeeping is done: before calling s.store.Accounts().DeleteGroup for pre-existing admin groups, call s.store.Authz().WriteRelationships to remove the MemberRelation tuples for that group's members (matching what RemoveGroupMember does), and when adding bootstrapped members do not call s.store.Accounts().CreateMember directly — instead call the service-level CreateMember (the method on the service that wraps store account creation and writes the MemberRelation via s.store.Authz().WriteRelationships) or explicitly write the MemberRelation tuple after creating the member; both fixes should run in the same transaction already used by Initialize so the authz tuples stay consistent with the store.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy/templates/admin-group-template.yml`:
- Around line 19-24: The template in deploy/templates/admin-group-template.yml
hardcodes a single admin entry under data.admin-group.yaml (members: with
${ADMIN_USERNAME}/${ADMIN_EMAIL}) and a fixed name: admin, preventing bootstrap
of multiple admins or different group names; change the template to accept a
multi-line parameter (e.g., ADMIN_MEMBERS) and inline it under members: (or
iterate over indexed vars like ADMIN_USERNAME_1/ADMIN_EMAIL_1) so multiple
entries can be rendered, and expose a GROUP_NAME parameter to replace the
hardcoded name: admin; update the template references to data.admin-group.yaml,
members, and name to use these parameters so the configmap can bootstrap
multiple admins and arbitrary group names.
In `@internal/api_server/server.go`:
- Around line 175-199: There are multiple redundant AccountsServicer instances
and Initialize is being called on the authz-wrapped service; create a single
unwrapped accounts service (use service.NewAccountsService or
service.NewAccountsServicer consistently) and assign it to an innerAccountsSvc
variable, reuse that single instance when constructing partnerSvc and
assessmentSvc (pass innerAccountsSvc into
NewPartnerService/NewAssessmentService) and only wrap with
NewAuthzAccountsService for the runtime accountsSvc variable; when loading the
admin group via service.ParseAdminGroupFile call
innerAccountsSvc.Initialize(ctx, *adminGroup) (not the authz-wrapped
accountsSvc) so boot initialization does not require an auth user.
In `@internal/handlers/v1alpha1/accounts.go`:
- Around line 54-63: Replace all instances where a *service.ErrForbidden branch
returns server.*403JSONResponse with Message: err.Error() by logging the error
(logger.Error(err).Log()) and returning a static, non-sensitive message (e.g.,
"forbidden") instead; update the 403 handling in ListGroups (the shown switch),
CreateGroup, GetGroup, UpdateGroup (both calls), DeleteGroup (both calls),
ListGroupMembers, CreateGroupMember, UpdateGroupMember (both calls), and
RemoveGroupMember so they mirror the default branch logging pattern and return a
constant string rather than the raw err.Error().
In `@internal/service/accounts_test.go`:
- Around line 479-599: Collapse the nine ParseAdminGroupFile specs into a
table-driven test: extract the os.CreateTemp/WriteString/Close/Remove
boilerplate into a helper (e.g. writeTempYAML) that returns the temp filename
and uses DeferCleanup to remove the file, then replace the repeated It blocks
with a single DescribeTable that calls service.ParseAdminGroupFile on
writeTempYAML(content) and asserts the error contains the expected substring;
use Entry rows for each case (empty name, whitespace name, empty members,
empty/whitespace username, empty/whitespace email) to keep tests concise and
reference ParseAdminGroupFile, writeTempYAML, DescribeTable, Entry, and
DeferCleanup when applying the change.
In `@internal/service/accounts.go`:
- Around line 42-72: In ParseAdminGroupFile validate member uniqueness and email
shape before returning: after trimming fields in ParseAdminGroupFile, check for
duplicate usernames (e.g., build a map[string]bool keyed by
ag.Members[i].Username and return a descriptive error if a duplicate is found)
and validate each ag.Members[i].Email with net/mail.ParseAddress (return an
error for invalid addresses); reference ParseAdminGroupFile, AdminGroup, and
ag.Members to locate where to add these checks so misconfigurations are caught
before CreateMember/Initialize DB calls.
- Around line 155-156: The current ListGroups call filters by both
ByName(adminGroup.Name) and ByCompany(adminGroup.Name) which ties admin groups
to company==name and prevents reconciling groups that share a Name but have a
different Company; update the call in Initialize (where
s.store.Accounts().ListGroups is invoked) to remove the
ByCompany(adminGroup.Name) filter so the lookup uses only ByName(...) and
ByKind("admin") unless the invariants require company==name — if that invariant
is intended instead add a one-line comment near this call (and near the admin
group creation) documenting that admin groups always set Company == Name so
future readers know this is deliberate.
In `@internal/service/authz_accounts.go`:
- Around line 21-23: The Initialize method on AuthzAccountsService bypasses
requireAdmin and must be documented and protected: add a clear doc comment on
AuthzAccountsService.Initialize stating it is bootstrap-only (called from
internal/api_server/server.go before HTTP serving), must not be called from
request handlers, and intentionally skips authz; additionally add a runtime
guard that panics or returns an error if Initialize is called when auth context
indicates an active request user (use auth.MustHaveUser or similar) OR
remove/move Initialize off the public AccountsServicer interface into a separate
bootstrap-only interface or concrete type so handlers cannot call it.
- Around line 99-108: Update AuthzAccountsService.requireAdmin to accept a
resource string and use the KindAdmin constant instead of the literal "admin":
call auth.MustHaveUser(ctx), use a.inner.GetIdentity as before, compare
identity.Kind to KindAdmin, and when not admin call NewErrForbidden(resource,
user.Username). Then update every call site (ListGroups, GetGroup, CreateGroup,
UpdateGroup, DeleteGroup, GetMember, ListGroupMembers, CreateMember,
UpdateGroupMember, RemoveGroupMember) to pass the appropriate resource label
("groups" for group ops, "members" or the correct member label for member ops).
Ensure imports/reference to KindAdmin (IdentityKind) are used from accounts.go.
In `@Makefile`:
- Line 223: The deploy-on-openshift and deploy-on-kind targets pass
MIGRATION_PLANNER_ADMIN_GROUP_FILE into service-template.yml but never create
the migration-planner-admin-group ConfigMap, so when
MIGRATION_PLANNER_ADMIN_GROUP_FILE is non-empty the pod crashes at startup;
update those Makefile targets to apply deploy/templates/admin-group-template.yml
(using the same templating/apply flow as deploy/e2e.mk) before applying
service-template.yml so the ConfigMap exists, ensuring you still pass
MIGRATION_PLANNER_ADMIN_GROUP_FILE into the template; alternatively, add a clear
note in the target help that callers must pre-create the
migration-planner-admin-group ConfigMap when MIGRATION_PLANNER_ADMIN_GROUP_FILE
is set.
---
Duplicate comments:
In `@internal/service/accounts.go`:
- Around line 155-185: When initializing admin groups, ensure authz relationship
bookkeeping is done: before calling s.store.Accounts().DeleteGroup for
pre-existing admin groups, call s.store.Authz().WriteRelationships to remove the
MemberRelation tuples for that group's members (matching what RemoveGroupMember
does), and when adding bootstrapped members do not call
s.store.Accounts().CreateMember directly — instead call the service-level
CreateMember (the method on the service that wraps store account creation and
writes the MemberRelation via s.store.Authz().WriteRelationships) or explicitly
write the MemberRelation tuple after creating the member; both fixes should run
in the same transaction already used by Initialize so the authz tuples stay
consistent with the store.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a4d0dd60-ba20-4442-8906-01a6521f92e2
📒 Files selected for processing (19)
Makefileapi/v1alpha1/openapi.yamlapi/v1alpha1/spec.gen.godeploy/e2e.mkdeploy/templates/admin-group-template.ymldeploy/templates/service-template.ymlgo.modinternal/api/client/client.gen.gointernal/api/server/server.gen.gointernal/api_server/server.gointernal/config/config.gointernal/handlers/v1alpha1/accounts.gointernal/handlers/v1alpha1/accounts_test.gointernal/handlers/v1alpha1/partner_test.gointernal/handlers/v1alpha1/source.gointernal/service/accounts.gointernal/service/accounts_test.gointernal/service/authz_accounts.gointernal/service/partner_test.go
There was a problem hiding this comment.
♻️ Duplicate comments (2)
internal/service/accounts.go (1)
192-200:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInitialize bypasses member authz relationship writes.
At Line 193, members are inserted directly via
s.store.Accounts().CreateMember(...), but the authz relationship side effect used inCreateMember(Lines 293-299) is not executed. This can leave initialized admin members without expected authorization bindings.🔧 Suggested fix
for _, m := range adminGroup.Members { if _, err := s.store.Accounts().CreateMember(ctx, model.Member{ Username: m.Username, Email: m.Email, GroupID: group.ID, }); err != nil { return fmt.Errorf("adding admin member %s: %w", m.Username, err) } + + updates := store.NewRelationshipBuilder(). + With(model.NewOrgResource(group.ID.String()), model.MemberRelation, model.NewUserSubject(m.Username)). + Build() + if err := s.store.Authz().WriteRelationships(ctx, updates); err != nil { + return fmt.Errorf("adding admin member authz relation %s: %w", m.Username, err) + } }🤖 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 `@internal/service/accounts.go` around lines 192 - 200, The loop is calling the low-level s.store.Accounts().CreateMember(...) which inserts members but skips the authz-relationship side effect implemented in the CreateMember code path (the authz write at the CreateMember block around lines 293-299), leaving admin members without required authorization bindings; fix by invoking the higher-level service method that performs both account creation and authz wiring (i.e., call the service-level CreateMember wrapper instead of s.store.Accounts().CreateMember), or if no wrapper exists, after creating the member call the same authz relationship write used in CreateMember (the authz-relations writer function used in that block) to attach the member to the admin group so the authz binding is always created during initialization.internal/handlers/v1alpha1/accounts.go (1)
56-62:⚠️ Potential issue | 🟠 Major | ⚡ Quick win403 branches still leak
err.Error()and bypass logging.This was raised previously and remains unaddressed across all twelve
*service.ErrForbiddenbranches in this file (Lines 58, 92, 117, 153, 167, 192, 204, 227, 257, 272, 300, 333). Two concrete problems:
NewErrForbidden(resource, username)formats the requesting username (and resource label) into the message, soMessage: err.Error()exposes those identifiers in 403 bodies served to unauthenticated/unauthorized callers — this is a needless PII/auth-state leak.- All other branches in the same switches return static messages (e.g.,
"group not found","group already exists","empty body") and thedefaultbranch logs vialogger.Error(err).Log(). The 403 branches uniquely skip both, so authz denials are inconsistent with sibling responses and invisible in logs.Suggested shape (apply identically to every 403 branch):
🛡️ Proposed fix
- case *service.ErrForbidden: - return server.ListGroups403JSONResponse{Message: err.Error()}, nil + case *service.ErrForbidden: + logger.Error(err).Log() + return server.ListGroups403JSONResponse{Message: "forbidden"}, nilAlso applies to Lines 92, 117, 153, 167, 192, 204, 227, 257, 272, 300, 333.
🤖 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 `@internal/handlers/v1alpha1/accounts.go` around lines 56 - 62, The 403 branches currently return err.Error() (leaking usernames/resource identifiers) and skip logging; for every switch branch that checks for *service.ErrForbidden and returns a server.*403JSONResponse (e.g., ListGroups403JSONResponse and the eleven other 403 handlers), replace the dynamic Message: err.Error() with a static, generic message such as "forbidden" or "unauthorized" and add a logger.Error(err).Log() (or equivalent logging call) immediately before returning so the original error is recorded server-side; keep all other response shapes unchanged so behavior matches the sibling default error branches.
🤖 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.
Duplicate comments:
In `@internal/handlers/v1alpha1/accounts.go`:
- Around line 56-62: The 403 branches currently return err.Error() (leaking
usernames/resource identifiers) and skip logging; for every switch branch that
checks for *service.ErrForbidden and returns a server.*403JSONResponse (e.g.,
ListGroups403JSONResponse and the eleven other 403 handlers), replace the
dynamic Message: err.Error() with a static, generic message such as "forbidden"
or "unauthorized" and add a logger.Error(err).Log() (or equivalent logging call)
immediately before returning so the original error is recorded server-side; keep
all other response shapes unchanged so behavior matches the sibling default
error branches.
In `@internal/service/accounts.go`:
- Around line 192-200: The loop is calling the low-level
s.store.Accounts().CreateMember(...) which inserts members but skips the
authz-relationship side effect implemented in the CreateMember code path (the
authz write at the CreateMember block around lines 293-299), leaving admin
members without required authorization bindings; fix by invoking the
higher-level service method that performs both account creation and authz wiring
(i.e., call the service-level CreateMember wrapper instead of
s.store.Accounts().CreateMember), or if no wrapper exists, after creating the
member call the same authz relationship write used in CreateMember (the
authz-relations writer function used in that block) to attach the member to the
admin group so the authz binding is always created during initialization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d67ca813-025f-43d2-912a-6648dc78c70e
📒 Files selected for processing (19)
Makefileapi/v1alpha1/openapi.yamlapi/v1alpha1/spec.gen.godeploy/e2e.mkdeploy/templates/admin-group-template.ymldeploy/templates/service-template.ymlgo.modinternal/api/client/client.gen.gointernal/api/server/server.gen.gointernal/api_server/server.gointernal/config/config.gointernal/handlers/v1alpha1/accounts.gointernal/handlers/v1alpha1/accounts_test.gointernal/handlers/v1alpha1/partner_test.gointernal/handlers/v1alpha1/source.gointernal/service/accounts.gointernal/service/accounts_test.gointernal/service/authz_accounts.gointernal/service/partner_test.go
tupyy
force-pushed
the
auhtz-groups
branch
2 times, most recently
from
900971a to
d5ad34b
Compare
This commit add auhtz to group endpoint. Also, it initiliaze the admin
group from an file:
```
name: platform-admins
members:
- username: alice
email: alice@redhat.com
- username: bob
email: bob@redhat.com
```
Signed-off-by: Cosmin Tupangiu <cosmin@redhat.com>
Jos-Jerus
reviewed
May 6, 2026
| ) | ||
|
|
||
| type AccountsServicer interface { | ||
| Initialize(ctx context.Context, adminGroup AdminGroup) error |
Collaborator
There was a problem hiding this comment.
do we really need Initialize on the AccountsServicer interface? its only called from server.go on boot where we already have innerAccountsSvc directly, so no reason to expose it on the interface where any handler could call it by mistake
| @@ -140,6 +152,9 @@ func (h *ServiceHandler) UpdateGroup(ctx context.Context, request server.UpdateG | |||
| existing, err := h.accountsSrv.GetGroup(ctx, request.Id) | |||
Collaborator
There was a problem hiding this comment.
on UpdateGroup the handler calls GetGroup then UpdateGroup on the authz wrapper so requireAdmin runs twice and we hit the DB for the same identity check on the same request. same thing on DeleteGroup. maybe worth having the handler use innerAccountsSvc for the initial read or combine it somehow
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nirarg The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
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
3 participants
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.
This PR adds authorization to the group endpoint. Also, it initializes the admin group from a file:
Signed-off-by: Cosmin Tupangiu cosmin@redhat.com
Summary by CodeRabbit
New Features
API Changes
Deployment / Chores
Tests