Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(PXP-8992): PATCH /user/{username} #140

Merged
merged 8 commits into from
Nov 30, 2021
4 changes: 2 additions & 2 deletions .secrets.baseline
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"files": "go.sum",
"lines": null
},
"generated_at": "2021-11-09T20:08:18Z",
"generated_at": "2021-11-29T14:09:22Z",
"plugins_used": [
{
"name": "AWSKeyDetector"
Expand Down Expand Up @@ -62,7 +62,7 @@
{
"hashed_secret": "f9fdc64928c96c7ad56bf7da557f70345d83a6ed",
"is_verified": false,
"line_number": 1615,
"line_number": 1643,
"type": "Base64 High Entropy String"
}
],
Expand Down
37 changes: 35 additions & 2 deletions arborist/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ func (server *Server) MakeRouter(out io.Writer) http.Handler {
router.Handle("/user", http.HandlerFunc(server.handleUserList)).Methods("GET")
router.Handle("/user", http.HandlerFunc(server.parseJSON(server.handleUserCreate))).Methods("POST")
router.Handle("/user/{username}", http.HandlerFunc(server.handleUserRead)).Methods("GET")
router.Handle("/user/{username}", http.HandlerFunc(server.parseJSON(server.handleUserUpdate))).Methods("PATCH")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel this /user API is a little odd using username as the id, since it's mutable now. I don't think there's much to do at this point b/c we don't want to rework the whole API... but we'll want to watch out b/c now clients must update their internal representation of username only on a successful PATCH (which is fine and makes sense, but it leaves room for some weird errors where a client might keep trying to patch something that's already been updated and starts getting errors b/c the username they're trying to patch doesn't exist anymore).

Maybe they didn't parse the success from Arborist right or persist it or something. Anything, since this is a new API and we are the client in most cases, this is fine. Just thinking out loud

router.Handle("/user/{username}", http.HandlerFunc(server.handleUserDelete)).Methods("DELETE")
router.Handle("/user/{username}/policy", http.HandlerFunc(server.parseJSON(server.handleUserGrantPolicy))).Methods("POST")
router.Handle("/user/{username}/bulk/policy", http.HandlerFunc(server.parseJSON(server.handleBulkUserGrantPolicy))).Methods("POST") // NEW bulk grant policy
Expand Down Expand Up @@ -660,8 +661,8 @@ func (server *Server) handlePolicyList(w http.ResponseWriter, r *http.Request) {
// generate expanded policies with role details
for _, policy := range policies {
expandedPolicy := ExpandedPolicy{
Name: policy.Name,
Description: policy.Description,
Name: policy.Name,
Description: policy.Description,
ResourcePaths: policy.ResourcePaths,
}
roles := []Role{}
Expand Down Expand Up @@ -1183,6 +1184,38 @@ func (server *Server) handleUserRead(w http.ResponseWriter, r *http.Request) {
_ = jsonResponseFrom(user, http.StatusOK).write(w, r)
}

func (server *Server) handleUserUpdate(w http.ResponseWriter, r *http.Request, body []byte) {
name := mux.Vars(r)["username"]
user := User{Name: name}

userWithScalars := &UserWithScalars{}
err := json.Unmarshal(body, userWithScalars)
if err != nil {
msg := fmt.Sprintf("could not unmarshal body: %s", err.Error())
errResponse := newErrorResponse(msg, 400, nil)
errResponse.log.write(server.logger)
_ = errResponse.write(w, r)
return
}

if userWithScalars.Name == nil && userWithScalars.Email == nil {
msg := `body must contain at least one valid field. possible valid fields are "name" and "email"`
errResponse := newErrorResponse(msg, 400, nil)
errResponse.log.write(server.logger)
_ = errResponse.write(w, r)
return
}

errResponse := user.updateInDb(server.db, userWithScalars.Name, userWithScalars.Email)
if errResponse != nil {
errResponse.log.write(server.logger)
_ = errResponse.write(w, r)
return
}
server.logger.Info("updated user %s", user.Name)
_ = jsonResponseFrom(nil, http.StatusNoContent).write(w, r)
}

func (server *Server) handleUserDelete(w http.ResponseWriter, r *http.Request) {
name := mux.Vars(r)["username"]
user := User{Name: name}
Expand Down
108 changes: 108 additions & 0 deletions arborist/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,34 @@ func TestServer(t *testing.T) {
}
}

updateUserBytes := func(t *testing.T, username string, body []byte, expectedHTTPCode int) {
w := httptest.NewRecorder()
url := fmt.Sprintf("/user/%s", username)
req := newRequest("PATCH", url, bytes.NewBuffer(body))
handler.ServeHTTP(w, req)
assert.Equalf(t, expectedHTTPCode, w.Code, "Wanted http response: %v \t Got: %v", expectedHTTPCode, w.Code)
}

assertUsernameAndEmail := func(t *testing.T, expectedUsername string, expectedUserEmail string) {
w := httptest.NewRecorder()
url := fmt.Sprintf("/user/%s", expectedUsername)
req := newRequest("GET", url, nil)
handler.ServeHTTP(w, req)
if w.Code != http.StatusOK {
httpError(t, w, "couldn't read user")
}
result := struct {
Name string `json:"name"`
Email string `json:"email"`
}{}
err = json.Unmarshal(w.Body.Bytes(), &result)
if err != nil {
httpError(t, w, "couldn't read response from user read")
}
assert.Equalf(t, expectedUsername, result.Name, "Wanted username: %v \t Got: %v", expectedUsername, result.Name)
assert.Equalf(t, expectedUserEmail, result.Email, "Wanted email: %v \t Got: %v", expectedUserEmail, result.Email)
}

createResourceBytes := func(t *testing.T, body []byte) {
w := httptest.NewRecorder()
req := newRequest("POST", "/resource", bytes.NewBuffer(body))
Expand Down Expand Up @@ -1688,6 +1716,86 @@ func TestServer(t *testing.T) {
assert.ElementsMatchf(t, expectedGroups, result.Groups, "Wanted groups: %v \t Got: %v", expectedGroups, result.Groups)
})

t.Run("Update", func(t *testing.T) {
originalUsername := "johnsmith"
originalUserEmail := "johnsmith@domain.tld"
createUserBytes(t, []byte(fmt.Sprintf(`{"name": "%s", "email": "%s"}`, originalUsername, originalUserEmail)))

t.Run("OnlyName", func(t *testing.T) {
newUsername := "jsmith"
updateUserBytes(
t,
originalUsername,
[]byte(fmt.Sprintf(`{"name": "%s"}`, newUsername)),
http.StatusNoContent,
)
assertUsernameAndEmail(t, newUsername, originalUserEmail)
})

originalUsername = "jsmith"
t.Run("OnlyEmail", func(t *testing.T) {
newUserEmail := "jsmith@domain.tld"
updateUserBytes(
t,
originalUsername,
[]byte(fmt.Sprintf(`{"email": "%s"}`, newUserEmail)),
http.StatusNoContent,
)
assertUsernameAndEmail(t, originalUsername, newUserEmail)
})

t.Run("BothNameAndEmail", func(t *testing.T) {
newUsername := "janesmith"
newUserEmail := "janesmith@domain.tld"
updateUserBytes(
t,
originalUsername,
[]byte(fmt.Sprintf(`{"name": "%s", "email": "%s"}`, newUsername, newUserEmail)),
http.StatusNoContent,
)
assertUsernameAndEmail(t, newUsername, newUserEmail)
})

originalUsername = "janesmith"
t.Run("NeitherNameNorEmail", func(t *testing.T) {
updateUserBytes(
t,
originalUsername,
[]byte("{}"),
http.StatusBadRequest,
)
})

t.Run("InvalidName", func(t *testing.T) {
updateUserBytes(
t,
originalUsername,
[]byte(`{"name": 42}`),
http.StatusBadRequest,
)
})

t.Run("NonExistentUsername", func(t *testing.T) {
updateUserBytes(
t,
"nonexistentuser",
[]byte(`{"name": "existentuser"}`),
http.StatusNotFound,
)
})

t.Run("ConflictingName", func(t *testing.T) {
otherUsername := "joesmith"
createUserBytes(t, []byte(fmt.Sprintf(`{"name": "%s"}`, otherUsername)))
updateUserBytes(
t,
originalUsername,
[]byte(fmt.Sprintf(`{"name": "%s"}`, otherUsername)),
http.StatusConflict,
)
})
})

// do some preliminary setup so we have a policy to work with
createResourceBytes(t, resourceBody)
createRoleBytes(t, roleBody)
Expand Down
32 changes: 32 additions & 0 deletions arborist/user.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@ type User struct {
Policies []PolicyBinding `json:"policies"`
}

type UserWithScalars struct {
Name *string `json:"name,omitempty"`
Email *string `json:"email,omitempty"`
}

func (user *User) UnmarshalJSON(data []byte) error {
fields := make(map[string]interface{})
err := json.Unmarshal(data, &fields)
Expand Down Expand Up @@ -210,6 +215,33 @@ func (user *User) createInDb(db *sqlx.DB) *ErrorResponse {
return nil
}

func (user *User) updateInDb(db *sqlx.DB, name *string, email *string) *ErrorResponse {
stmt := `
UPDATE usr
SET
name = COALESCE($1, name),
email = COALESCE($2, email)
WHERE
name = $3
`
result, err := db.Exec(stmt, name, email, user.Name)
if err != nil {
// this should only fail because the target name was not unique
msg := fmt.Sprintf(`failed to update name to "%s": user with this name already exists`, *name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if a user with name = $3 doesn't exist at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case that name = $3 doesn't exist at all, the SQL query still executes successfully, but rowsAffected below is 0 and a 404 is returned.

return newErrorResponse(msg, 409, &err)
}

rowsAffected, _ := result.RowsAffected()
if rowsAffected == 0 {
msg := fmt.Sprintf(
"failed to update user: user does not exist: %s",
user.Name,
)
return newErrorResponse(msg, 404, nil)
}
return nil
}

func (user *User) deleteInDb(db *sqlx.DB) *ErrorResponse {
stmt := "DELETE FROM usr WHERE name = $1"
_, err := db.Exec(stmt, user.Name)
Expand Down
28 changes: 28 additions & 0 deletions docs/openapi.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -845,6 +845,25 @@ paths:
$ref: '#/components/schemas/User'
404:
description: user not found
patch:
tags:
- user
description: >-
Update either the name, email or both for a specific user.
requestBody:
content:
application/json:
schema:
$ref: '#/components/schemas/UserWithScalars'
responses:
204:
description: user successfully updated
400:
description: invalid input
404:
description: user not found
409:
description: user with target name already exists
delete:
tags:
- user
Expand Down Expand Up @@ -1599,6 +1618,15 @@ components:
expires_at:
type: string
example: '2019-08-12T12:34:56Z'
UserWithScalars:
type: object
properties:
name:
type: string
example: 'username'
email:
type: string
example: 'user@example.net'
UsersList:
type: object
properties:
Expand Down