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 2022/06/10 15:26:38 UTC
[GitHub] [trafficcontrol] hritvikpatel4 opened a new pull request, #6895: Fix user update to raise exception when the username contains spaces
hritvikpatel4 opened a new pull request, #6895:
URL: https://github.com/apache/trafficcontrol/pull/6895
This PR fixes #4240 by adding checks to raise an exception when the username contains spaces.
<hr/>
## Which Traffic Control components are affected by this PR?
- Traffic Ops
## What is the best way to verify this PR?
Run integration tests
## If this is a bugfix, which Traffic Control versions contained the bug?
- master
## PR submission checklist
- [x] This PR has tests <!-- If not, please delete this text and explain why this PR does not need tests. -->
- [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] hritvikpatel4 commented on a diff in pull request #6895: Fix user update to raise exception when the username contains spaces
Posted by GitBox <gi...@apache.org>.
hritvikpatel4 commented on code in PR #6895:
URL: https://github.com/apache/trafficcontrol/pull/6895#discussion_r900689192
##########
traffic_ops/testing/api/v2/user_test.go:
##########
@@ -273,6 +273,30 @@ func UserSelfUpdateTest(t *testing.T) {
} else if *resp2[0].Email != currentEmail {
t.Errorf("Expected Email to still be '%s', but it was '%s'", currentEmail, *resp2[0].Email)
}
+
+ // Now test using an invalid username
+ currentUsername := *user.Username
+ user.Username = new("ops man")
+ updateResp, _, err = TOSession.UpdateCurrentUser(user)
+ if err == nil {
+ t.Fatal("error was expected updating user with username: 'ops man' - got none")
+ }
+
+ // Ensure it wasn't actually updated
+ resp2, _, err = TOSession.GetUserByID(*user.ID)
+ if err != nil {
+ t.Fatalf("error getting user #%d: %v", *user.ID, err)
+ }
+
+ if len(resp2) < 1 {
+ t.Fatalf("no user returned when requesting user #%d", *user.ID)
+ }
+
+ if resp2[0].Username == nil {
+ t.Errorf("Username missing or null after update")
+ } else if *resp2[0].Username != currentUsername {
+ t.Errorf("Expected Username to still be '%s', but it was '%s'", currentUsername, *resp2[0].Username)
+ }
Review Comment:
Added tests for APIv2, v3 and v4.
--
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] ocket8888 commented on a diff in pull request #6895: Fix user update to raise exception when the username contains spaces
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on code in PR #6895:
URL: https://github.com/apache/trafficcontrol/pull/6895#discussion_r918279502
##########
lib/go-tc/users.go:
##########
@@ -487,6 +488,8 @@ func (u *CurrentUserUpdateRequestUser) UnmarshalAndValidate(user *User) error {
errs = append(errs, fmt.Errorf("username: %w", err))
} else if user.Username == nil || *user.Username == "" {
errs = append(errs, errors.New("username: cannot be null or empty string"))
+ } else if strings.Contains(*user.Username, " ") {
+ errs = append(errs, errors.New("username: cannot contain spaces"))
Review Comment:
The controller for the user details page view can be found at `traffic_portal/app/src/common/modules/form/user/FormUserController.js`
--
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] hritvikpatel4 commented on a diff in pull request #6895: Fix user update to raise exception when the username contains spaces
Posted by GitBox <gi...@apache.org>.
hritvikpatel4 commented on code in PR #6895:
URL: https://github.com/apache/trafficcontrol/pull/6895#discussion_r908643876
##########
traffic_ops/testing/api/v2/user_test.go:
##########
@@ -273,6 +273,30 @@ func UserSelfUpdateTest(t *testing.T) {
} else if *resp2[0].Email != currentEmail {
t.Errorf("Expected Email to still be '%s', but it was '%s'", currentEmail, *resp2[0].Email)
}
+
+ // Now test using an invalid username
+ currentUsername := *user.Username
+ user.Username = new("ops man")
+ updateResp, _, err = TOSession.UpdateCurrentUser(user)
+ if err == nil {
+ t.Fatal("error was expected updating user with username: 'ops man' - got none")
+ }
+
+ // Ensure it wasn't actually updated
+ resp2, _, err = TOSession.GetUserByID(*user.ID)
+ if err != nil {
+ t.Fatalf("error getting user #%d: %v", *user.ID, err)
+ }
+
+ if len(resp2) < 1 {
+ t.Fatalf("no user returned when requesting user #%d", *user.ID)
+ }
+
+ if resp2[0].Username == nil {
+ t.Errorf("Username missing or null after update")
+ } else if *resp2[0].Username != currentUsername {
+ t.Errorf("Expected Username to still be '%s', but it was '%s'", currentUsername, *resp2[0].Username)
+ }
Review Comment:
@ocket8888 - I am not sure why the API v4 test failed.
https://github.com/apache/trafficcontrol/runs/7009420118?check_suite_focus=true
I think it failed due to some other test case.
--
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] hritvikpatel4 commented on a diff in pull request #6895: Fix user update to raise exception when the username contains spaces
Posted by GitBox <gi...@apache.org>.
hritvikpatel4 commented on code in PR #6895:
URL: https://github.com/apache/trafficcontrol/pull/6895#discussion_r896340738
##########
lib/go-tc/users.go:
##########
@@ -487,6 +488,8 @@ func (u *CurrentUserUpdateRequestUser) UnmarshalAndValidate(user *User) error {
errs = append(errs, fmt.Errorf("username: %w", err))
} else if user.Username == nil || *user.Username == "" {
errs = append(errs, errors.New("username: cannot be null or empty string"))
+ } else if strings.Contains(*user.Username, " ") {
+ errs = append(errs, errors.New("username: cannot contain spaces"))
Review Comment:
So if the condition defined in line 491 is true, I would need to change the form validation in the Traffic Portal?
If yes, where can I find the part of form validation in Traffic Portal?
And would I need to make the change for the form validation from the Go code?
--
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] ocket8888 commented on a diff in pull request #6895: Fix user update to raise exception when the username contains spaces
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on code in PR #6895:
URL: https://github.com/apache/trafficcontrol/pull/6895#discussion_r918278478
##########
traffic_ops/testing/api/v2/user_test.go:
##########
@@ -273,6 +273,30 @@ func UserSelfUpdateTest(t *testing.T) {
} else if *resp2[0].Email != currentEmail {
t.Errorf("Expected Email to still be '%s', but it was '%s'", currentEmail, *resp2[0].Email)
}
+
+ // Now test using an invalid username
+ currentUsername := *user.Username
+ user.Username = new("ops man")
+ updateResp, _, err = TOSession.UpdateCurrentUser(user)
+ if err == nil {
+ t.Fatal("error was expected updating user with username: 'ops man' - got none")
+ }
+
+ // Ensure it wasn't actually updated
+ resp2, _, err = TOSession.GetUserByID(*user.ID)
+ if err != nil {
+ t.Fatalf("error getting user #%d: %v", *user.ID, err)
+ }
+
+ if len(resp2) < 1 {
+ t.Fatalf("no user returned when requesting user #%d", *user.ID)
+ }
+
+ if resp2[0].Username == nil {
+ t.Errorf("Username missing or null after update")
+ } else if *resp2[0].Username != currentUsername {
+ t.Errorf("Expected Username to still be '%s', but it was '%s'", currentUsername, *resp2[0].Username)
+ }
Review Comment:
I re-ran them and they passed.
--
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] hritvikpatel4 commented on a diff in pull request #6895: Fix user update to raise exception when the username contains spaces
Posted by GitBox <gi...@apache.org>.
hritvikpatel4 commented on code in PR #6895:
URL: https://github.com/apache/trafficcontrol/pull/6895#discussion_r930816759
##########
lib/go-tc/users.go:
##########
@@ -487,6 +488,8 @@ func (u *CurrentUserUpdateRequestUser) UnmarshalAndValidate(user *User) error {
errs = append(errs, fmt.Errorf("username: %w", err))
} else if user.Username == nil || *user.Username == "" {
errs = append(errs, errors.New("username: cannot be null or empty string"))
+ } else if strings.Contains(*user.Username, " ") {
+ errs = append(errs, errors.New("username: cannot contain spaces"))
Review Comment:
> The controller for the user details page view can be found at `traffic_portal/app/src/common/modules/form/user/FormUserController.js`
Alright, I'll look into that file. Thanks!
--
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] ocket8888 commented on a diff in pull request #6895: Fix user update to raise exception when the username contains spaces
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on code in PR #6895:
URL: https://github.com/apache/trafficcontrol/pull/6895#discussion_r896042815
##########
traffic_ops/testing/api/v2/user_test.go:
##########
@@ -273,6 +273,30 @@ func UserSelfUpdateTest(t *testing.T) {
} else if *resp2[0].Email != currentEmail {
t.Errorf("Expected Email to still be '%s', but it was '%s'", currentEmail, *resp2[0].Email)
}
+
+ // Now test using an invalid username
+ currentUsername := *user.Username
+ user.Username = new("ops man")
+ updateResp, _, err = TOSession.UpdateCurrentUser(user)
+ if err == nil {
+ t.Fatal("error was expected updating user with username: 'ops man' - got none")
+ }
+
+ // Ensure it wasn't actually updated
+ resp2, _, err = TOSession.GetUserByID(*user.ID)
+ if err != nil {
+ t.Fatalf("error getting user #%d: %v", *user.ID, err)
+ }
+
+ if len(resp2) < 1 {
+ t.Fatalf("no user returned when requesting user #%d", *user.ID)
+ }
+
+ if resp2[0].Username == nil {
+ t.Errorf("Username missing or null after update")
+ } else if *resp2[0].Username != currentUsername {
+ t.Errorf("Expected Username to still be '%s', but it was '%s'", currentUsername, *resp2[0].Username)
+ }
Review Comment:
Tests must not be added only to APIv2; it's actually *more* important that they be in versions 3 and 4, since 2 is close to being removed.
##########
lib/go-tc/users.go:
##########
@@ -487,6 +488,8 @@ func (u *CurrentUserUpdateRequestUser) UnmarshalAndValidate(user *User) error {
errs = append(errs, fmt.Errorf("username: %w", err))
} else if user.Username == nil || *user.Username == "" {
errs = append(errs, errors.New("username: cannot be null or empty string"))
+ } else if strings.Contains(*user.Username, " ") {
+ errs = append(errs, errors.New("username: cannot contain spaces"))
Review Comment:
Since in older versions of Traffic Ops, it was possible to have users with spaces in their usernames, it's a breaking change to the API to no longer allow that. As such, that change cannot be made in APIv2 or v3.
However, I think a better solution than returning an error when the username contains spaces is to simply change the form validation in Traffic Portal to allow spaces, since the API already does. That also allows you to avoid the mess of API versioning.
--
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