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

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

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)

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
 

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

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
 

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

app/views/users/finish_signup.html.erb

@@ -1,41 +0,0 @@
-<div id="add-email" class="container">
-  <br><br><br><br><br>
-  <% if current_user.unconfirmed_email %>
-    <h2>An email has been sent to you, please confirm!</h2>
-  <% else %>
-    <h1><img alt="<%= current_user.username %> profile image" src="<%= current_user.profile_image_url%>"> Welcome!</h1>
-    <h2>Just add a couple details to get going</h2>
-  <% 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? %>
-      <div id="error_explanation">
-        <% current_user.errors.full_messages.each do |msg| %>
-          <%= msg %><br>
-        <% end %>
-      </div>
-    <% end %>
-    <div class="form-group">
-      <%= f.label :email %>
-      <div class="controls">
-        <%= f.email_field :email, :autofocus => true, :value => current_user.unconfirmed_email, class: 'form-control input-lg', placeholder: 'Example: email@me.com' %>
-      </div>
-    </div>
-    <div class="form-group">
-      <%= f.label :name, "Full Name" %>
-      <div class="controls">
-        <%= f.text_field :name, class: 'form-control input-lg', placeholder: 'Jane Doe' %>
-      </div>
-    </div>
-
-    <!-- <div class="form-group">
-      <%= f.label :password %>
-      <div class="controls">
-        <%= f.password_field :password, placeholder: 'At least 8 characters' %>
-        <p class="help-block">Password will be safely stored.</p>
-      </div>
-    </div> -->
-    <div class="actions">
-      <%= f.submit 'Continue', :class => 'btn btn-primary' %>
-    </div>
-  <% end %>
-</div>

app/views/users/index.html.erb

@@ -1,40 +0,0 @@
-<% title params[:user_board].titleize %>
-
-<div class="leaderboard-page">
-  <div class="leaderboard-container">
-    <header>
-      <h1><%=params[:user_board].titleize%></h1>
-    </header>
-    <% @users.each do |user| %>
-      <div class="leaderboard-user">
-        <div class="profile-pic">
-          <a href="<%= user.path %>">
-            <img alt="<%= user.username %> profile image" src="<%= ProfileImage.new(user).get(100)%>" />
-          </a>
-        </div>
-        <div class="content">
-          <a href="<%= user.path %>" class="user-name"><h3><%= user.name %></h3></a>
-          <div>
-            <% if user.twitter_username.present? %>
-              <a href="http://twitter.com/<%= user.twitter_username %>" target="_blank" rel="noopener"><%= icon("twitter","22") %> <%= user.twitter_username %></a>
-            <% end %>
-            <% if user.github_username.present? %>
-              <a href="http://github.com/<%= user.github_username %>" target="_blank" rel="noopener"><%= icon("github","22") %> <%= user.github_username %></a>
-            <% end %>
-            <% if user.website_url.present? %>
-              <a href="<%= user.website_url %>" target="_blank" rel="noopener"><%= icon("link","22") %> <%= beautified_url(user.website_url) %></a>
-            <% end %>
-
-          </div>
-        </div>
-      </div>
-    <% end %>
-  </div>
-  <div class="bottom">
-    <% 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 %>
-  </div>
-</div>

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"

spec/controllers/users_controller_spec.rb

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

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

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!

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
+end

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

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