You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2021/12/01 21:30:34 UTC
[GitHub] [trafficcontrol] TaylorCFrey opened a new pull request #6389: Update PUT user/current to work with v4 User Roles
TaylorCFrey opened a new pull request #6389:
URL: https://github.com/apache/trafficcontrol/pull/6389
<!--
Thank you for contributing! Please be sure to read our contribution guidelines: https://github.com/apache/trafficcontrol/blob/master/CONTRIBUTING.md
If this closes or relates to an existing issue, please reference it using one of the following:
Closes: #ISSUE
Related: #ISSUE
If this PR fixes a security vulnerability, DO NOT submit! Instead, contact
the Apache Traffic Control Security Team at security@trafficcontrol.apache.org and follow the
guidelines at https://apache.org/security regarding vulnerability disclosure.
-->
Closes: #6367
The `user/current` PUT request was failing to correctly *UPDATE* the DB table with the correct role value. Added logic to perform a v4 UpdateQuery if the API is v4 or greater.
<!-- **^ Add meaningful description above** --><hr>
## Which Traffic Control components are affected by this PR?
<!-- Please delete all components from this list that are NOT affected by this PR.
Feel free to add the name of a tool or script that is affected but not on the list.
-->
- Traffic Ops
## What is the best way to verify this PR?
<!-- Please include here ALL the steps necessary to test your PR.
If your PR has tests (and most should), provide the steps needed to run the tests.
If not, please provide step-by-step instructions to test the PR manually and explain why your PR does not need tests. -->
Ensure a valid user is created via the API with appropriate role (say _operations_):
````bash
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":"john.frank@local.tld",
"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:
````bash
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_:
````bash
curl --request GET \
--url https://<traffic_ops_host>/api/4.0/user/current/
````
_Response_:
````json
{
"response": {
"username": "operations",
"localUser": true,
"addressLine1": "1234 Somewhere ln",
"addressLine2": null,
"city": "Manhattan",
"company": "Stihl",
"country": "USA",
"email": "john.frank@local.tld",
"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):
````bash
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": "john.frank@local.tld",
"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:
````json
{
"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": "john.frank@local.tld",
"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_:
````bash
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": "john.frank@local.tld",
"fullName": "John",
"id": 89,
"phoneNumber": "555-123-4567",
"postalCode": "80212",
"publicSshKey": null,
"role": 2,
"stateOrProvince": "Colorado",
"tenant": "root",
"tenantId": 1
}'
````
_Response_:
````json
{
"alerts": [
{
"text": "missing required 'user' object",
"level": "error"
}
]
}
````
## PR submission checklist
- [ ] This PR has tests - No previous tests exist.
- [ ] This PR has documentation - No changes to documentation were necessary
- [x] This PR has a CHANGELOG.md entry <!-- A fix for a bug from an ATC release, an improvement, or a new feature should have a changelog entry. -->
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://apache.org/security) for details)
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #6389: Update PUT user/current to work with v4 User Roles
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #6389:
URL: https://github.com/apache/trafficcontrol/pull/6389#discussion_r760653351
##########
File path: traffic_ops/traffic_ops_golang/user/current.go
##########
@@ -309,7 +316,16 @@ func ReplaceCurrent(w http.ResponseWriter, r *http.Request) {
}
}
- if err = updateUser(&user, tx, changePasswd, changeConfirmPasswd); err != nil {
+ if useV4User {
+ userV4 = user.Upgrade()
+ }
+
+ if useV4User {
+ err = updateUserV4(userV4, inf.Tx)
Review comment:
So, as far as I can tell, the difference between userV4 and user is really just `Role`, which isn't even used in the `replaceCurrentUser` query in the `updateUser` function. So, why doesn't just calling `updateUser(&user, tx, changePasswd, changeConfirmPasswd)` actually work?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] TaylorCFrey commented on pull request #6389: Update PUT user/current to work with v4 User Roles
Posted by GitBox <gi...@apache.org>.
TaylorCFrey commented on pull request #6389:
URL: https://github.com/apache/trafficcontrol/pull/6389#issuecomment-984129913
Good question @mitchell852. There are no unit tests, but there does appear to be integration API test. I'll look into those asap.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #6389: Update PUT user/current to work with v4 User Roles
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #6389:
URL: https://github.com/apache/trafficcontrol/pull/6389#discussion_r760653351
##########
File path: traffic_ops/traffic_ops_golang/user/current.go
##########
@@ -309,7 +316,16 @@ func ReplaceCurrent(w http.ResponseWriter, r *http.Request) {
}
}
- if err = updateUser(&user, tx, changePasswd, changeConfirmPasswd); err != nil {
+ if useV4User {
+ userV4 = user.Upgrade()
+ }
+
+ if useV4User {
+ err = updateUserV4(userV4, inf.Tx)
Review comment:
So, as far as I can tell, the difference between userV4 and user is really just `Role`, which isn't even used/updated in the `updateUser` function. So, why doesn't just calling `updateUser(&user, tx, changePasswd, changeConfirmPasswd)` actually work?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 commented on pull request #6389: Update PUT user/current to work with v4 User Roles
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on pull request #6389:
URL: https://github.com/apache/trafficcontrol/pull/6389#issuecomment-984204693
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 commented on pull request #6389: Update PUT user/current to work with v4 User Roles
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on pull request #6389:
URL: https://github.com/apache/trafficcontrol/pull/6389#issuecomment-984113436
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 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] TaylorCFrey commented on a change in pull request #6389: Update PUT user/current to work with v4 User Roles
Posted by GitBox <gi...@apache.org>.
TaylorCFrey commented on a change in pull request #6389:
URL: https://github.com/apache/trafficcontrol/pull/6389#discussion_r760665654
##########
File path: traffic_ops/traffic_ops_golang/user/current.go
##########
@@ -309,7 +316,16 @@ func ReplaceCurrent(w http.ResponseWriter, r *http.Request) {
}
}
- if err = updateUser(&user, tx, changePasswd, changeConfirmPasswd); err != nil {
+ if useV4User {
+ userV4 = user.Upgrade()
+ }
+
+ if useV4User {
+ err = updateUserV4(userV4, inf.Tx)
Review comment:
Good catch, and actually I didn't want to use `UpdateQueryV40` anyways. Hold pls for an update.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] TaylorCFrey commented on pull request #6389: Update PUT user/current to work with v4 User Roles
Posted by GitBox <gi...@apache.org>.
TaylorCFrey commented on pull request #6389:
URL: https://github.com/apache/trafficcontrol/pull/6389#issuecomment-984886784
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`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on pull request #6389: Update PUT user/current to work with v4 User Roles
Posted by GitBox <gi...@apache.org>.
rawlinp commented on pull request #6389:
URL: https://github.com/apache/trafficcontrol/pull/6389#issuecomment-984742696
I think the real mystery is how [the existing TO API](https://github.com/apache/trafficcontrol/blob/master/traffic_ops/testing/api/v4/user_test.go#L247-L275) test wasn't failing, because it definitely attempts to change fields then checks to see that the changes stuck.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp merged pull request #6389: Update PUT user/current to work with v4 User Roles
Posted by GitBox <gi...@apache.org>.
rawlinp merged pull request #6389:
URL: https://github.com/apache/trafficcontrol/pull/6389
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org