From dd5a22dc80dcf7cc3f4435c212a582ecdb9b1054 Mon Sep 17 00:00:00 2001 From: Mac Siri Date: Fri, 22 Jun 2018 20:40:08 -0400 Subject: [PATCH] Create UserPolicy (#480) * Fix lint * Create User#settings_tab_list * Remove UsersController#finish_signup * Remove UsersController#index * Create UserPolicy WIP * Replace UsersController's spec with request spec * Fix broken specs * Fix broken specs * Remove comment * Upgrade UserPolicy * Fix specs --- app/controllers/application_controller.rb | 13 ---- app/controllers/organizations_controller.rb | 4 +- app/controllers/stories_controller.rb | 2 +- app/controllers/users_controller.rb | 63 ++++++------------- app/models/user.rb | 13 ++++ app/policies/user_policy.rb | 43 +++++++++++++ app/views/users/finish_signup.html.erb | 41 ------------ app/views/users/index.html.erb | 40 ------------ config/routes.rb | 6 +- spec/controllers/users_controller_spec.rb | 48 -------------- spec/policies/user_policy_spec.rb | 56 +++++++++++++++++ spec/rails_helper.rb | 1 - .../requests/additional_content_boxes_spec.rb | 14 ++--- spec/requests/user_organization_spec.rb | 4 +- spec/requests/user_settings_spec.rb | 62 +++++++++++------- 15 files changed, 181 insertions(+), 229 deletions(-) create mode 100644 app/policies/user_policy.rb delete mode 100644 app/views/users/finish_signup.html.erb delete mode 100644 app/views/users/index.html.erb delete mode 100644 spec/controllers/users_controller_spec.rb create mode 100644 spec/policies/user_policy_spec.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index f44f5f169..4bf5c75ec 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -75,19 +75,6 @@ class ApplicationController < ActionController::Base response.headers["Expires"] = "Fri, 01 Jan 1990 00:00:00 GMT" end - def tab_list(user) - tab_list = ["Profile", - "Integrations", - "Notifications", - "Publishing from RSS", - "Organization", - "Billing"] - tab_list << "Membership" if user&.monthly_dues&.positive? && user&.stripe_id_code - tab_list << "Switch Organizations" if user&.has_role?(:switch_between_orgs) - tab_list << "Misc" - tab_list - end - def touch_current_user current_user.touch end diff --git a/app/controllers/organizations_controller.rb b/app/controllers/organizations_controller.rb index c3dbdfda7..86ba22efa 100644 --- a/app/controllers/organizations_controller.rb +++ b/app/controllers/organizations_controller.rb @@ -4,7 +4,7 @@ class OrganizationsController < ApplicationController def create @tab = "organization" @user = current_user - @tab_list = tab_list(@user) + @tab_list = @user.settings_tab_list @organization = Organization.new(organization_params) if @organization.save current_user.update(organization_id: @organization.id, org_admin: true) @@ -20,7 +20,7 @@ class OrganizationsController < ApplicationController def update @user = current_user @tab = "organization" - @tab_list = tab_list(@user) + @tab_list = @user.settings_tab_list raise unless @user.org_admin @organization = @user.organization if @organization.update(organization_params) diff --git a/app/controllers/stories_controller.rb b/app/controllers/stories_controller.rb index 1ed9d1346..0ce474cf6 100644 --- a/app/controllers/stories_controller.rb +++ b/app/controllers/stories_controller.rb @@ -89,7 +89,7 @@ class StoriesController < ApplicationController @stories = @stories.where(approved: true) end - @stories = stories_by_timeframe + @stories = stories_by_timeframe @stories = @stories.decorate diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index ae96ac47e..dbaa9e797 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -1,35 +1,27 @@ class UsersController < ApplicationController - before_action :set_user, only: %i[show update destroy finish_signup] - before_action :set_no_cache_header, except: [:index] - - def index - #Soft internal deprecation. Should be removed at some point. - return redirect_to "/" unless current_user - @users = case params[:user_board].downcase - when "following" then current_user.following_users - when "followers" then current_user.user_followers - end - end + before_action :set_no_cache_header + after_action :verify_authorized, except: [:signout_confirm] # GET /settings/@tab def edit - @user = current_user - @tab_list = tab_list(@user) - unless @user - redirect_to "/enter" - return + unless current_user + skip_authorization + return redirect_to "/enter" end + @user = current_user + @tab_list = @user.settings_tab_list @tab = params["tab"] + authorize @user handle_settings_tab - # authorize! :update, @user end # PATCH/PUT /users/:id.:format def update @user = current_user - @tab_list = tab_list(@user) + @tab_list = @user.settings_tab_list @tab = params["user"]["tab"] || "profile" - if !@user.banned && @user.update(user_params) + authorize @user + if @user.update(user_params) RssReader.new.delay.fetch_user(@user) if @user.feed_url.present? notice = "Your profile was successfully updated." follow_hiring_tag(@user) @@ -41,6 +33,7 @@ class UsersController < ApplicationController def onboarding_update current_user.saw_onboarding = true + authorize User if current_user.save! respond_to do |format| format.json { render json: { outcome: "onboarding closed" } } @@ -53,6 +46,7 @@ class UsersController < ApplicationController end def join_org + authorize User if @organization = Organization.find_by_secret(params[:org_secret]) current_user.update(organization_id: @organization.id) redirect_to "/settings/organization", @@ -63,15 +57,15 @@ class UsersController < ApplicationController end def leave_org + authorize User current_user.update(organization_id: nil, org_admin: nil) redirect_to "/settings/organization", notice: "You have left your organization." end def add_org_admin - raise unless current_user.org_admin user = User.find(params[:user_id]) - raise unless current_user.organization_id == user.organization_id + authorize user user.update(org_admin: true) user.add_role :analytics_beta_tester if user.organization.approved redirect_to "/settings/organization", @@ -79,19 +73,16 @@ class UsersController < ApplicationController end def remove_org_admin - raise unless current_user.org_admin - raise if current_user.id == params[:user_id] user = User.find(params[:user_id]) - raise unless current_user.organization_id == user.organization_id + authorize user user.update(org_admin: false) redirect_to "/settings/organization", notice: "#{user.name} is no longer an admin." end def remove_from_org - raise unless current_user.org_admin - raise if current_user.id == params[:user_id] user = User.find(params[:user_id]) + authorize user user.update(organization_id: nil) redirect_to "/settings/organization", notice: "#{user.name} is no longer part of your organization." @@ -99,20 +90,6 @@ class UsersController < ApplicationController def signout_confirm; end - # GET/PATCH /users/:id/finish_signup - def finish_signup - # authorize! :update, @user - if request.patch? && params[:user] # && params[:user][:email] - if @user.update(user_params) - # @user.skip_reconfirmation! - sign_in(@user, bypass: true) - redirect_to "/", notice: "Your profile was successfully updated." - else - @show_errors = true - end - end - end - def follow_hiring_tag(user) return unless user.looking_for_work? user.delay.follow(Tag.find_by(name: "hiring")) @@ -134,7 +111,7 @@ class UsersController < ApplicationController @customer = Stripe::Customer.retrieve(current_user.stripe_id_code) if current_user.stripe_id_code when "membership" if current_user.monthly_dues.zero? - redirect_to "/membership" + redirect_to "/membership" return end end @@ -142,10 +119,6 @@ class UsersController < ApplicationController private - def set_user - @user = User.find(params[:id]) - end - def user_params accessible = %i[name email diff --git a/app/models/user.rb b/app/models/user.rb index 1d1112a39..d64a93b6a 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -279,6 +279,19 @@ class User < ApplicationRecord end end + def settings_tab_list + tab_list = ["Profile", + "Integrations", + "Notifications", + "Publishing from RSS", + "Organization", + "Billing"] + tab_list << "Membership" if monthly_dues&.positive? && stripe_id_code + tab_list << "Switch Organizations" if has_role?(:switch_between_orgs) + tab_list << "Misc" + tab_list + end + private diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb new file mode 100644 index 000000000..dc4f8263a --- /dev/null +++ b/app/policies/user_policy.rb @@ -0,0 +1,43 @@ +class UserPolicy < ApplicationPolicy + def edit? + user == record + end + + def onboarding_update? + true + end + + def update? + user == record + end + + def join_org? + !user_is_banned? + end + + def leave_org? + true + end + + def add_org_admin? + user.org_admin && within_the_same_org? + end + + def remove_org_admin? + user.org_admin && not_self? && within_the_same_org? + end + + def remove_from_org? + user.org_admin && not_self? && within_the_same_org? + end + + private + + def within_the_same_org? + user.organization == record.organization + end + + def not_self? + user != record + end +end diff --git a/app/views/users/finish_signup.html.erb b/app/views/users/finish_signup.html.erb deleted file mode 100644 index 9e9aa2bd5..000000000 --- a/app/views/users/finish_signup.html.erb +++ /dev/null @@ -1,41 +0,0 @@ -
-




- <% if current_user.unconfirmed_email %> -

An email has been sent to you, please confirm!

- <% else %> -

<%= current_user.username %> profile image Welcome!

-

Just add a couple details to get going

- <% end %> - <%= form_for(current_user, :as => 'user', :url => finish_signup_path(current_user), :html => { role: 'form'}) do |f| %> - <% if @show_errors && current_user.errors.any? %> -
- <% current_user.errors.full_messages.each do |msg| %> - <%= msg %>
- <% end %> -
- <% end %> -
- <%= f.label :email %> -
- <%= f.email_field :email, :autofocus => true, :value => current_user.unconfirmed_email, class: 'form-control input-lg', placeholder: 'Example: email@me.com' %> -
-
-
- <%= f.label :name, "Full Name" %> -
- <%= f.text_field :name, class: 'form-control input-lg', placeholder: 'Jane Doe' %> -
-
- - -
- <%= f.submit 'Continue', :class => 'btn btn-primary' %> -
- <% end %> -
diff --git a/app/views/users/index.html.erb b/app/views/users/index.html.erb deleted file mode 100644 index 3c50bafec..000000000 --- a/app/views/users/index.html.erb +++ /dev/null @@ -1,40 +0,0 @@ -<% title params[:user_board].titleize %> - -
-
-
-

<%=params[:user_board].titleize%>

-
- <% @users.each do |user| %> -
-
- - <%= user.username %> profile image - -
-
-

<%= user.name %>

 -
- <% if user.twitter_username.present? %> - <%= icon("twitter","22") %> <%= user.twitter_username %> - <% end %> - <% if user.github_username.present? %> - <%= icon("github","22") %> <%= user.github_username %> - <% end %> - <% if user.website_url.present? %> - <%= icon("link","22") %> <%= beautified_url(user.website_url) %> - <% end %> - -
-
-
- <% end %> -
-
- <% if params[:user_board].downcase == "followers" %> - When someone follows you, your posts are prioritized on their home page. ❤️ - <% else %> - When you follow someone, their posts are prioritized on your home page. ❤️ - <% end %> -
-
diff --git a/config/routes.rb b/config/routes.rb index 0746f6e53..10f6796de 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -99,7 +99,7 @@ Rails.application.routes.draw do resources :video_states, only: [:create] resources :twilio_tokens, only: [:show] resources :push_notification_subscriptions, only: [:create] - + get "/notifications/:username" => "notifications#index" patch "/onboarding_update" => "users#onboarding_update" get "email_subscriptions/unsubscribe" @@ -193,12 +193,8 @@ Rails.application.routes.draw do end end - get "/:user_board" => "users#index", :constraints => { user_board: /following|followers|leaderboard/} - match "/delayed_job_admin" => DelayedJobWeb, :anchor => false, via: [:get, :post] - match "/users/:id/finish_signup" => "users#finish_signup", via: [:get, :patch], :as => :finish_signup - get "/settings/(:tab)" => "users#edit" get "/signout_confirm" => "users#signout_confirm" get "/dashboard" => "dashboards#show" diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb deleted file mode 100644 index 083c6b904..000000000 --- a/spec/controllers/users_controller_spec.rb +++ /dev/null @@ -1,48 +0,0 @@ -# controller specs are now discouraged in favor of request specs. -# This file should eventually be removed -require "rails_helper" - -RSpec.describe UsersController, type: :controller do - let(:user) { create(:user) } - - describe "GET #index" do - context "without current_user" do - it "returns a 302" do - get :index, params: { user_board: "following" } - expect(response.status).to eq(302) - end - end - - context "with current_user" do - before { sign_in user } - - it "works for followings" do - get :index, params: { user_board: "following" } - expect(response.status).to eq(200) - end - - it" works for followers" do - get :index, params: { user_board: "followers" } - expect(response.status).to eq(200) - end - end - end - - describe "GET #edit" do - context "without being signed in" do - it "returns redirect" do - get :edit - expect(response).to redirect_to("/enter") - end - end - - context "with authorized user" do - before { sign_in user } - - it "returns 200" do - get :edit - expect(response.status).to eq(200) - end - end - end -end diff --git a/spec/policies/user_policy_spec.rb b/spec/policies/user_policy_spec.rb new file mode 100644 index 000000000..6594ddbcd --- /dev/null +++ b/spec/policies/user_policy_spec.rb @@ -0,0 +1,56 @@ +require "rails_helper" + +RSpec.describe UserPolicy do + subject { described_class.new(user, other_user) } + + let(:other_user) { build(:user) } + + context "when user is not signed-in" do + let(:user) { nil } + + it { within_block_is_expected.to raise_error(Pundit::NotAuthorizedError) } + end + + context "when user is signed-in" do + let(:user) { other_user } + + it { is_expected.to permit_actions(%i[edit update onboarding_update join_org leave_org]) } + + context "with banned status" do + before { user.add_role(:banned) } + + it { is_expected.to forbid_actions(%i[join_org]) } + end + end + + context "when user is org_admin" do + let(:org) { build(:organization) } + let(:other_org) { build(:organization) } + let(:user) { build(:user, org_admin: true, organization: org) } + + + context "with other_user as org_member of same org" do + let(:other_user) { build(:user, organization: org) } + + it { is_expected.to permit_actions(%i[add_org_admin remove_from_org]) } + end + + context "with other_user as org_member of a different org" do + let(:other_user) { build(:user, organization: other_org) } + + it { is_expected.to forbid_actions(%i[add_org_admin remove_from_org]) } + end + + context "with other_user as org admin" do + let(:other_user) { build(:user, org_admin: true, organization: org) } + + it { is_expected.to permit_actions(%i[remove_org_admin]) } + end + + context "with other_user as org adming of a different org" do + let(:other_user) { build(:user, org_admin: true, organization: other_org) } + + it { is_expected.to forbid_actions(%i[remove_org_admin]) } + end + end +end diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 831a6d6b1..544854362 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -20,7 +20,6 @@ require "shoulda/matchers" require "pundit/rspec" require "pundit/matchers" - WebMock.disable_net_connect!(allow_localhost: true) # Add additional requires below this line. Rails is not loaded until this point! diff --git a/spec/requests/additional_content_boxes_spec.rb b/spec/requests/additional_content_boxes_spec.rb index 23da2e473..00689f6cd 100644 --- a/spec/requests/additional_content_boxes_spec.rb +++ b/spec/requests/additional_content_boxes_spec.rb @@ -1,19 +1,19 @@ require "rails_helper" RSpec.describe "AdditionalContentBoxes", type: :request do + let(:regular_article) { create(:article) } + describe "GET /additional_content_boxes" do it "returns an article if there is a published/featured one" do suggestion = create(:article, published: true, featured: true) - suggestion.update_column(:published, true) - article = create(:article) - get "/additional_content_boxes?article_id=#{article.id}" + get "/additional_content_boxes?article_id=#{regular_article.id}" expect(response.body).to include CGI.escapeHTML(suggestion.title) end + it "returns no article if not published/featured" do suggestion = create(:article) - article = create(:article) - get "/additional_content_boxes?article_id=#{article.id}" - expect(response.body).to_not include CGI.escapeHTML(suggestion.title) + get "/additional_content_boxes?article_id=#{regular_article.id}" + expect(response.body).not_to include CGI.escapeHTML(suggestion.title) end end -end \ No newline at end of file +end diff --git a/spec/requests/user_organization_spec.rb b/spec/requests/user_organization_spec.rb index d7d39512a..343546532 100644 --- a/spec/requests/user_organization_spec.rb +++ b/spec/requests/user_organization_spec.rb @@ -44,7 +44,7 @@ RSpec.describe "UserOrganization", type: :request do user.update(organization_id: organization.id) user2 = create(:user, organization_id: organization.id, org_admin: true) expect { post "/users/add_org_admin", params: { user_id: user2.id } }. - to raise_error RuntimeError + to raise_error Pundit::NotAuthorizedError end it "removes org admin" do @@ -58,6 +58,6 @@ RSpec.describe "UserOrganization", type: :request do user.update(organization_id: organization.id) user2 = create(:user, organization_id: organization.id, org_admin: true) expect { post "/users/remove_org_admin", params: { user_id: user2.id } }. - to raise_error RuntimeError + to raise_error Pundit::NotAuthorizedError end end diff --git a/spec/requests/user_settings_spec.rb b/spec/requests/user_settings_spec.rb index 978a11fe2..33d49bbf9 100644 --- a/spec/requests/user_settings_spec.rb +++ b/spec/requests/user_settings_spec.rb @@ -1,39 +1,53 @@ require "rails_helper" RSpec.describe "UserSettings", type: :request do - before do - @user = create(:user) - login_as @user - end + let(:user) { create(:user) } - describe "GET /settings" do - it "renders various settings tabs properly" do - get "/settings/organization" - expect(response.body).to include("Settings for") - get "/settings/switch-organizations" - expect(response.body).to include("Settings for") - # get "/settings/integrations" - # expect(response.body).to include("Settings for") - get "/settings/billing" - expect(response.body).to include("Settings for") - get "/settings/misc" - expect(response.body).to include("Settings for") - get "/settings/membership" - expect(response.body).not_to include("Settings for") - @user.update_column(:monthly_dues, 5) - get "/settings/membership" - expect(response.body).to include("Settings for") + describe "GET /settings/:tab" do + context "when not signed-in" do + it "returns not_foudn" do + get "/settings" + expect(response).to redirect_to("/enter") + end + end + + context "when signed-in" do + before do + login_as user + end + + it "renders various settings tabs properly" do + %w[organization switch-organizations billing misc].each do |tab| + get "/settings/#{tab}" + expect(response.body).to include("Settings for") + end + end + + it "doesn't let user access membership if user has no monthly_dues" do + get "/settings/membership" + expect(response.body).not_to include("Settings for") + end + + it "allows user with monthly_dues to access membership" do + user.update_column(:monthly_dues, 5) + get "/settings/membership" + expect(response.body).to include("Settings for") + end end end describe "PUT /update/:id" do + before do + login_as user + end + it "updates summary" do - put "/users/#{@user.id}", params: { user: {tab: "profile", summary: "Hello new summary"} } - expect(@user.summary).to eq("Hello new summary") + put "/users/#{user.id}", params: { user: { tab: "profile", summary: "Hello new summary" } } + expect(user.summary).to eq("Hello new summary") end it "updates username to too short username" do - put "/users/#{@user.id}", params: { user: {tab: "profile", username: "h"} } + put "/users/#{user.id}", params: { user: { tab: "profile", username: "h" } } expect(response.body).to include("Username is too short") end end