Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal(op): enhance the handling of authentication response for prompt=none #728

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

m11o
Copy link

@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 muhlemmer and Copilot March 27, 2025 13:37
Copy link

@Copilot 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) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 📋 Sprint Backlog
Development

Successfully merging this pull request may close these issues.

3 participants