From c2379bb0c26575be5f5ab6280a93601932e7bed3 Mon Sep 17 00:00:00 2001
From: Andrew Lytvynov <andrew@goteleport.com>
Date: Fri, 7 May 2021 11:44:56 -0700
Subject: [PATCH] mfa: fix startup crash when SSO users with MFA expire

The user loading code is kind of convoluted: it loads all the separate
backend items from the `/web/users/` prefix into a struct. That struct
is then converted to a full `types.User` object.

For each user there are 3 kinds of backend items:
- `/params` which is the main one, containing a marshalled `types.User`
  object
- `/pwd` which contains the hashed password for local users
- `/mfa/...` which contain registered MFA devices

When an SSO user expires, we delete the first two items but not
`/mfa/...`. This is intentional, to persist MFA devices across logins.
The user loading code would fail because the user was "found" (thanks to
MFA items), but didn't have the mandatory `/params` item.

This PR ignores any users that don't have `/params` instead of
hard-failing all `GetUsers` calls.
---
 lib/services/local/resource.go      | 15 +++++++++++++++
 lib/services/local/resource_test.go | 15 ++++++++++++---
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/lib/services/local/resource.go b/lib/services/local/resource.go
index 4ce5dc6f08c03..a107a08c90606 100644
--- a/lib/services/local/resource.go
+++ b/lib/services/local/resource.go
@@ -557,6 +557,15 @@ func collectUserItems(items []backend.Item) (users map[string]userItems, rem []b
 		}
 		users[name] = collector
 	}
+	// Remove user entries that are incomplete.
+	//
+	// For example, an SSO user entry that expired but still has MFA devices
+	// persisted. These users should not be loaded until they login again.
+	for user, items := range users {
+		if !items.complete() {
+			delete(users, user)
+		}
+	}
 	return users, rem, nil
 }
 
@@ -613,3 +622,9 @@ func (u *userItems) Len() int {
 	l += len(u.mfa)
 	return l
 }
+
+// complete checks whether userItems is complete enough to be parsed by
+// userFromUserItems.
+func (u *userItems) complete() bool {
+	return u.params != nil
+}
diff --git a/lib/services/local/resource_test.go b/lib/services/local/resource_test.go
index 06c83ba1b1cb1..7bba5a3cef28d 100644
--- a/lib/services/local/resource_test.go
+++ b/lib/services/local/resource_test.go
@@ -104,8 +104,10 @@ func (r *ResourceSuite) TestUserResourceWithSecrets(c *check.C) {
 }
 
 func (r *ResourceSuite) runUserResourceTest(c *check.C, withSecrets bool) {
-	alice := newUserTestCase(c, "alice", nil, withSecrets)
-	bob := newUserTestCase(c, "bob", nil, withSecrets)
+	expiry := r.bk.Clock().Now().Add(time.Minute)
+
+	alice := newUserTestCase(c, "alice", nil, withSecrets, expiry)
+	bob := newUserTestCase(c, "bob", nil, withSecrets, expiry)
 	// Check basic dynamic item creation
 	r.runCreationChecks(c, alice, bob)
 	// Check that dynamically created item is compatible with service
@@ -126,6 +128,12 @@ func (r *ResourceSuite) runUserResourceTest(c *check.C, withSecrets bool) {
 			c.Errorf("Unexpected user %q", user.GetName())
 		}
 	}
+
+	// Advance the clock to let the users to expire.
+	r.bk.Clock().(clockwork.FakeClock).Advance(2 * time.Minute)
+	allUsers, err = s.GetUsers(withSecrets)
+	c.Assert(err, check.IsNil)
+	c.Assert(len(allUsers), check.Equals, 0, check.Commentf("expected all users to expire"))
 }
 
 func (r *ResourceSuite) TestCertAuthorityResource(c *check.C) {
@@ -235,13 +243,14 @@ func localAuthSecretsTestCase(c *check.C) services.LocalAuthSecrets {
 	return auth
 }
 
-func newUserTestCase(c *check.C, name string, roles []string, withSecrets bool) services.User {
+func newUserTestCase(c *check.C, name string, roles []string, withSecrets bool, expires time.Time) services.User {
 	user := services.UserV2{
 		Kind:    services.KindUser,
 		Version: services.V2,
 		Metadata: services.Metadata{
 			Name:      name,
 			Namespace: defaults.Namespace,
+			Expires:   &expires,
 		},
 		Spec: services.UserSpecV2{
 			Roles: roles,