Repository: google/exposure-notifications-server (go)

Issue:
Evaluate holding current day's key
Currently, a key that is still valid (rolling period + rolling period count covers the current time) causes a whole publish API request to fail.

As this has come up a couple of times, perhaps it is better to hold the still valid key until the end of the UTC day, but allow others to be sent out immediately. 

/assign

Pull request description:
Accept, but embargo currently valid keys
Instead of rejecting keys that have started before the instant now, but aren't yet expired, accept those keys and emabargo them until they can no longer be used.
This is done by setting the createdAt time on that key.

There is an invariant here that there can only be one key in this state since we don't allow overlapping intervals and we don't allow keys to start in the future.

/fixes #543

Patch:
diff --git a/internal/generate/handler.go b/internal/generate/handler.go
index 210afe5..123fe1e 100644
--- a/internal/generate/handler.go
+++ b/internal/generate/handler.go
@@ -87,7 +87,7 @@ func (h *generateHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 			AppPackageName: "generated.data",
 		}
 
-		exposures, err := h.transformer.TransformPublish(&publish, batchTime)
+		exposures, err := h.transformer.TransformPublish(ctx, &publish, batchTime)
 		if err != nil {
 			message := fmt.Sprintf("Error transofmring generated exposures: %v", err)
 			span.SetStatus(trace.Status{Code: trace.StatusCodeInternal, Message: message})
diff --git a/internal/publish/config.go b/internal/publish/config.go
index ecca802..5574d75 100644
--- a/internal/publish/config.go
+++ b/internal/publish/config.go
@@ -43,8 +43,8 @@ type Config struct {
 	TruncateWindow     time.Duration `envconfig:"TRUNCATE_WINDOW" default:"1h"`
 
 	// Flags for local development and testing.
-	DebugAPIResponses   bool `envconfig:"DEBUG_API_RESPONSES"`
-	DebugAllowRestOfDay bool `envconfig:"DEBUG_ALLOW_REST_OF_DAY"`
+	DebugAPIResponses       bool `envconfig:"DEBUG_API_RESPONSES"`
+	DebugReleaseSameDayKeys bool `envconfig:"DEBUG_RELEASE_SAME_DAY_KEYS"`
 }
 
 func (c *Config) AuthorizedAppConfig() *authorizedapp.Config {
diff --git a/internal/publish/model/exposure_model.go b/internal/publish/model/exposure_model.go
index ea87013..6c51b94 100644
--- a/internal/publish/model/exposure_model.go
+++ b/internal/publish/model/exposure_model.go
@@ -15,12 +15,14 @@
 package model
 
 import (
+	"context"
 	"fmt"
 	"sort"
 	"strings"
 	"time"
 
 	"github.com/google/exposure-notifications-server/internal/base64util"
+	"github.com/google/exposure-notifications-server/internal/logging"
 
 	verifyapi "github.com/google/exposure-notifications-server/pkg/api/v1alpha1"
 )
@@ -92,18 +94,24 @@ func TruncateWindow(t time.Time, d time.Duration) time.Time {
 	return t.Truncate(d)
 }
 
+// TimeForIntervalNumber returns the time at which a specific interval starts.
+// The interval number * 600 (10m = 600s) is the corresponding unix timestamp.
+func TimeForIntervalNumber(interval int32) time.Time {
+	return time.Unix(int64(verifyapi.IntervalLength.Seconds())*int64(interval), 0)
+}
+
 // Transformer represents a configured Publish -> Exposure[] transformer.
 type Transformer struct {
 	maxExposureKeys     int
 	maxIntervalStartAge time.Duration // How many intervals old does this server accept?
 	truncateWindow      time.Duration
-	debugAllowRestOfDay bool // raises end time of keys to the end of day, but doesn't embargo. For e2e testing only.
+	debugReleaseSameDay bool // If true, still valid keys are not embargoed.
 }
 
 // NewTransformer creates a transformer for turning publish API requests into
 // records for insertion into the database. On the call to TransformPublish
 // all data is validated according to the transformer that is used.
-func NewTransformer(maxExposureKeys int, maxIntervalStartAge time.Duration, truncateWindow time.Duration, allowRestOfDay bool) (*Transformer, error) {
+func NewTransformer(maxExposureKeys int, maxIntervalStartAge time.Duration, truncateWindow time.Duration, releaseSameDayKeys bool) (*Transformer, error) {
 	if maxExposureKeys < 0 || maxExposureKeys > verifyapi.MaxKeysPerPublish {
 		return nil, fmt.Errorf("maxExposureKeys must be > 0 and <= %v, got %v", verifyapi.MaxKeysPerPublish, maxExposureKeys)
 	}
@@ -111,10 +119,19 @@ func NewTransformer(maxExposureKeys int, maxIntervalStartAge time.Duration, trun
 		maxExposureKeys:     maxExposureKeys,
 		maxIntervalStartAge: maxIntervalStartAge,
 		truncateWindow:      truncateWindow,
-		debugAllowRestOfDay: allowRestOfDay,
+		debugReleaseSameDay: releaseSameDayKeys,
 	}, nil
 }
 
+type KeyTransform struct {
+	MinStartInterval      int32
+	MaxStartInterval      int32
+	MaxEndInteral         int32
+	CreatedAt             time.Time
+	ReleaseStillValidKeys bool
+	BatchWindow           time.Duration
+}
+
 // TransformExposureKey converts individual key data to an exposure entity.
 // Validations during the transform include:
 //
@@ -122,7 +139,7 @@ func NewTransformer(maxExposureKeys int, maxIntervalStartAge time.Duration, trun
 // * minInterval <= interval number <= maxInterval
 // * MinIntervalCount <= interval count <= MaxIntervalCount
 //
-func TransformExposureKey(exposureKey verifyapi.ExposureKey, appPackageName string, upcaseRegions []string, createdAt time.Time, minIntervalNumber, maxIntervalNumber int32) (*Exposure, error) {
+func TransformExposureKey(exposureKey verifyapi.ExposureKey, appPackageName string, upcaseRegions []string, settings *KeyTransform) (*Exposure, error) {
 	binKey, err := base64util.DecodeString(exposureKey.Key)
 	if err != nil {
 		return nil, err
@@ -137,17 +154,20 @@ func TransformExposureKey(exposureKey verifyapi.ExposureKey, appPackageName stri
 	}
 
 	// Validate the IntervalNumber.
-	if exposureKey.IntervalNumber < minIntervalNumber {
-		return nil, fmt.Errorf("interval number %v is too old, must be >= %v", exposureKey.IntervalNumber, minIntervalNumber)
+	if exposureKey.IntervalNumber < settings.MinStartInterval {
+		return nil, fmt.Errorf("interval number %v is too old, must be >= %v", exposureKey.IntervalNumber, settings.MinStartInterval)
 	}
-	if exposureKey.IntervalNumber >= maxIntervalNumber {
-		return nil, fmt.Errorf("interval number %v is in the future, must be < %v", exposureKey.IntervalNumber, maxIntervalNumber)
+	if exposureKey.IntervalNumber > settings.MaxStartInterval {
+		return nil, fmt.Errorf("interval number %v is in the future, must be <= %v", exposureKey.IntervalNumber, settings.MaxStartInterval)
 	}
 
-	// Validate that the key is no longer effective.
-	if exposureKey.IntervalNumber+exposureKey.IntervalCount > maxIntervalNumber {
-		return nil, fmt.Errorf("interval number %v + interval count %v represents a key that is still valid, must end <= %v",
-			exposureKey.IntervalNumber, exposureKey.IntervalCount, maxIntervalNumber)
+	createdAt := settings.CreatedAt
+	// If the key is valid beyond the current interval number. Adjust the createdAt time for the key.
+	if exposureKey.IntervalNumber+exposureKey.IntervalCount > settings.MaxStartInterval {
+		// key is still valid. The created At for this key needs to be adjusted unless debuggin is enabled.
+		if !settings.ReleaseStillValidKeys {
+			createdAt = TimeForIntervalNumber(exposureKey.IntervalNumber + exposureKey.IntervalCount).Truncate(settings.BatchWindow)
+		}
 	}
 
 	if tr := exposureKey.TransmissionRisk; tr < verifyapi.MinTransmissionRisk || tr > verifyapi.MaxTransmissionRisk {
@@ -172,7 +192,11 @@ func TransformExposureKey(exposureKey verifyapi.ExposureKey, appPackageName stri
 // * 0 exposure Keys in the requests
 // * > Transformer.maxExposureKeys in the request
 //
-func (t *Transformer) TransformPublish(inData *verifyapi.Publish, batchTime time.Time) ([]*Exposure, error) {
+func (t *Transformer) TransformPublish(ctx context.Context, inData *verifyapi.Publish, batchTime time.Time) ([]*Exposure, error) {
+	if t.debugReleaseSameDay {
+		logging.FromContext(ctx).Errorf("DEBUG SERVER - Current day keys are not being embargoed.")
+	}
+
 	// Validate the number of keys that want to be published.
 	if len(inData.Keys) == 0 {
 		return nil, fmt.Errorf("no exposure keys in publish request")
@@ -181,18 +205,19 @@ func (t *Transformer) TransformPublish(inData *verifyapi.Publish, batchTime time
 		return nil, fmt.Errorf("too many exposure keys in publish: %v, max of %v is allowed", len(inData.Keys), t.maxExposureKeys)
 	}
 
-	createdAt := TruncateWindow(batchTime, t.truncateWindow)
+	defaultCreatedAt := TruncateWindow(batchTime, t.truncateWindow)
 	entities := make([]*Exposure, 0, len(inData.Keys))
 
-	// An exposure key must have an interval >= minInterval (max configured age)
-	minIntervalNumber := IntervalNumber(batchTime.Add(-1 * t.maxIntervalStartAge))
-	// And have an interval <= maxInterval (configured allowed clock skew)
-	maxIntervalNumber := IntervalNumber(batchTime)
-
-	// If, for testing, we are accepting keys that are valid the rest of the day:
-	// adjust the maxIntervalNumber to be the end of the UTC day.
-	if t.debugAllowRestOfDay {
-		maxIntervalNumber = IntervalNumber(batchTime.Add(24 * time.Hour).Truncate(24 * time.Hour))
+	settings := KeyTransform{
+		// An exposure key must have an interval >= minInterval (max configured age)
+		MinStartInterval: IntervalNumber(batchTime.Add(-1 * t.maxIntervalStartAge)),
+		// A key must have been issued on the device in the current interval or earlier.
+		MaxStartInterval: IntervalNumber(batchTime),
+		// And the max valid interval is the maxStartInterval + 144
+		MaxEndInteral:         IntervalNumber(batchTime) + verifyapi.MaxIntervalCount,
+		CreatedAt:             defaultCreatedAt,
+		ReleaseStillValidKeys: t.debugReleaseSameDay,
+		BatchWindow:           t.truncateWindow,
 	}
 
 	// Regions are a multi-value property, uppercase them for storage.
@@ -205,7 +230,7 @@ func (t *Transformer) TransformPublish(inData *verifyapi.Publish, batchTime time
 	}
 
 	for _, exposureKey := range inData.Keys {
-		exposure, err := TransformExposureKey(exposureKey, inData.AppPackageName, upcaseRegions, createdAt, minIntervalNumber, maxIntervalNumber)
+		exposure, err := TransformExposureKey(exposureKey, inData.AppPackageName, upcaseRegions, &settings)
 		if err != nil {
 			return nil, fmt.Errorf("invalid publish data: %v", err)
 		}
diff --git a/internal/publish/publish.go b/internal/publish/publish.go
index 7789deb..ab855cf 100644
--- a/internal/publish/publish.go
+++ b/internal/publish/publish.go
@@ -46,7 +46,7 @@ func NewHandler(ctx context.Context, config *Config, env *serverenv.ServerEnv) (
 		return nil, fmt.Errorf("missing AuthorizedApp provider in server environment")
 	}
 
-	transformer, err := model.NewTransformer(config.MaxKeysOnPublish, config.MaxIntervalAge, config.TruncateWindow, config.DebugAllowRestOfDay)
+	transformer, err := model.NewTransformer(config.MaxKeysOnPublish, config.MaxIntervalAge, config.TruncateWindow, config.DebugReleaseSameDayKeys)
 	if err != nil {
 		return nil, fmt.Errorf("model.NewTransformer: %w", err)
 	}
@@ -160,7 +160,7 @@ func (h *publishHandler) handleRequest(w http.ResponseWriter, r *http.Request) r
 	}
 
 	batchTime := time.Now()
-	exposures, err := h.transformer.TransformPublish(&data, batchTime)
+	exposures, err := h.transformer.TransformPublish(ctx, &data, batchTime)
 	if err != nil {
 		message := fmt.Sprintf("unable to read request data: %v", err)
 		logger.Error(message)
diff --git a/tools/export-generate/main.go b/tools/export-generate/main.go
index bd07983..0963948 100644
--- a/tools/export-generate/main.go
+++ b/tools/export-generate/main.go
@@ -106,7 +106,13 @@ func main() {
 			log.Fatalf("unable to parse json: %v", err)
 		}
 		for _, k := range data.Keys {
-			ek, err := publishmodel.TransformExposureKey(k, "", []string{}, time.Now(), int32(0), int32(time.Now().Unix()/600))
+			settings := publishmodel.KeyTransform{
+				MinStartInterval:      0,
+				MaxStartInterval:      int32(time.Now().Unix() / 600),
+				CreatedAt:             time.Now(),
+				ReleaseStillValidKeys: false,
+			}
+			ek, err := publishmodel.TransformExposureKey(k, "", []string{}, &settings)
 			if err != nil {
 				log.Fatalf("invalid exposure key: %v", err)
 			}


Test patch:
diff --git a/internal/publish/model/exposure_model_test.go b/internal/publish/model/exposure_model_test.go
index 06284ed..dc44c71 100644
--- a/internal/publish/model/exposure_model_test.go
+++ b/internal/publish/model/exposure_model_test.go
@@ -15,6 +15,7 @@
 package model
 
 import (
+	"context"
 	"crypto/rand"
 	"encoding/base64"
 	"fmt"
@@ -28,6 +29,18 @@ import (
 	"github.com/google/go-cmp/cmp"
 )
 
+func TestIntervalNumber(t *testing.T) {
+	// Since time to interval is lossy, truncate down to the beginnging of a window.
+	now := time.Now().Truncate(verifyapi.IntervalLength)
+
+	interval := IntervalNumber(now)
+	timeForInterval := TimeForIntervalNumber(interval)
+
+	if now.Unix() != timeForInterval.Unix() {
+		t.Errorf("interval mismatch, want: %v got %v", now.Unix(), timeForInterval.Unix())
+	}
+}
+
 func TestInvalidNew(t *testing.T) {
 	errMsg := fmt.Sprintf("maxExposureKeys must be > 0 and <= %v", verifyapi.MaxKeysPerPublish)
 	cases := []struct {
@@ -53,6 +66,7 @@ func TestInvalidNew(t *testing.T) {
 }
 
 func TestInvalidBase64(t *testing.T) {
+	ctx := context.Background()
 	transformer, err := NewTransformer(1, time.Hour*24, time.Hour, false)
 	if err != nil {
 		t.Fatalf("error creating transformer: %v", err)
@@ -69,7 +83,7 @@ func TestInvalidBase64(t *testing.T) {
 	}
 	batchTime := time.Date(2020, 3, 1, 10, 43, 1, 0, time.UTC)
 
-	_, err = transformer.TransformPublish(source, batchTime)
+	_, err = transformer.TransformPublish(ctx, source, batchTime)
 	expErr := `invalid publish data: illegal base64 data at input byte 4`
 	if err == nil || err.Error() != expErr {
 		t.Errorf("expected error '%v', got: %v", expErr, err)
@@ -219,20 +233,7 @@ func TestPublishValidation(t *testing.T) {
 					},
 				},
 			},
-			m: fmt.Sprintf("interval number %v is in the future, must be < %v", currentInterval+1, currentInterval),
-		},
-		{
-			name: "interval end too high",
-			p: &verifyapi.Publish{
-				Keys: []verifyapi.ExposureKey{
-					{
-						Key:            encodeKey(generateKey(t)),
-						IntervalNumber: currentInterval - 143,
-						IntervalCount:  144,
-					},
-				},
-			},
-			m: fmt.Sprintf("interval number %v + interval count %v represents a key that is still valid, must end <= %v", currentInterval-143, 144, currentInterval),
+			m: fmt.Sprintf("interval number %v is in the future, must be <= %v", currentInterval+1, currentInterval),
 		},
 		{
 			name: "DEBUG: allow end of current UTC day still valid",
@@ -251,12 +252,13 @@ func TestPublishValidation(t *testing.T) {
 
 	for _, c := range cases {
 		t.Run(c.name, func(t *testing.T) {
+			ctx := context.Background()
 			tf, err := NewTransformer(2, maxAge, time.Hour, c.sameDay)
 			if err != nil {
 				t.Fatalf("unepected error: %v", err)
 			}
 
-			_, err = tf.TransformPublish(c.p, captureStartTime)
+			_, err = tf.TransformPublish(ctx, c.p, captureStartTime)
 			if err == nil {
 				if c.m != "" {
 					t.Errorf("want error '%v', got nil", c.m)
@@ -292,6 +294,74 @@ func decodeKey(b64key string, t *testing.T) []byte {
 	return k
 }
 
+func TestStillValidKey(t *testing.T) {
+	now := time.Now()
+	batchWindow := TruncateWindow(now, time.Minute)
+	intervalNumber := IntervalNumber(now) - 1
+
+	cases := []struct {
+		name               string
+		source             verifyapi.Publish
+		createdAt          time.Time
+		releaseSameDayKeys bool
+	}{
+		{
+			name: "release same day keys",
+			source: verifyapi.Publish{
+				Keys: []verifyapi.ExposureKey{
+					{
+						Key:              encodeKey(generateKey(t)),
+						IntervalNumber:   intervalNumber,
+						IntervalCount:    verifyapi.MaxIntervalCount,
+						TransmissionRisk: 1,
+					},
+				},
+			},
+			createdAt:          batchWindow,
+			releaseSameDayKeys: true,
+		},
+		{
+			name: "proper embargo",
+			source: verifyapi.Publish{
+				Keys: []verifyapi.ExposureKey{
+					{
+						Key:              encodeKey(generateKey(t)),
+						IntervalNumber:   intervalNumber,
+						IntervalCount:    verifyapi.MaxIntervalCount,
+						TransmissionRisk: 1,
+					},
+				},
+			},
+			createdAt:          TruncateWindow(TimeForIntervalNumber(intervalNumber+verifyapi.MaxIntervalCount), time.Minute),
+			releaseSameDayKeys: false,
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			allowedAge := 2 * 24 * time.Hour
+			transformer, err := NewTransformer(10, allowedAge, time.Minute, tc.releaseSameDayKeys)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			ctx := context.Background()
+			tf, err := transformer.TransformPublish(ctx, &tc.source, now)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			if len(tf) != 1 {
+				t.Fatalf("wrong number of keys, want: 1 got :%v", len(tf))
+			}
+
+			if !tf[0].CreatedAt.Equal(tc.createdAt) {
+				t.Errorf("wrong createdAt time, want: %v got: %v", tc.createdAt, tf[0].CreatedAt)
+			}
+		})
+	}
+}
+
 func TestTransform(t *testing.T) {
 	captureStartTime := time.Date(2020, 2, 29, 11, 15, 1, 0, time.UTC)
 	intervalNumber := IntervalNumber(captureStartTime)
@@ -374,7 +444,8 @@ func TestTransform(t *testing.T) {
 	if err != nil {
 		t.Fatalf("NewTransformer returned unexpected error: %v", err)
 	}
-	got, err := transformer.TransformPublish(source, batchTime)
+	ctx := context.Background()
+	got, err := transformer.TransformPublish(ctx, source, batchTime)
 	if err != nil {
 		t.Fatalf("TransformPublish returned unexpected error: %v", err)
 	}
@@ -436,13 +507,14 @@ func TestTransformOverlapping(t *testing.T) {
 
 	for _, c := range cases {
 		t.Run(c.name, func(t *testing.T) {
+			ctx := context.Background()
 			batchTime := captureStartTime.Add(time.Hour * 24 * 7)
 			allowedAge := 14 * 24 * time.Hour
 			transformer, err := NewTransformer(10, allowedAge, time.Hour, false)
 			if err != nil {
 				t.Fatalf("NewTransformer returned unexpected error: %v", err)
 			}
-			_, err = transformer.TransformPublish(&c.source, batchTime)
+			_, err = transformer.TransformPublish(ctx, &c.source, batchTime)
 			if err == nil {
 				t.Fatalf("Expected error, got nil")
 			}