From bf3326405e38caf32328144e718573aea4066e88 Mon Sep 17 00:00:00 2001 From: Ivaylo Novakov Date: Wed, 16 Mar 2022 18:57:33 +0100 Subject: [PATCH 1/5] Implement `DELETE /user/pubkey/:pubKey`. --- api/handlers.go | 38 ++++++++++++++++ api/routes.go | 3 +- main.go | 4 +- test/api/challenge_test.go | 90 ++++++++++++++++++++++++++++++++++++++ test/api/handlers_test.go | 1 + 5 files changed, 133 insertions(+), 3 deletions(-) diff --git a/api/handlers.go b/api/handlers.go index 5db391da..1fe378a6 100644 --- a/api/handlers.go +++ b/api/handlers.go @@ -2,6 +2,7 @@ package api import ( "context" + "crypto/subtle" "encoding/json" "io" "io/ioutil" @@ -656,6 +657,43 @@ func (api *API) userPUT(u *database.User, w http.ResponseWriter, req *http.Reque api.loginUser(w, u, true) } +// userPubKeyDELETE removes a given pubkey from the list of pubkeys associated +// with this user. It does not require a challenge-response because the used +// does not need to prove the key is theirs. +func (api *API) userPubKeyDELETE(u *database.User, w http.ResponseWriter, req *http.Request, ps httprouter.Params) { + ctx := req.Context() + var pk database.PubKey + err := pk.LoadString(ps.ByName("pubKey")) + if err != nil { + api.WriteError(w, err, http.StatusBadRequest) + return + } + if !u.HasKey(pk) { + // This pubkey does not belong to this user. + api.WriteError(w, errors.New("the given pubkey is not associated with this user"), http.StatusBadRequest) + return + } + // Find the position of the pubkey in the list. + keyIdx := -1 + for i, k := range u.PubKeys { + if subtle.ConstantTimeCompare(pk[:], k[:]) == 1 { + keyIdx = i + break + } + } + if keyIdx == -1 { + build.Critical("Reaching this should be impossible. It would indicate a concurrent change of the user struct.") + } + // Remove the pubkey. + u.PubKeys = append(u.PubKeys[:keyIdx], u.PubKeys[keyIdx+1:]...) + err = api.staticDB.UserSave(ctx, u) + if err != nil { + api.WriteError(w, err, http.StatusInternalServerError) + return + } + api.WriteSuccess(w) +} + // userPubKeyRegisterGET generates an update challenge for the caller. func (api *API) userPubKeyRegisterGET(u *database.User, w http.ResponseWriter, req *http.Request, _ httprouter.Params) { ctx := req.Context() diff --git a/api/routes.go b/api/routes.go index c23c924e..e9dcca1c 100644 --- a/api/routes.go +++ b/api/routes.go @@ -16,7 +16,7 @@ import ( var ( // APIKeyHeader holds the name of the header we use for API keys. This // header name matches the established standard used by Swagger and others. - APIKeyHeader = "Skynet-API-Key" + APIKeyHeader = "Skynet-API-Key" // #nosec // ErrNoAPIKey is an error returned when we expect an API key but we don't // find one. ErrNoAPIKey = errors.New("no api key found") @@ -55,6 +55,7 @@ func (api *API) buildHTTPRoutes() { api.staticRouter.DELETE("/user", api.withAuth(api.userDELETE)) api.staticRouter.GET("/user/limits", api.noAuth(api.userLimitsGET)) api.staticRouter.GET("/user/stats", api.withAuth(api.userStatsGET)) + api.staticRouter.DELETE("/user/pubkey/:pubKey", api.WithDBSession(api.withAuth(api.userPubKeyDELETE))) api.staticRouter.GET("/user/pubkey/register", api.WithDBSession(api.withAuth(api.userPubKeyRegisterGET))) api.staticRouter.POST("/user/pubkey/register", api.WithDBSession(api.withAuth(api.userPubKeyRegisterPOST))) api.staticRouter.GET("/user/uploads", api.withAuth(api.userUploadsGET)) diff --git a/main.go b/main.go index 71cd5a9a..aa98e89b 100644 --- a/main.go +++ b/main.go @@ -53,12 +53,12 @@ const ( envServerDomain = "SERVER_DOMAIN" // envStripeAPIKey hold the name of the environment variable for Stripe's // API key. It's only required when integrating with Stripe. - envStripeAPIKey = "STRIPE_API_KEY" + envStripeAPIKey = "STRIPE_API_KEY" // #nosec // envMaxNumAPIKeysPerUser hold the name of the environment variable which // sets the limit for number of API keys a single user can create. If a user // reaches that limit they can always delete some API keys in order to make // space for new ones. - envMaxNumAPIKeysPerUser = "ACCOUNTS_MAX_NUM_API_KEYS_PER_USER" + envMaxNumAPIKeysPerUser = "ACCOUNTS_MAX_NUM_API_KEYS_PER_USER" // #nosec ) type ( diff --git a/test/api/challenge_test.go b/test/api/challenge_test.go index c3a91ab3..b5e628a5 100644 --- a/test/api/challenge_test.go +++ b/test/api/challenge_test.go @@ -309,3 +309,93 @@ func testUserAddPubKey(t *testing.T, at *test.AccountsTester) { t.Fatalf("Expected pubKey '%s', got '%s',", hex.EncodeToString(pk[:]), hex.EncodeToString(u3.PubKeys[0])) } } + +// testUserDeletePubKey ensures that users can delete pubkeys from their +// accounts. +func testUserDeletePubKey(t *testing.T, at *test.AccountsTester) { + name := test.DBNameForTest(t.Name()) + u, c, err := test.CreateUserAndLogin(at, name) + if err != nil { + t.Fatal("Failed to create a user and log in:", err) + } + defer func() { + if err = u.Delete(at.Ctx); err != nil { + t.Error(errors.AddContext(err, "failed to delete user in defer")) + } + }() + at.SetCookie(c) + defer at.ClearCredentials() + + sk, pk := crypto.GenerateKeyPair() + var ch database.Challenge + + // Request a new challenge. + queryParams := url.Values{} + queryParams.Set("pubKey", hex.EncodeToString(pk[:])) + r, b, err := at.Get("/user/pubkey/register", queryParams) + err = json.Unmarshal(b, &ch) + if err != nil || r.StatusCode != http.StatusOK { + t.Fatal("Failed to get a challenge:", err, r.Status, string(b)) + } + chBytes, err := hex.DecodeString(ch.Challenge) + if err != nil { + t.Fatal("Invalid challenge:", err) + } + // Solve the challenge. + response := append(chBytes, append([]byte(database.ChallengeTypeUpdate), []byte(database.PortalName)...)...) + bodyParams := url.Values{} + bodyParams.Set("response", hex.EncodeToString(response)) + bodyParams.Set("signature", hex.EncodeToString(ed25519.Sign(sk[:], response))) + r, b, err = at.Post("/user/pubkey/register", nil, bodyParams) + if err != nil { + t.Fatalf("Failed to confirm the update. Status %d, body '%s', error '%s'", r.StatusCode, string(b), err) + } + // Make sure the user's pubKey is properly set. + u1, err := at.DB.UserBySub(at.Ctx, u.Sub) + if err != nil { + t.Fatal(err) + } + if len(u1.PubKeys) != 1 { + t.Fatal("Expected one pubkey assigned, got none.") + } + if subtle.ConstantTimeCompare(u1.PubKeys[0], pk[:]) != 1 { + t.Fatalf("Expected pubKey '%s', got '%s',", hex.EncodeToString(pk[:]), hex.EncodeToString(u1.PubKeys[0])) + } + + // Call DELETE without a cookie. + at.ClearCredentials() + r, b, err = at.Delete("/user/pubkey/"+hex.EncodeToString(pk[:]), nil) + if err == nil || r.StatusCode != http.StatusUnauthorized { + t.Fatalf("Expected to fail with 401. Status %d, body '%s', error '%s'", r.StatusCode, string(b), err) + } + at.SetCookie(c) + // Call DELETE with an invalid key. + r, b, err = at.Delete("/user/pubkey/INVALID_KEY", nil) + if err == nil || r.StatusCode != http.StatusBadRequest { + t.Fatalf("Expected to fail with 400. Status %d, body '%s', error '%s'", r.StatusCode, string(b), err) + } + _, pk1 := crypto.GenerateKeyPair() + // Call DELETE with a key that doesn't belong to this user. + r, b, err = at.Delete("/user/pubkey/"+hex.EncodeToString(pk1[:]), nil) + if err == nil || r.StatusCode != http.StatusBadRequest { + t.Fatalf("Expected to fail with 400. Status %d, body '%s', error '%s'", r.StatusCode, string(b), err) + } + // Call DELETE with correct parameters. + r, b, err = at.Delete("/user/pubkey/"+hex.EncodeToString(pk[:]), nil) + if err != nil || r.StatusCode != http.StatusNoContent { + t.Fatalf("Expected to succeed. Status %d, body '%s', error '%s'", r.StatusCode, string(b), err) + } + // Verify that the key was deleted. + u2, err := at.DB.UserBySub(at.Ctx, u.Sub) + if err != nil { + t.Fatal(err) + } + if len(u2.PubKeys) > 0 { + t.Fatal("Expected no public keys, got", len(u2.PubKeys)) + } + // Call DELETE with the already deleted key. + r, b, err = at.Delete("/user/pubkey/"+hex.EncodeToString(pk[:]), nil) + if err == nil || r.StatusCode != http.StatusBadRequest { + t.Fatalf("Expected to fail with 400. Status %d, body '%s', error '%s'", r.StatusCode, string(b), err) + } +} diff --git a/test/api/handlers_test.go b/test/api/handlers_test.go index 3626ae34..7b3e635e 100644 --- a/test/api/handlers_test.go +++ b/test/api/handlers_test.go @@ -59,6 +59,7 @@ func TestHandlers(t *testing.T) { {name: "LoginLogout", test: testHandlerLoginPOST}, {name: "UserEdit", test: testUserPUT}, {name: "UserAddPubKey", test: testUserAddPubKey}, + {name: "DeletePubKey", test: testUserDeletePubKey}, {name: "UserDelete", test: testUserDELETE}, {name: "UserLimits", test: testUserLimits}, {name: "UserDeleteUploads", test: testUserUploadsDELETE}, From f1c6a022d8603306288e758a40b6236c9552e810 Mon Sep 17 00:00:00 2001 From: Ivaylo Novakov Date: Mon, 21 Mar 2022 13:03:10 +0100 Subject: [PATCH 2/5] Use dedicated DB methods for adding and removing pubkeys from the user record. --- api/cache_test.go | 2 +- api/handlers.go | 26 ++++--------- database/user.go | 38 ++++++++++++++++-- test/api/apikeys_test.go | 19 +++++++-- test/database/user_test.go | 80 ++++++++++++++++++++++++++++++++++++++ test/tester.go | 7 ++-- 6 files changed, 141 insertions(+), 31 deletions(-) diff --git a/api/cache_test.go b/api/cache_test.go index fa49f5a6..161099e8 100644 --- a/api/cache_test.go +++ b/api/cache_test.go @@ -21,7 +21,7 @@ func TestUserTierCache(t *testing.T) { if ok || tier != database.TierAnonymous { t.Fatalf("Expected to get tier %d and %t, got %d and %t.", database.TierAnonymous, false, tier, ok) } - // Set the use in the cache. + // Set the user in the cache. cache.Set(u.Sub, u) // Check again. tier, qe, ok := cache.Get(u.Sub) diff --git a/api/handlers.go b/api/handlers.go index 7aa6eb16..18d57f0f 100644 --- a/api/handlers.go +++ b/api/handlers.go @@ -2,7 +2,6 @@ package api import ( "context" - "crypto/subtle" "encoding/json" "io" "io/ioutil" @@ -748,20 +747,7 @@ func (api *API) userPubKeyDELETE(u *database.User, w http.ResponseWriter, req *h api.WriteError(w, errors.New("the given pubkey is not associated with this user"), http.StatusBadRequest) return } - // Find the position of the pubkey in the list. - keyIdx := -1 - for i, k := range u.PubKeys { - if subtle.ConstantTimeCompare(pk[:], k[:]) == 1 { - keyIdx = i - break - } - } - if keyIdx == -1 { - build.Critical("Reaching this should be impossible. It would indicate a concurrent change of the user struct.") - } - // Remove the pubkey. - u.PubKeys = append(u.PubKeys[:keyIdx], u.PubKeys[keyIdx+1:]...) - err = api.staticDB.UserSave(ctx, u) + err = api.staticDB.UserPubKeyRemove(ctx, *u, pk) if err != nil { api.WriteError(w, err, http.StatusInternalServerError) return @@ -852,8 +838,12 @@ func (api *API) userPubKeyRegisterPOST(u *database.User, w http.ResponseWriter, api.WriteError(w, errors.New("user's sub doesn't match update sub"), http.StatusBadRequest) return } - u.PubKeys = append(u.PubKeys, pk) - err = api.staticDB.UserSave(ctx, u) + err = api.staticDB.UserPubKeyAdd(ctx, *u, pk) + if err != nil { + api.WriteError(w, err, http.StatusInternalServerError) + return + } + updatedUser, err := api.staticDB.UserByID(ctx, u.ID) if err != nil { api.WriteError(w, err, http.StatusInternalServerError) return @@ -863,7 +853,7 @@ func (api *API) userPubKeyRegisterPOST(u *database.User, w http.ResponseWriter, api.WriteError(w, err, http.StatusInternalServerError) return } - api.loginUser(w, u, true) + api.loginUser(w, updatedUser, true) } // userUploadsGET returns all uploads made by the current user. diff --git a/database/user.go b/database/user.go index de53d08d..64136872 100644 --- a/database/user.go +++ b/database/user.go @@ -128,10 +128,7 @@ type ( SubscriptionCancelAtPeriodEnd bool `bson:"subscription_cancel_at_period_end" json:"subscriptionCancelAtPeriodEnd"` StripeID string `bson:"stripe_id" json:"stripeCustomerId"` QuotaExceeded bool `bson:"quota_exceeded" json:"quotaExceeded"` - // The currently active (or default) key is going to be the first one in - // the list. If we want to activate a new pubkey, we'll just move it to - // the first position in the list. - PubKeys []PubKey `bson:"pub_keys" json:"-"` + PubKeys []PubKey `bson:"pub_keys" json:"-"` } // UserStats contains statistical information about the user. UserStats struct { @@ -477,6 +474,39 @@ func (db *DB) UserSave(ctx context.Context, u *User) error { return nil } +// UserPubKeyAdd adds a new PubKey to the given user's set. +func (db *DB) UserPubKeyAdd(ctx context.Context, u User, pk PubKey) (err error) { + // If the set of pubkeys is not initialised we cannot use mongo's mutation + // operations, such as $addToSet, so we'll save the entire user record. + if u.PubKeys == nil { + u.PubKeys = make([]PubKey, 1) + u.PubKeys[0] = pk + return db.UserSave(ctx, &u) + } + filter := bson.M{"_id": u.ID} + update := bson.M{ + "$addToSet": bson.M{"pub_keys": pk}, + } + opts := options.UpdateOptions{ + Upsert: &False, + } + _, err = db.staticUsers.UpdateOne(ctx, filter, update, &opts) + return err +} + +// UserPubKeyRemove removes a PubKey from the given user's set. +func (db *DB) UserPubKeyRemove(ctx context.Context, u User, pk PubKey) error { + filter := bson.M{"_id": u.ID} + update := bson.M{ + "$pull": bson.M{"pub_keys": pk}, + } + opts := options.UpdateOptions{ + Upsert: &False, + } + _, err := db.staticUsers.UpdateOne(ctx, filter, update, &opts) + return err +} + // UserSetStripeID changes the user's stripe id in the DB. func (db *DB) UserSetStripeID(ctx context.Context, u *User, stripeID string) error { filter := bson.M{"_id": u.ID} diff --git a/test/api/apikeys_test.go b/test/api/apikeys_test.go index 2f7f2c8e..a08dc71f 100644 --- a/test/api/apikeys_test.go +++ b/test/api/apikeys_test.go @@ -255,12 +255,12 @@ func testPublicAPIKeysUsage(t *testing.T, at *test.AccountsTester) { Public: true, Skylinks: []string{sl.Skylink}, } - akWithKey, _, err := at.UserAPIKeysPOST(apiKeyPOST) + pakWithKey, _, err := at.UserAPIKeysPOST(apiKeyPOST) if err != nil { t.Fatal(err) } // Stop using the cookie, use the public API key instead. - at.SetAPIKey(akWithKey.Key.String()) + at.SetAPIKey(pakWithKey.Key.String()) // Try to fetch the user's stats with the new public API key. // Expect this to fail. _, _, err = at.Get("/user/stats", nil) @@ -269,7 +269,7 @@ func testPublicAPIKeysUsage(t *testing.T, at *test.AccountsTester) { } // Get the user's limits for downloading a skylink covered by the public // API key. Expect to get TierFree values. - ul, _, err := at.UserLimitsSkylink(sl.Skylink, "byte", nil) + ul, _, err := at.UserLimitsSkylink(sl.Skylink, "byte", "", nil) if err != nil { t.Fatal(err) } @@ -278,11 +278,22 @@ func testPublicAPIKeysUsage(t *testing.T, at *test.AccountsTester) { } // Get the user's limits for downloading a skylink that is not covered by // the public API key. Expect to get TierAnonymous values. - ul, _, err = at.UserLimitsSkylink(sl2.Skylink, "byte", nil) + ul, _, err = at.UserLimitsSkylink(sl2.Skylink, "byte", "", nil) if err != nil { t.Fatal(err) } if ul.DownloadBandwidth != database.UserLimits[database.TierAnonymous].DownloadBandwidth { t.Fatalf("Expected to get download bandwidth of %d, got %d", database.UserLimits[database.TierAnonymous].DownloadBandwidth, ul.DownloadBandwidth) } + // Stop using the header, pass the skylink as a query parameter. + at.ClearCredentials() + // Get the user's limits for downloading a skylink covered by the public + // API key. Expect to get TierFree values. + ul, _, err = at.UserLimitsSkylink(sl.Skylink, "byte", pakWithKey.Key.String(), nil) + if err != nil { + t.Fatal(err) + } + if ul.DownloadBandwidth != database.UserLimits[database.TierFree].DownloadBandwidth { + t.Fatalf("Expected to get download bandwidth of %d, got %d", database.UserLimits[database.TierFree].DownloadBandwidth, ul.DownloadBandwidth) + } } diff --git a/test/database/user_test.go b/test/database/user_test.go index f8caa4d6..ac3315c5 100644 --- a/test/database/user_test.go +++ b/test/database/user_test.go @@ -2,6 +2,7 @@ package database import ( "context" + "crypto/subtle" "reflect" "testing" "time" @@ -438,6 +439,85 @@ func TestUserSetStripeID(t *testing.T) { } } +// TestUserPubKey tests UserPubKeyAdd and UserPubKeyRemove. +func TestUserPubKey(t *testing.T) { + ctx := context.Background() + dbName := test.DBNameForTest(t.Name()) + db, err := database.NewCustomDB(ctx, dbName, test.DBTestCredentials(), nil) + if err != nil { + t.Fatal(err) + } + // Create a test user. + u, err := db.UserCreate(ctx, t.Name()+"@siasky.net", t.Name()+"pass", t.Name()+"sub", database.TierFree) + if err != nil { + t.Fatal(err) + } + defer func(user *database.User) { + _ = db.UserDelete(ctx, user) + }(u) + // Add a pubkey. + pk := database.PubKey(make([]byte, database.PubKeySize)) + copy(pk[:], fastrand.Bytes(database.PubKeySize)) + err = db.UserPubKeyAdd(ctx, *u, pk) + if err != nil { + t.Fatal(err) + } + u1, err := db.UserByID(ctx, u.ID) + if err != nil { + t.Fatal(err) + } + if len(u1.PubKeys) == 1 && subtle.ConstantTimeCompare(u1.PubKeys[0][:], pk[:]) != 1 { + t.Fatalf("Expected the user to have a single pubkey which matches ours. Got %+v, pubkey %+v", u1.PubKeys, pk) + } + // Add another. + var pk1 database.PubKey + copy(pk1[:], fastrand.Bytes(database.PubKeySize)) + err = db.UserPubKeyAdd(ctx, *u, pk) + if err != nil { + t.Fatal(err) + } + u2, err := db.UserByID(ctx, u.ID) + if err != nil { + t.Fatal(err) + } + if len(u2.PubKeys) == 2 && subtle.ConstantTimeCompare(u1.PubKeys[1][:], pk1[:]) == 1 { + t.Fatalf("Expected the user to have a single pubkey which matches ours. Got %+v, pubkey %+v", u2.PubKeys, pk1) + } + // Delete a pubkey. + err = db.UserPubKeyRemove(ctx, *u, pk) + if err != nil { + t.Fatal(err) + } + u3, err := db.UserByID(ctx, u.ID) + if err != nil { + t.Fatal(err) + } + if len(u3.PubKeys) == 1 && subtle.ConstantTimeCompare(u3.PubKeys[0][:], pk1[:]) == 1 { + t.Fatalf("Expected the user to have a single pubkey which matches ours. Got %+v, pubkey %+v", u3.PubKeys, pk1) + } + // Make sure UserPubKeyRemove removes all copies of the pubkey from the set. + // We don't expect there to be multiple but we still want to make sure. + u.PubKeys = make([]database.PubKey, 0) + u.PubKeys = append(u.PubKeys, pk) + u.PubKeys = append(u.PubKeys, pk) + u.PubKeys = append(u.PubKeys, pk) + err = db.UserSave(ctx, u) + if err != nil { + t.Fatal(err) + } + err = db.UserPubKeyRemove(ctx, *u, pk) + if err != nil { + t.Fatal(err) + } + u4, err := db.UserByID(ctx, u.ID) + if err != nil { + t.Fatal(err) + } + if len(u4.PubKeys) > 0 { + t.Fatal("Expected zero pubkeys.") + } +} + // TestUserSetTier ensures that UserSetTier works as expected. func TestUserSetTier(t *testing.T) { ctx := context.Background() diff --git a/test/tester.go b/test/tester.go index 0ed24e77..ab4ff637 100644 --- a/test/tester.go +++ b/test/tester.go @@ -475,11 +475,10 @@ func (at *AccountsTester) UserLimits(unit string, headers map[string]string) (ap } // UserLimitsSkylink performs a `GET /user/limits/:skylink` request. -func (at *AccountsTester) UserLimitsSkylink(sl string, unit string, headers map[string]string) (api.UserLimitsGET, int, error) { +func (at *AccountsTester) UserLimitsSkylink(sl string, unit, apikey string, headers map[string]string) (api.UserLimitsGET, int, error) { queryParams := url.Values{} - if unit != "" { - queryParams.Set("unit", unit) - } + queryParams.Set("unit", unit) + queryParams.Set("apiKey", apikey) if !database.ValidSkylinkHash(sl) { return api.UserLimitsGET{}, 0, database.ErrInvalidSkylink } From c73c78b839c456bbc4539fa02ab6f4ce324941a9 Mon Sep 17 00:00:00 2001 From: Ivaylo Novakov Date: Mon, 21 Mar 2022 13:15:15 +0100 Subject: [PATCH 3/5] Return a 404 on delete request for a pubkey that doesn't exist. --- api/handlers.go | 4 ++++ database/user.go | 4 ++++ test/api/apikeys_test.go | 5 +++++ test/database/user_test.go | 12 +++++++++--- 4 files changed, 22 insertions(+), 3 deletions(-) diff --git a/api/handlers.go b/api/handlers.go index 18d57f0f..7482ad59 100644 --- a/api/handlers.go +++ b/api/handlers.go @@ -748,6 +748,10 @@ func (api *API) userPubKeyDELETE(u *database.User, w http.ResponseWriter, req *h return } err = api.staticDB.UserPubKeyRemove(ctx, *u, pk) + if errors.Contains(err, mongo.ErrNoDocuments) { + api.WriteError(w, err, http.StatusNotFound) + return + } if err != nil { api.WriteError(w, err, http.StatusInternalServerError) return diff --git a/database/user.go b/database/user.go index 64136872..c036a7e1 100644 --- a/database/user.go +++ b/database/user.go @@ -5,6 +5,7 @@ import ( "crypto/subtle" "fmt" "net/mail" + "strings" "sync" "time" @@ -504,6 +505,9 @@ func (db *DB) UserPubKeyRemove(ctx context.Context, u User, pk PubKey) error { Upsert: &False, } _, err := db.staticUsers.UpdateOne(ctx, filter, update, &opts) + if err != nil && strings.Contains(err.Error(), "Cannot apply $pull to a non-array value") { + return mongo.ErrNoDocuments + } return err } diff --git a/test/api/apikeys_test.go b/test/api/apikeys_test.go index a08dc71f..39b2a839 100644 --- a/test/api/apikeys_test.go +++ b/test/api/apikeys_test.go @@ -222,6 +222,11 @@ func testPublicAPIKeysFlow(t *testing.T, at *test.AccountsTester) { if len(aks) != 0 { t.Fatalf("Expected no API keys, got %d.", len(aks)) } + // Delete the same key again. Expect a 404. + status, err = at.UserAPIKeysDELETE(akr.ID) + if status != http.StatusNotFound { + t.Fatal("Expected status 404, got", status) + } } // testPublicAPIKeysUsage makes sure that we can use public API keys to make diff --git a/test/database/user_test.go b/test/database/user_test.go index ac3315c5..dba767f9 100644 --- a/test/database/user_test.go +++ b/test/database/user_test.go @@ -14,6 +14,7 @@ import ( "gitlab.com/NebulousLabs/errors" "gitlab.com/NebulousLabs/fastrand" "go.mongodb.org/mongo-driver/bson/primitive" + "go.mongodb.org/mongo-driver/mongo" "go.sia.tech/siad/crypto" ) @@ -455,9 +456,16 @@ func TestUserPubKey(t *testing.T) { defer func(user *database.User) { _ = db.UserDelete(ctx, user) }(u) - // Add a pubkey. pk := database.PubKey(make([]byte, database.PubKeySize)) copy(pk[:], fastrand.Bytes(database.PubKeySize)) + pk1 := database.PubKey(make([]byte, database.PubKeySize)) + copy(pk1[:], fastrand.Bytes(database.PubKeySize)) + // Try to remove a pubkey. Expect this to fail. + err = db.UserPubKeyRemove(ctx, *u, pk) + if err == nil || !errors.Contains(err, mongo.ErrNoDocuments) { + t.Fatal(err) + } + // Add a pubkey. err = db.UserPubKeyAdd(ctx, *u, pk) if err != nil { t.Fatal(err) @@ -470,8 +478,6 @@ func TestUserPubKey(t *testing.T) { t.Fatalf("Expected the user to have a single pubkey which matches ours. Got %+v, pubkey %+v", u1.PubKeys, pk) } // Add another. - var pk1 database.PubKey - copy(pk1[:], fastrand.Bytes(database.PubKeySize)) err = db.UserPubKeyAdd(ctx, *u, pk) if err != nil { t.Fatal(err) From 7a76f664e86c0f48cd9ba24309ae6f1fdf554880 Mon Sep 17 00:00:00 2001 From: Ivaylo Novakov Date: Mon, 21 Mar 2022 14:05:36 +0100 Subject: [PATCH 4/5] Fix the `$addToSet` on a `null` field. Thanks, PJ! --- database/user.go | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/database/user.go b/database/user.go index c036a7e1..d8e04a56 100644 --- a/database/user.go +++ b/database/user.go @@ -477,16 +477,23 @@ func (db *DB) UserSave(ctx context.Context, u *User) error { // UserPubKeyAdd adds a new PubKey to the given user's set. func (db *DB) UserPubKeyAdd(ctx context.Context, u User, pk PubKey) (err error) { - // If the set of pubkeys is not initialised we cannot use mongo's mutation - // operations, such as $addToSet, so we'll save the entire user record. - if u.PubKeys == nil { - u.PubKeys = make([]PubKey, 1) - u.PubKeys[0] = pk - return db.UserSave(ctx, &u) - } filter := bson.M{"_id": u.ID} - update := bson.M{ - "$addToSet": bson.M{"pub_keys": pk}, + // This update is so complicated because we can't use mutation operations + // like $push, $addToSet and so on if the target field is null. That's why + // here we check if the field is an array and then merge the key in. If the + // field is not an array (i.e. it's null) we set it to an empty array before + // performing the merge. + update := bson.A{ + bson.M{ + "$set": bson.M{ + "pub_keys": bson.M{ + "$cond": bson.A{ + bson.M{"$eq": bson.A{bson.M{"$type": "$pub_keys"}, "array"}}, + bson.M{"$setUnion": bson.A{"$pub_keys", bson.A{pk}}}, + bson.A{pk}, + }}, + }, + }, } opts := options.UpdateOptions{ Upsert: &False, From d40e0c981919457a7644fc2185e3d9c76bf15cf9 Mon Sep 17 00:00:00 2001 From: Ivaylo Novakov Date: Thu, 24 Mar 2022 12:00:13 +0100 Subject: [PATCH 5/5] More reliable pubkey deletion. --- database/user.go | 12 +++++++----- test/database/user_test.go | 3 +-- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/database/user.go b/database/user.go index d8e04a56..9018eff8 100644 --- a/database/user.go +++ b/database/user.go @@ -5,7 +5,6 @@ import ( "crypto/subtle" "fmt" "net/mail" - "strings" "sync" "time" @@ -504,16 +503,19 @@ func (db *DB) UserPubKeyAdd(ctx context.Context, u User, pk PubKey) (err error) // UserPubKeyRemove removes a PubKey from the given user's set. func (db *DB) UserPubKeyRemove(ctx context.Context, u User, pk PubKey) error { - filter := bson.M{"_id": u.ID} + filter := bson.M{ + "_id": u.ID, + "pub_keys": bson.M{"$ne": nil}, + } update := bson.M{ "$pull": bson.M{"pub_keys": pk}, } opts := options.UpdateOptions{ Upsert: &False, } - _, err := db.staticUsers.UpdateOne(ctx, filter, update, &opts) - if err != nil && strings.Contains(err.Error(), "Cannot apply $pull to a non-array value") { - return mongo.ErrNoDocuments + ur, err := db.staticUsers.UpdateOne(ctx, filter, update, &opts) + if err == nil && ur.MatchedCount == 0 { + err = mongo.ErrNoDocuments } return err } diff --git a/test/database/user_test.go b/test/database/user_test.go index dba767f9..2614ca04 100644 --- a/test/database/user_test.go +++ b/test/database/user_test.go @@ -14,7 +14,6 @@ import ( "gitlab.com/NebulousLabs/errors" "gitlab.com/NebulousLabs/fastrand" "go.mongodb.org/mongo-driver/bson/primitive" - "go.mongodb.org/mongo-driver/mongo" "go.sia.tech/siad/crypto" ) @@ -462,7 +461,7 @@ func TestUserPubKey(t *testing.T) { copy(pk1[:], fastrand.Bytes(database.PubKeySize)) // Try to remove a pubkey. Expect this to fail. err = db.UserPubKeyRemove(ctx, *u, pk) - if err == nil || !errors.Contains(err, mongo.ErrNoDocuments) { + if err == nil { t.Fatal(err) } // Add a pubkey.