DIfferentiate Between Subject and Email

pkg/handler/organizations/client.go

@@ -171,7 +171,7 @@ func (c *Client) getUserbyEmail(ctx context.Context, rbacClient *rbac.RBAC, info
 		}
 	}
 
-	user, err := rbacClient.GetActiveUser(ctx, email)
+	user, err := rbacClient.GetActiveUserByEmail(ctx, email)
 	if err != nil {
 		return nil, errors.HTTPNotFound().WithError(err)
 	}
@@ -202,7 +202,7 @@ func (c *Client) organizationIDs(ctx context.Context, rbacClient *rbac.RBAC, ema
 			return nil, err
 		}
 	} else {
-		user, err = rbacClient.GetActiveUser(ctx, info.Userinfo.Sub)
+		user, err = rbacClient.GetActiveUserByID(ctx, info.Userinfo.Sub)
 		if err != nil {
 			return nil, errors.HTTPNotFound().WithError(err)
 		}

pkg/middleware/audit/logging.go

@@ -136,13 +136,19 @@ func (l *Logger) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	subject := info.Userinfo.Sub
+
+	if info.Userinfo.Email != nil {
+		subject = *info.Userinfo.Email
+	}
+
 	logParams := []any{
 		"component", &Component{
 			Name:    l.application,
 			Version: l.version,
 		},
 		"actor", &Actor{
-			Subject: info.Userinfo.Sub,
+			Subject: subject,
 		},
 		"operation", &Operation{
 			Verb: r.Method,

pkg/oauth2/oauth2.go

@@ -926,7 +926,7 @@ func (a *Authenticator) Callback(w http.ResponseWriter, r *http.Request) {
 	// Now we have done code exchange, we have access to the id_token and that
 	// allows us to see if the user actually exists.  If it doesn't then we
 	// either deny entry or let them signup.
-	user, err := a.rbac.GetUser(r.Context(), idToken.Email.Email)
+	user, err := a.rbac.GetUserByEmail(r.Context(), idToken.Email.Email)
 	if err != nil {
 		if !goerrors.Is(err, rbac.ErrResourceReference) {
 			redirector.raise(ErrorServerError, "user lookup failure")
@@ -1275,7 +1275,7 @@ func (a *Authenticator) Onboard(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	shadowUser, err := a.rbac.GetUser(r.Context(), state.IDToken.Email.Email)
+	shadowUser, err := a.rbac.GetUserByEmail(r.Context(), state.IDToken.Email.Email)
 	if err != nil {
 		redirector.raise(ErrorServerError, "failed to read shadow user")
 		return
@@ -1399,7 +1399,7 @@ func oidcHash(value string) string {
 }
 
 // oidcIDToken builds an OIDC ID token.
-func (a *Authenticator) oidcIDToken(r *http.Request, idToken *oidc.IDToken, query url.Values, expiry time.Duration, atHash string, lastAuthenticationTime time.Time) (*string, error) {
+func (a *Authenticator) oidcIDToken(r *http.Request, userID string, idToken *oidc.IDToken, query url.Values, expiry time.Duration, atHash string, lastAuthenticationTime time.Time) (*string, error) {
 	scope := strings.Split(query.Get("scope"), " ")
 
 	//nolint:nilnil
@@ -1409,9 +1409,8 @@ func (a *Authenticator) oidcIDToken(r *http.Request, idToken *oidc.IDToken, quer
 
 	claims := &oidc.IDToken{
 		Claims: jwt.Claims{
-			Issuer: "https://" + r.Host,
-			// TODO: we should use the user ID.
-			Subject: idToken.Email.Email,
+			Issuer:  "https://" + r.Host,
+			Subject: userID,
 			Audience: []string{
 				query.Get("client_id"),
 			},
@@ -1480,8 +1479,8 @@ func (a *Authenticator) validateClientSecret(r *http.Request, query url.Values)
 }
 
 // revokeSession revokes all tokens for a clientID.
-func (a *Authenticator) revokeSession(ctx context.Context, clientID, codeID, subject string) error {
-	user, err := a.rbac.GetActiveUser(ctx, subject)
+func (a *Authenticator) revokeSession(ctx context.Context, clientID, codeID, userID string) error {
+	user, err := a.rbac.GetActiveUserByID(ctx, userID)
 	if err != nil {
 		return errors.OAuth2ServerError("failed to lookup user").WithError(err)
 	}
@@ -1542,7 +1541,7 @@ func (a *Authenticator) TokenAuthorizationCode(w http.ResponseWriter, r *http.Re
 	// authentication code, we just clear out anything associated with the client
 	// session.
 	if _, ok := a.codeCache.Get(codeRaw); !ok {
-		_ = a.revokeSession(r.Context(), clientID, code.ID, code.IDToken.Email.Email)
+		_ = a.revokeSession(r.Context(), clientID, code.ID, code.UserID)
 
 		return nil, errors.OAuth2InvalidGrant("code is not present in cache")
 	}
@@ -1552,12 +1551,10 @@ func (a *Authenticator) TokenAuthorizationCode(w http.ResponseWriter, r *http.Re
 	info := &IssueInfo{
 		Issuer:   "https://" + r.Host,
 		Audience: r.Host,
-		// TODO: we should probably use the user ID here.
-		Subject: code.IDToken.Email.Email,
-		Type:    TokenTypeFederated,
+		Subject:  code.UserID,
+		Type:     TokenTypeFederated,
 		Federated: &FederatedClaims{
 			ClientID: clientID,
-			UserID:   code.UserID,
 			Provider: code.OAuth2Provider,
 			Scope:    NewScope(clientQuery.Get("scope")),
 		},
@@ -1571,7 +1568,7 @@ func (a *Authenticator) TokenAuthorizationCode(w http.ResponseWriter, r *http.Re
 	}
 
 	// Handle OIDC.
-	idToken, err := a.oidcIDToken(r, code.IDToken, clientQuery, a.options.AccessTokenDuration, oidcHash(tokens.AccessToken), tokens.LastAuthenticationTime)
+	idToken, err := a.oidcIDToken(r, code.UserID, code.IDToken, clientQuery, a.options.AccessTokenDuration, oidcHash(tokens.AccessToken), tokens.LastAuthenticationTime)
 	if err != nil {
 		return nil, err
 	}
@@ -1625,7 +1622,7 @@ func (a *Authenticator) validateRefreshToken(ctx context.Context, r *http.Reques
 		return err
 	}
 
-	user, err := a.rbac.GetActiveUser(ctx, claims.Claims.Subject)
+	user, err := a.rbac.GetActiveUserByID(ctx, claims.Claims.Subject)
 	if err != nil {
 		return errors.OAuth2ServerError("failed to lookup user").WithError(err)
 	}

pkg/oauth2/oauth2_test.go

@@ -99,13 +99,11 @@ func TestTokens(t *testing.T) {
 	time.Sleep(2 * josetesting.RefreshPeriod)
 
 	issueInfo := &oauth2.IssueInfo{
-		Issuer:   "https://foo.com",
-		Audience: "foo.com",
-		Subject:  "[email protected]",
-		Type:     oauth2.TokenTypeFederated,
-		Federated: &oauth2.FederatedClaims{
-			UserID: "fake",
-		},
+		Issuer:    "https://foo.com",
+		Audience:  "foo.com",
+		Subject:   "fake",
+		Type:      oauth2.TokenTypeFederated,
+		Federated: &oauth2.FederatedClaims{},
 	}
 
 	tokens, err := authenticator.Issue(ctx, issueInfo)

pkg/oauth2/tokens.go

@@ -60,9 +60,6 @@ type FederatedClaims struct {
 	Provider string `json:"idp"`
 	// ClientID is the oauth2 client that the user is using.
 	ClientID string `json:"cid"`
-	// UserID is set when the token is issued to a user.
-	// TODO: this should be the subject.
-	UserID string `json:"uid"`
 	// Scope is the set of scopes requested by the client, and is used to
 	// populate the userinfo response.
 	Scope Scope `json:"sco"`
@@ -233,7 +230,7 @@ func (a *Authenticator) Issue(ctx context.Context, info *IssueInfo) (*Tokens, er
 	}
 
 	if info.Federated != nil {
-		user, err := a.getUser(ctx, info.Federated.UserID)
+		user, err := a.getUser(ctx, info.Subject)
 		if err != nil {
 			return nil, err
 		}
@@ -364,8 +361,7 @@ func (a *Authenticator) verifyUserSession(ctx context.Context, info *VerifyInfo,
 		return nil
 	}
 
-	// TODO: the subject should be the user ID anyway...
-	user, err := a.rbac.GetActiveUser(ctx, claims.Subject)
+	user, err := a.rbac.GetActiveUserByID(ctx, claims.Subject)
 	if err != nil {
 		return err
 	}

pkg/rbac/rbac.go

@@ -67,15 +67,15 @@ func New(client client.Client, namespace string, options *Options) *RBAC {
 	}
 }
 
-func (r *RBAC) GetUser(ctx context.Context, subject string) (*unikornv1.User, error) {
+func (r *RBAC) GetUserByEmail(ctx context.Context, email string) (*unikornv1.User, error) {
 	result := &unikornv1.UserList{}
 
 	if err := r.client.List(ctx, result, &client.ListOptions{}); err != nil {
 		return nil, err
 	}
 
 	index := slices.IndexFunc(result.Items, func(user unikornv1.User) bool {
-		return user.Spec.Subject == subject
+		return user.Spec.Subject == email
 	})
 
 	if index < 0 {
@@ -85,9 +85,32 @@ func (r *RBAC) GetUser(ctx context.Context, subject string) (*unikornv1.User, er
 	return &result.Items[index], nil
 }
 
+func (r *RBAC) GetActiveUserByEmail(ctx context.Context, email string) (*unikornv1.User, error) {
+	user, err := r.GetUserByEmail(ctx, email)
+	if err != nil {
+		return nil, err
+	}
+
+	if user.Spec.State != unikornv1.UserStateActive {
+		return nil, fmt.Errorf("%w: user is not active", ErrResourceReference)
+	}
+
+	return user, nil
+}
+
+func (r *RBAC) GetUserByID(ctx context.Context, userID string) (*unikornv1.User, error) {
+	result := &unikornv1.User{}
+
+	if err := r.client.Get(ctx, client.ObjectKey{Namespace: r.namespace, Name: userID}, result); err != nil {
+		return nil, err
+	}
+
+	return result, nil
+}
+
 // GetActiveUser returns a user that match the subject and is active.
-func (r *RBAC) GetActiveUser(ctx context.Context, subject string) (*unikornv1.User, error) {
-	user, err := r.GetUser(ctx, subject)
+func (r *RBAC) GetActiveUserByID(ctx context.Context, userID string) (*unikornv1.User, error) {
+	user, err := r.GetUserByID(ctx, userID)
 	if err != nil {
 		return nil, err
 	}
@@ -398,7 +421,7 @@ func (r *RBAC) GetACL(ctx context.Context, organizationID string) (*openapi.Acl,
 	default:
 		// A subject may be part of any organization's group, so look for that user
 		// and a record that indicates they are part of an organization.
-		user, err := r.GetActiveUser(ctx, info.Userinfo.Sub)
+		user, err := r.GetActiveUserByID(ctx, info.Userinfo.Sub)
 		if err != nil {
 			return nil, err
 		}