Repository: dexidp/dex
Pull request #3828 (2024-11-06 13:40:45)

Create offline sessions if approval is skipped
<!--
Thank you for sending a pull request! Here are some tips for contributors:

1. Fill the description template below.
2. Sign a DCO (if you haven't already signed it).
3. Include appropriate tests (if necessary). Make sure that all CI checks passed.
4. If the Pull Request is a work in progress, make use of GitHub's "Draft PR" feature and mark it as such.
-->

#### Overview

Offline session is not created if the skip approval option is toggled. In this case Dex looses connector data and cannot refresh tokens.

It leads to the different refreshing errors in logs like:
```
{"time":"2024-11-06T10:19:31.541474938Z","level":"ERROR","msg":"failed to refresh identity","err":"gitlab: failed to get refresh token: oauth2: \"invalid_grant\" \"The provided authorization grant is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client.\"","client_remote_addr":"10.244.28.54","request_id":"8e46b1aa-3bd2-4ef1-a844-ad0db91dbc1e"}
```

#### What this PR does / why we need it

This PR adjust the part of the code to make it creating and updating connectorData when it is needed avoiding refresh errors.

#### Special notes for your reviewer



Patch:
diff --git a/.github/workflows/artifacts.yaml b/.github/workflows/artifacts.yaml
index c0c90bfe..ec81b9f7 100644
--- a/.github/workflows/artifacts.yaml
+++ b/.github/workflows/artifacts.yaml
@@ -215,6 +215,9 @@ jobs:
           scan-type: 'fs'
           scan-ref: '.'
           cache-dir: "./cache"
+        env:
+          TRIVY_SKIP_DB_UPDATE: true
+          TRIVY_SKIP_JAVA_DB_UPDATE: true
 
       ## Trivy-db uses `0600` permissions.
       ## But `action/cache` use `runner` user by default
diff --git a/Dockerfile b/Dockerfile
index af920485..839ec3ed 100644
--- a/Dockerfile
+++ b/Dockerfile
@@ -2,7 +2,7 @@ ARG BASE_IMAGE=alpine
 
 FROM --platform=$BUILDPLATFORM tonistiigi/xx:1.5.0@sha256:0c6a569797744e45955f39d4f7538ac344bfb7ebf0a54006a0a4297b153ccf0f AS xx
 
-FROM --platform=$BUILDPLATFORM golang:1.23.2-alpine3.20@sha256:9dd2625a1ff2859b8d8b01d8f7822c0f528942fe56cfe7a1e7c38d3b8d72d679 AS builder
+FROM --platform=$BUILDPLATFORM golang:1.23.3-alpine3.20@sha256:09742590377387b931261cbeb72ce56da1b0d750a27379f7385245b2b058b63a AS builder
 
 COPY --from=xx / /
 
@@ -35,13 +35,13 @@ RUN make release-binary
 
 RUN xx-verify /go/bin/dex && xx-verify /go/bin/docker-entrypoint
 
-FROM alpine:3.20.3@sha256:beefdbd8a1da6d2915566fde36db9db0b524eb737fc57cd1367effd16dc0d06d AS stager
+FROM alpine:3.20.3@sha256:1e42bbe2508154c9126d48c2b8a75420c3544343bf86fd041fb7527e017a4b4a AS stager
 
 RUN mkdir -p /var/dex
 RUN mkdir -p /etc/dex
 COPY config.docker.yaml /etc/dex/
 
-FROM alpine:3.20.3@sha256:beefdbd8a1da6d2915566fde36db9db0b524eb737fc57cd1367effd16dc0d06d AS gomplate
+FROM alpine:3.20.3@sha256:1e42bbe2508154c9126d48c2b8a75420c3544343bf86fd041fb7527e017a4b4a AS gomplate
 
 ARG TARGETOS
 ARG TARGETARCH
@@ -54,7 +54,7 @@ RUN wget -O /usr/local/bin/gomplate \
   && chmod +x /usr/local/bin/gomplate
 
 # For Dependabot to detect base image versions
-FROM alpine:3.20.3@sha256:beefdbd8a1da6d2915566fde36db9db0b524eb737fc57cd1367effd16dc0d06d AS alpine
+FROM alpine:3.20.3@sha256:1e42bbe2508154c9126d48c2b8a75420c3544343bf86fd041fb7527e017a4b4a AS alpine
 FROM gcr.io/distroless/static-debian12:nonroot@sha256:3a03fc0826340c7deb82d4755ca391bef5adcedb8892e58412e1a6008199fa91 AS distroless
 
 FROM $BASE_IMAGE
diff --git a/examples/go.mod b/examples/go.mod
index c2b28e9e..882c6aed 100644
--- a/examples/go.mod
+++ b/examples/go.mod
@@ -6,7 +6,7 @@ require (
 	github.com/coreos/go-oidc/v3 v3.11.0
 	github.com/dexidp/dex/api/v2 v2.2.0
 	github.com/spf13/cobra v1.8.1
-	golang.org/x/oauth2 v0.23.0
+	golang.org/x/oauth2 v0.24.0
 	google.golang.org/grpc v1.67.1
 )
 
diff --git a/examples/go.sum b/examples/go.sum
index 0bcdb008..a3240994 100644
--- a/examples/go.sum
+++ b/examples/go.sum
@@ -24,8 +24,8 @@ golang.org/x/crypto v0.26.0 h1:RrRspgV4mU+YwB4FYnuBoKsUapNIL5cohGAmSH3azsw=
 golang.org/x/crypto v0.26.0/go.mod h1:GY7jblb9wI+FOo5y8/S2oY4zWP07AkOJ4+jxCqdqn54=
 golang.org/x/net v0.28.0 h1:a9JDOJc5GMUJ0+UDqmLT86WiEy7iWyIhz8gz8E4e5hE=
 golang.org/x/net v0.28.0/go.mod h1:yqtgsTWOOnlGLG9GFRrK3++bGOUEkNBoHZc8MEDWPNg=
-golang.org/x/oauth2 v0.23.0 h1:PbgcYx2W7i4LvjJWEbf0ngHV6qJYr86PkAV3bXdLEbs=
-golang.org/x/oauth2 v0.23.0/go.mod h1:XYTD2NtWslqkgxebSiOHnXEap4TF09sJSc7H1sXbhtI=
+golang.org/x/oauth2 v0.24.0 h1:KTBBxWqUa0ykRPLtV69rRto9TLXcqYkeswu48x/gvNE=
+golang.org/x/oauth2 v0.24.0/go.mod h1:XYTD2NtWslqkgxebSiOHnXEap4TF09sJSc7H1sXbhtI=
 golang.org/x/sys v0.24.0 h1:Twjiwq9dn6R1fQcyiK+wQyHWfaz/BJB+YIpzU/Cv3Xg=
 golang.org/x/sys v0.24.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
 golang.org/x/text v0.17.0 h1:XtiM5bkSOt+ewxlOE/aE/AKEHibwj/6gvWMl9Rsh0Qc=
diff --git a/server/handlers.go b/server/handlers.go
index 6521bf6a..5954820c 100644
--- a/server/handlers.go
+++ b/server/handlers.go
@@ -534,23 +534,6 @@ func (s *Server) finalizeLogin(ctx context.Context, identity connector.Identity,
 		"connector_id", authReq.ConnectorID, "username", claims.Username,
 		"preferred_username", claims.PreferredUsername, "email", email, "groups", claims.Groups)
 
-	// we can skip the redirect to /approval and go ahead and send code if it's not required
-	if s.skipApproval && !authReq.ForceApprovalPrompt {
-		return "", true, nil
-	}
-
-	// an HMAC is used here to ensure that the request ID is unpredictable, ensuring that an attacker who intercepted the original
-	// flow would be unable to poll for the result at the /approval endpoint
-	h := hmac.New(sha256.New, authReq.HMACKey)
-	h.Write([]byte(authReq.ID))
-	mac := h.Sum(nil)
-
-	returnURL := path.Join(s.issuerURL.Path, "/approval") + "?req=" + authReq.ID + "&hmac=" + base64.RawURLEncoding.EncodeToString(mac)
-	_, ok := conn.(connector.RefreshConnector)
-	if !ok {
-		return returnURL, false, nil
-	}
-
 	offlineAccessRequested := false
 	for _, scope := range authReq.Scopes {
 		if scope == scopeOfflineAccess {
@@ -558,45 +541,55 @@ func (s *Server) finalizeLogin(ctx context.Context, identity connector.Identity,
 			break
 		}
 	}
-	if !offlineAccessRequested {
-		return returnURL, false, nil
-	}
+	_, canRefresh := conn.(connector.RefreshConnector)
 
-	// Try to retrieve an existing OfflineSession object for the corresponding user.
-	session, err := s.storage.GetOfflineSessions(identity.UserID, authReq.ConnectorID)
-	if err != nil {
-		if err != storage.ErrNotFound {
-			s.logger.ErrorContext(ctx, "failed to get offline session", "err", err)
-			return "", false, err
-		}
-		offlineSessions := storage.OfflineSessions{
-			UserID:        identity.UserID,
-			ConnID:        authReq.ConnectorID,
-			Refresh:       make(map[string]*storage.RefreshTokenRef),
-			ConnectorData: identity.ConnectorData,
-		}
+	if offlineAccessRequested && canRefresh {
+		// Try to retrieve an existing OfflineSession object for the corresponding user.
+		session, err := s.storage.GetOfflineSessions(identity.UserID, authReq.ConnectorID)
+		switch {
+		case err != nil && err == storage.ErrNotFound:
+			offlineSessions := storage.OfflineSessions{
+				UserID:        identity.UserID,
+				ConnID:        authReq.ConnectorID,
+				Refresh:       make(map[string]*storage.RefreshTokenRef),
+				ConnectorData: identity.ConnectorData,
+			}
 
-		// Create a new OfflineSession object for the user and add a reference object for
-		// the newly received refreshtoken.
-		if err := s.storage.CreateOfflineSessions(ctx, offlineSessions); err != nil {
-			s.logger.ErrorContext(ctx, "failed to create offline session", "err", err)
+			// Create a new OfflineSession object for the user and add a reference object for
+			// the newly received refreshtoken.
+			if err := s.storage.CreateOfflineSessions(ctx, offlineSessions); err != nil {
+				s.logger.ErrorContext(ctx, "failed to create offline session", "err", err)
+				return "", false, err
+			}
+		case err == nil:
+			// Update existing OfflineSession obj with new RefreshTokenRef.
+			if err := s.storage.UpdateOfflineSessions(session.UserID, session.ConnID, func(old storage.OfflineSessions) (storage.OfflineSessions, error) {
+				if len(identity.ConnectorData) > 0 {
+					old.ConnectorData = identity.ConnectorData
+				}
+				return old, nil
+			}); err != nil {
+				s.logger.ErrorContext(ctx, "failed to update offline session", "err", err)
+				return "", false, err
+			}
+		default:
+			s.logger.ErrorContext(ctx, "failed to get offline session", "err", err)
 			return "", false, err
 		}
-
-		return returnURL, false, nil
 	}
 
-	// Update existing OfflineSession obj with new RefreshTokenRef.
-	if err := s.storage.UpdateOfflineSessions(session.UserID, session.ConnID, func(old storage.OfflineSessions) (storage.OfflineSessions, error) {
-		if len(identity.ConnectorData) > 0 {
-			old.ConnectorData = identity.ConnectorData
-		}
-		return old, nil
-	}); err != nil {
-		s.logger.ErrorContext(ctx, "failed to update offline session", "err", err)
-		return "", false, err
+	// we can skip the redirect to /approval and go ahead and send code if it's not required
+	if s.skipApproval && !authReq.ForceApprovalPrompt {
+		return "", true, nil
 	}
 
+	// an HMAC is used here to ensure that the request ID is unpredictable, ensuring that an attacker who intercepted the original
+	// flow would be unable to poll for the result at the /approval endpoint
+	h := hmac.New(sha256.New, authReq.HMACKey)
+	h.Write([]byte(authReq.ID))
+	mac := h.Sum(nil)
+
+	returnURL := path.Join(s.issuerURL.Path, "/approval") + "?req=" + authReq.ID + "&hmac=" + base64.RawURLEncoding.EncodeToString(mac)
 	return returnURL, false, nil
 }
 


Test patch:
diff --git a/server/handlers_test.go b/server/handlers_test.go
index d32101b1..08b02f75 100644
--- a/server/handlers_test.go
+++ b/server/handlers_test.go
@@ -519,7 +519,7 @@ func TestHandlePasswordLoginWithSkipApproval(t *testing.T) {
 				Scopes:              []string{"offline_access"},
 			},
 			expectedRes:           "/auth/mockPw/cb",
-			offlineSessionCreated: false,
+			offlineSessionCreated: true,
 		},
 	}