Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 9 commits into from
Jul 13, 2023
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8504775
fix: reduce db lookups in whoami for aal check
aeneasr Jul 6, 2023
496a130
feat: remove credential lookups in whoami call
aeneasr Jul 7, 2023
0415a80
Update session/manager_http.go
aeneasr Jul 10, 2023
6ed6ecf
u
aeneasr Jul 10, 2023
b854129
u
aeneasr Jul 10, 2023
a483d7b
u
aeneasr Jul 11, 2023
5034468
chore: synchronize workspaces
aeneasr Jul 11, 2023
da2623a
chore: synchronize workspaces
aeneasr Jul 11, 2023
8fedaf1
chore: synchronize workspaces
aeneasr Jul 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ migrations-sync: .bin/ory

.PHONY: test-update-snapshots
test-update-snapshots:
UPDATE_SNAPSHOTS=true go test -p 4 -tags sqlite -short ./...
UPDATE_SNAPSHOTS=true go test -tags sqlite,json1,refresh -short ./...

.PHONY: post-release
post-release: .bin/yq
Expand Down
4 changes: 4 additions & 0 deletions identity/identity.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ type Identity struct {
// Credentials represents all credentials that can be used for authenticating this identity.
Credentials map[CredentialsType]Credentials `json:"credentials,omitempty" faker:"-" db:"-"`

// AvailableAAL defines the maximum available AAL for this identity. If the user has only a password
// configured, the AAL will be 1. If the user has a password and a TOTP configured, the AAL will be 2.
AvailableAAL AuthenticatorAssuranceLevel `json:"-" faker:"-" db:"available_aal"`

// // IdentifierCredentials contains the access and refresh token for oidc identifier
// IdentifierCredentials []IdentifierCredential `json:"identifier_credentials,omitempty" faker:"-" db:"-"`

Expand Down
33 changes: 33 additions & 0 deletions identity/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,10 @@ func (m *Manager) Create(ctx context.Context, i *Identity, opts ...ManagerOption
return err
}

if err := m.setAAL(ctx, i); err != nil {
return err
}

if err := m.r.PrivilegedIdentityPool().CreateIdentity(ctx, i); err != nil {
return err
}
Expand All @@ -107,6 +111,10 @@ func (m *Manager) CreateIdentities(ctx context.Context, identities []*Identity,
i.SchemaID = m.r.Config().DefaultIdentityTraitsSchemaID(ctx)
}

if err := m.setAAL(ctx, i); err != nil {
return err
}

o := newManagerOptions(opts)
if err := m.ValidateIdentity(ctx, i, o); err != nil {
return err
Expand Down Expand Up @@ -164,9 +172,34 @@ func (m *Manager) Update(ctx context.Context, updated *Identity, opts ...Manager
return err
}

if err := m.setAAL(ctx, updated); err != nil {
return err
}

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

i.AvailableAAL = NoAuthenticatorAssuranceLevel
if c, err := m.CountActiveFirstFactorCredentials(ctx, i); err != nil {
return err
} else if c == 0 {
// No first factor set up - AAL is 0
return nil
}

i.AvailableAAL = AuthenticatorAssuranceLevel1
if c, err := m.CountActiveMultiFactorCredentials(ctx, i); err != nil {
return err
} else if c == 0 {
// No second factor set up - AAL is 1
return nil
}

i.AvailableAAL = AuthenticatorAssuranceLevel2
return nil
}

func (m *Manager) UpdateSchemaID(ctx context.Context, id uuid.UUID, schemaID string, opts ...ManagerOption) (err error) {
ctx, span := m.r.Tracer(ctx).Tracer().Start(ctx, "identity.Manager.UpdateSchemaID")
defer otelx.End(span, &err)
Expand Down
124 changes: 122 additions & 2 deletions identity/manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import (
"testing"
"time"

"github.com/gofrs/uuid"

"github.com/ory/x/sqlxx"

"github.com/ory/kratos/internal/testhelpers"
Expand Down Expand Up @@ -65,10 +67,72 @@ func TestManager(t *testing.T) {

t.Run("method=Create", func(t *testing.T) {
t.Run("case=should create identity and track extension fields", func(t *testing.T) {
email := uuid.Must(uuid.NewV4()).String() + "@ory.sh"
original := identity.NewIdentity(config.DefaultIdentityTraitsSchemaID)
original.Traits = newTraits("[email protected]", "")
original.Traits = newTraits(email, "")
require.NoError(t, reg.IdentityManager().Create(context.Background(), original))
checkExtensionFieldsForIdentities(t, "[email protected]", original)
checkExtensionFieldsForIdentities(t, email, original)
assert.Equal(t, identity.NoAuthenticatorAssuranceLevel, original.AvailableAAL)
})

t.Run("case=correctly set AAL", func(t *testing.T) {
t.Run("case=should set AAL to 0 if no credentials are available", func(t *testing.T) {
email := uuid.Must(uuid.NewV4()).String() + "@ory.sh"
original := identity.NewIdentity(config.DefaultIdentityTraitsSchemaID)
original.Traits = newTraits(email, "")
require.NoError(t, reg.IdentityManager().Create(context.Background(), original))
assert.Equal(t, identity.NoAuthenticatorAssuranceLevel, original.AvailableAAL)
})

t.Run("case=should set AAL to 1 if password is set", func(t *testing.T) {
email := uuid.Must(uuid.NewV4()).String() + "@ory.sh"
original := identity.NewIdentity(config.DefaultIdentityTraitsSchemaID)
original.Traits = newTraits(email, "")
original.Credentials = map[identity.CredentialsType]identity.Credentials{
identity.CredentialsTypePassword: {
Type: identity.CredentialsTypePassword,
Identifiers: []string{email},
Config: sqlxx.JSONRawMessage(`{"hashed_password":"$2a$08$.cOYmAd.vCpDOoiVJrO5B.hjTLKQQ6cAK40u8uB.FnZDyPvVvQ9Q."}`),
},
}
require.NoError(t, reg.IdentityManager().Create(context.Background(), original))
assert.Equal(t, identity.AuthenticatorAssuranceLevel1, original.AvailableAAL)
})

t.Run("case=should set AAL to 2 if password and TOTP is set", func(t *testing.T) {
email := uuid.Must(uuid.NewV4()).String() + "@ory.sh"
original := identity.NewIdentity(config.DefaultIdentityTraitsSchemaID)
original.Traits = newTraits(email, "")
original.Credentials = map[identity.CredentialsType]identity.Credentials{
identity.CredentialsTypePassword: {
Type: identity.CredentialsTypePassword,
Identifiers: []string{email},
Config: sqlxx.JSONRawMessage(`{"hashed_password":"$2a$08$.cOYmAd.vCpDOoiVJrO5B.hjTLKQQ6cAK40u8uB.FnZDyPvVvQ9Q."}`),
},
identity.CredentialsTypeTOTP: {
Type: identity.CredentialsTypeTOTP,
Identifiers: []string{email},
Config: sqlxx.JSONRawMessage(`{"totp_url":"otpauth://totp/test"}`),
},
}
require.NoError(t, reg.IdentityManager().Create(context.Background(), original))
assert.Equal(t, identity.AuthenticatorAssuranceLevel2, original.AvailableAAL)
})

t.Run("case=should set AAL to 0 if only TOTP is set", func(t *testing.T) {
email := uuid.Must(uuid.NewV4()).String() + "@ory.sh"
original := identity.NewIdentity(config.DefaultIdentityTraitsSchemaID)
original.Traits = newTraits(email, "")
original.Credentials = map[identity.CredentialsType]identity.Credentials{
identity.CredentialsTypeTOTP: {
Type: identity.CredentialsTypeTOTP,
Identifiers: []string{email},
Config: sqlxx.JSONRawMessage(`{"totp_url":"otpauth://totp/test"}`),
},
}
require.NoError(t, reg.IdentityManager().Create(context.Background(), original))
assert.Equal(t, identity.NoAuthenticatorAssuranceLevel, original.AvailableAAL)
})
})

t.Run("case=should expose validation errors with option", func(t *testing.T) {
Expand Down Expand Up @@ -100,6 +164,62 @@ func TestManager(t *testing.T) {
checkExtensionFieldsForIdentities(t, "[email protected]", original)
})

t.Run("case=should set AAL to 1 if password is set", func(t *testing.T) {
email := uuid.Must(uuid.NewV4()).String() + "@ory.sh"
original := identity.NewIdentity(config.DefaultIdentityTraitsSchemaID)
original.Traits = newTraits(email, "")
require.NoError(t, reg.IdentityManager().Create(context.Background(), original))
original.Credentials = map[identity.CredentialsType]identity.Credentials{
identity.CredentialsTypePassword: {
Type: identity.CredentialsTypePassword,
Identifiers: []string{email},
Config: sqlxx.JSONRawMessage(`{"hashed_password":"$2a$08$.cOYmAd.vCpDOoiVJrO5B.hjTLKQQ6cAK40u8uB.FnZDyPvVvQ9Q."}`),
},
}
require.NoError(t, reg.IdentityManager().Update(context.Background(), original, identity.ManagerAllowWriteProtectedTraits))
assert.Equal(t, identity.AuthenticatorAssuranceLevel1, original.AvailableAAL)
})

t.Run("case=should set AAL to 2 if password and TOTP is set", func(t *testing.T) {
email := uuid.Must(uuid.NewV4()).String() + "@ory.sh"
original := identity.NewIdentity(config.DefaultIdentityTraitsSchemaID)
original.Traits = newTraits(email, "")
original.Credentials = map[identity.CredentialsType]identity.Credentials{
identity.CredentialsTypePassword: {
Type: identity.CredentialsTypePassword,
Identifiers: []string{email},
Config: sqlxx.JSONRawMessage(`{"hashed_password":"$2a$08$.cOYmAd.vCpDOoiVJrO5B.hjTLKQQ6cAK40u8uB.FnZDyPvVvQ9Q."}`),
},
}
require.NoError(t, reg.IdentityManager().Create(context.Background(), original))
assert.Equal(t, identity.AuthenticatorAssuranceLevel1, original.AvailableAAL)
require.NoError(t, reg.IdentityManager().Update(context.Background(), original, identity.ManagerAllowWriteProtectedTraits))
assert.Equal(t, identity.AuthenticatorAssuranceLevel1, original.AvailableAAL, "Updating without changes should not change AAL")
original.Credentials[identity.CredentialsTypeTOTP] = identity.Credentials{
Type: identity.CredentialsTypeTOTP,
Identifiers: []string{email},
Config: sqlxx.JSONRawMessage(`{"totp_url":"otpauth://totp/test"}`),
}
require.NoError(t, reg.IdentityManager().Update(context.Background(), original, identity.ManagerAllowWriteProtectedTraits))
assert.Equal(t, identity.AuthenticatorAssuranceLevel2, original.AvailableAAL)
})

t.Run("case=should set AAL to 0 if only TOTP is set", func(t *testing.T) {
email := uuid.Must(uuid.NewV4()).String() + "@ory.sh"
original := identity.NewIdentity(config.DefaultIdentityTraitsSchemaID)
original.Traits = newTraits(email, "")
require.NoError(t, reg.IdentityManager().Create(context.Background(), original))
original.Credentials = map[identity.CredentialsType]identity.Credentials{
identity.CredentialsTypeTOTP: {
Type: identity.CredentialsTypeTOTP,
Identifiers: []string{email},
Config: sqlxx.JSONRawMessage(`{"totp_url":"otpauth://totp/test"}`),
},
}
require.NoError(t, reg.IdentityManager().Update(context.Background(), original, identity.ManagerAllowWriteProtectedTraits))
assert.Equal(t, identity.NoAuthenticatorAssuranceLevel, original.AvailableAAL)
})

t.Run("case=should not update protected traits without option", func(t *testing.T) {
original := identity.NewIdentity(config.DefaultIdentityTraitsSchemaID)
original.Traits = newTraits("[email protected]", "")
Expand Down
5 changes: 3 additions & 2 deletions persistence/sql/batch/.snapshots/Test_buildInsertQueryArgs-case=Identities.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
{
"TableName": "\"identities\"",
"ColumnsDecl": "\"created_at\", \"id\", \"metadata_admin\", \"metadata_public\", \"nid\", \"schema_id\", \"state\", \"state_changed_at\", \"traits\", \"updated_at\"",
"ColumnsDecl": "\"available_aal\", \"created_at\", \"id\", \"metadata_admin\", \"metadata_public\", \"nid\", \"schema_id\", \"state\", \"state_changed_at\", \"traits\", \"updated_at\"",
"Columns": [
"available_aal",
"created_at",
"id",
"metadata_admin",
Expand All @@ -13,5 +14,5 @@
"traits",
"updated_at"
],
"Placeholders": "(?, ?, ?, ?, ?, ?, ?, ?, ?, ?),\n(?, ?, ?, ?, ?, ?, ?, ?, ?, ?),\n(?, ?, ?, ?, ?, ?, ?, ?, ?, ?),\n(?, ?, ?, ?, ?, ?, ?, ?, ?, ?),\n(?, ?, ?, ?, ?, ?, ?, ?, ?, ?),\n(?, ?, ?, ?, ?, ?, ?, ?, ?, ?),\n(?, ?, ?, ?, ?, ?, ?, ?, ?, ?),\n(?, ?, ?, ?, ?, ?, ?, ?, ?, ?),\n(?, ?, ?, ?, ?, ?, ?, ?, ?, ?),\n(?, ?, ?, ?, ?, ?, ?, ?, ?, ?)"
"Placeholders": "(?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?),\n(?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?),\n(?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?),\n(?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?),\n(?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?),\n(?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?),\n(?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?),\n(?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?),\n(?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?),\n(?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)"
}
17 changes: 17 additions & 0 deletions persistence/sql/migratest/fixtures/identity/0149ce5f-76a8-4efe-b2e3-431b8c6cceb6.json
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this lack the new column?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it's not exposed in JSON

Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
{
"id": "0149ce5f-76a8-4efe-b2e3-431b8c6cceb6",
"schema_id": "default",
"schema_url": "https://www.ory.sh/schemas/ZGVmYXVsdA",
"state": "active",
"traits": {
"email": "[email protected]"
},
"metadata_public": {
"foo": "bar"
},
"metadata_admin": {
"baz": "bar"
},
"created_at": "2013-10-07T08:23:19Z",
"updated_at": "2013-10-07T08:23:19Z"
}
5 changes: 5 additions & 0 deletions persistence/sql/migratest/testdata/20230706000000_testdata.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
INSERT INTO identities (id, nid, schema_id, traits, created_at, updated_at, metadata_public, metadata_admin,
available_aal)
VALUES ('0149ce5f-76a8-4efe-b2e3-431b8c6cceb6', '884f556e-eb3a-4b9f-bee3-11345642c6c0', 'default',
'{"email":"[email protected]"}', '2013-10-07 08:23:19', '2013-10-07 08:23:19', '{"foo":"bar"}', '{"baz":"bar"}',
'aal1');
1 change: 1 addition & 0 deletions persistence/sql/migrations/sql/20230706000000000001_available_aal.down.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER TABLE identities DROP COLUMN available_aal;
1 change: 1 addition & 0 deletions persistence/sql/migrations/sql/20230706000000000001_available_aal.up.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER TABLE identities ADD COLUMN available_aal VARCHAR(4) NOT NULL DEFAULT 'aal0';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's now nullable

5 changes: 4 additions & 1 deletion session/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,10 @@ func (h *Handler) whoami(w http.ResponseWriter, r *http.Request, ps httprouter.P
}

var aalErr *ErrAALNotSatisfied
if err := h.r.SessionManager().DoesSessionSatisfy(r, s, c.SessionWhoAmIAAL(r.Context())); errors.As(err, &aalErr) {
if err := h.r.SessionManager().DoesSessionSatisfy(r, s, c.SessionWhoAmIAAL(r.Context()),
// For the time being we want to update the AAL in the database if it is unset.
UpsertAAL,
Comment on lines +209 to +210
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

); errors.As(err, &aalErr) {
h.r.Audit().WithRequest(r).WithError(err).Info("Session was found but AAL is not satisfied for calling this endpoint.")
h.r.Writer().WriteError(w, r, err)
return
Expand Down
72 changes: 45 additions & 27 deletions session/manager_http.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ func NewManagerHTTP(r managerHTTPDependencies) *ManagerHTTP {

type options struct {
requestURL string
upsertAAL bool
}

type ManagerOptions func(*options)
Expand All @@ -74,6 +75,12 @@ func WithRequestURL(requestURL string) ManagerOptions {
}
}

// UpsertAAL will update the available AAL of the identity if it was previoulsy unset. This is used to migrate
// identities from older versions of Ory Kratos.
func UpsertAAL(opts *options) {
opts.upsertAAL = true
}

func (s *ManagerHTTP) UpsertAndIssueCookie(ctx context.Context, w http.ResponseWriter, r *http.Request, ss *Session) (err error) {
ctx, span := s.r.Tracer(ctx).Tracer().Start(ctx, "sessions.ManagerHTTP.UpsertAndIssueCookie")
defer otelx.End(span, &err)
Expand Down Expand Up @@ -225,15 +232,7 @@ func (s *ManagerHTTP) FetchFromRequest(ctx context.Context, r *http.Request) (_
return nil, errors.WithStack(NewErrNoCredentialsForSession())
}

expand := identity.ExpandDefault
if s.r.Config().SessionWhoAmIAAL(r.Context()) == config.HighestAvailableAAL {
// When the session endpoint requires the highest AAL, we fetch all credentials immediately to save a
// query later in "DoesSessionSatisfy". This is a SQL optimization, because the identity manager fetches
// the data in parallel, which is a bit faster than fetching it in sequence.
expand = identity.ExpandEverything
}

se, err := s.r.SessionPersister().GetSessionByToken(ctx, token, ExpandEverything, expand)
se, err := s.r.SessionPersister().GetSessionByToken(ctx, token, ExpandEverything, identity.ExpandDefault)
if err != nil {
if errors.Is(err, herodot.ErrNotFound) || errors.Is(err, sqlcon.ErrNoRows) {
return nil, errors.WithStack(NewErrNoActiveSessionFound())
Expand Down Expand Up @@ -295,31 +294,50 @@ func (s *ManagerHTTP) DoesSessionSatisfy(r *http.Request, sess *Session, request
return nil
}
case config.HighestAvailableAAL:
i := sess.Identity
if i == nil {
i, err = s.r.IdentityPool().GetIdentity(ctx, sess.IdentityID, identity.ExpandCredentials)
if sess.Identity == nil {
sess.Identity, err = s.r.IdentityPool().GetIdentity(ctx, sess.IdentityID, identity.ExpandNothing)
if err != nil {
return err
}
sess.Identity = i
} else if len(i.Credentials) == 0 {
// If credentials are not expanded, we load them here.
if err := s.r.PrivilegedIdentityPool().HydrateIdentityAssociations(ctx, i, identity.ExpandCredentials); err != nil {
}

i := sess.Identity
available := i.AvailableAAL
if len(available) == 0 || available == identity.NoAuthenticatorAssuranceLevel {
// 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.
Comment on lines +307 to +312
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is not accurate anymore.

if len(i.Credentials) == 0 {
// The identity was apparently fetched without credentials. Let's hydrate them.
if err := s.r.PrivilegedIdentityPool().HydrateIdentityAssociations(ctx, i, identity.ExpandCredentials); err != nil {
return err
}
}

available = identity.NoAuthenticatorAssuranceLevel
if firstCount, err := s.r.IdentityManager().CountActiveFirstFactorCredentials(ctx, i); err != nil {
return err
} else if firstCount > 0 {
available = identity.AuthenticatorAssuranceLevel1
}

if secondCount, err := s.r.IdentityManager().CountActiveMultiFactorCredentials(ctx, i); err != nil {
return err
} else if secondCount > 0 {
available = identity.AuthenticatorAssuranceLevel2
}
}

available := identity.NoAuthenticatorAssuranceLevel
if firstCount, err := s.r.IdentityManager().CountActiveFirstFactorCredentials(ctx, i); err != nil {
return err
} else if firstCount > 0 {
available = identity.AuthenticatorAssuranceLevel1
}
i.AvailableAAL = available
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if secondCount, err := s.r.IdentityManager().CountActiveMultiFactorCredentials(ctx, i); err != nil {
return err
} else if secondCount > 0 {
available = identity.AuthenticatorAssuranceLevel2
// This is the migration strategy for identities that already exist.
if managerOpts.upsertAAL {
Comment on lines +326 to +327
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, makes sense.

if _, err := s.r.SessionPersister().GetConnection(ctx).Where("id = ? AND nid = ?", i.ID, i.NID).UpdateQuery(i, "available_aal"); err != nil {
return err
}
}
}

if sess.AuthenticatorAssuranceLevel >= available {
Expand Down
Loading
Loading