Adding Pundit, applies to User and Person models #514

armahillo · 2020-06-14T23:46:05Z

Will update this shortly.

maebeale · 2020-06-14T23:58:10Z

app/controllers/people_controller.rb

@@ -1,4 +1,5 @@
 class PeopleController < ApplicationController
+  before_action :authenticate_user!


isn't this already in ApplicationController?

Yeah -- whoops! Gotta remove that :(

Looks like it! I can take this one out

maebeale · 2020-06-14T23:58:52Z

app/controllers/people_controller.rb

+  before_action :authenticate_user!
  before_action :set_person, only: [:show, :edit, :update, :destroy]

  def index


should this have authorize @people?

Maybe -- I'm still learning how to setup Pundit

shrug Maybe? We don't really have a lot of info about the expectations of how this stuff works so I wasn't sure how the /people route was intended to be used. Should it be locked down?

IME, authorizing a collection isn't very useful, but verifying that the collection was generated using a policy scope is. The best way to do that is to let the verify_policy_scoped after_action catch it if you forget.

maebeale · 2020-06-14T23:59:48Z

app/controllers/users_controller.rb

    end
  end

+  # NOTE: I'm not entirely sure this is used at all.


yeah, i'm considering a new controller so admins can do create user+person in one step and removing this from here bc it's not used/useable

I can't get the tests for it to run, or even route to it so that I can insert a byebug! So I'm going to remove it.

maebeale · 2020-06-15T00:00:32Z

app/policies/person_policy.rb

+    @record = record
+  end
+
+  def edit?


we get this from ApplicationPolicy? not sure what standard is for adding to child policies

I think originally I had changed it to something else and was just changing it back here. I realize it's redundant, but it also explicitly declares the expected behavior (were ApplicationPolicy to be changed later, that would create problems here). My personal preference for stuff like this is to have the default behaviors as a safety net but to explicitly define things that are definitely used.

I'm happy to defer to yours / the team's preferences on this though!

maebeale · 2020-06-15T00:01:28Z

app/policies/user_policy.rb

+  end
+
+  def update?
+    user == record || user.sys_admin_role? || user.admin_role?


should users bc able to edit their own roles tho? maybe dropdown is only their permission level and lower?

yeah I think the controller layer is the right place to set that

maebeale · 2020-06-15T00:01:47Z

app/views/errors/403.html.erb

@@ -0,0 +1,2 @@
+<h1>Sorry, you aren't authorized to do that.</h1>
+<p>If you think this is wrong, please contact your administrator.</p>


maebeale · 2020-06-15T00:02:13Z

app/views/users/show.html.erb

@@ -1,15 +1,25 @@
-<div class="title is-3">Profile</div>
-<div class="title is-5"><%= @user.person&.name %></div>
+<div class="title is-3">Account Settings</div>


maebeale · 2020-06-15T00:03:05Z

config/locales/en.yml

      other: Match Feedback
      zero: Match Feedback
+  pundit:
+    default: 'You cannot perform this action.'


totally! wtg w the details!

maebeale · 2020-06-15T00:04:53Z

spec/requests/people_spec.rb

+  describe "POST /create" do
+    subject { post people_url, params: { person: valid_attributes } }
+
+    it_behaves_like "basic user authorization required"


maebeale · 2020-06-15T00:06:42Z

spec/requests/people_spec.rb

+
+      before(:each) { create(:person, user_id: self.id) }
+
+      # TODO: What happens when the user already has a record created for themselves


🤷‍♀️

haha 😆

This could be decided on later, it was just something that came to mind while I was doing it

maebeale · 2020-06-15T00:07:12Z

spec/requests/people_spec.rb

+
+    context "when attempting to create a person for someone else's user ID" do
+      let(:other_user) { create(:user) }
+      subject { post people_url, params: { person: valid_attributes.merge(user_id: other_user.id) } }


maebeale · 2020-06-15T00:08:22Z

spec/support/basic_authorization_requirements_shared_examples.rb

@@ -0,0 +1,54 @@
+RSpec.shared_examples 'redirects without authorization' do


💃 awesome!

app/controllers/application_controller.rb

maebeale · 2020-06-15T00:11:41Z

ack! didn't notice the draft tag. my b!!!

solebared · 2020-06-18T01:56:59Z

This is looking great @armahillo ❤️💥.

One thought on testing: i wonder if it would be sufficient to directly test policies (spec/policies/...) and verify controller coverage by getting pundit to ensure authorization happened.

I'm guessing it will be easier to test policy behavior w/o having to go through the controller and could also lead to less duplication/boiler-plate across request specs (most controllers will behave exactly the same based on whether the policy throws or not).

…-progress still)

solebared

This is looking great!! 🙏🏾 🙌🏾
A few thoughts and questions inline.

.env

app/controllers/application_controller.rb

app/controllers/concerns/not_using_pundit_yet.rb

solebared · 2020-09-13T13:16:45Z

app/policies/application_policy.rb

+  attr_reader :acting_user, :record
+
+  def initialize(acting_user, record)
+    @acting_user = acting_user


I think i understand the concept of an acting_user and like the term. I do wonder however if there's value in sticking with current_user since that's an existing concept we already have to incorporate from devise. Is there a conceptual or concrete difference between them?

Or alternatively, use plain user within policies since that seems to be the defacto norm (used in pundit docs).

app/policies/person_policy.rb

spec/rails_helper.rb

solebared · 2020-09-13T16:03:21Z

spec/support/shared_contexts/authorization.rb

+RSpec.shared_context "acting as signed-in user" do
+  let(:acting_user) { FactoryBot.create(:user) }
+end 
+
+RSpec.shared_context "acting as sysadmin" do
+  let(:acting_user) { FactoryBot.create(:user, :sys_admin) }
+end
+
+RSpec.shared_context "acting as admin" do
+  let(:acting_user) { FactoryBot.create(:user, :admin) }
+end


Have to admit, i've grown to dislike globally shared contexts, particularly those that define lets referenced in specs. I find the cognitive load involved in jumping back and forth between spec, local context, shared context and sometimes other connected contexts to be quite heavy.

I really like the local ones in user|person_policy_spec because the context names provide more meaning that the lets therein. These ones definitely also provide meaning, but i'm finding it difficult have certainty just from the names exactly what they do. Perhaps this is case of needing to get used to the terminology ('acting-as', 'signed-in', etc) and once i do, my mind won't want to jump around. OTOH, i wonder if inlining these ends up being easier to grok, even at the cost of some more duplication?

The ones below seem widely used in request specs so probably worth their weight.

solebared · 2020-09-13T16:05:38Z

app/policies/application_policy.rb

+  # By default, we group permissions into:
+  #   - read (show)
+  #   - add (new/create)
+  #   - change (edit/update)
+  #   - delete (destroy)
+  # We recommend that you override read?/add?/change?/delete? rather than action-level policies.
+  def show?;     read?;    end
+  def create?;   add?;     end
+  def new?;      add?;     end
+  def update?;   change?;  end
+  def edit?;     change?;  end
+  def destroy?;  delete?;  end
+
+  # We default all permissions to false, and expect you to override as needed.
+  private def read?;    false;  end
+  private def add?;     false;  end
+  private def change?;  false;  end
+  private def delete?;  false;  end


Conflicts resolved: * Gemfile * Gemfile.lock * app/controllers/public_pages_controller.rb

solebared · 2020-10-02T18:39:59Z

@bhaibel , i noticed this branch was showing some minor conflicts against main.
I went ahead and merged it in, resolving conflicts, with the hope this is more helpful than annoying! 🤞🏾😅

Co-authored-by: maebeale <maebeale@gmail.com> Conflicts: app/controllers/admin_controller.rb app/controllers/announcements_controller.rb app/controllers/application_controller.rb app/controllers/asks_controller.rb app/controllers/categories_controller.rb app/controllers/communication_logs_controller.rb app/controllers/community_resources_controller.rb app/controllers/contact_methods_controller.rb app/controllers/contributions_controller.rb app/controllers/custom_form_questions_controller.rb app/controllers/donations_controller.rb app/controllers/feedbacks_controller.rb app/controllers/form_questions_controller.rb app/controllers/forms_controller.rb app/controllers/history_logs_controller.rb app/controllers/listings_controller.rb app/controllers/location_types_controller.rb app/controllers/locations_controller.rb app/controllers/matches_controller.rb app/controllers/mobility_string_translations_controller.rb app/controllers/offers_controller.rb app/controllers/organizations_controller.rb app/controllers/people_controller.rb app/controllers/positions_controller.rb app/controllers/public_controller.rb app/controllers/service_areas_controller.rb app/controllers/shared_accounts_controller.rb app/controllers/shift_matches_controller.rb app/controllers/shifts_controller.rb app/controllers/software_feedbacks_controller.rb app/controllers/submission_response_imports_controller.rb app/controllers/submission_responses_controller.rb app/controllers/submissions_controller.rb app/controllers/system_locales_controller.rb app/controllers/system_settings_controller.rb app/controllers/teams_controller.rb app/controllers/users_controller.rb spec/factories/users.rb spec/requests/people_spec.rb

Ref: varvet/pundit#113 (comment)

See d055fd2.

These are controllers that were added since the pundit branch was worked on.

solebared · 2021-01-18T23:48:39Z

maebeale

YESSSSS!!!!!!

THANK YOU @Exbinary @svileshina @armahillo @bhaibel @h-m-m !!!!!

maebeale reviewed Jun 14, 2020

View reviewed changes

maebeale reviewed Jun 15, 2020

View reviewed changes

app/controllers/application_controller.rb Show resolved Hide resolved

Adds Pundit gem and some default policies

d32b632

h-m-m force-pushed the 61-add-pundit branch from df1ef44 to f45911d Compare July 30, 2020 18:59

armahillo and others added 3 commits July 30, 2020 15:07

Sets pundit permissions for Users

1c84671

Configures Pundit and applies to User and Person models (Person is in…

f894158

…-progress still)

clean up formatting with some minor changes

7cb8af2

h-m-m force-pushed the 61-add-pundit branch from f45911d to 7cb8af2 Compare July 30, 2020 19:07

add naive fix of people controler tests

fd6b4da

solebared changed the base branch from master to main September 2, 2020 23:44

svileshina mentioned this pull request Sep 4, 2020

[DRAFT] Change link to Manage Volunteers param #200

Closed

svileshina assigned h-m-m Sep 4, 2020

svileshina linked an issue Sep 4, 2020 that may be closed by this pull request

Add Pundit #61

Closed

Betsy Haibel added 2 commits September 6, 2020 13:07

Merge branch 'main' into 61-add-pundit

75c1430

moar pundit

0bea0b4

solebared reviewed Sep 13, 2020

View reviewed changes

solebared marked this pull request as ready for review September 13, 2020 16:10

solebared mentioned this pull request Sep 23, 2020

SiteAdmin assigns permissions #731

Closed

Merge branch 'main' into 61-add-pundit

693291e

Conflicts resolved: * Gemfile * Gemfile.lock * app/controllers/public_pages_controller.rb

solebared mentioned this pull request Oct 5, 2020

Disable user sign_up until pundit work is ready #767

Merged

5 tasks

gingeralbertson mentioned this pull request Oct 13, 2020

SiteAdmin invites users #787

Closed

solebared force-pushed the 61-add-pundit branch from 2840b12 to 83314ee Compare January 18, 2021 05:03

solebared added 5 commits January 18, 2021 13:25

Exclude Devise controllers from pundit auth verification

e596fbb

Ref: varvet/pundit#113 (comment)

Secret key base issue was fixed on main so drop .env entry

0c162d7

See d055fd2.

Remove unused default_url_options from rails_helper

5376c19

Exclude new controllers from pundit auth verification

d852faa

These are controllers that were added since the pundit branch was worked on.

solebared force-pushed the 61-add-pundit branch from 0058ed4 to d852faa Compare January 18, 2021 18:30

Merge branch 'main' into 61-add-pundit

492deb1

maebeale approved these changes Jan 19, 2021

View reviewed changes

solebared merged commit e2de56d into main Jan 19, 2021

solebared deleted the 61-add-pundit branch January 19, 2021 15:18

solebared mentioned this pull request Jan 26, 2021

Re-enable registration #840

Merged

30 tasks

		@@ -1,4 +1,5 @@
		class PeopleController < ApplicationController
		before_action :authenticate_user!

		@@ -0,0 +1,2 @@
		<h1>Sorry, you aren't authorized to do that.</h1>
		<p>If you think this is wrong, please contact your administrator.</p> No newline at end of file


		before(:each) { create(:person, user_id: self.id) }

		# TODO: What happens when the user already has a record created for themselves

		@@ -0,0 +1,54 @@
		RSpec.shared_examples 'redirects without authorization' do

Uh oh!

Adding Pundit, applies to User and Person models #514

Adding Pundit, applies to User and Person models #514

Uh oh!

Conversation

armahillo commented Jun 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maebeale commented Jun 15, 2020

Uh oh!

solebared commented Jun 18, 2020

Uh oh!

solebared left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

solebared commented Oct 2, 2020

Uh oh!

solebared commented Jan 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maebeale left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

solebared commented Jan 18, 2021 •

edited

Loading