Skip to content

Fix Copilot review findings from PR #305 and #308 #318

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
duanemay merged 2 commits into from
Jul 17, 2026
Merged

Fix Copilot review findings from PR #305 and #308 #318

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
5678e43
Fix update-user Copilot review findings from PR #305
duanemay Jul 6, 2026
8087fae
Fix change-client-secret Copilot review findings from PR #308
duanemay Jul 6, 2026
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
18 changes: 11 additions & 7 deletions cmd/change_client_secret.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,26 +33,30 @@ func ChangeClientSecretValidation(cfg config.Config, oldSecret, newSecret string
return nil
}

type changeClientSecretRequest struct {
OldSecret string `json:"oldSecret"`
Secret string `json:"secret"`
}

func ChangeClientSecretCmd(api *uaa.API, log cli.Logger, cfg config.Config, oldSecret, newSecret string) error {
context := cfg.GetActiveContext()
clientId := context.ClientId

// Prepare the request body for the secret change
requestBody := map[string]interface{}{
"oldSecret": oldSecret,
"secret": newSecret,
requestBody := changeClientSecretRequest{
OldSecret: oldSecret,
Secret: newSecret,
}

requestBodyJSON, err := json.Marshal(requestBody)
if err != nil {
return err
}

// Make the API call to change the client secret
// api.Curl bypasses the go-uaa SDK's structured request path, so the zone
// header that GetAPIFromSavedTokenInContext's WithZoneID would otherwise
// add automatically must be set explicitly here.
path := fmt.Sprintf("/oauth/clients/%s/secret", clientId)
headers := []string{"Content-Type: application/json"}

// Add zone header if specified
if cfg.ZoneSubdomain != "" {
headers = append(headers, fmt.Sprintf("X-Identity-Zone-Id: %s", cfg.ZoneSubdomain))
}
Expand Down
17 changes: 10 additions & 7 deletions cmd/update_user.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,21 +78,24 @@ func UpdateUserCmd(api *uaa.API, printer cli.Printer, username, familyName, give
return printer.Print(updatedUser)
}

func UpdateUserValidation(cfg config.Config, args []string) error {
func UpdateUserValidation(cfg config.Config, args []string, familyName, givenName string, emails, phones, delAttrs []string) error {
if err := cli.EnsureContextInConfig(cfg); err != nil {
return err
}
if len(args) == 0 {
return errors.New("The positional argument USERNAME must be specified.")
}
if familyName == "" && givenName == "" && len(emails) == 0 && len(phones) == 0 && len(delAttrs) == 0 {
return errors.New("At least one of --familyName, --givenName, --email, --phone, or --delAttrs must be specified.")
}
return nil
}

var updateUserCmd = &cobra.Command{
Use: "update-user USERNAME",
Short: "Update a user account",
PreRun: func(cmd *cobra.Command, args []string) {
cli.NotifyValidationErrors(UpdateUserValidation(GetSavedConfig(), args), cmd, log)
cli.NotifyValidationErrors(UpdateUserValidation(GetSavedConfig(), args, familyName, givenName, emails, phoneNumbers, delAttrs), cmd, log)
},
Run: func(cmd *cobra.Command, args []string) {
cfg := GetSavedConfig()
Expand All @@ -111,11 +114,11 @@ func init() {
updateUserCmd.Annotations = make(map[string]string)
updateUserCmd.Annotations[USER_CRUD_CATEGORY] = "true"

updateUserCmd.Flags().StringVarP(&familyName, "family_name", "", "", "family name")
updateUserCmd.Flags().StringVarP(&givenName, "given_name", "", "", "given name")
updateUserCmd.Flags().StringVarP(&familyName, "familyName", "", "", "family name")
updateUserCmd.Flags().StringVarP(&givenName, "givenName", "", "", "given name")
updateUserCmd.Flags().StringVarP(&origin, "origin", "o", "", "user origin")
updateUserCmd.Flags().StringSliceVarP(&emails, "emails", "", []string{}, "email addresses (multiple may be specified)")
updateUserCmd.Flags().StringSliceVarP(&phoneNumbers, "phones", "", []string{}, "phone numbers (multiple may be specified)")
updateUserCmd.Flags().StringSliceVarP(&delAttrs, "del_attrs", "", []string{}, "attributes to remove (phoneNumbers, name, etc.)")
updateUserCmd.Flags().StringSliceVarP(&emails, "email", "", []string{}, "email address (multiple may be specified)")
updateUserCmd.Flags().StringSliceVarP(&phoneNumbers, "phone", "", []string{}, "phone number (multiple may be specified)")
updateUserCmd.Flags().StringSliceVarP(&delAttrs, "delAttrs", "", []string{}, "attributes to remove (phoneNumbers, name, etc.)")
updateUserCmd.Flags().StringVarP(&zoneSubdomain, "zone", "z", "", "the identity zone subdomain in which to update the user")
}
125 changes: 100 additions & 25 deletions cmd/update_user_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
package cmd_test

import (
"bytes"
"encoding/json"
"io"
"net/http"

"code.cloudfoundry.org/uaa-cli/cli"
"code.cloudfoundry.org/uaa-cli/config"
"code.cloudfoundry.org/uaa-cli/fixtures"
Expand All @@ -10,9 +15,19 @@ import (
. "github.com/onsi/gomega/gbytes"
. "github.com/onsi/gomega/gexec"
. "github.com/onsi/gomega/ghttp"
"net/http"
)

// captureRequestBody records the raw request body into dest and restores it
// so downstream ghttp handlers (e.g. VerifyRequest) can still read it.
func captureRequestBody(dest *[]byte) http.HandlerFunc {
return func(w http.ResponseWriter, req *http.Request) {
body, err := io.ReadAll(req.Body)
Expect(err).NotTo(HaveOccurred())
*dest = body
req.Body = io.NopCloser(bytes.NewReader(body))
}
Comment on lines +23 to +28
}

var _ = Describe("UpdateUser", func() {
BeforeEach(func() {
cfg := config.NewConfigWithServerURL(server.URL())
Expand Down Expand Up @@ -45,11 +60,21 @@ var _ = Describe("UpdateUser", func() {
Eventually(session).Should(Exit(1))
Expect(session.Err).To(Say("The positional argument USERNAME must be specified."))
})

It("requires at least one update flag", func() {
session := runCommand("update-user", "marcus@stoicism.com")

Eventually(session).Should(Exit(1))
Expect(session.Err).To(Say("At least one of --familyName, --givenName, --email, --phone, or --delAttrs must be specified."))
Expect(server.ReceivedRequests()).To(HaveLen(0))
})
})

Describe("UpdateUserCmd", func() {
Describe("Success cases", func() {
It("updates user with given_name only", func() {
var putBody []byte

// First GET to retrieve user
server.RouteToHandler("GET", "/Users", CombineHandlers(
RespondWith(http.StatusOK, fixtures.PaginatedResponse(uaa.User{ID: "fb5f32e1-5cb3-49e6-93df-6df9c8c8bd70", Username: "marcus@stoicism.com"})),
Expand All @@ -60,21 +85,30 @@ var _ = Describe("UpdateUser", func() {

// Then PUT to update user
server.RouteToHandler("PUT", "/Users/fb5f32e1-5cb3-49e6-93df-6df9c8c8bd70", CombineHandlers(
captureRequestBody(&putBody),
RespondWith(http.StatusOK, fixtures.MarcusUserResponse),
VerifyRequest("PUT", "/Users/fb5f32e1-5cb3-49e6-93df-6df9c8c8bd70"),
VerifyHeaderKV("Authorization", "Bearer eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiYWRtaW4iOnRydWV9.TJVA95OrM7E2cBab30RMHrHDcEfxjoYZgeFONFh7HgQ"),
VerifyHeaderKV("Accept", "application/json"),
VerifyHeaderKV("Content-Type", "application/json"),
))

session := runCommand("update-user", "marcus@stoicism.com", "--given_name", "Bob")
session := runCommand("update-user", "marcus@stoicism.com", "--givenName", "Bob")

Expect(server.ReceivedRequests()).To(HaveLen(2))
Expect(session).To(Exit(0))
Expect(session.Out).To(Say("Account for user marcus@stoicism.com successfully updated"))

var payload map[string]interface{}
Expect(json.Unmarshal(putBody, &payload)).To(Succeed())
name, ok := payload["name"].(map[string]interface{})
Expect(ok).To(BeTrue())
Expect(name["givenName"]).To(Equal("Bob"))
})

It("updates user with multiple attributes", func() {
var putBody []byte

// First GET to retrieve user
server.RouteToHandler("GET", "/Users", CombineHandlers(
RespondWith(http.StatusOK, fixtures.PaginatedResponse(uaa.User{ID: "fb5f32e1-5cb3-49e6-93df-6df9c8c8bd70", Username: "marcus@stoicism.com"})),
Expand All @@ -83,17 +117,25 @@ var _ = Describe("UpdateUser", func() {

// Then PUT to update user
server.RouteToHandler("PUT", "/Users/fb5f32e1-5cb3-49e6-93df-6df9c8c8bd70", CombineHandlers(
captureRequestBody(&putBody),
RespondWith(http.StatusOK, fixtures.MarcusUserResponse),
VerifyRequest("PUT", "/Users/fb5f32e1-5cb3-49e6-93df-6df9c8c8bd70"),
))

session := runCommand("update-user", "marcus@stoicism.com",
"--given_name", "Bob",
"--family_name", "Smith")
session := runCommand("update-user", "marcus@stoicism.com",
"--givenName", "Bob",
"--familyName", "Smith")

Expect(server.ReceivedRequests()).To(HaveLen(2))
Expect(session).To(Exit(0))
Expect(session.Out).To(Say("Account for user marcus@stoicism.com successfully updated"))

var payload map[string]interface{}
Expect(json.Unmarshal(putBody, &payload)).To(Succeed())
name, ok := payload["name"].(map[string]interface{})
Expect(ok).To(BeTrue())
Expect(name["givenName"]).To(Equal("Bob"))
Expect(name["familyName"]).To(Equal("Smith"))
})

It("updates user with origin specified", func() {
Expand All @@ -110,15 +152,17 @@ var _ = Describe("UpdateUser", func() {
VerifyRequest("PUT", "/Users/fb5f32e1-5cb3-49e6-93df-6df9c8c8bd70"),
))

session := runCommand("update-user", "marcus@stoicism.com",
"--origin", "ldap",
"--given_name", "Bob")
session := runCommand("update-user", "marcus@stoicism.com",
"--origin", "ldap",
"--givenName", "Bob")

Expect(server.ReceivedRequests()).To(HaveLen(2))
Expect(session).To(Exit(0))
})

It("updates user with emails", func() {
var putBody []byte

// First GET to retrieve user
server.RouteToHandler("GET", "/Users", CombineHandlers(
RespondWith(http.StatusOK, fixtures.PaginatedResponse(uaa.User{ID: "fb5f32e1-5cb3-49e6-93df-6df9c8c8bd70", Username: "marcus@stoicism.com"})),
Expand All @@ -127,18 +171,29 @@ var _ = Describe("UpdateUser", func() {

// Then PUT to update user
server.RouteToHandler("PUT", "/Users/fb5f32e1-5cb3-49e6-93df-6df9c8c8bd70", CombineHandlers(
captureRequestBody(&putBody),
RespondWith(http.StatusOK, fixtures.MarcusUserResponse),
VerifyRequest("PUT", "/Users/fb5f32e1-5cb3-49e6-93df-6df9c8c8bd70"),
))

session := runCommand("update-user", "marcus@stoicism.com",
"--emails", "new@email.com")
session := runCommand("update-user", "marcus@stoicism.com",
"--email", "new@email.com")

Expect(server.ReceivedRequests()).To(HaveLen(2))
Expect(session).To(Exit(0))

var payload map[string]interface{}
Expect(json.Unmarshal(putBody, &payload)).To(Succeed())
emails, ok := payload["emails"].([]interface{})
Expect(ok).To(BeTrue())
Expect(emails).To(HaveLen(1))
email := emails[0].(map[string]interface{})
Expect(email["value"]).To(Equal("new@email.com"))
})

It("updates user with phones", func() {
var putBody []byte

// First GET to retrieve user
server.RouteToHandler("GET", "/Users", CombineHandlers(
RespondWith(http.StatusOK, fixtures.PaginatedResponse(uaa.User{ID: "fb5f32e1-5cb3-49e6-93df-6df9c8c8bd70", Username: "marcus@stoicism.com"})),
Expand All @@ -147,35 +202,55 @@ var _ = Describe("UpdateUser", func() {

// Then PUT to update user
server.RouteToHandler("PUT", "/Users/fb5f32e1-5cb3-49e6-93df-6df9c8c8bd70", CombineHandlers(
captureRequestBody(&putBody),
RespondWith(http.StatusOK, fixtures.MarcusUserResponse),
VerifyRequest("PUT", "/Users/fb5f32e1-5cb3-49e6-93df-6df9c8c8bd70"),
))

session := runCommand("update-user", "marcus@stoicism.com",
"--phones", "555-1234")
session := runCommand("update-user", "marcus@stoicism.com",
"--phone", "555-1234")

Expect(server.ReceivedRequests()).To(HaveLen(2))
Expect(session).To(Exit(0))

var payload map[string]interface{}
Expect(json.Unmarshal(putBody, &payload)).To(Succeed())
phones, ok := payload["phoneNumbers"].([]interface{})
Expect(ok).To(BeTrue())
Expect(phones).To(HaveLen(1))
phone := phones[0].(map[string]interface{})
Expect(phone["value"]).To(Equal("555-1234"))
})

It("updates user with del_attrs removing phone numbers", func() {
// First GET to retrieve user
It("updates user with delAttrs removing phone numbers", func() {
var putBody []byte

// First GET to retrieve user, which already has a phone number
server.RouteToHandler("GET", "/Users", CombineHandlers(
RespondWith(http.StatusOK, fixtures.PaginatedResponse(uaa.User{ID: "fb5f32e1-5cb3-49e6-93df-6df9c8c8bd70", Username: "marcus@stoicism.com"})),
RespondWith(http.StatusOK, fixtures.PaginatedResponse(uaa.User{
ID: "fb5f32e1-5cb3-49e6-93df-6df9c8c8bd70",
Username: "marcus@stoicism.com",
PhoneNumbers: []uaa.PhoneNumber{{Value: "555-0000"}},
})),
VerifyRequest("GET", "/Users"),
))

// Then PUT to update user
server.RouteToHandler("PUT", "/Users/fb5f32e1-5cb3-49e6-93df-6df9c8c8bd70", CombineHandlers(
captureRequestBody(&putBody),
RespondWith(http.StatusOK, fixtures.MarcusUserResponse),
VerifyRequest("PUT", "/Users/fb5f32e1-5cb3-49e6-93df-6df9c8c8bd70"),
))

session := runCommand("update-user", "marcus@stoicism.com",
"--del_attrs", "phoneNumbers")
session := runCommand("update-user", "marcus@stoicism.com",
"--delAttrs", "phoneNumbers")

Expect(server.ReceivedRequests()).To(HaveLen(2))
Expect(session).To(Exit(0))

var payload map[string]interface{}
Expect(json.Unmarshal(putBody, &payload)).To(Succeed())
Expect(payload["phoneNumbers"]).To(BeNil())
})

It("works with zone parameter", func() {
Expand All @@ -193,8 +268,8 @@ var _ = Describe("UpdateUser", func() {
VerifyHeaderKV("X-Identity-Zone-Id", "twilight-zone"),
))

session := runCommand("update-user", "marcus@stoicism.com",
"--given_name", "Bob",
session := runCommand("update-user", "marcus@stoicism.com",
"--givenName", "Bob",
"--zone", "twilight-zone")

Expect(server.ReceivedRequests()).To(HaveLen(2))
Expand All @@ -214,7 +289,7 @@ var _ = Describe("UpdateUser", func() {
VerifyRequest("PUT", "/Users/fb5f32e1-5cb3-49e6-93df-6df9c8c8bd70"),
))

session := runCommand("update-user", "marcus@stoicism.com", "--given_name", "Bob")
session := runCommand("update-user", "marcus@stoicism.com", "--givenName", "Bob")

Expect(server.ReceivedRequests()).To(HaveLen(2))
Expect(session).To(Exit(0))
Expand All @@ -230,7 +305,7 @@ var _ = Describe("UpdateUser", func() {
VerifyRequest("GET", "/Users"),
))

session := runCommand("update-user", "nobody", "--given_name", "Bob")
session := runCommand("update-user", "nobody", "--givenName", "Bob")

Expect(server.ReceivedRequests()).To(HaveLen(1))
Expect(session).To(Exit(1))
Expand All @@ -249,7 +324,7 @@ var _ = Describe("UpdateUser", func() {
VerifyRequest("PUT", "/Users/fb5f32e1-5cb3-49e6-93df-6df9c8c8bd70"),
))

session := runCommand("update-user", "marcus@stoicism.com", "--given_name", "Bob")
session := runCommand("update-user", "marcus@stoicism.com", "--givenName", "Bob")

Expect(server.ReceivedRequests()).To(HaveLen(2))
Expect(session).To(Exit(1))
Expand All @@ -270,8 +345,8 @@ var _ = Describe("UpdateUser", func() {
VerifyRequest("PUT", "/Users/fb5f32e1-5cb3-49e6-93df-6df9c8c8bd70"),
))

session := runCommand("update-user", "marcus@stoicism.com",
"--given_name", "Bob",
session := runCommand("update-user", "marcus@stoicism.com",
"--givenName", "Bob",
"--verbose")

Expect(server.ReceivedRequests()).To(HaveLen(2))
Expand All @@ -281,4 +356,4 @@ var _ = Describe("UpdateUser", func() {
})
})
})
})
})
Loading