Merge pull request #159 from SkynetLabs/ivo/delete_pubkey

Implement `DELETE /user/pubkey/:pubKey`

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)

api/handlers.go

@@ -731,6 +731,34 @@ 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
+	}
+	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
+	}
+	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()
@@ -814,8 +842,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
@@ -825,7 +857,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.

api/routes.go

@@ -53,6 +53,7 @@ func (api *API) buildHTTPRoutes() {
 	api.staticRouter.GET("/user/limits", api.noAuth(api.userLimitsGET))
 	api.staticRouter.GET("/user/limits/:skylink", api.noAuth(api.userLimitsSkylinkGET))
 	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))

database/user.go

@@ -124,10 +124,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 {
@@ -494,6 +491,52 @@ 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) {
+	filter := bson.M{"_id": u.ID}
+	// 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,
+	}
+	_, 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,
+		"pub_keys": bson.M{"$ne": nil},
+	}
+	update := bson.M{
+		"$pull": bson.M{"pub_keys": pk},
+	}
+	opts := options.UpdateOptions{
+		Upsert: &False,
+	}
+	ur, err := db.staticUsers.UpdateOne(ctx, filter, update, &opts)
+	if err == nil && ur.MatchedCount == 0 {
+		err = mongo.ErrNoDocuments
+	}
+	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}

test/api/apikeys_test.go

@@ -228,6 +228,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
@@ -261,12 +266,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)
@@ -275,7 +280,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)
 	}
@@ -284,11 +289,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)
+	}
 }

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)
+	}
+}

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},