Skip to content

Conversation

@m11o
Copy link
Contributor

@m11o m11o commented Mar 22, 2025

Summary of Changes

Improved authentication request handling by extracting the CallbackURL generation logic. This allows for direct redirection to the CallbackURL for prompt=none requests.

Key Changes

  • Separated the CallbackURL generation logic from the AuthResponseCode function
  • Added new functions: BuildAuthResponseCodeResponsePayload and BuildAuthResponseCallbackURL
  • Clearly separated form post response and redirect response handling
  • Added tests

Motivation

This change addresses the need to optimize authentication requests with prompt=none. Currently, the Authorize method in the Server interface can only return a Redirect struct and doesn't have direct access to HTTP request/response objects.

To streamline the authentication flow, we need to generate a callback URL directly and redirect to it when prompt=none is specified. However, in the current implementation, the authorization code generation and callback URL creation process depends on http.ResponseWriter and http.Request, making it difficult to reuse for this purpose.

This change extracts key logic into reusable functions, enabling more efficient handling of special cases like prompt=none in future implementations.

Usage

We expect to use these functions when handling prompt=none, as shown below.

func (c Controller) Authorize(ctx context.Context, r *op.ClientRequest[oidc.AuthRequest]) (*op.Redirect, error) {
    // do something

    if authReq.IsPromptNone() {
        if currentAccount != nil {
            // process completed authReq ...

            callbackURL, err := BuildAuthResponseCallbackURL(ctx, authReq, c.authorizer)
            if err != nil {
                return op.TryErrorRedirect(ctx, authReq, oidc.DefaultToServerError(err, "unable to build callback URL"), c.authorizer.encoder, c.Logger())
            }

            return op.NewRedirect(callbackURL), nil
        } else {
            return op.TryErrorRedirect(ctx, authReq, oidc.ErrLoginRequired(), c.authorizer.encoder, c.Logger())
        }
    }
}

Impact

This change improves internal implementation without affecting the public API. All existing behavior is preserved.

Definition of Ready

  • I am happy with the code
  • Short description of the feature/issue is added in the pr description
  • PR is linked to the corresponding user story
  • Acceptance criteria are met
  • All open todos and follow ups are defined in a new ticket and justified
  • Deviations from the acceptance criteria and design are agreed with the PO and documented.
  • No debug or dead code
  • My code has no repetitions
  • Critical parts are tested automatically
  • Where possible E2E tests are implemented
  • Documentation/examples are up-to-date
  • All non-functional requirements are met
  • Functionality of the acceptance criteria is checked manually on the dev system.

- Introduced CodeResponseType struct to encapsulate response data.
- Added handleFormPostResponse and handleRedirectResponse functions to manage different response modes.
- Created BuildAuthResponseCodeResponsePayload and BuildAuthResponseCallbackURL functions for better modularity in response generation.
@m11o m11o changed the title proposal(op): enhance authentication response handling proposal(op): enhance the handling of authentication response for prompt=none Mar 23, 2025
@muhlemmer muhlemmer moved this to 📝 Prioritized Product Backlog in Product Management Mar 24, 2025
@muhlemmer muhlemmer moved this from 📝 Prioritized Product Backlog to 📋 Sprint Backlog in Product Management Mar 24, 2025
@muhlemmer
Copy link
Collaborator

Thanks for the PR. I was a bit busy, but I'll have a look in the coming days.

@muhlemmer muhlemmer requested review from Copilot and muhlemmer March 27, 2025 13:37
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request refactors the handling of authentication responses for prompt=none requests by extracting code generation and URL building logic into dedicated functions, allowing for direct redirection without direct dependency on HTTP request/response objects. Key changes include:

  • Extraction of CallbackURL generation into the BuildAuthResponseCallbackURL function.
  • Addition of BuildAuthResponseCodeResponsePayload function to generate the authorization code payload.
  • Separation of response handling into form post and redirect methods.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
pkg/op/auth_request_test.go Adds tests for the new BuildAuthResponseCodeResponsePayload and BuildAuthResponseCallbackURL functions.
pkg/op/auth_request.go Refactors AuthResponseCode by extracting response logic into separate helper functions.
Comments suppressed due to low confidence (1)

pkg/op/auth_request.go:487

  • The error returned by handleFormPostResponse or handleRedirectResponse is not handled in AuthResponseCode. Consider checking 'err' after these calls and using AuthRequestError to handle errors appropriately.
func AuthResponseCode(w http.ResponseWriter, r *http.Request, authReq AuthRequest, authorizer Authorizer) {

@muhlemmer muhlemmer enabled auto-merge (squash) April 29, 2025 14:11
@muhlemmer muhlemmer merged commit 5913c5a into zitadel:main Apr 29, 2025
4 checks passed
@github-project-automation github-project-automation bot moved this from 📋 Sprint Backlog to ✅ Done in Product Management Apr 29, 2025
@github-actions
Copy link

🎉 This PR is included in version 3.38.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@m11o m11o deleted the add-prompt-none-interface branch April 29, 2025 23:48
EthanHeilman pushed a commit to openpubkey/oidc that referenced this pull request May 11, 2025
- Introduced CodeResponseType struct to encapsulate response data.
- Added handleFormPostResponse and handleRedirectResponse functions to manage different response modes.
- Created BuildAuthResponseCodeResponsePayload and BuildAuthResponseCallbackURL functions for better modularity in response generation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants