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

Restrict ability on clients with archived intakes on hub client related actions #5491

Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 2 additions & 15 deletions app/controllers/hub/clients_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,7 @@ class ClientsController < Hub::BaseController
before_action :redirect_unless_client_is_hub_status_editable, only: [:edit, :edit_take_action, :update, :update_take_action]
layout "hub"

MAX_COUNT = 1000
Copy link
Contributor Author

Choose a reason for hiding this comment

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

used to be used for pagination, now not getting used anything


def index
@page_title = I18n.t("hub.clients.index.title")

@clients = @client_sorter.filtered_and_sorted_clients.page(params[:page]).load
@message_summaries = RecentMessageSummaryService.messages(@clients.map(&:id))
end
Expand Down Expand Up @@ -47,8 +43,6 @@ def destroy
end

def edit
return render "public_pages/page_not_found", status: 404 if @client.intake.is_ctc?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this "public_pages/page_not_found" does not actually exist in the hub so this page is right now showing up blank, "raise CanCan::AccessDenied" will show the Access Denied page


@form = UpdateClientForm.from_client(@client)
end

Expand Down Expand Up @@ -105,8 +99,6 @@ def update_take_action
end

def unlock
raise CanCan::AccessDenied unless current_user.admin? || current_user.org_lead? || current_user.site_coordinator?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is now specified in ability.rb, functionality is the same with the addition that they will also see this page if client has archived intake


@client.unlock_access! if @client.access_locked?
flash[:notice] = I18n.t("hub.clients.unlock.account_unlocked", name: @client.preferred_name)
redirect_to(hub_client_path(id: @client))
Expand Down Expand Up @@ -297,13 +289,8 @@ def initialize(client)
@client = client
__setobj__(client)
@intake = client.intake
if @intake.present? && @intake.product_year != Rails.configuration.product_year
@archived = true
end
if @intake.blank?
@intake = Archived::Intake2021.find_by(client_id: @client.id)
@archived = true if @intake
end
@archived = client.has_archived_intake?
@intake = @archived ? client.archived_intake : client.intake
# For a short while, we created Client records with no intake and/or moved which client the intake belonged to.
if !@intake && @client.created_at < Date.parse('2022-04-15')
@missing_intake = true
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/hub/ctc_clients_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ class CtcClientsController < Hub::BaseController
layout "hub"

def edit
return render "public_pages/page_not_found", status: 404 unless @client.intake.is_ctc?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above this page doesn't exist for the hub

raise CanCan::AccessDenied unless @client.intake.is_ctc?

@is_dropoff = @client.tax_returns.any? { |tax_return| tax_return.service_type == "drop_off" }
@form = UpdateCtcClientForm.from_client(@client)
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/hub/notes_controller.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
module Hub
class NotesController < Hub::BaseController
load_and_authorize_resource :client
load_and_authorize_resource through: :client, only: [:create]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we want it to authorize on all the actions

load_and_authorize_resource through: :client
load_and_authorize_resource :user, parent: false, only: [:index]
layout "hub"

Expand Down
64 changes: 38 additions & 26 deletions app/lib/ability.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,14 @@ def initialize(user)
return
end

# Custom actions
alias_action :flag, :toggle_field, :edit_take_action, :update_take_action,
:unlock, :edit_13614c_form_page1, :edit_13614c_form_page2,
:edit_13614c_form_page3, :save_and_maybe_exit,
# Custom client controller actions
alias_action :flag, :toggle_field,
:edit_take_action, :update_take_action,
:edit_13614c_form_page1, :edit_13614c_form_page2,
:edit_13614c_form_page3, :edit_13614c_form_page4, :edit_13614c_form_page5,
:update_13614c_form_page1, :update_13614c_form_page2,
:update_13614c_form_page3, :cancel_13614c,
:resource_to_client_redirect,
Copy link
Contributor

Choose a reason for hiding this comment

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

just asking for my own curiosity: what was :resource_to_client_redirect originally for/doing? (i'm seeing that it got removed here.)

:update_13614c_form_page3, :update_13614c_form_page4, :update_13614c_form_page5,
:cancel_13614c, :save_and_maybe_exit,
to: :hub_client_management

accessible_groups = user.accessible_vita_partners
Expand All @@ -22,6 +23,7 @@ def initialize(user)
if user.admin?
# All admins who are also state file
can :manage, :all
can :destroy, Client if user.admin?

# Non-NJ staff cannot manage NJ EfileErrors, EfileSubmissions or FAQs
cannot :manage, EfileError, service_type: "state_file_nj"
Expand Down Expand Up @@ -76,57 +78,67 @@ def initialize(user)
can :read, Organization, id: accessible_groups.pluck(:id)
can :read, Site, id: accessible_groups.pluck(:id)

# This was overly permissive. We should work out what the permissions should
# be for each role and reduce this check. As we need to modify this, please
# break out the role and specify permissions more granularly
# HUB CLIENT CONTROLLER PERMISSIONS
# overly permissive, need to narrow permissions
# break out role and specify permissions when making modifications
client_role_whitelist = [
:client_success, :admin, :org_lead, :site_coordinator,
:coalition_lead, :state_file_admin, :team_member
].freeze

if user.role?(client_role_whitelist)
can :manage, Client, vita_partner: accessible_groups
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept existing functionality that admins can manage everything for the Client. In the future, since the client management actions are now broken out for client_role_whitelist users and not a blanket :manage action, we'll have to remember to explicitly allow them for non-admins if needed. Not sure we should explicitly state each client action permission for admins too as a guardrail for pushing up work that needs to touch this ability file?

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

can :read, Client, vita_partner: accessible_groups

can [:create, :update, :edit, :hub_client_management],
Client, vita_partner: accessible_groups, intake: { product_year: Rails.configuration.product_year }
end

if user.role?([:admin, :org_lead, :site_coordinator])
can :unlock, Client, vita_partner: accessible_groups, intake: { product_year: Rails.configuration.product_year }
end

if user.greeter?
can [:update, :read, :hub_client_management],
Client,
tax_returns: {
current_state: [
'intake_ready',
'intake_greeter_info_requested',
'intake_needs_doc_help',
],
current_state: %w[intake_ready intake_greeter_info_requested intake_needs_doc_help],
},
vita_partner: accessible_groups
vita_partner: accessible_groups, intake: { product_year: Rails.configuration.product_year }

can [:update, :read, :hub_client_management],
Client,
tax_returns: {
current_state: [
'file_not_filing',
'file_hold',
],
current_state: %w[file_not_filing file_hold],
assigned_user: user,
},
vita_partner: accessible_groups
vita_partner: accessible_groups, intake: { product_year: Rails.configuration.product_year }
end

# Only admins can destroy clients
cannot :destroy, Client unless user.admin?
can :manage, [

can [:create, :update, :destroy], [
Note,
Document,
TaxReturn
], client: { vita_partner: accessible_groups, intake: { product_year: Rails.configuration.product_year } }

can [:read], [
Note,
Document,
TaxReturn
], client: { vita_partner: accessible_groups }

can :manage, [
IncomingEmail,
IncomingTextMessage,
Note,
OutgoingEmail,
OutgoingTextMessage,
SystemNote,
TaxReturn,
], client: { vita_partner: accessible_groups }

can :manage, TaxReturnSelection, tax_returns: { client: { vita_partner: accessible_groups } }
cannot :manage, TaxReturnSelection, tax_returns: { client: { vita_partner: VitaPartner.where.not(id: accessible_groups) }}
can :manage, TaxReturnSelection, tax_returns: { client: { vita_partner: accessible_groups, intake: { product_year: Rails.configuration.product_year } } }
cannot :manage, TaxReturnSelection, tax_returns: { client: { vita_partner: VitaPartner.where.not(id: accessible_groups) } }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

technically i don't think we need this line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

never mind i removed it and it caused issues


can :manage, EfileSubmission, tax_return: { client: { vita_partner: accessible_groups } }

Expand Down
12 changes: 12 additions & 0 deletions app/models/client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,18 @@ def online_ctc?
intake.is_ctc? && intake.tax_returns.any? { |tr| tr.service_type == "online_intake" }
end

def has_archived_intake?
archived_intake.present?
end

def archived_intake
if intake.present? && intake.product_year != Rails.configuration.product_year
intake
elsif intake.blank?
Archived::Intake2021.find_by(client_id: self.id)
end
end

def recaptcha_scores_average
return efile_security_informations.last&.recaptcha_score unless recaptcha_scores.present?

Expand Down
2 changes: 1 addition & 1 deletion app/views/devise/invitations/new.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@
<%= f.hidden_field(:role, value: params[:role]) %>

<div>
<%= f.submit t(".submit"), class: "button button--primary" %>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

buttons were overlapping

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2025-01-30 at 1 32 30 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After:
Screenshot 2025-01-30 at 1 32 46 PM

<%= f.submit t(".submit"), class: "button button--primary spacing-below-25" %>
</div>
<div>
<%= link_to "Back", :back, class: "button button--secondary" %>
Expand Down
1 change: 1 addition & 0 deletions app/views/hub/clients/index.html.erb
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
<% @page_title = I18n.t("hub.clients.index.title") %>
<% content_for :page_title, @page_title %>
<% unless @hide_filters %>
<% content_for :filters do %>
Expand Down
Loading
Loading