Make client authorizations tolerant of UID changes

This change makes it so that if an OAuthClientAuthorization
references a user with the wrong UID, that client authorization is
deleted.  This UID mismatch is caused by the deletion and recreation
of users.  Thus this makes it so that the "new" user is unaffected
by the "old" user's client authorization.  The new user simply goes
through the grant flow just as if no client authorization existed.

Signed-off-by: Monis Khan <mkhan@redhat.com>

Author: enj

pkg/auth/oauth/registry/grantchecker.go

@@ -1,18 +1,24 @@
 package registry
 
 import (
-	"fmt"
+	stderrors "errors"
 
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apiserver/pkg/authentication/user"
+	kuser "k8s.io/apiserver/pkg/authentication/user"
+	"k8s.io/kubernetes/pkg/client/retry"
 
 	"github.com/openshift/origin/pkg/auth/api"
+	"github.com/openshift/origin/pkg/oauth/apis/oauth"
 	oauthclient "github.com/openshift/origin/pkg/oauth/generated/internalclientset/typed/oauth/internalversion"
 	"github.com/openshift/origin/pkg/oauth/registry/oauthclientauthorization"
 	"github.com/openshift/origin/pkg/oauth/scope"
+
+	"github.com/golang/glog"
 )
 
+var errEmptyUID = stderrors.New("user from request has empty UID and thus cannot perform a grant flow")
+
 type ClientAuthorizationGrantChecker struct {
 	client oauthclient.OAuthClientAuthorizationInterface
 }
@@ -21,21 +27,61 @@ func NewClientAuthorizationGrantChecker(client oauthclient.OAuthClientAuthorizat
 	return &ClientAuthorizationGrantChecker{client}
 }
 
-func (c *ClientAuthorizationGrantChecker) HasAuthorizedClient(user user.Info, grant *api.Grant) (approved bool, err error) {
-	id := oauthclientauthorization.ClientAuthorizationName(user.GetName(), grant.Client.GetId())
-	authorization, err := c.client.Get(id, metav1.GetOptions{})
-	if errors.IsNotFound(err) {
-		return false, nil
+func (c *ClientAuthorizationGrantChecker) HasAuthorizedClient(user kuser.Info, grant *api.Grant) (approved bool, err error) {
+	// Validation prevents OAuthClientAuthorization.UserUID from being empty (and always has).
+	// However, user.GetUID() is empty during impersonation, meaning this flow does not work for impersonation.
+	// This is fine because no OAuth / grant flow works with impersonation in general.
+	if len(user.GetUID()) == 0 {
+		return false, errEmptyUID
 	}
-	if err != nil {
+
+	id := oauthclientauthorization.ClientAuthorizationName(user.GetName(), grant.Client.GetId())
+	var authorization *oauth.OAuthClientAuthorization
+
+	// getClientAuthorization ignores not found errors, thus it is possible for authorization to be nil
+	// getClientAuthorization does not ignore conflict errors, so we retry those in case we having multiple clients racing on this grant flow
+	// all other errors are considered fatal
+	if err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
+		authorization, err = c.getClientAuthorization(id, user)
+		return err
+	}); err != nil {
 		return false, err
 	}
-	if len(authorization.UserUID) != 0 && authorization.UserUID != user.GetUID() {
-		return false, fmt.Errorf("user %s UID %s does not match stored client authorization value for UID %s", user.GetName(), user.GetUID(), authorization.UserUID)
-	}
+
 	// TODO: improve this to allow the scope implementation to determine overlap
-	if !scope.Covers(authorization.Scopes, scope.Split(grant.Scope)) {
+	if authorization == nil || !scope.Covers(authorization.Scopes, scope.Split(grant.Scope)) {
 		return false, nil
 	}
+
 	return true, nil
 }
+
+// getClientAuthorization gets the OAuthClientAuthorization with the given name and validates that it matches the given user
+// it attempts to delete stale client authorizations, and thus must be retried in case of conflicts
+func (c *ClientAuthorizationGrantChecker) getClientAuthorization(name string, user kuser.Info) (*oauth.OAuthClientAuthorization, error) {
+	authorization, err := c.client.Get(name, metav1.GetOptions{})
+	if errors.IsNotFound(err) {
+		// if no such authorization exists, it simply means the user needs to go through the grant flow
+		return nil, nil
+	}
+	if err != nil {
+		// any other error is fatal (this will never be retried since it cannot return a conflict error)
+		return nil, err
+	}
+
+	// check to see if we have a stale authorization
+	// user.GetUID() and authorization.UserUID are both guaranteed to be non-empty
+	if user.GetUID() != authorization.UserUID {
+		glog.Infof("%#v does not match stored client authorization %#v, attempting to delete stale authorization", user, authorization)
+		if err := c.client.Delete(name, metav1.NewPreconditionDeleteOptions(string(authorization.UID))); err != nil && !errors.IsNotFound(err) {
+			// ignore not found since that could be caused by multiple grant flows occurring at once (the other flow deleted the authorization before we did)
+			// this could be a conflict error, which will cause this whole function to be retried
+			return nil, err
+		}
+		// we successfully deleted the authorization so the user needs to go through the grant flow
+		return nil, nil
+	}
+
+	// everything looks good so we can return the authorization
+	return authorization, nil
+}

pkg/auth/oauth/registry/registry_test.go

@@ -130,6 +130,7 @@ func TestRegistryAndServer(t *testing.T) {
 			AuthSuccess: true,
 			AuthUser: &user.DefaultInfo{
 				Name: "user",
+				UID:  "1",
 			},
 			Check: func(h *testHandlers, _ *http.Request) {
 				if h.AuthNeed || !h.GrantNeed || h.AuthErr != nil || h.GrantErr != nil || h.HandleAuthorizeReq.Authorized {
@@ -168,10 +169,12 @@ func TestRegistryAndServer(t *testing.T) {
 			AuthSuccess: true,
 			AuthUser: &user.DefaultInfo{
 				Name: "user",
+				UID:  "1",
 			},
 			ClientAuth: &oapi.OAuthClientAuthorization{
 				ObjectMeta: metav1.ObjectMeta{Name: "user:test"},
 				UserName:   "user",
+				UserUID:    "1",
 				ClientName: "test",
 				Scopes:     []string{"user:info"},
 			},
@@ -187,10 +190,12 @@ func TestRegistryAndServer(t *testing.T) {
 			AuthSuccess: true,
 			AuthUser: &user.DefaultInfo{
 				Name: "user",
+				UID:  "1",
 			},
 			ClientAuth: &oapi.OAuthClientAuthorization{
 				ObjectMeta: metav1.ObjectMeta{Name: "user:test"},
 				UserName:   "user",
+				UserUID:    "1",
 				ClientName: "test",
 				Scopes:     []string{"user:info", "user:check-access"},
 			},
@@ -206,10 +211,12 @@ func TestRegistryAndServer(t *testing.T) {
 			AuthSuccess: true,
 			AuthUser: &user.DefaultInfo{
 				Name: "user",
+				UID:  "1",
 			},
 			ClientAuth: &oapi.OAuthClientAuthorization{
 				ObjectMeta: metav1.ObjectMeta{Name: "user:test"},
 				UserName:   "user",
+				UserUID:    "1",
 				ClientName: "test",
 				Scopes:     []string{"user:full"},
 			},
@@ -227,6 +234,45 @@ func TestRegistryAndServer(t *testing.T) {
 				}
 			},
 		},
+		"has auth with no UID, mimics impersonation": {
+			Client:      validClient,
+			AuthSuccess: true,
+			AuthUser: &user.DefaultInfo{
+				Name: "user",
+			},
+			ClientAuth: &oapi.OAuthClientAuthorization{
+				ObjectMeta: metav1.ObjectMeta{Name: "user:test"},
+				UserName:   "user",
+				UserUID:    "2",
+				ClientName: "test",
+				Scopes:     []string{"user:full"},
+			},
+			Check: func(h *testHandlers, r *http.Request) {
+				if h.AuthNeed || h.GrantNeed || h.AuthErr != nil || h.GrantErr != nil || h.HandleAuthorizeReq.Authorized || h.HandleAuthorizeResp.ErrorId != "server_error" {
+					t.Errorf("expected server_error error: %#v, %#v, %#v", h, h.HandleAuthorizeReq, h.HandleAuthorizeResp)
+				}
+			},
+		},
+		"has auth and grant with different UIDs": {
+			Client:      validClient,
+			AuthSuccess: true,
+			AuthUser: &user.DefaultInfo{
+				Name: "user",
+				UID:  "1",
+			},
+			ClientAuth: &oapi.OAuthClientAuthorization{
+				ObjectMeta: metav1.ObjectMeta{Name: "user:test"},
+				UserName:   "user",
+				UserUID:    "2",
+				ClientName: "test",
+				Scopes:     []string{"user:full"},
+			},
+			Check: func(h *testHandlers, _ *http.Request) {
+				if h.AuthNeed || !h.GrantNeed || h.AuthErr != nil || h.GrantErr != nil || h.HandleAuthorizeReq.Authorized {
+					t.Errorf("expected request to need to grant access due to UID mismatch: %#v", h)
+				}
+			},
+		},
 	}
 
 	for _, testCase := range testCases {

test/integration/oauth_serviceaccount_client_test.go

@@ -67,7 +67,8 @@ func TestOAuthServiceAccountClient(t *testing.T) {
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
-	clusterAdminOAuthClient := oauthclient.NewForConfigOrDie(clusterAdminClientConfig)
+	clusterAdminOAuthClient := oauthclient.NewForConfigOrDie(clusterAdminClientConfig).Oauth()
+	clusterAdminUserClient := userclient.NewForConfigOrDie(clusterAdminClientConfig)
 
 	projectName := "hammer-project"
 	if _, _, err := testserver.CreateNewProject(clusterAdminClientConfig, projectName, "harold"); err != nil {
@@ -374,6 +375,85 @@ func TestOAuthServiceAccountClient(t *testing.T) {
 		})
 		clusterAdminOAuthClient.OAuthClientAuthorizations().Delete("harold:"+oauthClientConfig.ClientId, nil)
 	}
+
+	{
+		oauthClientConfig := &osincli.ClientConfig{
+			ClientId:     apiserverserviceaccount.MakeUsername(defaultSA.Namespace, defaultSA.Name),
+			ClientSecret: string(oauthSecret.Data[kapi.ServiceAccountTokenKey]),
+			AuthorizeUrl: clusterAdminClientConfig.Host + "/oauth/authorize",
+			TokenUrl:     clusterAdminClientConfig.Host + "/oauth/token",
+			RedirectUrl:  redirectURL,
+			Scope:        scope.Join([]string{"user:info", "role:edit:" + projectName}),
+			SendClientSecretInParams: true,
+		}
+		t.Log("Testing grant flow is reentrant")
+		// First time, the approval steps are needed
+		// Second time, the approval steps are skipped
+		// Then we delete and recreate the user to make the client authorization UID no longer match
+		// Third time, the approval steps are needed
+		// Fourth time, the approval steps are skipped
+		runOAuthFlow(t, clusterAdminClientConfig, projectName, oauthClientConfig, nil, authorizationCodes, authorizationErrors, true, true, []string{
+			"GET /oauth/authorize",
+			"received challenge",
+			"GET /oauth/authorize",
+			"redirect to /oauth/authorize/approve",
+			"form",
+			"POST /oauth/authorize/approve",
+			"redirect to /oauth/authorize",
+			"redirect to /oauthcallback",
+			"code",
+			"scope:" + oauthClientConfig.Scope,
+		})
+		runOAuthFlow(t, clusterAdminClientConfig, projectName, oauthClientConfig, nil, authorizationCodes, authorizationErrors, true, true, []string{
+			"GET /oauth/authorize",
+			"received challenge",
+			"GET /oauth/authorize",
+			"redirect to /oauthcallback",
+			"code",
+			"scope:" + oauthClientConfig.Scope,
+		})
+
+		// Delete the user to make the client authorization UID no longer match
+		// runOAuthFlow will cause the creation of the same user with a different UID during its challenge phase
+		if err := deleteUser(clusterAdminUserClient, "harold"); err != nil {
+			t.Fatalf("Failed to delete and recreate harold user: %v", err)
+		}
+
+		runOAuthFlow(t, clusterAdminClientConfig, projectName, oauthClientConfig, nil, authorizationCodes, authorizationErrors, true, true, []string{
+			"GET /oauth/authorize",
+			"received challenge",
+			"GET /oauth/authorize",
+			"redirect to /oauth/authorize/approve",
+			"form",
+			"POST /oauth/authorize/approve",
+			"redirect to /oauth/authorize",
+			"redirect to /oauthcallback",
+			"code",
+			"scope:" + oauthClientConfig.Scope,
+		})
+		runOAuthFlow(t, clusterAdminClientConfig, projectName, oauthClientConfig, nil, authorizationCodes, authorizationErrors, true, true, []string{
+			"GET /oauth/authorize",
+			"received challenge",
+			"GET /oauth/authorize",
+			"redirect to /oauthcallback",
+			"code",
+			"scope:" + oauthClientConfig.Scope,
+		})
+		clusterAdminOAuthClient.OAuthClientAuthorizations().Delete("harold:"+oauthClientConfig.ClientId, nil)
+	}
+}
+
+func deleteUser(clusterAdminUserClient userclient.UserInterface, name string) error {
+	oldUser, err := clusterAdminUserClient.Users().Get(name, metav1.GetOptions{})
+	if err != nil {
+		return err
+	}
+	for _, identity := range oldUser.Identities {
+		if err := clusterAdminUserClient.Identities().Delete(identity, nil); err != nil {
+			return err
+		}
+	}
+	return clusterAdminUserClient.Users().Delete(name, nil)
 }
 
 func drain(ch chan string) {