Modify User (as a regular user) fails #891 by murphybytes · Pull Request #959 · kolide/fleet

murphybytes · 2017-01-13T22:56:51Z

No description provided.

groob · 2017-01-14T17:53:47Z

Shouldn't validateModifyUser be deleted now? With the changes that @zwass added to change password and the removal of this function call in this PR, it looks like dead code to me. Let's remove it.

Other than that, the change LGTM.

groob · 2017-01-14T21:39:29Z

using the canPerformactions Middleware allows the user to modify any of the properties in the ModifyUser method. https://github.com/kolide/kolide-ose/blob/3802f3098ecd3c0a5fab7b955e4ff71803dbfddc/server/service/service_users.go#L52

This would include some very undesirable security issues such as allowing a user to disable their own account and promote themselves to Admin.

Either update the validateModifyUser endpoint and use that, or introduce separate endpoints for security sensitive modifications on a user. I think @zwass had plans to do the latter.

groob

implement changes in the comments above

murphybytes · 2017-01-16T18:05:22Z

After discussing with @groob I added two new endpoints that allow admin users to set/unset admin and enabled status for users. Not the id in the url is the id of the user whose admin or enabled status will be changed. This simplifies the authorization of these operations.

{ "enabled": trueorfalse}
/api/v1/kolide/users/{id}/enable
{"admin": trueorfalse}
/api/v1/kolide/users/{id}/admin

If operations succeed an empty response is returned. If a user isn't an admin the http status is 503, otherwise a standard error structure is returned describing what went horribly awry.

groob · 2017-01-16T18:40:05Z

The new endpoints should not use PATCH, use POST instead.

groob · 2017-01-16T18:41:54Z

If you're returning an empty response, make sure the code is 204 NO Content.

IMO it would be more consistent to return the modified user in the response.

marpaia · 2017-01-16T18:46:03Z

+	SaveUserEnabledFuncInvoked bool
+}
+
+func (s *UserStore) SaveUserAdmin(id uint, isAdmin bool) error {


Was this generated?

This file is for generated code. There's a helpers file for handwritten methods.

murphybytes · 2017-01-16T19:56:04Z

@groob I'd favor returning the request with the new values instead of the whole user object. The reason being is that fetching the user would entail another sql select request and a network round trip for nothing. Admittedly, it's not a huge cost, but essentially it's burning cycles for nothing.

groob · 2017-01-16T20:06:24Z

 	SaveUser(user *User) error
+	// SaveUserAdmin sets the value of the user admin field for user identified
+	// by id.
+	SaveUserAdmin(id uint, isAdmin bool) error


These methods should not exist. Just use SaveUser

groob · 2017-01-17T00:57:54Z


 type SaveUserFunc func(user *kolide.User) error

+type SaveUserAdminFunc func(id uint, isAdmin bool) error


Need to remove these methods. they're not in the PR anymore.

groob · 2017-01-17T01:01:57Z

+		payload := kolide.UserPayload{
+			Admin: &req.Admin,
+		}
+		user, err := svc.ModifyUser(ctx, req.ID, payload)


These endpoints can't use the ModifyUser method on service as they're shared by the ModifyUser endpoint, which has different authorization.

The service methods must exist and be different. Only the datastore method for SaveUser can be shared. Sorry for the confusion.

groob · 2017-01-17T14:10:55Z

-			Admin: &req.Admin,
-		}
-		user, err := svc.ModifyUser(ctx, req.ID, payload)
+		user, err := svc.User(ctx, req.ID)


The endpoint factory function is for creating an endpoint, not for business logic. Any app changes should happen within the service layer. This might sound odd but the separation of concerns allows us to cleanly add metrics, logging, validation and so on.

I'd revert this commit and create two service methods for the actions you're adding.

groob

a few more small nits.

groob · 2017-01-17T14:55:21Z


 	// ModifyUser updates a user's parameters given a UserPayload.
 	ModifyUser(ctx context.Context, userID uint, p UserPayload) (user *User, err error)
+	// ChangeUserAdmin is used to modify the admin state of the user identified by id.


Add spacing like the rest of the documented methods on this service.

groob · 2017-01-17T14:59:46Z

 	// ModifyUser updates a user's parameters given a UserPayload.
 	ModifyUser(ctx context.Context, userID uint, p UserPayload) (user *User, err error)
+	// ChangeUserAdmin is used to modify the admin state of the user identified by id.
+	ChangeUserAdmin(id uint, isAdmin bool) (*User, error)


add the ctx context.Context method as a first param to all service methods. We populate context with information like who is making the request.

Finally, add the user and err params for the return types. I am only asking for this last one because when working with the types package to do codegen/refactoring, it helps to have the variable names.

groob · 2017-01-17T15:04:01Z

+	// ChangeUserAdmin is used to modify the admin state of the user identified by id.
+	ChangeUserAdmin(id uint, isAdmin bool) (*User, error)
+	// ChangeUserEnabled is used to enable/disable the user identified by id.
+	ChangeUserEnabled(id uint, isEnabled bool) (*User, error)


in addition to implementing these methods on service , implement them with the loggingMiddleware and metricsMiddleware types in the service package.

groob

Address the review comment regarding 1 line spacing above documented methods. the rest looks good.

The ability to modify a users admin and enabled status was erroneously left in place during development of #959. To mitigate a privilege escalation vulnerability we need to ensure those values can only be modified through the explicit methods. This patch includes a unit test and fix for the vulnerability. Thanks to 'Quikke' for submitting this vulnerability.

Modify User (as a regular user) fails #891

048226a

murphybytes added Bug Needs Review labels Jan 13, 2017

murphybytes requested review from groob and marpaia January 13, 2017 22:56

groob suggested changes Jan 14, 2017

View reviewed changes

murphybytes added 3 commits January 17, 2017 00:35

WIP tests pass after adding endpoints

563ddf0

Created two new endpoints for setting user admin and enabled

6ceedc1

Removed dead code

0036a55

Added datastore tests for storage of user enabled and admin

8b7e3ce

marpaia reviewed Jan 16, 2017

View reviewed changes

Code review changes

11e208f

groob reviewed Jan 16, 2017

View reviewed changes

Added user to response, using ModifyUser service

dea1df4

groob suggested changes Jan 17, 2017

View reviewed changes

groob reviewed Jan 17, 2017

View reviewed changes

Moved logic from endpoint to service

6ea538f

groob reviewed Jan 17, 2017

View reviewed changes

Added logging and metrics for new user endpoints

9b0089d

groob approved these changes Jan 17, 2017

View reviewed changes

added whitespace above comments

b0132b6

murphybytes merged commit 56b8772 into kolide:master Jan 17, 2017

murphybytes deleted the bug_891 branch January 17, 2017 16:44

groob mentioned this pull request Jan 17, 2017

Modify User (as a regular user) fails #891

Closed

3 tasks


		type SaveUserFunc func(user *kolide.User) error

		type SaveUserAdminFunc func(id uint, isAdmin bool) error

Conversation

murphybytes commented Jan 13, 2017

Uh oh!

groob commented Jan 14, 2017

Uh oh!

groob commented Jan 14, 2017

Uh oh!

groob left a comment

Choose a reason for hiding this comment

Uh oh!

murphybytes commented Jan 16, 2017

Uh oh!

groob commented Jan 16, 2017

Uh oh!

groob commented Jan 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

murphybytes commented Jan 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

groob left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

groob left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants