Configurable identity mapper strategies

pkg/auth/userregistry/identitymapper/lookup.go

@@ -9,16 +9,14 @@ import (
 	"github.com/openshift/origin/pkg/user/registry/useridentitymapping"
 )
 
+var _ = authapi.UserIdentityMapper(&lookupIdentityMapper{})
+
+// lookupIdentityMapper does not provision a new identity or user, it only allows identities already associated with users
 type lookupIdentityMapper struct {
 	mappings useridentitymapping.Registry
 	users    user.Registry
 }
 
-// NewLookupIdentityMapper returns a mapper that will look up existing mappings for identities
-func NewLookupIdentityMapper(mappings useridentitymapping.Registry, users user.Registry) authapi.UserIdentityMapper {
-	return &lookupIdentityMapper{mappings, users}
-}
-
 // UserFor returns info about the user for whom identity info has been provided
 func (p *lookupIdentityMapper) UserFor(info authapi.UserIdentityInfo) (kuser.Info, error) {
 	ctx := kapi.NewContext()

pkg/auth/userregistry/identitymapper/lookup_test.go

@@ -0,0 +1,157 @@
+package identitymapper
+
+import (
+	"reflect"
+	"testing"
+
+	authapi "github.com/openshift/origin/pkg/auth/api"
+	"github.com/openshift/origin/pkg/user/api"
+	userapi "github.com/openshift/origin/pkg/user/api"
+	"github.com/openshift/origin/pkg/user/registry/test"
+	mappingregistry "github.com/openshift/origin/pkg/user/registry/useridentitymapping"
+)
+
+func TestLookup(t *testing.T) {
+	testcases := map[string]struct {
+		ProviderName     string
+		ProviderUserName string
+
+		ExistingIdentity *userapi.Identity
+		ExistingUser     *userapi.User
+
+		ExpectedActions  []test.Action
+		ExpectedError    bool
+		ExpectedUserName string
+	}{
+		"no identity": {
+			ProviderName:     "idp",
+			ProviderUserName: "bob",
+
+			ExistingIdentity: nil,
+			ExistingUser:     nil,
+
+			ExpectedActions: []test.Action{
+				{"GetIdentity", "idp:bob"},
+			},
+			ExpectedError: true,
+		},
+
+		"existing identity, no user reference": {
+			ProviderName:     "idp",
+			ProviderUserName: "bob",
+
+			ExistingIdentity: makeIdentity("bobIdentityUID", "idp", "bob", "", ""),
+			ExistingUser:     nil,
+
+			ExpectedActions: []test.Action{
+				{"GetIdentity", "idp:bob"},
+			},
+			ExpectedError: true,
+		},
+		"existing identity, missing user reference": {
+			ProviderName:     "idp",
+			ProviderUserName: "bob",
+
+			ExistingIdentity: makeIdentity("bobIdentityUID", "idp", "bob", "bobUserUID", "bob"),
+			ExistingUser:     nil,
+
+			ExpectedActions: []test.Action{
+				{"GetIdentity", "idp:bob"},
+				{"GetUser", "bob"},
+			},
+			ExpectedError: true,
+		},
+		"existing identity, invalid user UID reference": {
+			ProviderName:     "idp",
+			ProviderUserName: "bob",
+
+			ExistingIdentity: makeIdentity("bobIdentityUID", "idp", "bob", "bobUserUIDInvalid", "bob"),
+			ExistingUser:     makeUser("bobUserUID", "bob", "idp:bob"),
+
+			ExpectedActions: []test.Action{
+				{"GetIdentity", "idp:bob"},
+				{"GetUser", "bob"},
+			},
+			ExpectedError: true,
+		},
+		"existing identity, user reference without identity backreference": {
+			ProviderName:     "idp",
+			ProviderUserName: "bob",
+
+			ExistingIdentity: makeIdentity("bobIdentityUID", "idp", "bob", "bobUserUID", "bob"),
+			ExistingUser:     makeUser("bobUserUID", "bob" /*, "idp:bob"*/),
+
+			ExpectedActions: []test.Action{
+				{"GetIdentity", "idp:bob"},
+				{"GetUser", "bob"},
+			},
+			ExpectedError: true,
+		},
+		"existing identity, user reference": {
+			ProviderName:     "idp",
+			ProviderUserName: "bob",
+
+			ExistingIdentity: makeIdentity("bobIdentityUID", "idp", "bob", "bobUserUID", "bob"),
+			ExistingUser:     makeUser("bobUserUID", "bob", "idp:bob"),
+
+			ExpectedActions: []test.Action{
+				{"GetIdentity", "idp:bob"},
+				{"GetUser", "bob"},
+				{"GetUser", "bob"}, // extra request is for group lookup
+			},
+			ExpectedUserName: "bob",
+		},
+	}
+
+	for k, tc := range testcases {
+		actions := []test.Action{}
+		identityRegistry := &test.IdentityRegistry{
+			Get:     map[string]*api.Identity{},
+			Actions: &actions,
+		}
+		userRegistry := &test.UserRegistry{
+			Get:     map[string]*api.User{},
+			Actions: &actions,
+		}
+		if tc.ExistingIdentity != nil {
+			identityRegistry.Get[tc.ExistingIdentity.Name] = tc.ExistingIdentity
+		}
+		if tc.ExistingUser != nil {
+			userRegistry.Get[tc.ExistingUser.Name] = tc.ExistingUser
+		}
+
+		mappingStorage := mappingregistry.NewREST(userRegistry, identityRegistry)
+		mappingRegistry := mappingregistry.NewRegistry(mappingStorage)
+
+		lookupMapper := &lookupIdentityMapper{
+			mappings: mappingRegistry,
+			users:    userRegistry,
+		}
+
+		identity := authapi.NewDefaultUserIdentityInfo(tc.ProviderName, tc.ProviderUserName)
+		user, err := lookupMapper.UserFor(identity)
+		if tc.ExpectedError != (err != nil) {
+			t.Errorf("%s: Expected error=%v, got %v", k, tc.ExpectedError, err)
+			continue
+		}
+		if !tc.ExpectedError && user.GetName() != tc.ExpectedUserName {
+			t.Errorf("%s: Expected username %v, got %v", k, tc.ExpectedUserName, user.GetName())
+			continue
+		}
+
+		for i, action := range actions {
+			if len(tc.ExpectedActions) <= i {
+				t.Fatalf("%s: expected %d actions, got extras: %#v", k, len(tc.ExpectedActions), actions[i:])
+				continue
+			}
+			expectedAction := tc.ExpectedActions[i]
+			if !reflect.DeepEqual(expectedAction, action) {
+				t.Fatalf("%s: expected\n\t%s %#v\nGot\n\t%s %#v", k, expectedAction.Name, expectedAction.Object, action.Name, action.Object)
+				continue
+			}
+		}
+		if len(actions) < len(tc.ExpectedActions) {
+			t.Errorf("Missing %d additional actions:\n\t%#v", len(tc.ExpectedActions)-len(actions), tc.ExpectedActions[len(actions):])
+		}
+	}
+}

pkg/auth/userregistry/identitymapper/mapper.go

@@ -0,0 +1,59 @@
+package identitymapper
+
+import (
+	"fmt"
+
+	authapi "github.com/openshift/origin/pkg/auth/api"
+	"github.com/openshift/origin/pkg/user"
+	identityregistry "github.com/openshift/origin/pkg/user/registry/identity"
+	userregistry "github.com/openshift/origin/pkg/user/registry/user"
+	mappingregistry "github.com/openshift/origin/pkg/user/registry/useridentitymapping"
+)
+
+type MappingMethodType string
+
+const (
+	// MappingMethodLookup does not provision a new identity or user, it only allows identities already associated with users
+	MappingMethodLookup MappingMethodType = "lookup"
+
+	// MappingMethodClaim associates a new identity with a user with the identity's preferred username
+	// if no other identities are already associated with the user
+	MappingMethodClaim MappingMethodType = "claim"
+
+	// MappingMethodAdd associates a new identity with a user with the identity's preferred username,
+	// creating the user if needed, and adding to any existing identities associated with the user
+	MappingMethodAdd MappingMethodType = "add"
+
+	// MappingMethodGenerate finds an available username for a new identity, based on its preferred username
+	// If a user with the preferred username already exists, a unique username is generated
+	MappingMethodGenerate MappingMethodType = "generate"
+)
+
+// NewIdentityUserMapper returns a UserIdentityMapper that does the following:
+// 1. Returns an existing user if the identity exists and is associated with an existing user
+// 2. Returns an error if the identity exists and is not associated with a user (or is associated with a missing user)
+// 3. Handles new identities according to the requested method
+func NewIdentityUserMapper(identities identityregistry.Registry, users userregistry.Registry, method MappingMethodType) (authapi.UserIdentityMapper, error) {
+	// initUser initializes fields in a User API object from its associated Identity
+	// called when adding the first Identity to a User (during create or update of a User)
+	initUser := user.NewDefaultUserInitStrategy()
+
+	switch method {
+	case MappingMethodLookup:
+		mappingStorage := mappingregistry.NewREST(users, identities)
+		mappingRegistry := mappingregistry.NewRegistry(mappingStorage)
+		return &lookupIdentityMapper{mappingRegistry, users}, nil
+
+	case MappingMethodClaim:
+		return &provisioningIdentityMapper{identities, users, NewStrategyClaim(users, initUser)}, nil
+
+	case MappingMethodAdd:
+		return &provisioningIdentityMapper{identities, users, NewStrategyAdd(users, initUser)}, nil
+
+	case MappingMethodGenerate:
+		return &provisioningIdentityMapper{identities, users, NewStrategyGenerate(users, initUser)}, nil
+
+	default:
+		return nil, fmt.Errorf("unsupported mapping method %q", method)
+	}
+}

pkg/auth/userregistry/identitymapper/provision.go

@@ -1,51 +1,35 @@
 package identitymapper
 
 import (
-	"errors"
-	"fmt"
-
 	"github.com/golang/glog"
 	kapi "k8s.io/kubernetes/pkg/api"
 	kerrs "k8s.io/kubernetes/pkg/api/errors"
 	kuser "k8s.io/kubernetes/pkg/auth/user"
 	"k8s.io/kubernetes/pkg/util/sets"
 
 	authapi "github.com/openshift/origin/pkg/auth/api"
-	"github.com/openshift/origin/pkg/user"
 	userapi "github.com/openshift/origin/pkg/user/api"
 	identityregistry "github.com/openshift/origin/pkg/user/registry/identity"
 	userregistry "github.com/openshift/origin/pkg/user/registry/user"
 )
 
-// UserNameGenerator returns a username
-type UserNameGenerator func(base string, sequence int) string
-
-var (
-	// MaxGenerateAttempts limits how many times we try to find an available username for a new identity
-	MaxGenerateAttempts = 100
+// UserForNewIdentityGetter is responsible for creating or locating the persisted User for the given Identity.
+// The preferredUserName is available to the strategies
+type UserForNewIdentityGetter interface {
+	// UserForNewIdentity returns a persisted User object for the given Identity, creating it if needed
+	UserForNewIdentity(ctx kapi.Context, preferredUserName string, identity *userapi.Identity) (*userapi.User, error)
+}
 
-	// DefaultGenerator attempts to use the base name first, then "base2", "base3", ...
-	DefaultGenerator = UserNameGenerator(func(base string, sequence int) string {
-		if sequence == 0 {
-			return base
-		}
-		return fmt.Sprintf("%s%d", base, sequence+1)
-	})
-)
+var _ = authapi.UserIdentityMapper(&provisioningIdentityMapper{})
 
+// provisioningIdentityMapper implements api.UserIdentityMapper
+// If an existing UserIdentityMapping exists for an identity, it is returned.
+// If an identity does not exist, it creates an Identity referencing the user returned from provisioningStrategy.UserForNewIdentity
+// Otherwise an error is returned
 type provisioningIdentityMapper struct {
-	identity    identityregistry.Registry
-	user        userregistry.Registry
-	generator   UserNameGenerator
-	initializer user.Initializer
-}
-
-// NewAlwaysCreateUserIdentityToUserMapper returns an IdentityMapper that does the following:
-// 1. Returns an existing user if the identity exists and is associated with an existing user
-// 2. Returns an error if the identity exists and is not associated with a user
-// 3. Creates the identity and creates and returns a new user with a unique username if the identity does not yet exist
-func NewAlwaysCreateUserIdentityToUserMapper(identityRegistry identityregistry.Registry, userRegistry userregistry.Registry) authapi.UserIdentityMapper {
-	return &provisioningIdentityMapper{identityRegistry, userRegistry, DefaultGenerator, user.NewDefaultUserInitStrategy()}
+	identity             identityregistry.Registry
+	user                 userregistry.Registry
+	provisioningStrategy UserForNewIdentityGetter
 }
 
 // UserFor returns info about the user for whom identity info have been provided
@@ -67,11 +51,14 @@ func (p *provisioningIdentityMapper) userForWithRetries(info authapi.UserIdentit
 
 	if kerrs.IsNotFound(err) {
 		user, err := p.createIdentityAndMapping(ctx, info)
-		// Only retry for AlreadyExists errors, which can occur in the following cases:
+		// Only retry for the following types of errors:
+		// AlreadyExists errors:
 		// * The same user was created by another identity provider with the same preferred username
-		// * The same user was created by another instance of this identity provider
-		// * The same identity was created by another instance of this identity provider
-		if kerrs.IsAlreadyExists(err) && allowedRetries > 0 {
+		// * The same user was created by another instance of this identity provider (e.g. double-clicked login button)
+		// * The same identity was created by another instance of this identity provider (e.g. double-clicked login button)
+		// Conflict errors:
+		// * The same user was updated be another identity provider to add identity info
+		if (kerrs.IsAlreadyExists(err) || kerrs.IsConflict(err)) && allowedRetries > 0 {
 			return p.userForWithRetries(info, allowedRetries-1)
 		}
 		return user, err
@@ -97,7 +84,7 @@ func (p *provisioningIdentityMapper) createIdentityAndMapping(ctx kapi.Context,
 	}
 
 	// Get or create a persisted user pointing to the identity
-	persistedUser, err := p.getOrCreateUserForIdentity(ctx, identity)
+	persistedUser, err := p.provisioningStrategy.UserForNewIdentity(ctx, getPreferredUserName(identity), identity)
 	if err != nil {
 		return nil, err
 	}
@@ -118,54 +105,6 @@ func (p *provisioningIdentityMapper) createIdentityAndMapping(ctx kapi.Context,
 	}, nil
 }
 
-func (p *provisioningIdentityMapper) getOrCreateUserForIdentity(ctx kapi.Context, identity *userapi.Identity) (*userapi.User, error) {
-
-	preferredUserName := getPreferredUserName(identity)
-
-	// Iterate through the max allowed generated usernames
-	// If an existing user references this identity, associate the identity with that user and return
-	// Otherwise, create a user with the first generated user name that does not already exist and return.
-	// Names are created in a deterministic order, so the first one that isn't present gets created.
-	// In the case of a race, one will get to persist the user object and the other will fail.
-	for sequence := 0; sequence < MaxGenerateAttempts; sequence++ {
-		// Get the username we want
-		potentialUserName := p.generator(preferredUserName, sequence)
-
-		// See if it already exists
-		persistedUser, err := p.user.GetUser(ctx, potentialUserName)
-
-		if err != nil && !kerrs.IsNotFound(err) {
-			// Fail on errors other than "not found"
-			return nil, err
-		}
-
-		if err != nil && kerrs.IsNotFound(err) {
-			// Try to create a user with the available name
-			desiredUser := &userapi.User{
-				ObjectMeta: kapi.ObjectMeta{Name: potentialUserName},
-				Identities: []string{identity.Name},
-			}
-
-			// Initialize from the identity
-			p.initializer.InitializeUser(identity, desiredUser)
-
-			// Create the user
-			createdUser, err := p.user.CreateUser(ctx, desiredUser)
-			if err != nil {
-				return nil, err
-			}
-			return createdUser, nil
-		}
-
-		if sets.NewString(persistedUser.Identities...).Has(identity.Name) {
-			// If the existing user references our identity, we're done
-			return persistedUser, nil
-		}
-	}
-
-	return nil, errors.New("Could not create user, max attempts exceeded")
-}
-
 func (p *provisioningIdentityMapper) getMapping(ctx kapi.Context, identity *userapi.Identity) (kuser.Info, error) {
 	if len(identity.User.Name) == 0 {
 		return nil, kerrs.NewNotFound("UserIdentityMapping", identity.Name)