{server,storage}: add LoggedIn flag to AuthRequest and improve storage docs

Currently, whether or not a user has authenticated themselves through
a connector is indicated by a pointer being nil or non-nil. Instead
add an explicit flag that marks this.

server/handlers.go

@@ -264,7 +264,8 @@ func (s *Server) finalizeLogin(identity connector.Identity, authReqID, connector
 	}
 
 	updater := func(a storage.AuthRequest) (storage.AuthRequest, error) {
-		a.Claims = &claims
+		a.LoggedIn = true
+		a.Claims = claims
 		a.ConnectorID = connectorID
 		a.ConnectorData = identity.ConnectorData
 		return a, nil
@@ -282,7 +283,7 @@ func (s *Server) handleApproval(w http.ResponseWriter, r *http.Request) {
 		s.renderError(w, http.StatusInternalServerError, errServerError, "")
 		return
 	}
-	if authReq.Claims == nil {
+	if !authReq.LoggedIn {
 		log.Printf("Auth request does not have an identity for approval")
 		s.renderError(w, http.StatusInternalServerError, errServerError, "")
 		return
@@ -341,7 +342,7 @@ func (s *Server) sendCodeResponse(w http.ResponseWriter, r *http.Request, authRe
 				ConnectorID: authReq.ConnectorID,
 				Nonce:       authReq.Nonce,
 				Scopes:      authReq.Scopes,
-				Claims:      *authReq.Claims,
+				Claims:      authReq.Claims,
 				Expiry:      s.now().Add(time.Minute * 5),
 				RedirectURI: authReq.RedirectURI,
 			}
@@ -358,7 +359,7 @@ func (s *Server) sendCodeResponse(w http.ResponseWriter, r *http.Request, authRe
 			}
 			q.Set("code", code.ID)
 		case responseTypeToken:
-			idToken, expiry, err := s.newIDToken(authReq.ClientID, *authReq.Claims, authReq.Scopes, authReq.Nonce)
+			idToken, expiry, err := s.newIDToken(authReq.ClientID, authReq.Claims, authReq.Scopes, authReq.Nonce)
 			if err != nil {
 				log.Printf("failed to create ID token: %v", err)
 				tokenErr(w, errServerError, "", http.StatusInternalServerError)

storage/kubernetes/types.go

@@ -118,9 +118,11 @@ type AuthRequest struct {
 	// attempts.
 	ForceApprovalPrompt bool `json:"forceApprovalPrompt,omitempty"`
 
+	LoggedIn bool `json:"loggedIn"`
+
 	// The identity of the end user. Generally nil until the user authenticates
 	// with a backend.
-	Claims *Claims `json:"claims,omitempty"`
+	Claims Claims `json:"claims,omitempty"`
 	// The connector used to login the user. Set when the user authenticates.
 	ConnectorID   string `json:"connectorID,omitempty"`
 	ConnectorData []byte `json:"connectorData,omitempty"`
@@ -145,13 +147,11 @@ func toStorageAuthRequest(req AuthRequest) storage.AuthRequest {
 		Nonce:               req.Nonce,
 		State:               req.State,
 		ForceApprovalPrompt: req.ForceApprovalPrompt,
+		LoggedIn:            req.LoggedIn,
 		ConnectorID:         req.ConnectorID,
 		ConnectorData:       req.ConnectorData,
 		Expiry:              req.Expiry,
-	}
+		Claims:              toStorageClaims(req.Claims),
-	if req.Claims != nil {
-		i := toStorageClaims(*req.Claims)
-		a.Claims = &i
 	}
 	return a
 }
@@ -172,14 +172,12 @@ func (cli *client) fromStorageAuthRequest(a storage.AuthRequest) AuthRequest {
 		RedirectURI:         a.RedirectURI,
 		Nonce:               a.Nonce,
 		State:               a.State,
+		LoggedIn:            a.LoggedIn,
 		ForceApprovalPrompt: a.ForceApprovalPrompt,
 		ConnectorID:         a.ConnectorID,
 		ConnectorData:       a.ConnectorData,
 		Expiry:              a.Expiry,
-	}
+		Claims:              fromStorageClaims(a.Claims),
-	if a.Claims != nil {
-		i := fromStorageClaims(*a.Claims)
-		req.Claims = &i
 	}
 	return req
 }

storage/storage.go

@@ -70,28 +70,41 @@ type Storage interface {
 	DeleteRefresh(id string) error
 
 	// Update functions are assumed to be a performed within a single object transaction.
+	//
+	// updaters may be called multiple times.
 	UpdateClient(id string, updater func(old Client) (Client, error)) error
 	UpdateKeys(updater func(old Keys) (Keys, error)) error
 	UpdateAuthRequest(id string, updater func(a AuthRequest) (AuthRequest, error)) error
+
+	// TODO(ericchiang): Add a GarbageCollect(now time.Time) method so conformance tests
+	// can test implementations.
 }
 
-// Client is an OAuth2 client.
+// Client represents an OAuth2 client.
 //
 // For further reading see:
 //   * Trusted peers: https://developers.google.com/identity/protocols/CrossClientAuth
 //   * Public clients: https://developers.google.com/api-client-library/python/auth/installed-app
 type Client struct {
-	ID           string   `json:"id" yaml:"id"`
+	// Client ID and secret used to identify the client.
-	Secret       string   `json:"secret" yaml:"secret"`
+	ID     string `json:"id" yaml:"id"`
+	Secret string `json:"secret" yaml:"secret"`
+
+	// A registered set of redirect URIs. When redirecting from dex to the client, the URI
+	// requested to redirect to MUST match one of these values, unless the client is "public".
 	RedirectURIs []string `json:"redirectURIs" yaml:"redirectURIs"`
 
-	// TrustedPeers are a list of peers which can issue tokens on this client's behalf.
+	// TrustedPeers are a list of peers which can issue tokens on this client's behalf using
+	// the dynamic "oauth2:server:client_id:(client_id)" scope. If a peer makes such a request,
+	// this client's ID will appear as the ID Token's audience.
+	//
 	// Clients inherently trust themselves.
 	TrustedPeers []string `json:"trustedPeers" yaml:"trustedPeers"`
 
 	// Public clients must use either use a redirectURL 127.0.0.1:X or "urn:ietf:wg:oauth:2.0:oob"
 	Public bool `json:"public" yaml:"public"`
 
+	// Name and LogoURL used when displaying this client to the end user.
 	Name    string `json:"name" yaml:"name"`
 	LogoURL string `json:"logoURL" yaml:"logoURL"`
 }
@@ -109,53 +122,79 @@ type Claims struct {
 // AuthRequest represents a OAuth2 client authorization request. It holds the state
 // of a single auth flow up to the point that the user authorizes the client.
 type AuthRequest struct {
-	ID       string
+	// ID used to identify the authorization request.
+	ID string
+
+	// ID of the client requesting authorization from a user.
 	ClientID string
 
+	// Values parsed from the initial request. These describe the resources the client is
+	// requesting as well as values describing the form of the response.
 	ResponseTypes []string
 	Scopes        []string
 	RedirectURI   string
-
+	Nonce         string
-	Nonce string
+	State         string
-	State string
 
 	// The client has indicated that the end user must be shown an approval prompt
 	// on all requests. The server cannot cache their initial action for subsequent
 	// attempts.
 	ForceApprovalPrompt bool
 
+	Expiry time.Time
+
+	// Has the user proved their identity through a backing identity provider?
+	//
+	// If false, the following fields are invalid.
+	LoggedIn bool
+
 	// The identity of the end user. Generally nil until the user authenticates
 	// with a backend.
-	Claims *Claims
+	Claims Claims
 
 	// The connector used to login the user and any data the connector wishes to persists.
 	// Set when the user authenticates.
 	ConnectorID   string
 	ConnectorData []byte
-
-	Expiry time.Time
 }
 
 // AuthCode represents a code which can be exchanged for an OAuth2 token response.
+//
+// This value is created once an end user has authorized a client, the server has
+// redirect the end user back to the client, but the client hasn't exchanged the
+// code for an access_token and id_token.
 type AuthCode struct {
+	// Actual string returned as the "code" value.
 	ID string
 
-	ClientID    string
+	// The client this code value is valid for. When exchanging the code for a
+	// token response, the client must use its client_secret to authenticate.
+	ClientID string
+
+	// As part of the OAuth2 spec when a client makes a token request it MUST
+	// present the same redirect_uri as the initial redirect. This values is saved
+	// to make this check.
+	//
+	// https://tools.ietf.org/html/rfc6749#section-4.1.3
 	RedirectURI string
 
-	ConnectorID   string
+	// If provided by the client in the initial request, the provider MUST create
-	ConnectorData []byte
+	// a ID Token with this nonce in the JWT payload.
-
 	Nonce string
 
+	// Scopes authorized by the end user for the client.
 	Scopes []string
 
-	Claims Claims
+	// Authentication data provided by an upstream source.
+	ConnectorID   string
+	ConnectorData []byte
+	Claims        Claims
 
 	Expiry time.Time
 }
 
-// RefreshToken is an OAuth2 refresh token.
+// RefreshToken is an OAuth2 refresh token which allows a client to request new
+// tokens on the end user's behalf.
 type RefreshToken struct {
 	// The actual refresh token.
 	RefreshToken string
@@ -163,17 +202,19 @@ type RefreshToken struct {
 	// Client this refresh token is valid for.
 	ClientID string
 
+	// Authentication data provided by an upstream source.
 	ConnectorID   string
 	ConnectorData []byte
+	Claims        Claims
 
 	// Scopes present in the initial request. Refresh requests may specify a set
 	// of scopes different from the initial request when refreshing a token,
 	// however those scopes must be encompassed by this set.
 	Scopes []string
 
+	// Nonce value supplied during the initial redirect. This is required to be part
+	// of the claims of any future id_token generated by the client.
 	Nonce string
-
-	Claims Claims
 }
 
 // VerificationKey is a rotated signing key which can still be used to verify
@@ -188,6 +229,7 @@ type Keys struct {
 	// Key for creating and verifying signatures. These may be nil.
 	SigningKey    *jose.JSONWebKey
 	SigningKeyPub *jose.JSONWebKey
+
 	// Old signing keys which have been rotated but can still be used to validate
 	// existing signatures.
 	VerificationKeys []VerificationKey

storage/storagetest/storagetest.go

@@ -35,7 +35,7 @@ func testUpdateAuthRequest(t *testing.T, s storage.Storage) {
 		t.Fatalf("failed creating auth request: %v", err)
 	}
 	if err := s.UpdateAuthRequest(a.ID, func(old storage.AuthRequest) (storage.AuthRequest, error) {
-		old.Claims = &identity
+		old.Claims = identity
 		old.ConnectorID = "connID"
 		return old, nil
 	}); err != nil {
@@ -46,11 +46,8 @@ func testUpdateAuthRequest(t *testing.T, s storage.Storage) {
 	if err != nil {
 		t.Fatalf("failed to get auth req: %v", err)
 	}
-	if got.Claims == nil {
+	if !reflect.DeepEqual(got.Claims, identity) {
-		t.Fatalf("no identity in auth request")
+		t.Fatalf("update failed, wanted identity=%#v got %#v", identity, got.Claims)
-	}
-	if !reflect.DeepEqual(*got.Claims, identity) {
-		t.Fatalf("update failed, wanted identity=%#v got %#v", identity, *got.Claims)
 	}
 }