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

embarnard · 2025-01-30T23:00:56Z

Link to pivotal/JIRA issue

e.g. jira link

Is PM acceptance required? (delete one)

Yes - don't merge until JIRA issue is accepted!

Reminder: merge main into this branch and get green tests before merging to main

What was done?

Restrict ability of hub users non-admins from making changes to clients with archived intakes including:

add/edit/delete a tax return for a previous year
add notes
add documents

There was also some general clean up of the ability and client controller file including:

replaced broken renderings of public_pages/page_not_found with 403 page
new methods for seeing if client has archived intake
add new 13614c_form pages to ability authorization checks
client actions in ability files are split between reads which can happen for any client in the accessible group and other actions that should only happen on present year intakes
Added a lot of tests to make sure an access denied error is raised if user is trying to hit an action on an archived intake

How to test?

Try to take any of the following actions on GYR clients with an archived intake:
: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, :update_13614c_form_page4, :update_13614c_form_page5, :cancel_13614c, :save_and_maybe_exit, unlock
also any actions on notes, documents or tax returns besides read
each action on the bulk action page like change org, assignees and send message

Screenshots (for visual changes)

fixed random overlapping buttons, everything else visually should be the same

Before

After

github-actions · 2025-01-30T23:05:29Z

Heroku app: https://gyr-review-app-5491-7545bb5f1cb5.herokuapp.com/
View logs: heroku logs --app gyr-review-app-5491 (optionally add --tail)

embarnard · 2025-02-10T17:24:20Z

app/controllers/hub/clients_controller.rb

@@ -47,8 +43,6 @@ def destroy
    end

    def edit
-      return render "public_pages/page_not_found", status: 404 if @client.intake.is_ctc?


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

embarnard · 2025-02-10T17:26:26Z

app/controllers/hub/clients_controller.rb

@@ -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?


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

embarnard · 2025-02-10T17:26:56Z

app/controllers/hub/ctc_clients_controller.rb

@@ -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?


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

embarnard · 2025-02-10T17:27:25Z

app/controllers/hub/notes_controller.rb

@@ -1,7 +1,7 @@
 module Hub
  class NotesController < Hub::BaseController
    load_and_authorize_resource :client
-    load_and_authorize_resource through: :client, only: [:create]


we want it to authorize on all the actions

embarnard · 2025-02-10T17:30:34Z

app/views/devise/invitations/new.html.erb

@@ -53,7 +53,7 @@
      <%= f.hidden_field(:role, value: params[:role]) %>

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


buttons were overlapping

embarnard · 2025-02-10T17:31:15Z

app/lib/ability.rb

-    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) } }


technically i don't think we need this line

never mind i removed it and it caused issues

… return

embarnard · 2025-02-11T05:21:33Z

app/lib/ability.rb

    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


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?

…strict-editing-client-tax-returns-from-previous-years

embarnard · 2025-02-11T16:32:40Z

app/controllers/hub/clients_controller.rb

@@ -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


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

spompea-cfa · 2025-02-12T00:02:32Z

app/lib/ability.rb

                 :update_13614c_form_page1, :update_13614c_form_page2,
-                 :update_13614c_form_page3, :cancel_13614c,
-                 :resource_to_client_redirect,


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

spompea-cfa

approved 🌟

Wip: client, notes and documents ability

ca8929e

embarnard added 7 commits January 31, 2025 12:45

flag / ability is failing for some reason argghhhh

2ccc163

Add to ability file

7f8593c

wip: more stuff

437921b

Ability and specs

170f28a

Add back method

668ef38

Add changes

3dfd8f6

Add controller spec and unlock to ability

fdd98d6

embarnard commented Feb 10, 2025

View reviewed changes

embarnard added 4 commits February 10, 2025 13:47

ddd

5260645

break out document ability and add more tests for controllers and tax…

17164c5

… return

Remove unneccessary line

f518d9d

Add ability changes

4c9e37c

embarnard commented Feb 11, 2025

View reviewed changes

embarnard added 6 commits February 11, 2025 00:37

Add changes

6c19e18

Add changes

b9fc506

Add tests

3dc0a6d

remove change

26f2126

Merge remote-tracking branch 'origin/main' into GYR1-566-bug-crowd-re…

8f8a7ba

…strict-editing-client-tax-returns-from-previous-years

Add page title back

0d50985

embarnard commented Feb 11, 2025

View reviewed changes

embarnard changed the title ~~Wip: client, notes and documents ability~~ Restrict ability on client, notes and documents in hub for clients with archived intakes Feb 11, 2025

Add changes

c38ea4d

embarnard marked this pull request as ready for review February 11, 2025 18:52

embarnard changed the title ~~Restrict ability on client, notes and documents in hub for clients with archived intakes~~ Restrict ability on clients with archived intakes on hub client related actions Feb 11, 2025

spompea-cfa reviewed Feb 12, 2025

View reviewed changes

spompea-cfa approved these changes Feb 12, 2025

View reviewed changes

mpidcock added the in-acceptance label Feb 12, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

embarnard commented Jan 30, 2025 •

edited

Loading

github-actions bot commented Jan 30, 2025

embarnard Feb 10, 2025

embarnard Feb 10, 2025

embarnard Feb 10, 2025

embarnard Feb 10, 2025

embarnard Feb 10, 2025

embarnard Feb 11, 2025

embarnard Feb 11, 2025

embarnard Feb 10, 2025

embarnard Feb 11, 2025

embarnard Feb 11, 2025

spompea-cfa Feb 12, 2025

embarnard Feb 11, 2025

spompea-cfa Feb 12, 2025

spompea-cfa left a comment

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

Are you sure you want to change the base?

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

Conversation

embarnard commented Jan 30, 2025 • edited Loading

Link to pivotal/JIRA issue

Is PM acceptance required? (delete one)

What was done?

How to test?

Screenshots (for visual changes)

github-actions bot commented Jan 30, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spompea-cfa left a comment

Choose a reason for hiding this comment

embarnard commented Jan 30, 2025 •

edited

Loading