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

Use secure token in path instead of reg. identifier #579

Merged
merged 36 commits into from
Dec 10, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
9f45d63
To you!
cintamani Dec 5, 2019
595c35d
Switch to using new SecureTokenService
Cruikshanks Dec 5, 2019
ba5395f
Take away changes related to order copy cards refactoring
cintamani Dec 5, 2019
74ff6ea
requests/../declaration_forms_spec passing
Cruikshanks Dec 6, 2019
0029ae9
request/../conviction_details_forms_spec.rb passing
Cruikshanks Dec 6, 2019
4a6714d
requests/../main_people_forms_spec passing
Cruikshanks Dec 6, 2019
e6dc31b
requests/../register_in_wales_forms_spec passing
Cruikshanks Dec 6, 2019
b43383c
requests/../registration_number_forms_spec passing
Cruikshanks Dec 6, 2019
9cfd18d
Refactor copy_cards_forms_spec
cintamani Dec 6, 2019
9f088bf
requests/../tier_check_forms_spec passing
Cruikshanks Dec 6, 2019
bea9e13
Fix copy cards payment forms spec
cintamani Dec 6, 2019
660361a
requests/../renewal_complete_forms_spec passing
Cruikshanks Dec 6, 2019
49b3ef6
WIP renewal_start_forms_spec
Cruikshanks Dec 6, 2019
f570b9f
requests/../worldpay_forms_spec passing
Cruikshanks Dec 9, 2019
e7f4e22
Merging in latest
Cruikshanks Dec 9, 2019
feb775f
requests/../renewal_start_forms_spec passing
Cruikshanks Dec 9, 2019
eb4232a
forms/../service_provided_forms_spec
Cruikshanks Dec 9, 2019
983edfe
forms/../construction_demolition_forms_spec passing
Cruikshanks Dec 9, 2019
dcb488a
forms/../other_businesses_forms_spec passing
Cruikshanks Dec 9, 2019
b18dc80
forms/../waste_types_forms_spec
Cruikshanks Dec 9, 2019
b667afc
forms/../location_forms_spec passing
Cruikshanks Dec 9, 2019
6b70903
forms/../tier_check_forms_spec passing
Cruikshanks Dec 9, 2019
e4761dd
forms/../contact_name_forms_spec passing
Cruikshanks Dec 9, 2019
e6c7208
forms/../company_name_forms_spec passing
Cruikshanks Dec 9, 2019
7b6066f
forms/../payment_summary_forms_spec passing
Cruikshanks Dec 9, 2019
6a1d5b3
services/../worldpay_url_service_spec passing
Cruikshanks Dec 9, 2019
cd5d9d1
Resolve basic rubocop errors
Cruikshanks Dec 9, 2019
8f08bab
Add rubocop exception
Cruikshanks Dec 9, 2019
9b9395a
Use token instead of reg ID in back links
Cruikshanks Dec 9, 2019
2c72734
Remove hidden field from views
Cruikshanks Dec 9, 2019
69dd806
Remove params from people delete forms
Cruikshanks Dec 9, 2019
92c1f34
Add missing <% end %> to view
Cruikshanks Dec 9, 2019
7c80be7
Rename param in renew start form
Cruikshanks Dec 10, 2019
c524b04
Fix copy_cards_payment_forms_spec
cintamani Dec 10, 2019
5b2e131
Remove <% end %> tag
Cruikshanks Dec 10, 2019
b7cdb15
Merge branch 'master' into secure-token
cintamani Dec 10, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
module WasteCarriersEngine
class AddressFormsController < FormsController
def skip_to_manual_address
find_or_initialize_transient_registration(params[:reg_identifier])
find_or_initialize_transient_registration(params[:token])

@transient_registration.skip_to_manual_address! if form_matches_state?
redirect_to_correct_form
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,5 @@ def create
def set_up_finance_details
@transient_registration.prepare_for_payment(:bank_transfer, current_user)
end

def transient_registration_attributes
# TODO: Remvoe when default empty params
# Nothing to submit
params.fetch(:bank_transfer_form).permit
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ def create
private

def transient_registration_attributes
params.fetch(:business_type_form, {}).permit(:business_type, :reg_identifier)
params.fetch(:business_type_form, {}).permit(:business_type, :token)
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@ def transient_registration_attributes
params.fetch(:copy_cards_form).permit(:temp_cards)
end

def find_or_initialize_transient_registration(reg_identifier)
@transient_registration = OrderCopyCardsRegistration.where(reg_identifier: reg_identifier).first ||
OrderCopyCardsRegistration.new(reg_identifier: reg_identifier)
def find_or_initialize_transient_registration(token)
@transient_registration = OrderCopyCardsRegistration.where(token: token).first ||
OrderCopyCardsRegistration.new(reg_identifier: token)
end
end
end
33 changes: 21 additions & 12 deletions app/controllers/waste_carriers_engine/forms_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,21 +6,22 @@ class FormsController < ApplicationController

before_action :authenticate_user!
before_action :back_button_cache_buster
before_action :validate_token

# Expects a form class name (eg BusinessTypeForm) and a snake_case name for the form (eg business_type_form)
def new(form_class, form)
set_up_form(form_class, form, params[:reg_identifier], true)
set_up_form(form_class, form, params[:token], true)
end

# Expects a form class name (eg BusinessTypeForm) and a snake_case name for the form (eg business_type_form)
def create(form_class, form)
return false unless set_up_form(form_class, form, params[form][:reg_identifier])
return false unless set_up_form(form_class, form, params[:token])

submit_form(instance_variable_get("@#{form}"), transient_registration_attributes)
end

def go_back
find_or_initialize_transient_registration(params[:reg_identifier])
find_or_initialize_transient_registration(params[:token])

@transient_registration.back! if form_matches_state?
redirect_to_correct_form
Expand All @@ -34,15 +35,23 @@ def transient_registration_attributes
params.permit
end

def find_or_initialize_transient_registration(reg_identifier)
@transient_registration = TransientRegistration.where(reg_identifier: reg_identifier).first ||
RenewingRegistration.new(reg_identifier: reg_identifier)
def validate_token
return redirect_to(page_path("invalid")) unless find_or_initialize_transient_registration(params[:token])
end

# We're not really memoizing this instance variable here, so we don't think
# this cop is valid in this context
# rubocop:disable Naming/MemoizedInstanceVariableName
def find_or_initialize_transient_registration(token)
@transient_registration ||= TransientRegistration.where(token: token).first
end
# rubocop:enable Naming/MemoizedInstanceVariableName

# Expects a form class name (eg BusinessTypeForm), a snake_case name for the form (eg business_type_form),
# and the reg_identifier param
def set_up_form(form_class, form, reg_identifier, get_request = false)
find_or_initialize_transient_registration(reg_identifier)
# and the token param
def set_up_form(form_class, form, token, get_request = false)
find_or_initialize_transient_registration(token)

set_workflow_state if get_request

return false unless setup_checks_pass?
Expand All @@ -68,10 +77,10 @@ def redirect_to_correct_form
redirect_to form_path
end

# Get the path based on the workflow state, with reg_identifier as params, ie:
# new_state_name_path/:reg_identifier
# Get the path based on the workflow state, with token as params, ie:
# new_state_name_path/:token
def form_path
send("new_#{@transient_registration.workflow_state}_path".to_sym, @transient_registration.reg_identifier)
send("new_#{@transient_registration.workflow_state}_path".to_sym, @transient_registration.token)
end

def setup_checks_pass?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ def create(form_class, form)
end

def submit_and_add_another(form_class, form)
return unless set_up_form(form_class, form, params[form][:reg_identifier])
return unless set_up_form(form_class, form, params[:token])

form_instance_variable = instance_variable_get("@#{form}")

Expand All @@ -25,7 +25,7 @@ def submit_and_add_another(form_class, form)
end

def delete_person(form_class, form)
return unless set_up_form(form_class, form, params[:reg_identifier])
return unless set_up_form(form_class, form, params[:token])

respond_to do |format|
# Check if there are any matches first, to avoid a Mongoid error
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
module WasteCarriersEngine
class PostcodeFormsController < FormsController
def skip_to_manual_address
find_or_initialize_transient_registration(params[:reg_identifier])
find_or_initialize_transient_registration(params[:token])

@transient_registration.skip_to_manual_address! if form_matches_state?
redirect_to_correct_form
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,12 @@ def new
def create
super(RenewalStartForm, "renewal_start_form")
end

private

def find_or_initialize_transient_registration(token)
@transient_registration = RenewingRegistration.where(token: token).first ||
RenewingRegistration.new(reg_identifier: token)
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ def create
private

def transient_registration_attributes
params.require(:tier_check_form).permit(:temp_tier_check)
params.fetch(:tier_check_form, {}).permit(:temp_tier_check)
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ def prepare_for_payment
end

def respond_to_acceptable_payment(action)
return unless valid_transient_registration?(params[:reg_identifier])
return unless valid_transient_registration?(params[:token])

if response_is_valid?(action, params)
log_and_send_worldpay_response(true, action)
Expand All @@ -60,7 +60,7 @@ def respond_to_acceptable_payment(action)
end

def respond_to_unsuccessful_payment(action)
return unless valid_transient_registration?(params[:reg_identifier])
return unless valid_transient_registration?(params[:token])

if response_is_valid?(action, params)
log_and_send_worldpay_response(true, action)
Expand All @@ -73,8 +73,8 @@ def respond_to_unsuccessful_payment(action)
go_back
end

def valid_transient_registration?(reg_identifier)
find_or_initialize_transient_registration(reg_identifier)
def valid_transient_registration?(token)
find_or_initialize_transient_registration(token)
setup_checks_pass?
end

Expand Down
12 changes: 10 additions & 2 deletions app/forms/waste_carriers_engine/base_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,17 @@ class BaseForm

attr_reader :transient_registration

delegate :reg_identifier, to: :transient_registration
delegate :token, :reg_identifier, to: :transient_registration

# If the record is new, and not yet persisted (which it is when the start
# page is first submitted) then we have nothing to validate hence the check
validates :token, presence: true, if: -> { transient_registration&.persisted? }
validates(
:reg_identifier,
"waste_carriers_engine/reg_identifier": true,
if: -> { transient_registration&.persisted? }
)

validates :reg_identifier, "waste_carriers_engine/reg_identifier": true
validate :transient_registration_valid?

define_model_callbacks :initialize
Expand Down
2 changes: 1 addition & 1 deletion app/forms/waste_carriers_engine/renewal_complete_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ def initialize(transient_registration)
private

def build_certificate_link
registration = Registration.where(reg_identifier: reg_identifier).first
registration = @transient_registration.registration
return unless registration.present?

id = registration.id
Expand Down
6 changes: 6 additions & 0 deletions app/forms/waste_carriers_engine/renewal_start_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,11 @@ def submit(_params)

super(attributes)
end

def find_or_initialize_transient_registration(token)
# TODO: Downtime at deploy when releasing token?
@transient_registration = RenewingRegistration.where(token: token).first ||
RenewingRegistration.new(reg_identifier: token)
end
end
end
18 changes: 18 additions & 0 deletions app/models/concerns/waste_carriers_engine/can_have_secure_token.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# frozen_string_literal: true

module WasteCarriersEngine
module CanHaveSecureToken
extend ActiveSupport::Concern
include Mongoid::Document

included do
field :token, type: String

before_create :generate_unique_secure_token

def generate_unique_secure_token
self.token = SecureTokenService.run
end
end
end
end
1 change: 1 addition & 0 deletions app/models/waste_carriers_engine/transient_registration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ class TransientRegistration
include CanCheckRegistrationStatus
include CanFilterConvictionStatus
include CanHaveRegistrationAttributes
include CanHaveSecureToken
include CanStripWhitespace

store_in collection: "transient_registrations"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ def send_confirmation_email
def copy_data_from_transient_registration
registration_attributes = @registration.attributes.except("_id", "financeDetails", "past_registrations")
renewal_attributes = @transient_registration.attributes.except("_id",
"token",
"financeDetails",
"temp_cards",
"temp_company_postcode",
Expand Down
2 changes: 1 addition & 1 deletion app/services/waste_carriers_engine/worldpay_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ def prepare_for_payment
reference = parse_response(response)

if reference.present?
worldpay_url_service = WorldpayUrlService.new(@transient_registration.reg_identifier, reference[:link])
worldpay_url_service = WorldpayUrlService.new(@transient_registration.token, reference[:link])
url = worldpay_url_service.format_link

{
Expand Down
6 changes: 3 additions & 3 deletions app/services/waste_carriers_engine/worldpay_url_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ module WasteCarriersEngine
# Builds the URL we use to redirect users to Worldpay.
# This should include URLs for our own service, which Worldpay will redirect users to after payment.
class WorldpayUrlService
def initialize(reg_identifier, base_url)
@reg_identifier = reg_identifier
def initialize(token, base_url)
@token = token
@base_url = base_url
end

Expand All @@ -29,7 +29,7 @@ def redirect_url_for(action)
def build_path_for(action)
path = "#{action}_worldpay_forms_path"
url = [Rails.configuration.host,
WasteCarriersEngine::Engine.routes.url_helpers.public_send(path, @reg_identifier)]
WasteCarriersEngine::Engine.routes.url_helpers.public_send(path, token: @token)]
CGI.escape(url.join)
end
end
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<%= render("waste_carriers_engine/shared/back", back_path: back_bank_transfer_forms_path(@bank_transfer_form.reg_identifier)) %>
<%= render("waste_carriers_engine/shared/back", back_path: back_bank_transfer_forms_path(@bank_transfer_form.token)) %>

<div class="text">
<%= form_for(@bank_transfer_form) do |f| %>
Expand Down Expand Up @@ -105,7 +105,6 @@
</strong>
</div>

<%= f.hidden_field :reg_identifier, value: @bank_transfer_form.reg_identifier %>
<div class="form-group">
<%= f.submit t(".next_button"), class: "button" %>
</div>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<%= render("waste_carriers_engine/shared/back", back_path: back_business_type_forms_path(@business_type_form.reg_identifier)) %>
<%= render("waste_carriers_engine/shared/back", back_path: back_business_type_forms_path(@business_type_form.token)) %>

<div class="text">
<%= form_for(@business_type_form) do |f| %>
Expand Down Expand Up @@ -45,7 +45,6 @@

<p><%= t(".help") %></p>

<%= f.hidden_field :reg_identifier, value: @business_type_form.reg_identifier %>
<div class="form-group">
<%= f.submit t(".next_button"), class: "button" %>
</div>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<%= render("waste_carriers_engine/shared/back", back_path: back_cannot_renew_company_no_change_forms_path(@cannot_renew_company_no_change_form.reg_identifier)) %>
<%= render("waste_carriers_engine/shared/back", back_path: back_cannot_renew_company_no_change_forms_path(@cannot_renew_company_no_change_form.token)) %>

<div class="text">
<h1 class="heading-large"><%= t(".heading", reg_identifier: @cannot_renew_company_no_change_form.reg_identifier) %></h1>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<%= render("waste_carriers_engine/shared/back", back_path: back_cannot_renew_lower_tier_forms_path(@cannot_renew_lower_tier_form.reg_identifier)) %>
<%= render("waste_carriers_engine/shared/back", back_path: back_cannot_renew_lower_tier_forms_path(@cannot_renew_lower_tier_form.token)) %>

<div class="text">
<h1 class="heading-large"><%= t(".heading", reg_identifier: @cannot_renew_lower_tier_form.reg_identifier) %></h1>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<%= render("waste_carriers_engine/shared/back", back_path: back_cannot_renew_type_change_forms_path(@cannot_renew_type_change_form.reg_identifier)) %>
<%= render("waste_carriers_engine/shared/back", back_path: back_cannot_renew_type_change_forms_path(@cannot_renew_type_change_form.token)) %>

<div class="text">
<h1 class="heading-large"><%= t(".heading", reg_identifier: @cannot_renew_type_change_form.reg_identifier) %></h1>
Expand Down
4 changes: 1 addition & 3 deletions app/views/waste_carriers_engine/cards_forms/new.html.erb
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<%= render("waste_carriers_engine/shared/back", back_path: back_cards_forms_path(@cards_form.reg_identifier)) %>
<%= render("waste_carriers_engine/shared/back", back_path: back_cards_forms_path(@cards_form.token)) %>

<div class="text">
<%= form_for(@cards_form) do |f| %>
Expand Down Expand Up @@ -30,8 +30,6 @@
</fieldset>
</div>

<%= f.hidden_field :reg_identifier, value: @cards_form.reg_identifier %>

<div class="form-group">
<%= f.submit t(".next_button"), class: "button" %>
</div>
Expand Down
3 changes: 1 addition & 2 deletions app/views/waste_carriers_engine/cbd_type_forms/new.html.erb
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<%= render("waste_carriers_engine/shared/back", back_path: back_cbd_type_forms_path(@cbd_type_form.reg_identifier)) %>
<%= render("waste_carriers_engine/shared/back", back_path: back_cbd_type_forms_path(@cbd_type_form.token)) %>

<div class="text">
<%= form_for(@cbd_type_form) do |f| %>
Expand Down Expand Up @@ -49,7 +49,6 @@
</details>
</div>

<%= f.hidden_field :reg_identifier, value: @cbd_type_form.reg_identifier %>
<div class="form-group">
<%= f.submit t(".next_button"), class: "button" %>
</div>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<%= render("waste_carriers_engine/shared/back", back_path: back_check_your_answers_forms_path(@check_your_answers_form.reg_identifier)) %>
<%= render("waste_carriers_engine/shared/back", back_path: back_check_your_answers_forms_path(@check_your_answers_form.token)) %>

<div class="text">
<% if @check_your_answers_form.errors.any? %>
Expand Down Expand Up @@ -114,7 +114,6 @@

<hr>

<%= f.hidden_field :reg_identifier, value: @check_your_answers_form.reg_identifier %>
<div class="form-group">
<%= f.submit t(".next_button"), class: "button" %>
</div>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<%= render("waste_carriers_engine/shared/back", back_path: back_company_address_forms_path(@company_address_form.reg_identifier)) %>
<%= render("waste_carriers_engine/shared/back", back_path: back_company_address_forms_path(@company_address_form.token)) %>

<div class="text">
<%= form_for(@company_address_form) do |f| %>
Expand All @@ -20,7 +20,6 @@
<%= link_to(t(".manual_address_link"), skip_to_manual_address_company_address_forms_path(@company_address_form.reg_identifier)) %>
</div>

<%= f.hidden_field :reg_identifier, value: @company_address_form.reg_identifier %>
<div class="form-group">
<%= f.submit t(".next_button"), class: "button" %>
</div>
Expand Down