v1: expose groups in user customization (HMS-4901) #1385
Conversation
croissanne
force-pushed
the
wheel-groups
branch
2 times, most recently
from
a93b13c
to
b6f1604
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
|
| @@ -751,13 +751,12 @@ func (h *Handlers) buildCustomizations(ctx echo.Context, cr *ComposeRequest, d * | |||
| if cust.Users != nil { | |||
| var users []composer.User | |||
| for _, u := range *cust.Users { | |||
| groups := &[]string{"wheel"} | |||
There was a problem hiding this comment.
I guess I'm stating the obvious here but I will do it anyway. Unless there is somewhere adding "wheel" to u.Group automatically this is a behavior change - I guess it's okay because the frontend is aware and takes care of this and we have no other consumers? Might still be nice to mention it in the commit message maybe?
There was a problem hiding this comment.
Fair point, I'll do that :)
There was a problem hiding this comment.
This is a chance to improve the API docs.
I am assuming that the code currently works that for a user named test a primary group test is added and now the new field allows customization of secondary (also known as supplementary) groups.
I suggest to document all of that, details do matter when it comes to security.
@mvo5 has a point, tho I think it is okay since the patch is actually removing a group. If this was the opposite, then it would have been much bigger problem.
croissanne
force-pushed
the
wheel-groups
branch
2 times, most recently
from
5074701
to
6de88f5
Compare
When adding a user, immediately add it to the specified groups. This patch technically includes a behavioural change (the wheel group will no longer be added by default). But the main consumers of this api (the edge-api) already add the wheel group each time, and the api doesn't have `additionalProperties` disabled for the users customization.
croissanne
force-pushed
the
wheel-groups
branch
from
6de88f5
to
65cb02d
Compare
|
/retest |
1 similar comment
|
/retest |
When adding a user, immediately add it to the specified groups.
It should be fine to drop the default as the edge-api always adds the groups already
https://github.com/RedHatInsights/edge-api/blob/622244f04989159709332e67788c04296989e431/pkg/clients/imagebuilder/client.go#L353-L358