From 11324111e31e0213c2126f3985e33186a249bd4f Mon Sep 17 00:00:00 2001 From: Stefan Benten Date: Fri, 2 Jun 2023 00:09:13 +0200 Subject: [PATCH] satellite/admin: secure accidental user email change It currently is possible to create a violation with regards to the uniqueness of the user account emails that is used for the login. When an update via the admin API is made, it currently is possible to set the accounts email to an already occupied email address. This will result in very flacky login behaviour, as well as creating a lot of other related issues. This small change adds a check to ensure the email is not attached to any account. Change-Id: I167be673082d59ef32cafe41047fce9f5ae534d0 --- satellite/admin/user.go | 11 +++++++++++ satellite/admin/user_test.go | 7 +++++++ 2 files changed, 18 insertions(+) diff --git a/satellite/admin/user.go b/satellite/admin/user.go index eb289ad13..fb8f41e56 100644 --- a/satellite/admin/user.go +++ b/satellite/admin/user.go @@ -296,6 +296,17 @@ func (server *Server) updateUser(w http.ResponseWriter, r *http.Request) { updateRequest.ShortName = &shortNamePtr } if input.Email != "" { + existingUser, err := server.db.Console().Users().GetByEmail(ctx, input.Email) + if err != nil && !errors.Is(sql.ErrNoRows, err) { + sendJSONError(w, "failed to check for user email", + err.Error(), http.StatusInternalServerError) + return + } + if existingUser != nil { + sendJSONError(w, fmt.Sprintf("user with email already exists %s", input.Email), + "", http.StatusConflict) + return + } updateRequest.Email = &input.Email } if len(input.PasswordHash) > 0 { diff --git a/satellite/admin/user_test.go b/satellite/admin/user_test.go index 6d833d1ce..f5a4022a9 100644 --- a/satellite/admin/user_test.go +++ b/satellite/admin/user_test.go @@ -262,6 +262,13 @@ func TestUserUpdate(t *testing.T) { responseBody := assertReq(ctx, t, link, http.MethodPut, body, http.StatusNotFound, "", planet.Satellites[0].Config.Console.AuthToken) require.Contains(t, string(responseBody), "does not exist") }) + + t.Run("Email already used", func(t *testing.T) { + link := fmt.Sprintf("http://"+address.String()+"/api/users/%s", "alice+2@mail.test") + body := `{"email":"alice+2@mail.test", "shortName":"Newbie"}` + responseBody := assertReq(ctx, t, link, http.MethodPut, body, http.StatusConflict, "", planet.Satellites[0].Config.Console.AuthToken) + require.Contains(t, string(responseBody), "already exists") + }) }) }