fix: reduce db lookups in whoami for aal check #3372

aeneasr · 2023-07-06T12:25:01Z

This PR removes queries to identity_credentials, identity_credential_types, and identity_credential_identifiers when performing the whoami check.

It does this by “caching” the maximum available AAL in the identity table. The value is updated whenever an identity is created or updated.

The mechanism can also fall back to generate the AAL value itself if the value is not available in the identity (this is a migration strategy).

One potential issue: If the available_aal is not correctly set, it might lead to issues where the user has 2FA enabled, but the system thinks they only have 1FA enabled.

Significantly improves performance by reducing the amount of queries we need to do when checking for the different AAL levels.

Benehiko

LGTM

session/manager_http.go

Co-authored-by: Alano Terblanche <[email protected]>

zepatrik

I have some doubts about the migration strategy, see comments.

zepatrik · 2023-07-10T09:07:00Z

persistence/sql/migrations/sql/20230706000000000001_available_aal.up.sql

@@ -0,0 +1 @@
+ALTER TABLE identities ADD COLUMN available_aal VARCHAR(4) NOT NULL DEFAULT 'aal0';


Doesn't NOT NULL DEFAULT ... do a full table update? Wouldn't it be better to use a nullable column here instead?
Like this every identity would have available_aal set to 0 after the migration, and only on update we would set the correct value.

Yep, absolutely. However as I understood the people from CRDB, this should not be an issue due to the way migrations work. We still need to verify that though.

It's now nullable

zepatrik · 2023-07-10T09:09:31Z

session/handler.go

+		// For the time being we want to update the AAL in the database if it is unset.
+		UpsertAAL,


A nullable column makes this unnecessary, because we can distinguish the "not yet computed" from the "zero" case.

I think that this state could also happen during an import, which is why I think this fallback is required.

zepatrik · 2023-07-10T09:10:36Z

persistence/sql/migratest/fixtures/identity/0149ce5f-76a8-4efe-b2e3-431b8c6cceb6.json

Why does this lack the new column?

Because it's not exposed in JSON

zepatrik · 2023-07-10T09:15:58Z

identity/manager.go

 	return m.r.PrivilegedIdentityPool().UpdateIdentity(ctx, updated)
 }

+func (m *Manager) setAAL(ctx context.Context, i *Identity) (err error) {


I think it makes more sense to make this a function on the identity, and provide the manager as a dependency there (potentially as an interface). The logic is duplicated in the session manager well.

zepatrik · 2023-07-10T09:18:49Z

session/manager_http.go

-		} else if firstCount > 0 {
-			available = identity.AuthenticatorAssuranceLevel1
-		}
+			i.AvailableAAL = available


Suggested change

i.AvailableAAL = available

i.SetAvailableAAL(s.r.IdentityManager())

zepatrik · 2023-07-10T09:21:05Z

session/manager_http.go

+			// This is the migration strategy for identities that already exist.
+			if managerOpts.upsertAAL {


During the "migration" we just recompute it for everyone. Users that don't log in during the migration period stay at AAL0 forever. I think a nullable column is the way to go instead.

Suggested change

// This is the migration strategy for identities that already exist.

if managerOpts.upsertAAL {

// This is the migration strategy for identities that already exist.

if !i.AvailableAAL.Valid {

Yup - it's now nullable. This parameter however is still here because we don't always want to perform an INSERT or UPDATE statement when this method is called, so there has to be a way to control it.

OK, makes sense.

zepatrik

Looks good now 👍

zepatrik · 2023-07-11T10:13:41Z

session/manager_http.go

+			// Available is 0 if the identity was created before the AAL feature was introduced, or if the identity
+			// was directly created in the persister and not the identity manager.
+			//
+			// aal0 indicates that the AAL state of the identity is probably unknown.
+			//
+			// In either case, we need to fetch the credentials from the database to determine the AAL.


This comment is not accurate anymore.

zepatrik · 2023-07-11T10:14:38Z

session/manager_http.go

+			// This is the migration strategy for identities that already exist.
+			if managerOpts.upsertAAL {


OK, makes sense.

codecov · 2023-07-11T14:50:29Z

Codecov Report

Merging #3372 (70e4eac) into master (eaa3f3c) will increase coverage by 0.00%.
The diff coverage is 75.86%.

❗ Current head 70e4eac differs from pull request most recent head 8fedaf1. Consider uploading reports for the commit 8fedaf1 to get more accurate results

@@           Coverage Diff           @@
##           master    #3372   +/-   ##
=======================================
  Coverage   77.98%   77.99%           
=======================================
  Files         327      327           
  Lines       21279    21331   +52     
=======================================
+ Hits        16595    16637   +42     
- Misses       3449     3456    +7     
- Partials     1235     1238    +3

Impacted Files	Coverage Δ
identity/manager.go	`76.92% <0.00%> (-3.73%)`	⬇️
identity/identity.go	`89.67% <53.84%> (-2.33%)`	⬇️
session/manager_http.go	`79.36% <68.75%> (+0.71%)`	⬆️
identity/credentials.go	`83.78% <84.00%> (+0.11%)`	⬆️
cmd/clidoc/main.go	`68.05% <100.00%> (ø)`
identity/test/pool.go	`100.00% <100.00%> (ø)`
selfservice/strategy/code/strategy_recovery.go	`70.56% <100.00%> (ø)`
selfservice/strategy/code/strategy_verification.go	`74.25% <100.00%> (ø)`
session/handler.go	`68.60% <100.00%> (+0.24%)`	⬆️
text/message_node.go	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

aeneasr marked this pull request as ready for review July 7, 2023 08:52

aeneasr requested a review from zepatrik as a code owner July 7, 2023 08:52

aeneasr added 2 commits July 7, 2023 10:52

fix: reduce db lookups in whoami for aal check

8504775

feat: remove credential lookups in whoami call

496a130

Significantly improves performance by reducing the amount of queries we need to do when checking for the different AAL levels.

aeneasr force-pushed the max-aal-in-sess branch from 635ea3c to 496a130 Compare July 7, 2023 08:53

Benehiko previously approved these changes Jul 7, 2023

View reviewed changes

session/manager_http.go Outdated Show resolved Hide resolved

Update session/manager_http.go

0415a80

Co-authored-by: Alano Terblanche <[email protected]>

aeneasr dismissed Benehiko’s stale review via 0415a80 July 10, 2023 07:51

aeneasr added 2 commits July 10, 2023 09:57

u

6ed6ecf

u

b854129

zepatrik requested changes Jul 10, 2023

View reviewed changes

aeneasr added 3 commits July 11, 2023 11:11

u

a483d7b

chore: synchronize workspaces

5034468

chore: synchronize workspaces

da2623a

zepatrik previously approved these changes Jul 11, 2023

View reviewed changes

chore: synchronize workspaces

8fedaf1

aeneasr dismissed zepatrik’s stale review via 8fedaf1 July 11, 2023 14:48

aeneasr merged commit d814a48 into master Jul 13, 2023
26 checks passed

aeneasr deleted the max-aal-in-sess branch July 13, 2023 08:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: reduce db lookups in whoami for aal check #3372

fix: reduce db lookups in whoami for aal check #3372

aeneasr commented Jul 6, 2023 •

edited

Loading

Benehiko left a comment

zepatrik left a comment

zepatrik Jul 10, 2023

aeneasr Jul 10, 2023

aeneasr Jul 11, 2023

zepatrik Jul 10, 2023

aeneasr Jul 10, 2023

zepatrik Jul 10, 2023

aeneasr Jul 10, 2023

zepatrik Jul 10, 2023

aeneasr Jul 11, 2023

zepatrik Jul 10, 2023

zepatrik Jul 10, 2023

aeneasr Jul 11, 2023

zepatrik Jul 11, 2023

zepatrik left a comment

zepatrik Jul 11, 2023

zepatrik Jul 11, 2023

codecov bot commented Jul 11, 2023 •

edited

Loading

		@@ -0,0 +1 @@
		ALTER TABLE identities ADD COLUMN available_aal VARCHAR(4) NOT NULL DEFAULT 'aal0';

		// For the time being we want to update the AAL in the database if it is unset.
		UpsertAAL,

	i.AvailableAAL = available
	i.SetAvailableAAL(s.r.IdentityManager())

		// This is the migration strategy for identities that already exist.
		if managerOpts.upsertAAL {

fix: reduce db lookups in whoami for aal check #3372

fix: reduce db lookups in whoami for aal check #3372

Conversation

aeneasr commented Jul 6, 2023 • edited Loading

Benehiko left a comment

Choose a reason for hiding this comment

zepatrik left a comment

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zepatrik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 11, 2023 • edited Loading

Codecov Report

aeneasr commented Jul 6, 2023 •

edited

Loading

codecov bot commented Jul 11, 2023 •

edited

Loading