Repository: google/exposure-notifications-server
Pull request #979 (2020-09-10 22:11:13)

Return more specific error codes for key_already_revised and invalid_report_type_transition
Fixes GH-974

**Release Note**

<!-- Enter your extended release note in the below block. If the PR requires
additional action from users switching to the new release, include the string
"action required". If no release note is required, write "NONE". -->

```release-note
Return specific error responses for `key_already_revised` and `invalid_report_type_transition` on publish
```

/assign @mikehelmick 

google-oss-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/google/exposure-notifications-server/pull/979#" title="Author self-approved">sethvargo</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=google%2Fexposure-notifications-server).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/google/exposure-notifications-server/blob/main/OWNERS)~~ [sethvargo]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
sethvargo: /cc @peetsasaki 
google-oss-robot: @sethvargo: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-en-server-release-unit | c13b2f6fc61b7d747b514401f2b5fabd4740dd30 | [link](https://oss-prow.knative.dev/view/gs/oss-prow/pr-logs/pull/google_exposure-notifications-server/979/pull-en-server-release-unit/1304180874933178369) | `/test pull-en-server-release-unit`

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
google-oss-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/google/exposure-notifications-server/pull/979#pullrequestreview-486351794" title="Approved">mikehelmick</a>*, *<a href="https://github.com/google/exposure-notifications-server/pull/979#" title="Author self-approved">sethvargo</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=google%2Fexposure-notifications-server).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/google/exposure-notifications-server/blob/main/OWNERS)~~ [mikehelmick,sethvargo]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
google-oss-robot: @sethvargo: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-en-server-release-unit | 736c4881b7d47d7fea2c5aef0de9aa1d5ec57b72 | [link](https://oss-prow.knative.dev/view/gs/oss-prow/pr-logs/pull/google_exposure-notifications-server/979/pull-en-server-release-unit/1304182636561829888) | `/test pull-en-server-release-unit`

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
google-oss-robot: New changes are detected. LGTM label has been removed.
google-oss-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/google/exposure-notifications-server/pull/979#pullrequestreview-486353589" title="Approved">mikehelmick</a>*, *<a href="https://github.com/google/exposure-notifications-server/pull/979#" title="Author self-approved">sethvargo</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=google%2Fexposure-notifications-server).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/google/exposure-notifications-server/blob/main/OWNERS)~~ [mikehelmick,sethvargo]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/internal/authorizedapp/database_provider.go b/internal/authorizedapp/database_provider.go
index cec74b4..3004911 100644
--- a/internal/authorizedapp/database_provider.go
+++ b/internal/authorizedapp/database_provider.go
@@ -17,7 +17,6 @@ package authorizedapp
 import (
 	"context"
 	"fmt"
-	"log"
 	"strings"
 	"time"
 
@@ -93,8 +92,6 @@ func (p *DatabaseProvider) AppConfig(ctx context.Context, name string) (*model.A
 		return nil, ErrAppNotFound
 	}
 
-	log.Printf("AppConfig: %+v %v", config, err)
-
 	// Returned config.
 	return config, nil
 }
diff --git a/internal/publish/model/exposure_model.go b/internal/publish/model/exposure_model.go
index e638d18..5540ec9 100644
--- a/internal/publish/model/exposure_model.go
+++ b/internal/publish/model/exposure_model.go
@@ -41,6 +41,20 @@ var (
 	ErrorKeyAlreadyRevised = fmt.Errorf("key has already been revised and cannot be revised again")
 )
 
+var _ error = (*ErrorKeyInvalidReportTypeTransition)(nil)
+
+// ErrorKeyInvalidReportTypeTransition is an error returned when the TEK tried
+// to move to an invalid state (e.g. positive -> likely).
+type ErrorKeyInvalidReportTypeTransition struct {
+	from, to string
+}
+
+// Error implements error.
+func (e *ErrorKeyInvalidReportTypeTransition) Error() string {
+	return fmt.Sprintf("invalid report type transition: cannot transition from %q to %q",
+		e.from, e.to)
+}
+
 // Exposure represents the record as stored in the database
 type Exposure struct {
 	ExposureKey      []byte
@@ -93,7 +107,10 @@ func (e *Exposure) Revise(in *Exposure) (bool, error) {
 		eReportType = verifyapi.ReportTypeClinical
 	}
 	if !(eReportType == verifyapi.ReportTypeClinical && (in.ReportType == verifyapi.ReportTypeConfirmed || in.ReportType == verifyapi.ReportTypeNegative)) {
-		return false, fmt.Errorf("invalid report type transition, cannot transition from '%v' to '%v'", e.ReportType, in.ReportType)
+		return false, &ErrorKeyInvalidReportTypeTransition{
+			from: e.ReportType,
+			to:   in.ReportType,
+		}
 	}
 
 	// Update fields.
diff --git a/internal/publish/publish.go b/internal/publish/publish.go
index 7a7dda9..5a109f0 100644
--- a/internal/publish/publish.go
+++ b/internal/publish/publish.go
@@ -347,6 +347,7 @@ func (h *PublishHandler) process(ctx context.Context, data *verifyapi.Publish, b
 	if err != nil {
 		status := http.StatusBadRequest
 		var logMessage, errorMessage, errorCode string
+		var errInvalidReportTypeTransition *model.ErrorKeyInvalidReportTypeTransition
 		metric := "publish-revision-token-issue"
 		switch {
 		case decryptFail || errors.Is(err, database.ErrExistingKeyNotInToken) || errors.Is(err, database.ErrRevisionTokenMetadataMismatch):
@@ -357,6 +358,14 @@ func (h *PublishHandler) process(ctx context.Context, data *verifyapi.Publish, b
 			logMessage = "no revision token"
 			errorMessage = "no revision token, but sent existing keys"
 			errorCode = verifyapi.ErrorMissingRevisionToken
+		case errors.Is(err, model.ErrorKeyAlreadyRevised):
+			logMessage = "key already revised"
+			errorMessage = "key was already revised"
+			errorCode = verifyapi.ErrorKeyAlreadyRevised
+		case errors.As(err, &errInvalidReportTypeTransition):
+			logMessage = errInvalidReportTypeTransition.Error()
+			errorMessage = errInvalidReportTypeTransition.Error()
+			errorCode = verifyapi.ErrorInvalidReportTypeTransition
 		default:
 			logMessage = fmt.Sprintf("error writing exposure record: %v", err)
 			errorMessage = http.StatusText(http.StatusInternalServerError)
diff --git a/pkg/api/v1/exposure_types.go b/pkg/api/v1/exposure_types.go
index dc1c52c..4cb6a93 100644
--- a/pkg/api/v1/exposure_types.go
+++ b/pkg/api/v1/exposure_types.go
@@ -54,8 +54,15 @@ const (
 	// ErrorInvalidRevisionToken indicates a revision token was passed, but is missing a
 	// key or has invalid metadata.
 	ErrorInvalidRevisionToken = "invalid_revision_token"
-	// ErrorPartialFailure indicates that some exposure keys in the publish request had invalid
-	// data (size, timing metadata) and were dropped. Other keys were saved.
+	// ErrorKeyAlreadyRevised indicates one of the uploaded TEKs was marked for
+	// revision, but it has already been revised.
+	ErrorKeyAlreadyRevised = "key_already_revised"
+	// ErrorInvalidReportTypeTransition indicates an uploaded TEK tried to
+	// transition to an invalid state (like "positive" -> "likely").
+	ErrorInvalidReportTypeTransition = "invalid_report_type_transition"
+	// ErrorPartialFailure indicates that some exposure keys in the publish
+	// request had invalid data (size, timing metadata) and were dropped. Other
+	// keys were saved.
 	ErrorPartialFailure = "partial_failure"
 )
 


Test patch:
diff --git a/internal/publish/model/exposure_model_test.go b/internal/publish/model/exposure_model_test.go
index 564b2f1..6734bf4 100644
--- a/internal/publish/model/exposure_model_test.go
+++ b/internal/publish/model/exposure_model_test.go
@@ -1169,7 +1169,7 @@ func TestExposureReview(t *testing.T) {
 				ReportType: verifyapi.ReportTypeClinical,
 			},
 			needsRevision: false,
-			err:           "invalid report type transition, cannot transition from 'confirmed' to 'likely'",
+			err:           `invalid report type transition: cannot transition from "confirmed" to "likely"`,
 		},
 		{
 			name: "invalid_transition_from_empty_report_type",
@@ -1181,7 +1181,7 @@ func TestExposureReview(t *testing.T) {
 				ReportType: verifyapi.ReportTypeClinical,
 			},
 			needsRevision: false,
-			err:           "invalid report type transition, cannot transition from '' to 'likely'",
+			err:           `invalid report type transition: cannot transition from "" to "likely"`,
 		},
 		{
 			name: "valid_transition_from_empty_report_type",
diff --git a/internal/publish/publish_test.go b/internal/publish/publish_test.go
index 42d71ab..5bac813 100644
--- a/internal/publish/publish_test.go
+++ b/internal/publish/publish_test.go
@@ -22,7 +22,6 @@ import (
 	"encoding/json"
 	"fmt"
 	"io/ioutil"
-	"log"
 	"net/http"
 	"net/http/httptest"
 	"sort"
@@ -525,8 +524,6 @@ func TestPublishWithBypass(t *testing.T) {
 					t.Fatal(err)
 				}
 
-				log.Printf("\n\n%#v\n\n", string(respBytes))
-
 				var response verifyapi.PublishResponse
 				if err := json.Unmarshal(respBytes, &response); err != nil {
 					t.Fatalf("unable to unmarshal response body: %v; data: %v", err, string(respBytes))
@@ -773,12 +770,13 @@ func TestKeyRevision(t *testing.T) {
 	pubDB := pubdb.New(testDB)
 
 	cases := []struct {
-		Name           string
-		Publish        verifyapi.Publish
-		ErrorCode      string
-		RevErrorCode   string
-		RevTokenMesser RevisionTokenChanger
-		PublishMesser  PublishDataChanger
+		Name               string
+		Publish            verifyapi.Publish
+		ErrorCode          string
+		RevErrorCode       string
+		RevTokenMesser     RevisionTokenChanger
+		PublishMesser      PublishDataChanger
+		VerificationMesser func(c *testutil.JWTConfig)
 	}{
 		{
 			Name: "normal_revision",
@@ -805,6 +803,65 @@ func TestKeyRevision(t *testing.T) {
 			},
 			PublishMesser: PublishIdentity,
 		},
+		{
+			Name: "key_already_revised",
+			Publish: verifyapi.Publish{
+				Keys:                util.GenerateExposureKeys(2, 0, false),
+				Traveler:            false,
+				HealthAuthorityID:   haName,
+				VerificationPayload: "totally not a JWT",
+			},
+			RevErrorCode:   verifyapi.ErrorKeyAlreadyRevised,
+			RevTokenMesser: TokenIdentity,
+			PublishMesser: func(t *testing.T, data *verifyapi.Publish) (bool, *verifyapi.Publish) {
+				// Create some keys in the database
+				var incoming []*model.Exposure
+				for _, key := range data.Keys {
+					b64Key, err := base64util.DecodeString(key.Key)
+					if err != nil {
+						t.Fatal(err)
+					}
+					incoming = append(incoming, &model.Exposure{
+						ExposureKey:    b64Key,
+						IntervalNumber: key.IntervalNumber,
+						IntervalCount:  key.IntervalCount,
+						ReportType:     verifyapi.ReportTypeClinical,
+					})
+				}
+				if _, err := pubDB.InsertAndReviseExposures(ctx, &pubdb.InsertAndReviseExposuresRequest{
+					Incoming: incoming,
+				}); err != nil {
+					t.Fatal(err)
+				}
+
+				// Revise them
+				for _, key := range incoming {
+					key.ReportType = verifyapi.ReportTypeConfirmed
+				}
+				if _, err := pubDB.InsertAndReviseExposures(ctx, &pubdb.InsertAndReviseExposuresRequest{
+					Incoming: incoming,
+				}); err != nil {
+					t.Fatal(err)
+				}
+
+				return PublishIdentity(t, data)
+			},
+		},
+		{
+			Name: "invalid_report_type_transition",
+			Publish: verifyapi.Publish{
+				Keys:                util.GenerateExposureKeys(2, 0, false),
+				Traveler:            false,
+				HealthAuthorityID:   haName,
+				VerificationPayload: "totally not a JWT",
+			},
+			RevErrorCode:   verifyapi.ErrorInvalidReportTypeTransition,
+			RevTokenMesser: TokenIdentity,
+			PublishMesser:  PublishIdentity,
+			VerificationMesser: func(c *testutil.JWTConfig) {
+				c.ReportType = "totally_not_valid"
+			},
+		},
 		{
 			Name: "token_missing_keys",
 			Publish: verifyapi.Publish{
@@ -926,7 +983,7 @@ func TestKeyRevision(t *testing.T) {
 				}
 
 				if response.Code != tc.ErrorCode {
-					t.Fatalf("wrong code on initial publish, want %v, got %v", tc.ErrorCode, response.Code)
+					t.Fatalf("wrong code on initial publish, want %#v, got %#v", tc.ErrorCode, response.Code)
 				}
 				revisionToken = response.RevisionToken
 			}
@@ -942,14 +999,19 @@ func TestKeyRevision(t *testing.T) {
 				keysAreRevisions, publishData = tc.PublishMesser(t, &tc.Publish)
 				tc.Publish = *publishData
 
-				cfg := testutil.JWTConfig{
+				cfg := &testutil.JWTConfig{
 					HealthAuthority:    healthAuthority,
 					HealthAuthorityKey: healthAuthorityKey,
 					ExposureKeys:       tc.Publish.Keys,
 					Key:                signingKey.Key,
 					ReportType:         verifyapi.ReportTypeConfirmed,
 				}
-				verification, salt := testutil.IssueJWT(t, &cfg)
+
+				if messer := tc.VerificationMesser; messer != nil {
+					messer(cfg)
+				}
+
+				verification, salt := testutil.IssueJWT(t, cfg)
 				tc.Publish.VerificationPayload = verification
 				tc.Publish.HMACKey = salt
 
@@ -978,7 +1040,7 @@ func TestKeyRevision(t *testing.T) {
 				}
 
 				if response.Code != tc.RevErrorCode {
-					t.Fatalf("wrong code on initial publish, want %v, got %v", tc.ErrorCode, response.Code)
+					t.Fatalf("wrong code on revision, want %#v, got %#v", tc.RevErrorCode, response.Code)
 				}
 			}