From 696cc1b59b18627fec63915070f4d8c5b3e3250d Mon Sep 17 00:00:00 2001 From: Arne Luenser Date: Wed, 17 Apr 2024 19:16:32 +0200 Subject: [PATCH] fix: allow updating just the verified_at timestamp of addresses (#3880) --- identity/handler_test.go | 92 +++++++++++++++++++ identity/identity_verification.go | 2 +- identity/identity_verification_test.go | 9 +- .../sql/identity/persister_identity.go | 3 + 4 files changed, 100 insertions(+), 6 deletions(-) diff --git a/identity/handler_test.go b/identity/handler_test.go index bfdf1a86dfc3..ab2ed0c0cbdc 100644 --- a/identity/handler_test.go +++ b/identity/handler_test.go @@ -928,6 +928,98 @@ func TestHandler(t *testing.T) { } }) + t.Run("case=PATCH should update verified_at timestamp", func(t *testing.T) { + for name, ts := range map[string]*httptest.Server{"public": publicTS, "admin": adminTS} { + t.Run("endpoint="+name, func(t *testing.T) { + email := x.NewUUID().String() + "@ory.sh" + var cr identity.CreateIdentityBody + cr.SchemaID = "employee" + cr.Traits = []byte(`{"email":"` + email + `"}`) + res := send(t, ts, "POST", "/identities", http.StatusCreated, &cr) + assert.EqualValues(t, email, res.Get("recovery_addresses.0.value").String(), "%s", res.Raw) + assert.EqualValues(t, email, res.Get("verifiable_addresses.0.value").String(), "%s", res.Raw) + assert.Falsef(t, res.Get("verifiable_addresses.0.verified").Bool(), "%s", res.Raw) + assert.Falsef(t, res.Get("verifiable_addresses.0.verified_at").Exists(), "%s", res.Raw) + identityID := res.Get("id").String() + + // set to verified, should also update verified_at timestamp + patch1 := []patch{ + { + "op": "replace", + "path": "/verifiable_addresses/0/verified", + "value": true, + }, + } + + now := time.Now() + + res = send(t, ts, "PATCH", "/identities/"+identityID, http.StatusOK, &patch1) + assert.EqualValues(t, email, res.Get("recovery_addresses.0.value").String(), "%s", res.Raw) + assert.EqualValues(t, email, res.Get("verifiable_addresses.0.value").String(), "%s", res.Raw) + assert.Truef(t, res.Get("verifiable_addresses.0.verified").Bool(), "%s", res.Raw) + assert.WithinDurationf(t, now, res.Get("verifiable_addresses.0.updated_at").Time(), 5*time.Second, "%s", res.Raw) + assert.WithinDurationf(t, now, res.Get("verifiable_addresses.0.verified_at").Time(), 5*time.Second, "%s", res.Raw) + + res = get(t, ts, "/identities/"+identityID, http.StatusOK) + assert.EqualValues(t, email, res.Get("recovery_addresses.0.value").String(), "%s", res.Raw) + assert.EqualValues(t, email, res.Get("verifiable_addresses.0.value").String(), "%s", res.Raw) + assert.Truef(t, res.Get("verifiable_addresses.0.verified").Bool(), "%s", res.Raw) + assert.WithinDurationf(t, now, res.Get("verifiable_addresses.0.updated_at").Time(), 5*time.Second, "%s", res.Raw) + assert.WithinDurationf(t, now, res.Get("verifiable_addresses.0.verified_at").Time(), 5*time.Second, "%s", res.Raw) + + // update only verified_at timestamp + verifiedAt := time.Date(1999, 1, 7, 8, 23, 19, 0, time.UTC) + patch2 := []patch{ + { + "op": "replace", + "path": "/verifiable_addresses/0/verified_at", + "value": verifiedAt.Format(time.RFC3339), + }, + } + + now = time.Now() + res = send(t, ts, "PATCH", "/identities/"+identityID, http.StatusOK, &patch2) + assert.EqualValues(t, email, res.Get("recovery_addresses.0.value").String(), "%s", res.Raw) + assert.EqualValues(t, email, res.Get("verifiable_addresses.0.value").String(), "%s", res.Raw) + assert.Truef(t, res.Get("verifiable_addresses.0.verified").Bool(), "%s", res.Raw) + assert.Equalf(t, verifiedAt, res.Get("verifiable_addresses.0.verified_at").Time(), "%s", res.Raw) + assert.WithinDurationf(t, now, res.Get("verifiable_addresses.0.updated_at").Time(), 5*time.Second, "%s", res.Raw) + + res = get(t, ts, "/identities/"+identityID, http.StatusOK) + assert.EqualValues(t, email, res.Get("recovery_addresses.0.value").String(), "%s", res.Raw) + assert.EqualValues(t, email, res.Get("verifiable_addresses.0.value").String(), "%s", res.Raw) + assert.Truef(t, res.Get("verifiable_addresses.0.verified").Bool(), "%s", res.Raw) + assert.Equalf(t, verifiedAt, res.Get("verifiable_addresses.0.verified_at").Time(), "%s", res.Raw) + assert.WithinDurationf(t, now, res.Get("verifiable_addresses.0.updated_at").Time(), 5*time.Second, "%s", res.Raw) + + // remove verified status + patch3 := []patch{ + { + "op": "replace", + "path": "/verifiable_addresses/0/verified", + "value": false, + }, + } + + now = time.Now() + + res = send(t, ts, "PATCH", "/identities/"+identityID, http.StatusOK, &patch3) + assert.EqualValues(t, email, res.Get("recovery_addresses.0.value").String(), "%s", res.Raw) + assert.EqualValues(t, email, res.Get("verifiable_addresses.0.value").String(), "%s", res.Raw) + assert.Falsef(t, res.Get("verifiable_addresses.0.verified").Bool(), "%s", res.Raw) + assert.Falsef(t, res.Get("verifiable_addresses.0.verified_at").Exists(), "%s", res.Raw) + assert.WithinDurationf(t, now, res.Get("verifiable_addresses.0.updated_at").Time(), 5*time.Second, "%s", res.Raw) + + res = get(t, ts, "/identities/"+identityID, http.StatusOK) + assert.EqualValues(t, email, res.Get("recovery_addresses.0.value").String(), "%s", res.Raw) + assert.EqualValues(t, email, res.Get("verifiable_addresses.0.value").String(), "%s", res.Raw) + assert.Falsef(t, res.Get("verifiable_addresses.0.verified").Bool(), "%s", res.Raw) + assert.Falsef(t, res.Get("verifiable_addresses.0.verified_at").Exists(), "%s", res.Raw) + assert.WithinDurationf(t, now, res.Get("verifiable_addresses.0.updated_at").Time(), 5*time.Second, "%s", res.Raw) + }) + } + }) + t.Run("case=PATCH update should not persist if schema id is invalid", func(t *testing.T) { uuid := x.NewUUID().String() i := &identity.Identity{Traits: identity.Traits(fmt.Sprintf(`{"subject":"%s"}`, uuid))} diff --git a/identity/identity_verification.go b/identity/identity_verification.go index 54ac435ec9fd..251fee019d3b 100644 --- a/identity/identity_verification.go +++ b/identity/identity_verification.go @@ -118,5 +118,5 @@ func (a VerifiableAddress) ValidateNID() error { // Hash returns a unique string representation for the recovery address. func (a VerifiableAddress) Hash() string { - return fmt.Sprintf("%v|%v|%v|%v|%v|%v", a.Value, a.Verified, a.Via, a.Status, a.IdentityID, a.NID) + return fmt.Sprintf("%v|%v|%v|%v|%v|%v|%v", a.Value, a.Verified, a.Via, a.Status, a.VerifiedAt, a.IdentityID, a.NID) } diff --git a/identity/identity_verification_test.go b/identity/identity_verification_test.go index 4559a5759dba..6f3ca2c69524 100644 --- a/identity/identity_verification_test.go +++ b/identity/identity_verification_test.go @@ -34,10 +34,10 @@ func TestNewVerifiableEmailAddress(t *testing.T) { } var tagsIgnoredForHashing = map[string]struct{}{ - "id": {}, - "created_at": {}, - "updated_at": {}, - "verified_at": {}, + "id": {}, + "created_at": {}, + "updated_at": {}, + // "verified_at": {}, // we explicitly want to be able to update just this field and nothing else } func reflectiveHash(record any) string { @@ -102,5 +102,4 @@ func TestVerifiableAddress_Hash(t *testing.T) { ) }) } - } diff --git a/persistence/sql/identity/persister_identity.go b/persistence/sql/identity/persister_identity.go index 8c001bc87e6e..ea532485944e 100644 --- a/persistence/sql/identity/persister_identity.go +++ b/persistence/sql/identity/persister_identity.go @@ -477,6 +477,9 @@ func (p *IdentityPersister) normalizeVerifiableAddresses(ctx context.Context, id if v.Verified && (v.VerifiedAt == nil || time.Time(*v.VerifiedAt).IsZero()) { v.VerifiedAt = pointerx.Ptr(sqlxx.NullTime(time.Now())) } + if !v.Verified { + v.VerifiedAt = nil + } id.VerifiableAddresses[k] = v }