Skip to content
This repository was archived by the owner on Nov 24, 2025. It is now read-only.

Fix user update to raise exception when the username contains spaces #6895

Open
hritvikpatel4 wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix user update to raise exception when the username contains spaces #6895

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a6149a3
Fix user update to raise exception when the username contains spaces
Jun 10, 2022
cdac1f0
Updated tests
Jun 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions lib/go-tc/users.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"encoding/json"
"errors"
"fmt"
"strings"
"time"

"github.com/apache/trafficcontrol/lib/go-rfc"
Expand Down Expand Up @@ -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"))

@ocket8888 ocket8888 Jun 13, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@hritvikpatel4 hritvikpatel4 Jun 14, 2022

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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?

@ocket8888 ocket8888 Jul 11, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The controller for the user details page view can be found at traffic_portal/app/src/common/modules/form/user/FormUserController.js

@hritvikpatel4 hritvikpatel4 Jul 27, 2022

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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!

}
}

Expand Down
27 changes: 26 additions & 1 deletion traffic_ops/testing/api/v2/user_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,13 @@ package v2
import (
"bytes"
"fmt"
"github.com/apache/trafficcontrol/lib/go-rfc"
"net/http"
"net/mail"
"strings"
"testing"
"time"

"github.com/apache/trafficcontrol/lib/go-rfc"
"github.com/apache/trafficcontrol/lib/go-tc"
"github.com/apache/trafficcontrol/lib/go-util"
toclient "github.com/apache/trafficcontrol/traffic_ops/v2-client"
Expand Down Expand Up @@ -273,6 +273,31 @@ 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
newUsername := "ops man"
user.Username = &newUsername
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)
}
Comment thread
ocket8888 marked this conversation as resolved.
}

func UserUpdateOwnRoleTest(t *testing.T) {
Expand Down
25 changes: 25 additions & 0 deletions traffic_ops/testing/api/v3/user_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,31 @@ 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
newUsername := "ops man"
user.Username = &newUsername
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)
}
}

func UserUpdateOwnRoleTest(t *testing.T) {
Expand Down
24 changes: 24 additions & 0 deletions traffic_ops/testing/api/v4/user_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,30 @@ func UserSelfUpdateTest(t *testing.T) {
} else if *resp2.Response[0].Email != currentEmail {
t.Errorf("Expected Email to still be '%s', but it was '%s'", currentEmail, *resp2.Response[0].Email)
}

// Now test using an invalid username
currentUsername := *&user.Username
user.Username = "ops man"
updateResp, _, err = TOSession.UpdateCurrentUser(user, client.RequestOptions{})
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.GetUsers(opts)
if err != nil {
t.Fatalf("error getting user #%d: %v", *user.ID, err)
}

if len(resp2.Response) < 1 {
t.Fatalf("no user returned when requesting user #%d", *user.ID)
}

if resp2.Response[0].Username == "" {
t.Errorf("Username missing or null after update")
} else if resp2.Response[0].Username != currentUsername {
t.Errorf("Expected Username to still be '%s', but it was '%s'", currentUsername, resp2.Response[0].Username)
}
}

func UserUpdateOwnRoleTest(t *testing.T) {
Expand Down