Skip to content

Commit b12765c

Browse files
kylecarbskylecarbs
committed
Jon comments
1 parent 8e17cb9 commit b12765c

File tree

8 files changed

+29
-33
lines changed

8 files changed

+29
-33
lines changed

‎cli/gitaskpass.go

+2-7
Original file line numberDiff line numberDiff line change
@@ -23,16 +23,11 @@ func gitAskpass() *cobra.Command {
2323
Use: "gitaskpass",
2424
Hidden: true,
2525
Args: cobra.ExactArgs(1),
26-
RunE: func(cmd *cobra.Command, args []string) (err error) {
26+
RunE: func(cmd *cobra.Command, args []string) error {
2727
ctx := cmd.Context()
2828

2929
ctx, stop := signal.NotifyContext(ctx, interruptSignals...)
3030
defer stop()
31-
defer func() {
32-
if ctx.Err() != nil {
33-
err = ctx.Err()
34-
}
35-
}()
3631

3732
user, host, err := gitauth.ParseAskpass(args[0])
3833
if err != nil {
@@ -57,7 +52,7 @@ func gitAskpass() *cobra.Command {
5752
}
5853
if token.URL != "" {
5954
if err := openURL(cmd, token.URL); err != nil {
60-
cmd.Printf("Your browser has been opened to visit:\n\n\t%s\n\n", token.URL)
55+
cmd.Printf("Your browser has been opened to authenticate with Git:\n\n\t%s\n\n", token.URL)
6156
} else {
6257
cmd.Printf("Open the following URL to authenticate with Git:\n\n\t%s\n\n", token.URL)
6358
}

‎coderd/gitauth/askpass.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ func CheckCommand(args, env []string) bool {
3232
// For details on how the prompt is formatted, see `credential_ask_one`:
3333
// https://github.com/git/git/blob/bbe21b64a08f89475d8a3818e20c111378daa621/credential.c#L173-L191
3434
func ParseAskpass(prompt string) (user string, host string, err error) {
35-
parts := strings.Split(prompt, " ")
35+
parts := strings.Fields(prompt)
3636
if len(parts) < 3 {
3737
return "", "", xerrors.Errorf("askpass prompt must contain 3 words; got %d: %q", len(parts), prompt)
3838
}

‎coderd/gitauth/config.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -47,8 +47,8 @@ func ConvertConfig(entries []codersdk.DeploymentConfigGitAuth, accessURL *url.UR
4747
// Default to the type.
4848
entry.ID = string(typ)
4949
}
50-
if valid, err := httpapi.UsernameValid(entry.ID); !valid {
51-
return nil, xerrors.Errorf("git auth provider %q doesn't have a valid id: %w", entry.ID, err)
50+
if valid := httpapi.UsernameValid(entry.ID); valid != nil {
51+
return nil, xerrors.Errorf("git auth provider %q doesn't have a valid id: %w", entry.ID, valid)
5252
}
5353

5454
_, exists := ids[entry.ID]

‎coderd/httpapi/httpapi.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,8 @@ func init() {
4040
if !ok {
4141
return false
4242
}
43-
valid, _ := UsernameValid(str)
44-
return valid
43+
valid := UsernameValid(str)
44+
return valid == nil
4545
}
4646
for _, tag := range []string{"username", "template_name", "workspace_name"} {
4747
err := validate.RegisterValidation(tag, nameValidator)

‎coderd/httpapi/username.go

+7-7
Original file line numberDiff line numberDiff line change
@@ -14,18 +14,18 @@ var (
1414
)
1515

1616
// UsernameValid returns whether the input string is a valid username.
17-
func UsernameValid(str string) (bool, error) {
17+
func UsernameValid(str string) error {
1818
if len(str) > 32 {
19-
return false, xerrors.New("must be <= 32 characters")
19+
return xerrors.New("must be <= 32 characters")
2020
}
2121
if len(str) < 1 {
22-
return false, xerrors.New("must be >= 1 character")
22+
return xerrors.New("must be >= 1 character")
2323
}
2424
matched := UsernameValidRegex.MatchString(str)
2525
if !matched {
26-
return false, xerrors.New("must be alphanumeric with hyphens")
26+
return xerrors.New("must be alphanumeric with hyphens")
2727
}
28-
return true, nil
28+
return nil
2929
}
3030

3131
// UsernameFrom returns a best-effort username from the provided string.
@@ -35,15 +35,15 @@ func UsernameValid(str string) (bool, error) {
3535
// the username from an email address. If no success happens during
3636
// these steps, a random username will be returned.
3737
func UsernameFrom(str string) string {
38-
if valid, _ := UsernameValid(str); valid {
38+
if valid := UsernameValid(str); valid == nil {
3939
return str
4040
}
4141
emailAt := strings.LastIndex(str, "@")
4242
if emailAt >= 0 {
4343
str = str[:emailAt]
4444
}
4545
str = usernameReplace.ReplaceAllString(str, "")
46-
if valid, _ := UsernameValid(str); valid {
46+
if valid := UsernameValid(str); valid == nil {
4747
return str
4848
}
4949
return strings.ReplaceAll(namesgenerator.GetRandomName(1), "_", "-")

‎coderd/httpapi/username_test.go

+4-4
Original file line numberDiff line numberDiff line change
@@ -59,8 +59,8 @@ func TestValid(t *testing.T) {
5959
testCase := testCase
6060
t.Run(testCase.Username, func(t *testing.T) {
6161
t.Parallel()
62-
valid, _ := httpapi.UsernameValid(testCase.Username)
63-
require.Equal(t, testCase.Valid, valid)
62+
valid := httpapi.UsernameValid(testCase.Username)
63+
require.Equal(t, testCase.Valid, valid == nil)
6464
})
6565
}
6666
}
@@ -92,8 +92,8 @@ func TestFrom(t *testing.T) {
9292
t.Parallel()
9393
converted := httpapi.UsernameFrom(testCase.From)
9494
t.Log(converted)
95-
valid, _ := httpapi.UsernameValid(converted)
96-
require.True(t, valid)
95+
valid := httpapi.UsernameValid(converted)
96+
require.True(t, valid == nil)
9797
if testCase.Match == "" {
9898
require.NotEqual(t, testCase.From, converted)
9999
} else {

‎coderd/userauth.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -261,8 +261,8 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
261261
// The username is a required property in Coder. We make a best-effort
262262
// attempt at using what the claims provide, but if that fails we will
263263
// generate a random username.
264-
usernameValid, _ := httpapi.UsernameValid(username)
265-
if !usernameValid {
264+
usernameValid := httpapi.UsernameValid(username)
265+
if usernameValid != nil {
266266
// If no username is provided, we can default to use the email address.
267267
// This will be converted in the from function below, so it's safe
268268
// to keep the domain.

‎coderd/workspaceagents.go

+9-8
Original file line numberDiff line numberDiff line change
@@ -936,7 +936,7 @@ func (api *API) workspaceAgentsGitAuth(rw http.ResponseWriter, r *http.Request)
936936
gitURL := r.URL.Query().Get("url")
937937
if gitURL == "" {
938938
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
939-
Message: "Missing url query parameter!",
939+
Message: "Missing 'url' query parameter!",
940940
})
941941
return
942942
}
@@ -962,23 +962,23 @@ func (api *API) workspaceAgentsGitAuth(rw http.ResponseWriter, r *http.Request)
962962
// We must get the workspace to get the owner ID!
963963
resource, err := api.Database.GetWorkspaceResourceByID(ctx, workspaceAgent.ResourceID)
964964
if err != nil {
965-
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
965+
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
966966
Message: "Failed to get workspace resource.",
967967
Detail: err.Error(),
968968
})
969969
return
970970
}
971971
build, err := api.Database.GetWorkspaceBuildByJobID(ctx, resource.JobID)
972972
if err != nil {
973-
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
973+
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
974974
Message: "Failed to get build.",
975975
Detail: err.Error(),
976976
})
977977
return
978978
}
979979
workspace, err := api.Database.GetWorkspaceByID(ctx, build.WorkspaceID)
980980
if err != nil {
981-
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
981+
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
982982
Message: "Failed to get workspace.",
983983
Detail: err.Error(),
984984
})
@@ -1013,6 +1013,7 @@ func (api *API) workspaceAgentsGitAuth(rw http.ResponseWriter, r *http.Request)
10131013
}
10141014
defer cancelFunc()
10151015
ticker := time.NewTicker(time.Second)
1016+
defer ticker.Stop()
10161017
for {
10171018
select {
10181019
case <-r.Context().Done():
@@ -1025,7 +1026,7 @@ func (api *API) workspaceAgentsGitAuth(rw http.ResponseWriter, r *http.Request)
10251026
UserID: workspace.OwnerID,
10261027
})
10271028
if err != nil {
1028-
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
1029+
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
10291030
Message: "Failed to get git auth link.",
10301031
Detail: err.Error(),
10311032
})
@@ -1042,7 +1043,7 @@ func (api *API) workspaceAgentsGitAuth(rw http.ResponseWriter, r *http.Request)
10421043
// This is the URL that will redirect the user with a state token.
10431044
redirectURL, err := api.AccessURL.Parse(fmt.Sprintf("/gitauth/%s", gitAuthConfig.ID))
10441045
if err != nil {
1045-
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
1046+
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
10461047
Message: "Failed to parse access URL.",
10471048
Detail: err.Error(),
10481049
})
@@ -1055,7 +1056,7 @@ func (api *API) workspaceAgentsGitAuth(rw http.ResponseWriter, r *http.Request)
10551056
})
10561057
if err != nil {
10571058
if !errors.Is(err, sql.ErrNoRows) {
1058-
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
1059+
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
10591060
Message: "Failed to get git auth link.",
10601061
Detail: err.Error(),
10611062
})
@@ -1091,7 +1092,7 @@ func (api *API) workspaceAgentsGitAuth(rw http.ResponseWriter, r *http.Request)
10911092
OAuthExpiry: token.Expiry,
10921093
})
10931094
if err != nil {
1094-
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
1095+
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
10951096
Message: "Failed to update git auth link.",
10961097
Detail: err.Error(),
10971098
})

0 commit comments

Comments
 (0)