[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #6895: Fix user update to raise exception when the username contains spaces

[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