Update PUT user/current to work with v4 User Roles #6389

tcfdev · 2021-12-01T21:30:20Z

Closes: #6367

The user/current PUT requests appeared to be failing on v4 API calls. However after testing I was able to successfully change a current user's information, but the response did not display said information. This will fix the response to ensure the correct information is returned.

Additionally, a check was added to ensure the request was wrapped in a legacy user{} object.

Lastly, the API integration tests were checking for values to update by issuing an update (PUT) request then performing a read (GET) request. This appears to work as intended for all API versions, it was only the API response for v4.0 that was incorrect.

Which Traffic Control components are affected by this PR?

Traffic Ops

What is the best way to verify this PR?

Ensure a valid user is created via the API with appropriate role (say operations):

curl --request POST \
  --url https://<traffic_ops_host>/api/4.0/users \
  --header 'Content-Type: application/json' \
  --data '{
	"addressLine1":"1234 Somewhere ln",
	"city":"Manhattan",
	"company":"Stihl",
	"confirmLocalPasswd":"passwdops",
	"country":"USA",
	"email":"[email protected]",
	"fullName":"John",
	"localPasswd":"passwdops",
	"phoneNumber":"555-123-4567",
	"postalCode":"10002",
	"role":"operations",
	"stateOrProvince":"New York",
	"username":"operations",
	"tenantId":1
}'

Then log in with said user:

curl --request POST \
  --url https://<traffic_ops_host>/api/4.0/user/login \
  --header 'Content-Type: application/json' \
  --data '{"u":"operations","p":"passwdops"}'

Send GET request to user/current to ensure correct user is returned

Request:

curl --request GET \
  --url https://<traffic_ops_host>/api/4.0/user/current/

Response:

{
  "response": {
    "username": "operations",
    "localUser": true,
    "addressLine1": "1234 Somewhere ln",
    "addressLine2": null,
    "city": "Manhattan",
    "company": "Stihl",
    "country": "USA",
    "email": "[email protected]",
    "fullName": "John",
    "gid": null,
    "id": 89,
    "newUser": false,
    "phoneNumber": "555-123-4567",
    "postalCode": "10002",
    "publicSshKey": null,
    "role": "operations",
    "stateOrProvince": "New York",
    "tenant": "root",
    "tenantId": 1,
    "uid": null,
    "lastUpdated": "2021-12-01T14:13:14.261472-07:00",
    "lastAuthenticated": "2021-12-01T14:13:14.261472-07:00"
  }
}

Send PUT request to user/current with editable fields modified (say addressLine1, city, country, postalCode for example):

curl --request PUT \
  --url https://<traffic_ops_host>/api/4.0/user/current \
  --header 'Content-Type: application/json' \
  --data '{
	"user": {
		"username": "operations",
		"addressLine1": "5432 Somewhere ln",
		"addressLine2": null,
		"city": "Denver",
		"company": "Stihl",
		"country": "US",
		"email": "[email protected]",
		"fullName": "John",
		"id": 89,
		"phoneNumber": "555-123-4567",
		"postalCode": "80221",
		"publicSshKey": null,
		"role": "operations",
		"stateOrProvince": "New York",
		"tenant": "root",
		"tenantId": 1
	}
}'

Verify fields were updated correctly in the response:

{
  "alerts": [
    {
      "text": "User profile was successfully updated",
      "level": "success"
    }
  ],
  "response": {
    "addressLine1": "5432 Somewhere ln",
    "addressLine2": null,
    "changeLogCount": null,
    "city": "Denver",
    "company": "Stihl",
    "country": "US",
    "email": "[email protected]",
    "fullName": "John",
    "gid": null,
    "id": 89,
    "lastAuthenticated": null,
    "lastUpdated": "2021-12-01T14:13:14.261472-07:00",
    "newUser": false,
    "phoneNumber": "555-123-4567",
    "postalCode": "80221",
    "publicSshKey": null,
    "registrationSent": null,
    "role": "operations",
    "stateOrProvince": "New York",
    "tenant": "root",
    "tenantId": 1,
    "uid": null,
    "username": "operations"
  }
}

Verify updated fields are also returned in GET user/current as well as GET users/:id

Perform the same above tests for API 3.0, BUT be aware that the role field must be a valid ID, not a string with API 3.0.

Lastly, attempt to send the body for a PUT request to user/current without the user containing object. This can be done with either v4.0 or v3.x of the API. An appropriate error message should be returned.

Request:

curl --request PUT \
  --url https://<traffic_ops_host>/api/3.0/user/current \
  --header 'Content-Type: application/json' \
  --data '{
		"username": "operations",
		"addressLine1": "5432 Somewhere ln",
		"addressLine2": null,
		"city": "Denver",
		"company": "Stihl",
		"country": "US",
		"email": "[email protected]",
		"fullName": "John",
		"id": 89,
		"phoneNumber": "555-123-4567",
		"postalCode": "80212",
		"publicSshKey": null,
		"role": 2,
		"stateOrProvince": "Colorado",
		"tenant": "root",
		"tenantId": 1
}'

Response:

{
  "alerts": [
    {
      "text": "missing required 'user' object",
      "level": "error"
    }
  ]
}

PR submission checklist

This PR has tests - API tests exist and do validate functionality, only the v4.0 API response was invalid.
This PR has documentation - No changes to documentation were necessary
This PR has a CHANGELOG.md entry
This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY (see the Apache Software Foundation's security guidelines for details)

mitchell852 · 2021-12-01T22:33:07Z

so this bug snuck thru due to zero tests for this endpoint, right? think you can create a simple positive test to prevent this from happening again @TaylorCFrey ?

tcfdev · 2021-12-01T22:51:19Z

Good question @mitchell852. There are no unit tests, but there does appear to be integration API test. I'll look into those asap.

traffic_ops/traffic_ops_golang/user/current.go

mitchell852 · 2021-12-02T01:18:22Z

Good question @mitchell852. There are no unit tests, but there does appear to be integration API test. I'll look into those asap.

yeah, i would think a simple api integration test to verify that calling PUT /user/current with a valid user object succeeds should be sufficient.

rawlinp · 2021-12-02T15:38:10Z

I think the real mystery is how the existing TO API test wasn't failing, because it definitely attempts to change fields then checks to see that the changes stuck.

tcfdev · 2021-12-02T18:27:44Z

After some exploration I believe I found two issues. First, the request body from #6367 needs to be wrapped in a "user":{...} object. Without the wrapper, the API would not parse the body resulting in the API call believing nothing has changed. The response would return a success thinking all fields were "unedited". I've modified the code to check for the existence of the user wrapper and submitted an issue to explore it's removal #6396

Second, if the request was for an API v4.0, it would appear as though some of the fields were not edited, but a success would be returned. Subsequent GET requests would appear to work and show that the field was updated. This was because we were saving the state of the user earlier in the function then returning that state, rather than the update state. This was not the case for v3.x API calls. And it only impacted the response body from the PUT requests. This explains why the API integration client tests were succeeding as they do not directly validate the response body from the PUT request, but rather initiate a PUT followed by a GET.

rawlinp

Manually verified that not providing the user wrapper object returns an error as expected and that the PUT response is correct for 4.0 now.

Taylor Frey added 2 commits December 1, 2021 14:01

Update PUT user/current to work with v4 User Roles

b2ba648

Move import to correct location per ordering

b5310e2

zrhoffman added high impact impacts the basic function, deployment, or operation of a CDN regression bug a bug in existing functionality introduced by a new version Traffic Ops related to Traffic Ops labels Dec 1, 2021

rawlinp reviewed Dec 1, 2021

View reviewed changes

traffic_ops/traffic_ops_golang/user/current.go Outdated Show resolved Hide resolved

Remove 'UpdateQuery40' implementation

f631ae4

rawlinp approved these changes Dec 3, 2021

View reviewed changes

rawlinp merged commit 6eefc5c into apache:master Dec 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Update PUT user/current to work with v4 User Roles #6389

Update PUT user/current to work with v4 User Roles #6389

Uh oh!

tcfdev commented Dec 1, 2021 •

edited

Loading

Uh oh!

mitchell852 commented Dec 1, 2021

Uh oh!

tcfdev commented Dec 1, 2021

Uh oh!

Uh oh!

mitchell852 commented Dec 2, 2021

Uh oh!

rawlinp commented Dec 2, 2021

Uh oh!

tcfdev commented Dec 2, 2021

Uh oh!

rawlinp left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Update PUT user/current to work with v4 User Roles #6389

Update PUT user/current to work with v4 User Roles #6389

Uh oh!

Conversation

tcfdev commented Dec 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which Traffic Control components are affected by this PR?

What is the best way to verify this PR?

PR submission checklist

Uh oh!

mitchell852 commented Dec 1, 2021

Uh oh!

tcfdev commented Dec 1, 2021

Uh oh!

Uh oh!

mitchell852 commented Dec 2, 2021

Uh oh!

rawlinp commented Dec 2, 2021

Uh oh!

tcfdev commented Dec 2, 2021

Uh oh!

rawlinp left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tcfdev commented Dec 1, 2021 •

edited

Loading