From 8f89b5d81b6e67f3e0fd98b695bc4f773196aafd Mon Sep 17 00:00:00 2001 From: Ivaylo Novakov Date: Wed, 16 Mar 2022 10:43:41 +0100 Subject: [PATCH 1/5] More informative error message. --- database/upload.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/database/upload.go b/database/upload.go index eb8f365f..2ff5fb88 100644 --- a/database/upload.go +++ b/database/upload.go @@ -50,7 +50,7 @@ func (db *DB) UploadCreate(ctx context.Context, user User, skylink Skylink) (*Up return nil, errors.New("invalid user") } if skylink.ID.IsZero() { - return nil, errors.New("invalid skylink") + return nil, errors.New("skylink doesn't exist") } up := Upload{ UserID: user.ID, From 2bee352875e94a14ad07fa587f45d86ae93de3a1 Mon Sep 17 00:00:00 2001 From: Ivaylo Novakov Date: Wed, 16 Mar 2022 10:45:48 +0100 Subject: [PATCH 2/5] Fix a race condition that prevents us from getting the ID of the skylink we need. --- database/skylink.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/database/skylink.go b/database/skylink.go index eb2f26db..088a729d 100644 --- a/database/skylink.go +++ b/database/skylink.go @@ -65,10 +65,17 @@ func (db *DB) Skylink(ctx context.Context, skylink string) (*Skylink, error) { opts := options.Update().SetUpsert(true) var ur *mongo.UpdateResult ur, err = db.staticSkylinks.UpdateOne(ctx, filter, upsert, opts) + if err != nil { + return nil, err + } // The UpsertedID might be nil in case the skylink got added to the DB - // by another server in between the calls. - if err == nil && ur.UpsertedID != nil { + // by another server in between the calls. In that case we'll fetch the + // skylink record from the DB. + if ur.UpsertedID != nil { skylinkRec.ID = ur.UpsertedID.(primitive.ObjectID) + } else { + sr = db.staticSkylinks.FindOne(ctx, filter) + err = sr.Decode(&skylinkRec) } } if err != nil { From 01e4de4afe1b9b8a78c160ad8c16b828351db1d5 Mon Sep 17 00:00:00 2001 From: Ivaylo Novakov Date: Wed, 16 Mar 2022 10:51:28 +0100 Subject: [PATCH 3/5] Silence some gosec false positives. --- api/routes.go | 2 +- main.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/api/routes.go b/api/routes.go index c23c924e..f0a5c572 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") diff --git a/main.go b/main.go index 71cd5a9a..405de0e0 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 ( From d6d9864b716e0f66efba976cd42dc629ddbaf803 Mon Sep 17 00:00:00 2001 From: Ivaylo Novakov Date: Wed, 16 Mar 2022 10:58:36 +0100 Subject: [PATCH 4/5] Formatting. --- main.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/main.go b/main.go index 405de0e0..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" //#nosec + 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" //#nosec + envMaxNumAPIKeysPerUser = "ACCOUNTS_MAX_NUM_API_KEYS_PER_USER" // #nosec ) type ( From 301d55bfdafbccf5e5f5cddaf94bebfc630586c5 Mon Sep 17 00:00:00 2001 From: Ivaylo Novakov Date: Wed, 16 Mar 2022 13:52:05 +0100 Subject: [PATCH 5/5] Simplify db.Skylink and find/create the skylink with a single DB call. --- database/skylink.go | 34 +++++++--------------------------- 1 file changed, 7 insertions(+), 27 deletions(-) diff --git a/database/skylink.go b/database/skylink.go index 088a729d..341b86a8 100644 --- a/database/skylink.go +++ b/database/skylink.go @@ -8,7 +8,6 @@ import ( "gitlab.com/SkynetLabs/skyd/skymodules" "go.mongodb.org/mongo-driver/bson" "go.mongodb.org/mongo-driver/bson/primitive" - "go.mongodb.org/mongo-driver/mongo" "go.mongodb.org/mongo-driver/mongo/options" ) @@ -51,33 +50,14 @@ func (db *DB) Skylink(ctx context.Context, skylink string) (*Skylink, error) { } // Try to find the skylink in the database. filter := bson.D{{"skylink", skylinkHash}} - sr := db.staticSkylinks.FindOne(ctx, filter) - err = sr.Decode(&skylinkRec) - if errors.Contains(err, mongo.ErrNoDocuments) { - // It's not there, upsert it. We use upsert instead of insert in order - // to avoid races. And we use an update object instead of just passing - // the skylink record to UpdateOne because we want to omit the _id in - // case it has a zero value. The struct tags instruct the compiler to - // omit it when it's empty but that doesn't cover the case where it's - // zero because in that case it's a valid array of ints which happen to - // be zeros. - upsert := bson.M{"$set": bson.M{"skylink": skylinkHash}} - opts := options.Update().SetUpsert(true) - var ur *mongo.UpdateResult - ur, err = db.staticSkylinks.UpdateOne(ctx, filter, upsert, opts) - if err != nil { - return nil, err - } - // The UpsertedID might be nil in case the skylink got added to the DB - // by another server in between the calls. In that case we'll fetch the - // skylink record from the DB. - if ur.UpsertedID != nil { - skylinkRec.ID = ur.UpsertedID.(primitive.ObjectID) - } else { - sr = db.staticSkylinks.FindOne(ctx, filter) - err = sr.Decode(&skylinkRec) - } + upsert := bson.M{"$setOnInsert": bson.M{"skylink": skylinkHash}} + after := options.After + opts := &options.FindOneAndUpdateOptions{ + ReturnDocument: &after, + Upsert: &True, } + sr := db.staticSkylinks.FindOneAndUpdate(ctx, filter, upsert, opts) + err = sr.Decode(&skylinkRec) if err != nil { return nil, err }