From 94830bdfcfd96f914edbb9d410101ec7f1900e1b Mon Sep 17 00:00:00 2001 From: jarradg Date: Wed, 21 Oct 2026 11:20:21 -0400 Subject: [PATCH 1/2] fix(login): reject account creation failures without recovery --- pkg/cmd/login/login.go | 6 +-- pkg/cmd/login/login_test.go | 82 +++++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 5 deletions(-) diff --git a/pkg/cmd/login/login.go b/pkg/cmd/login/login.go index 59707659..e8d4baec 100644 --- a/pkg/cmd/login/login.go +++ b/pkg/cmd/login/login.go @@ -300,11 +300,7 @@ func RunTasksForUser(t *terminal.Terminal) error { func CreateNewUser(loginStore LoginStore, idToken string) (bool, error) { _, err := loginStore.CreateUser(idToken) if err != nil { - if !(strings.Contains(err.Error(), "duplicate username") || strings.Contains(err.Error(), "duplicate external auth id") || strings.Contains(err.Error(), "identity already exists")) { - // This is a real error - return false, breverrors.WrapAndTrace(err) - } - return false, nil + return false, breverrors.WrapAndTrace(err) } return true, nil diff --git a/pkg/cmd/login/login_test.go b/pkg/cmd/login/login_test.go index 8afd7127..da2e1cfb 100644 --- a/pkg/cmd/login/login_test.go +++ b/pkg/cmd/login/login_test.go @@ -2,6 +2,9 @@ package login import ( "bytes" + "errors" + "net/http" + "net/http/httptest" "os" "testing" @@ -9,6 +12,7 @@ import ( "github.com/brevdev/brev-cli/pkg/entity" "github.com/brevdev/brev-cli/pkg/store" "github.com/brevdev/brev-cli/pkg/terminal" + "github.com/go-resty/resty/v2" "github.com/spf13/cobra" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -44,6 +48,7 @@ func (m *mockLoginAuth) LoginWithAPIKey(apiKey string, orgID string) error { type mockLoginStore struct { getCurrentUserCalls int createUserCalls int + createUserErr error getOrCreateOrgCalls int createOrganizationCalls int setDefaultOrgCalls int @@ -66,9 +71,86 @@ func (m *mockLoginStore) GetCurrentUser() (*entity.User, error) { func (m *mockLoginStore) CreateUser(_ string) (*entity.User, error) { m.createUserCalls++ + if m.createUserErr != nil { + return nil, m.createUserErr + } return &entity.User{}, nil } +func accountCreationHTTPError(t *testing.T, status int, body string) error { + t.Helper() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + _, _ = w.Write([]byte(body)) + })) + t.Cleanup(server.Close) + response, err := resty.New().R().Post(server.URL) + require.NoError(t, err) + return store.NewHTTPResponseError(response) +} + +func TestLoginAndGetOrCreateUser_CreationOutcomes(t *testing.T) { + tests := []struct { + name string + createError func(*testing.T) error + wantUser bool + }{ + {name: "successful account API response", wantUser: true}, + { + name: "conflict without a recovery contract", + createError: func(t *testing.T) error { + return accountCreationHTTPError(t, http.StatusConflict, `{"errors":[{"type":"AlreadyExistsError","message":"Conflict"}]}`) + }, + }, + { + name: "unrelated bad request", + createError: func(t *testing.T) error { + return accountCreationHTTPError(t, http.StatusBadRequest, `{"errors":[{"type":"BadRequestError","message":"Bad Request"}]}`) + }, + }, + { + name: "uncoded collision text", + createError: func(_ *testing.T) error { + return errors.New("duplicate username") + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + loginStore := &mockLoginStore{} + if tt.createError != nil { + loginStore.createUserErr = tt.createError(t) + } + auth := &mockLoginAuth{} + user, err := (LoginOptions{Auth: auth, LoginStore: loginStore}).loginAndGetOrCreateUser("", false) + if tt.wantUser { + require.NoError(t, err) + require.Equal(t, "user-1", user.ID) + require.Equal(t, 1, loginStore.getCurrentUserCalls) + } else { + require.Error(t, err) + require.Nil(t, user) + require.Equal(t, 0, loginStore.getCurrentUserCalls) + } + require.Equal(t, 1, auth.loginCalls) + require.Equal(t, 1, loginStore.createUserCalls) + }) + } +} + +func TestLoginAndGetOrCreateUser_WithTokenUsesExistingAccount(t *testing.T) { + loginStore := &mockLoginStore{} + auth := &mockLoginAuth{} + user, err := (LoginOptions{Auth: auth, LoginStore: loginStore}).loginAndGetOrCreateUser("login-token", false) + require.NoError(t, err) + require.Equal(t, "user-1", user.ID) + require.Equal(t, 1, auth.tokenCalls) + require.Equal(t, 0, auth.loginCalls) + require.Equal(t, 0, loginStore.createUserCalls) + require.Equal(t, 1, loginStore.getCurrentUserCalls) +} + func (m *mockLoginStore) SetDefaultOrganization(org *entity.Organization) error { m.setDefaultOrgCalls++ m.defaultOrg = org From 8f07a09a36ef97470127d8dd2478e49317e78ce9 Mon Sep 17 00:00:00 2001 From: jarradg Date: Wed, 21 Oct 2026 11:37:21 -0400 Subject: [PATCH 2/2] refactor(login): remove unused new-user result --- pkg/cmd/login/login.go | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/pkg/cmd/login/login.go b/pkg/cmd/login/login.go index e8d4baec..e6b24803 100644 --- a/pkg/cmd/login/login.go +++ b/pkg/cmd/login/login.go @@ -133,7 +133,7 @@ func (o LoginOptions) loginAndGetOrCreateUser(loginToken string, skipBrowser boo return nil, breverrors.WrapAndTrace(err) } - _, err = CreateNewUser(o.LoginStore, tokens.IDToken) + _, err = o.LoginStore.CreateUser(tokens.IDToken) if err != nil { return nil, breverrors.WrapAndTrace(err) } @@ -296,16 +296,6 @@ func RunTasksForUser(t *terminal.Terminal) error { return nil } -// returns if the user is indeed new -func CreateNewUser(loginStore LoginStore, idToken string) (bool, error) { - _, err := loginStore.CreateUser(idToken) - if err != nil { - return false, breverrors.WrapAndTrace(err) - } - - return true, nil -} - func (o LoginOptions) showBreadCrumbs(t *terminal.Terminal, org *entity.Organization, user *entity.User) error { orgs, err := o.LoginStore.GetOrganizations(nil) if err != nil {