feat: identity by identifier #3077

kelkarajay · 2023-02-07T14:13:13Z

Changes

Enabling identity lookup using an identifier

Related issue(s)

https://github.com/ory-corp/cloud/issues/3947

Checklist

I have read the contributing guidelines.
I have referenced an issue containing the design document if my change
introduces a new feature.
I am following the
contributing code guidelines.
I have read the security policy.
I confirm that this pull request does not address a security
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
security@ory.sh) from the maintainers to push
the changes.
I have added tests that prove my fix is effective or that my feature
works.
I have added or changed the documentation.

codecov · 2023-02-08T09:24:59Z

Codecov Report

Merging #3077 (0a78dae) into master (9477ea4) will increase coverage by 0.05%.
The diff coverage is 79.60%.

❗ Current head 0a78dae differs from pull request most recent head f5e8fb3. Consider uploading reports for the commit f5e8fb3 to get more accurate results

@@            Coverage Diff             @@
##           master    #3077      +/-   ##
==========================================
+ Coverage   77.35%   77.41%   +0.05%     
==========================================
  Files         315      315              
  Lines       19817    19885      +68     
==========================================
+ Hits        15330    15394      +64     
- Misses       3297     3300       +3     
- Partials     1190     1191       +1

Impacted Files	Coverage Δ
selfservice/flow/registration/handler.go	`79.37% <ø> (ø)`
selfservice/flow/verification/flow.go	`90.80% <ø> (ø)`
selfservice/hook/session_destroyer.go	`66.66% <0.00%> (ø)`
selfservice/strategy/code/strategy_verification.go	`75.11% <ø> (ø)`
selfservice/strategy/link/strategy_verification.go	`58.59% <50.00%> (ø)`
selfservice/strategy/link/sender.go	`70.10% <51.85%> (ø)`
selfservice/strategy/code/code_sender.go	`78.07% <66.66%> (ø)`
identity/handler.go	`84.91% <75.00%> (+0.33%)`	⬆️
selfservice/hook/web_hook.go	`78.78% <75.00%> (ø)`
driver/config/config.go	`82.77% <100.00%> (ø)`
... and 22 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

persistence/sql/persister_identity.go

jonas-jonas

Looking good already. Have a few comments on the semantics of the API.

identity/handler.go

persistence/sql/persister_identity.go

identity/handler.go

jonas-jonas

One more detail on how we want the identifier parameter to behave. IMO, it should behave like a filter to the outside consumer, even though technically speaking, there can only ever be one result returned from this (right now).

Btw. I do recall discussions about whether we can widen the scope of the identifier parameter filter to other fields, such as recovery addresses, OIDC connections, etc. Did you look into that? Is that possible?

Because if so, there could very well be more than one identity (I think) in the result set.

identity/handler.go

jonas-jonas · 2023-02-10T07:44:24Z

identity/handler_test.go

@@ -117,6 +117,10 @@ func TestHandler(t *testing.T) {
 		}
 	})

+	t.Run("case=should return 404 on a non-existing resource for identity lookup", func(t *testing.T) {


Well, no. IMO, this should return an empty array, since clients expect an array here.

How would this feature be used in the context of the ory network?
We'd probably add a "search" input to the "List identities" table, similar to the email delivery page:

Or do you have something else in mind?

And we already (should) have the logic for displaying a message for "no results found" if the array is empty. So if this endpoint all of a sudden returns 404 this will probably break that and we'd need to implement better error handling on that page.

jonas-jonas · 2023-02-10T07:45:59Z

identity/handler_test.go

+			res := get(t, adminTS, "/identities?identifier=find.by.identifier@bar.com", http.StatusOK)
+			assert.EqualValues(t, i1.ID.String(), res.Get("0.id").String(), "%s", res.Raw)
+			assert.EqualValues(t, "find.by.identifier@bar.com", res.Get("0.traits.username").String(), "%s", res.Raw)
+			assert.EqualValues(t, defaultSchemaExternalURL, res.Get("0.schema_url").String(), "%s", res.Raw)
+			assert.EqualValues(t, config.DefaultIdentityTraitsSchemaID, res.Get("0.schema_id").String(), "%s", res.Raw)
+			assert.EqualValues(t, identity.StateActive, res.Get("0.state").String(), "%s", res.Raw)
+			assert.Empty(t, res.Get("credentials").String(), "%s", res.Raw)


Nit: could use a snapshot instead.

jonas-jonas · 2023-02-10T08:13:59Z

Btw. I do recall discussions about whether we can widen the scope of the identifier parameter filter to other fields, such as recovery addresses, OIDC connections, etc. Did you look into that? Is that possible?

Okay, so OIDC connections and webauthn credentials would be stored in the same way the credentials are. So that should work. But I think it would be great, if we had a test that also checks whether accounts with those kinds of credentials can be found, too. Right now, we're only testing for password credentials, right?

aeneasr · 2023-02-10T12:35:49Z

identity/handler.go

+			return
+		}
+
+		h.r.Writer().Write(w, r, []Identity{*i})


Shouldn't this be analogous to:

// Identities using the marshaler for including metadata_admin isam := make([]WithAdminMetadataInJSON, len(is)) for i, identity := range is { isam[i] = WithAdminMetadataInJSON(identity) }

And more generally speaking, I think that the implementation does not cover both use cases:

Search users as an administrator to e.g. delete them

Show the user ways he/she can sign in (e.g. user A has password and social enabled -> show those two fields during login)

The second case is not (yet) covered, because we do not return the credentials metadata. Thus, the consumer of the API wouldn't know which credentials are available for the user.

Here too I'm fine with shipping this in its current state and adding credentials metainfo later.

Credentials use case is now fixed - 4a16b0f

persistence/sql/persister_identity.go

aeneasr · 2023-02-10T12:43:50Z

identity/test/pool.go

@@ -594,13 +594,33 @@ func TestPool(ctx context.Context, conf *config.Config, p interface {
 		})

 		t.Run("case=find identity by its credentials identifier", func(t *testing.T) {
+			expected := passwordIdentity("", "find-identity-by-identifier@ory.sh")


Note: The test has a medium score - it does test properly that the persister is able to find the identity by its identifier, but since we only create one identity it could also simply return the first identity in the list. What saves us is the implicit knowledge that we created 25 identities in the test prior, but that implies that we don't care about test isolation. In isolation, the test doesn't properly satisfy our requirements (of 10 identities give me exactly the one that has identifier X; for 10 identities tell me "no identity found" if I search for a non-existing user identifier)

Fair point about the test isolation, I'll fix the test to create identities of different credential types as well

aeneasr · 2023-02-10T12:47:05Z

persistence/sql/persister_identity.go

+		"identity_credentials",
+		"identity_credential_identifiers",
+	),
+		match,


Normalization like we do it in the original function is missing:

match = p.normalizeIdentifier(ct, match)

That means that the match is to be case sensitive, which will return different results depending on which persister API you use. Please normalize the match

@aeneasr Normalization was left out because OIDC credential identifier would be case-sensitive while Password/WebAuthn identifier's are lower-cased during identity creation. Prior to the query, we just have the identifier and not the credential type to perform the appropriate normalization

I see, that's a fair assumption. Let's view it in the full context:

Only webauthn and password use user-based input for their identifiers (e.g. the email or username), which is why they get normalized (because users mess up capitalization on mobile). All other methods use either the identity's UUID or in the case of Social Sign In a concatenated string provider:user_id_from_provider

IMO we only need to cover the use case where the identifier is based on user input (aka registration or settings form - email, username, phone number, ...) because it doesn't really make sense to search for these system generated strings from an API perspective.

By the way, an addition to the logic would be to search for recovery_addresses or verification_addresses using the via. Not saying that we have to do it now, but can add later.

aeneasr · 2023-02-10T12:47:07Z

persistence/sql/persister_identity.go

@@ -83,7 +83,7 @@ func (p *Persister) normalizeIdentifier(ct identity.CredentialsType, match strin
 	return match
 }

-func (p *Persister) FindByCredentialsIdentifier(ctx context.Context, ct identity.CredentialsType, match string) (*identity.Identity, *identity.Credentials, error) {
+func (p *Persister) FindByCredentialsIdentifier(ctx context.Context, match string) (*identity.Identity, error) {


Note: this is more or less a copy of FindByCredentialsTypeAndIdentifier with two distinctions

Credentials are not returned

We do not care about credentials type

It would be much nicer (and less error prone) to have a common functionality to cover "search for identity credentials identifier with and without the credentials type".

There's always a balance between abstraction and copy/pasting code. The latter does come with the usualy problems such as forgetting to add some type of transformation (in this case p.normalizeIdentifier).

If it was using the same underlying mechanism, existing tests might have caught that normalization is missing. But since this is net new code we need to prove again that it behaves like the other method:

Properly finds credentials by their identifier

Can deal with normalization

Doesn't return invalid results

I'm OK to keep it this way for the sake of getting it shipped, but only if we cover the same test requirements we have for FindByCredentialsTypeAndIdentifier

@aeneasr I didn't forget the normalization 😓 I left it out on purpose because the credential type isn't known before the query. I left a note in the github issue about that earlier - https://github.com/ory-corp/cloud/issues/3947#issuecomment-1422394212

aeneasr · 2023-02-10T12:49:11Z

identity/handler.go

+			if errors.Is(err, sqlcon.ErrNoRows) {
+				h.r.Writer().Write(w, r, []Identity{})
+				return
+			}


Note: This should be handled the persister / pool. It's doesn't affect the behavior of the code but would be "cleaner" in terms of architecture. It's ok to keep it here though, we can clean it up if when FindByCredentialsIdentifier is used somewhere else.

aeneasr · 2023-02-10T12:56:29Z

identity/handler_test.go

+		t.Run("case=should return an empty array on a failed lookup with identifier", func(t *testing.T) {
+			res := get(t, adminTS, "/identities?identifier=find.by.non.existing.identifier@bar.com", http.StatusOK)
+			assert.EqualValues(t, int64(0), res.Get("#").Int(), "%s", res.Raw)
+		})
+
+		t.Run("case=should be able to lookup the identity using identifier", func(t *testing.T) {
+			i1 := &identity.Identity{
+				Credentials: map[identity.CredentialsType]identity.Credentials{
+					identity.CredentialsTypePassword: {
+						Type:        identity.CredentialsTypePassword,
+						Identifiers: []string{"find.by.identifier@bar.com"},
+						Config:      sqlxx.JSONRawMessage(`{"hashed_password":"$2a$08$.cOYmAd.vCpDOoiVJrO5B.hjTLKQQ6cAK40u8uB.FnZDyPvVvQ9Q."}`), // foobar
+					}},
+				State:  identity.StateActive,
+				Traits: identity.Traits(`{"username":"find.by.identifier@bar.com"}`),
+			}


Note: If the persister test proves that "out of a list of X elements I find exactly the one element that matches the query", you don't need to test this again in the handler. So the test is fine here like this. This comment is in reference to another comment in the persister tests.

Co-authored-by: Jonas Hungershausen <jonas.hungershausen@ory.sh>

jonas-jonas

Nice changes.

identity/handler.go

persistence/sql/persister_identity.go

jonas-jonas

LGTM

kelkarajay changed the title ~~feat: adding persister impl for identifier search~~ feat: identity by identifier Feb 7, 2023

kelkarajay self-assigned this Feb 8, 2023

kelkarajay force-pushed the feat/Findidentity-By-Identifier branch from d9c3dec to 969a909 Compare February 8, 2023 08:33

kelkarajay force-pushed the feat/Findidentity-By-Identifier branch 3 times, most recently from ad7f864 to 19bd10f Compare February 9, 2023 12:08

kelkarajay marked this pull request as ready for review February 9, 2023 12:21

kelkarajay requested review from aeneasr and zepatrik as code owners February 9, 2023 12:21

kelkarajay commented Feb 9, 2023

View reviewed changes

persistence/sql/persister_identity.go Outdated Show resolved Hide resolved

jonas-jonas requested changes Feb 9, 2023

View reviewed changes

jonas-jonas requested changes Feb 10, 2023

View reviewed changes

kelkarajay force-pushed the feat/Findidentity-By-Identifier branch from 89565f8 to 8166ba1 Compare February 10, 2023 11:22

aeneasr reviewed Feb 10, 2023

View reviewed changes

kelkarajay force-pushed the feat/Findidentity-By-Identifier branch from 4ac6c3e to 884e6d2 Compare February 10, 2023 16:39

Ajay Kelkar and others added 10 commits February 10, 2023 17:52

feat: adding persister impl for identifier search

966ef82

feat: q param to filter

9846216

test: persister tests

2176087

feat: add query parameter to request model and gen sdk

cd2a2a8

fix: drop unnecessary block

14e9310

Co-authored-by: Jonas Hungershausen <jonas.hungershausen@ory.sh>

fix: query for identity to expand default

228f492

fix: wrap response as array

85a4fcc

test: add handler tests

4fb6af0

chore: empty array on no results

a101504

refactor: persister query and setup more test data

f3020d3

kelkarajay force-pushed the feat/Findidentity-By-Identifier branch from 884e6d2 to f3020d3 Compare February 10, 2023 16:52

fix: response should include credentials metadata

4a16b0f

kelkarajay force-pushed the feat/Findidentity-By-Identifier branch from dadb7f3 to 4a16b0f Compare February 13, 2023 09:20

Ajay Kelkar and others added 4 commits February 13, 2023 10:37

fix: use existing identifier on another network test

615b58b

test: check credentials in the resp

77ace15

fix: force lower case and trim

1bdbd45

chore: code review

3eebd96

aeneasr requested a review from jonas-jonas February 14, 2023 10:19

u

bd0f5f1

jonas-jonas previously approved these changes Feb 14, 2023

View reviewed changes

identity/handler.go Show resolved Hide resolved

persistence/sql/persister_identity.go Outdated Show resolved Hide resolved

persistence/sql/persister_identity.go Outdated Show resolved Hide resolved

u

5501028

aeneasr dismissed jonas-jonas’s stale review via 5501028 February 14, 2023 12:14

aeneasr assigned aeneasr and unassigned kelkarajay Feb 14, 2023

jonas-jonas previously approved these changes Feb 14, 2023

View reviewed changes

u

b473ff1

aeneasr dismissed jonas-jonas’s stale review via b473ff1 February 14, 2023 13:42

u

f5e8fb3

aeneasr merged commit c288d4d into master Feb 14, 2023

aeneasr deleted the feat/Findidentity-By-Identifier branch February 14, 2023 16:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: identity by identifier #3077

feat: identity by identifier #3077

kelkarajay commented Feb 7, 2023 •

edited

Loading

codecov bot commented Feb 8, 2023 •

edited

Loading

jonas-jonas left a comment

jonas-jonas left a comment

jonas-jonas Feb 10, 2023

kelkarajay Feb 10, 2023 •

edited

Loading

jonas-jonas Feb 10, 2023

jonas-jonas commented Feb 10, 2023

aeneasr Feb 10, 2023

kelkarajay Feb 13, 2023 •

edited

Loading

aeneasr Feb 10, 2023

kelkarajay Feb 10, 2023

aeneasr Feb 10, 2023

kelkarajay Feb 10, 2023 •

edited

Loading

aeneasr Feb 10, 2023

aeneasr Feb 10, 2023

kelkarajay Feb 10, 2023 •

edited

Loading

aeneasr Feb 10, 2023

aeneasr Feb 10, 2023

jonas-jonas left a comment

jonas-jonas left a comment

feat: identity by identifier #3077

feat: identity by identifier #3077

Conversation

kelkarajay commented Feb 7, 2023 • edited Loading

Changes

Related issue(s)

Checklist

codecov bot commented Feb 8, 2023 • edited Loading

Codecov Report

jonas-jonas left a comment

Choose a reason for hiding this comment

jonas-jonas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kelkarajay Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonas-jonas commented Feb 10, 2023

Choose a reason for hiding this comment

kelkarajay Feb 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kelkarajay Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kelkarajay Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonas-jonas left a comment

Choose a reason for hiding this comment

jonas-jonas left a comment

Choose a reason for hiding this comment

kelkarajay commented Feb 7, 2023 •

edited

Loading

codecov bot commented Feb 8, 2023 •

edited

Loading

kelkarajay Feb 10, 2023 •

edited

Loading

kelkarajay Feb 13, 2023 •

edited

Loading

kelkarajay Feb 10, 2023 •

edited

Loading

kelkarajay Feb 10, 2023 •

edited

Loading