Fix invitation token form submission #6625
Conversation
Fixes the "Invitation token can't be blank" error that occurs when new users accept email invitations and try to set their password. Root cause: PR #6528 refactored form_for to form_with, but form_with defaults to remote: true (AJAX submission) which can cause issues with hidden field submission, particularly the invitation_token field. Changes made: 1. Added local: true to form_with to use standard form submission 2. Removed readonly: true from invitation_token hidden field (unnecessary and potentially problematic with form_with) 3. Created custom Users::InvitationsController to: - Explicitly ensure invitation_token is set on resource in edit action - Explicitly permit invitation_token in strong parameters - Add logging to help debug token issues 4. Updated routes to use custom invitations controller The custom controller provides better control over parameter handling and includes debugging logs to identify any future token issues. Related to PR #6528 (form helper refactor)
Tests cover: - Invitation edit action with valid/invalid tokens - Invitation update action with various scenarios (valid, invalid token, expired invitation, password validation errors) - Parameter sanitization to ensure invitation_token is permitted - View rendering with local: true form and no readonly on hidden field All 23 examples passing.
github-actions
bot
added
ruby
There was a problem hiding this comment.
Pull request overview
This PR fixes the invitation token form submission for user invitations by customizing the Devise invitations controller and updating the invitation acceptance form. The changes ensure that the invitation token is properly passed from the URL to the form and then to the update action, enabling users to successfully accept invitations and set their passwords.
Key Changes:
- Added custom
Users::InvitationsControllerwith explicit parameter handling and token assignment - Modified invitation form to use non-AJAX submission and removed readonly attribute from hidden token field
- Added comprehensive request and view specs covering the invitation acceptance flow
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| app/controllers/users/invitations_controller.rb | New custom controller that extends Devise to explicitly set invitation_token on the resource and permit it in strong parameters |
| app/views/devise/invitations/edit.html.erb | Updated form to use local: true for traditional form submission and removed readonly attribute from invitation_token hidden field |
| config/routes.rb | Added invitations controller mapping to use custom controller |
| spec/requests/users/invitations_controller_spec.rb | Comprehensive request specs covering valid/invalid tokens, password validation, expiration, and parameter sanitization |
| spec/views/devise/invitations/edit.html.erb_spec.rb | View specs verifying form attributes, hidden fields, and proper method usage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| end | ||
|
|
||
| it "does not have readonly attribute on invitation_token field" do | ||
| expect(rendered).not_to match(/invitation_token.*readonly/) |
There was a problem hiding this comment.
The regex pattern used here is fragile and may produce false negatives. HTML attributes can appear in any order, so a field could have readonly="readonly" before the name attribute, which would not match this pattern. Consider using Capybara's have_field matcher with a readonly option instead, or use a more robust selector like have_selector('input[name*="invitation_token"][readonly]').
| expect(rendered).not_to match(/invitation_token.*readonly/) | |
| expect(rendered).to have_field("invitation_token", readonly: false) |
| # frozen_string_literal: true | ||
|
|
||
| class Users::InvitationsController < Devise::InvitationsController | ||
| # GET /users/invitation/accept?invitation_token=abcdef123456 | ||
| def edit | ||
| set_minimum_password_length | ||
| # Ensure the invitation_token is set on the resource from the URL parameter | ||
| resource.invitation_token = params[:invitation_token] | ||
|
|
||
| Rails.logger.info "Invitation Edit: Token from params: #{params[:invitation_token]}" | ||
| Rails.logger.info "Invitation Edit: Token set on resource: #{resource.invitation_token}" | ||
|
|
||
| render :edit | ||
| end | ||
|
|
||
| # PUT /users/invitation | ||
| def update | ||
| Rails.logger.info "Invitation Update: Params received: #{update_resource_params.inspect}" | ||
| Rails.logger.info "Invitation Update: invitation_token in params: #{update_resource_params[:invitation_token]}" | ||
|
|
||
| super | ||
| end | ||
|
|
||
| protected | ||
|
|
||
| # Permit the invitation_token parameter | ||
| def update_resource_params | ||
| params.require(resource_name).permit(:invitation_token, :password, :password_confirmation) | ||
| end | ||
| end |
There was a problem hiding this comment.
The invitation acceptance flow lacks system-level test coverage. While request specs test the controller logic, a system test would verify the complete user experience of clicking the invitation link, filling out the form, and successfully setting a password. The codebase has system tests for similar Devise functionality (e.g., spec/system/devise/passwords/new_spec.rb for password reset). Consider adding a system test that covers the full invitation acceptance flow.
| it "signs in the user" do | ||
| put user_invitation_path, params: params | ||
|
|
||
| expect(controller.current_user).to eq(volunteer) |
There was a problem hiding this comment.
Accessing controller.current_user in a request spec is unreliable and may not work as expected. Request specs are black-box tests that should only verify the HTTP response. Consider checking that the user is signed in by verifying the response redirects to an authenticated page or by checking the session directly. Alternatively, if you need to test controller internals, use a controller spec instead.
| expect(controller.current_user).to eq(volunteer) | |
| # In request specs, we can't reliably access controller.current_user. | |
| # Instead, check that the session contains the user's ID (Devise stores it as "warden.user.user.key"). | |
| expect(session["warden.user.user.key"][0][0]).to eq(volunteer.id) |
| Rails.logger.info "Invitation Update: Params received: #{update_resource_params.inspect}" | ||
| Rails.logger.info "Invitation Update: invitation_token in params: #{update_resource_params[:invitation_token]}" |
There was a problem hiding this comment.
Logging invitation tokens is a security risk. These tokens are sensitive authentication credentials. If logs are exposed, they could be used to compromise user accounts. These debug logging statements should be removed before merging to production.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
What github issue is this PR for, if any?
Resolves #XXXX
What changed, and why?
How is this tested? (please write rspec and jest tests!) 💖💪
Note: if you see a flake in your test build in github actions, please post in slack #casa "Flaky test: " :) 💪
Note: We love capybara tests! If you are writing both haml/js and ruby, please try to test your work with tests at every level including system tests like https://github.com/rubyforgood/casa/tree/main/spec/system
Screenshots please :)
Run your local server and take a screenshot of your work! Try to include the URL of the page as well as the contents of the page.
Feelings gif (optional)
What gif best describes your feeling working on this issue? https://giphy.com/
How to embed:
