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

Add after_session_confirm hook after session confirmation #237

Merged
merged 7 commits into from
Oct 7, 2024

Conversation

wilburhimself
Copy link
Contributor

@wilburhimself wilburhimself commented Oct 7, 2024

fixes #213

This PR introduces a new after_session_confirm hook to the Passwordless gem. This feature allows users to perform custom actions after a session has been successfully confirmed, such as marking an email as verified or updating user attributes after their first successful login.

Changes

  • Added after_session_confirm configuration option
  • Updated SessionsController to call the hook after successful authentication
  • Added new tests in SessionsControllerTest to cover the new functionality
  • Updated README with documentation for the new feature

Motivation

Users of the gem often need to perform specific actions after a successful login, such as updating user attributes or triggering other processes. The after_session_confirm hook provides a clean, configurable way to achieve this without modifying the gem's core code.

Implementation Details

  • The hook is called in the authenticate_and_sign_in method of SessionsController, right after successful authentication but before redirecting the user.
  • The hook can be configured to accept either one argument (the session) or two arguments (the session and the request), providing flexibility for different use cases.
  • If the hook is not defined or doesn't respond to call, it is silently ignored to maintain backwards compatibility.

How to Use

Users can configure the hook in their Passwordless configuration:

Passwordless.configure do |config|
  config.after_session_confirm = ->(session) {
    user = session.authenticatable
    user.update(email_verified: true)
  }
end

Testing

New tests have been added to SessionsControllerTest to ensure the hook is called correctly in various scenarios:

  • Successful authentication
  • Hook receiving both session and request objects
  • Hook not being called on failed authentication

All existing tests pass with these new changes.

Reviewer Notes

  • Please pay special attention to the placement of the hook call in SessionsController to ensure it's in the most appropriate location.
  • Consider if any additional error handling is needed around the hook execution.
  • Let me know if you think any additional tests or documentation would be beneficial.

Checklist

  • Code follows the style guidelines of the project
  • New tests added for the feature
  • All tests pass
  • Documentation (README) has been updated

Copy link
Owner

@mikker mikker left a comment

Choose a reason for hiding this comment

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

This is super great 👍
Thank you for such a thorough explanation. Few thoughts but happy to include this

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
app/controllers/passwordless/sessions_controller.rb Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
lib/passwordless/config.rb Outdated Show resolved Hide resolved
@mikker
Copy link
Owner

mikker commented Oct 7, 2024

Failing test is just rails#main

@mikker mikker merged commit d4df0b3 into mikker:master Oct 7, 2024
14 of 15 checks passed
@mikker
Copy link
Owner

mikker commented Oct 7, 2024

Great work! ❤️🧡💛💚💙💜

@wilburhimself wilburhimself deleted the add_after_session_confirm_hook branch October 7, 2024 21:07
@wkirby
Copy link

wkirby commented Oct 24, 2024

@mikker when can we expect to see this released?

@mikker
Copy link
Owner

mikker commented Oct 25, 2024

Just pushed 1.8.0 with this change 😊

@henrikbjorn
Copy link
Contributor

Am I correct in assuming the controller context is lot in the hook ?

A use case would be for Ahoy to call ahoy.authenticate for analytics. But it seems that can't be done since the hook is "just" a function.

Would it make sense to instead define the hook methods on the controller (kind of like Devise)? Then the Controller context is kept, and it is also easy to set a custom controller to use.

@henrikbjorn
Copy link
Contributor

Another note. It is not possible to have two different hooks. E.g one for AdminUsers and one for normal Users. Which having it as a controller method would handle.

@mikker
Copy link
Owner

mikker commented Oct 25, 2024

Good points Henrik!

It could be useful to have the controller context at hand.

Maybe something like if setting the hook setting to a :symbol, a method with that name will be called instead?

Like

class ApplicationController < ...
  private

  def pwless_after_session_confirm(session) # no need to pass request either
    # ...
  end
end

Passwordless.configure do |config|
  config.after_session_confirm = :pwless_after_session_confirm
end

That would make it a non-breaking change.

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.

Add after_session_confirm hook after session confirmation
4 participants