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

improvement (IAuthentication): extension of IAuthentication #722

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

zyAmo
Copy link
Contributor

@zyAmo zyAmo commented Jul 26, 2024

original description at #702

This PR extends the existing IAuthentication interface's Callback() function to include the *http.Request parameter.

This enhancement allows plugins implementing this interface to access the HTTP request directly, and thereby enabling them to retrieve additional information, such as cookies, via req.Cookie("my-cookie"). Additionally, it allows for request parsing to be handled by the plugin itself if desired.
Processing cookies during the Callback() can be highly beneficial for various login flows.

The PR is structured into two commits:

  1. first commit
    • extends the interface and updates existing plugins accordingly

Since this addition only involves adding a new parameter to the function signature, which is currently ignored in all existing plugins, it can be considered a minor change with essentially no impact on existing authentication plugins, although their signatures have to be updated, as I have done in this PR.

  1. second commit:
    • removes formData from the function signature to avoid redundancy in passing both the request and the parsed body
    • moves the existing parsing code to a utility function, which can be called from within the plugin, ensuring full compatibility with existing plugins

This introduces a minor change in error handling: Parsing errors from req.ParseForm() are now wrapped as NewError(err.Error(), 400) and handled by the error handling in session.go after the call to IAuthentication.Callback(), resulting in a slightly changed redirect behavior:

  • before: redirect to /?error=Not%20Valid&trace=parsing%20body%20-<error message>
  • after: redirect to /?error=Not%20Allowed&trace=redirect%20request%20failed%20-<error message>

If the new redirect behavior is undesirable, it can be adjusted to handle the error differently, although currently, all non-ErrAuthenticationFailed errors inside the Callback() are treated this way. Alternatively, the second commit can be left out, ignoring the redundancy in favor of simplicity. I'm happy for feedback.


Thank you for considering this change to improve the IAuthentication interface :)

currently waiting for frontend rewrite

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant