Company show: only admins, those in the co. can see an incomplete co (A…

…gileVentures#891)

* company policy show incomplete only to admin, those in Company

* (minor) Co features: Feature name, description

* show 404 error for show if user is not authorized to see it

app/controllers/companies_controller.rb

@@ -8,7 +8,7 @@ class CompaniesController < ApplicationController
   before_action :set_company, only: [:show, :edit, :update, :destroy,
                                      :edit_payment, :fetch_from_dinkurs,
                                      :company_h_brand]
-  before_action :authorize_company, only: [:update, :show, :edit, :destroy]
+  before_action :authorize_company, only: [:update, :edit, :destroy]
   before_action :set_app_config, only: [:company_h_brand]
   before_action :allow_iframe_request, only: [:company_h_brand]
 
@@ -77,15 +77,26 @@ def index
 
 
   def show
-    setup_events_and_events_pagination
-    set_meta_tags_for_company(@company)
-
-    @applications = @company.shf_applications
-                            .includes(:user, :business_categories, :shfapplications_business_categories)
-
-    show_events_list if request.xhr?
+    begin
+      authorize @company
+      setup_events_and_events_pagination
+      set_meta_tags_for_company(@company)
+
+      @applications = @company.shf_applications
+                              .includes(:user, :business_categories, :shfapplications_business_categories)
+
+      show_events_list if request.xhr?
+
+    # If someone is not authorized to view a company, we don't want to let them know that it exists,
+    #   so we want to return a 404, vs. a "You are not authorized to see this" error.
+    #   This is especially important for bots.  We don't want them repeatedly trying to crawl the page.
+    #   TODO probably want to generalize this and make it available to all main classes
+    rescue Pundit::NotAuthorizedError
+      render_company_not_found
+    end
   end
 
+
   def company_h_brand
     render_as = request.format.to_sym
 
@@ -144,7 +155,6 @@ def edit
 
     Ckeditor::Picture.images_category = 'company_' + @company.id.to_s
     Ckeditor::Picture.for_company_id = @company.id
-
   end
 
 
@@ -244,13 +254,7 @@ def set_company
     geocode_if_needed @company
 
   rescue ActiveRecord::RecordNotFound
-    id = params[:id]
-    Rails.logger.info("Company not found. id = #{id}")
-    render 'error_entity_not_found', locals: { entity_type_name: t('activerecord.models.company.one'),
-                                               id: id,
-                                               button_text: t('companies.list_all_companies'),
-                                               button_path: companies_path},
-           status: 404
+    render_company_not_found
   end
 
 
@@ -407,4 +411,15 @@ def set_meta_tags_for_company(company)
   def company_order
     {updated_at: :desc}
   end
+
+
+  def render_company_not_found
+    id = params[:id]
+    Rails.logger.info("Company not found. id = #{id}")
+    render 'error_entity_not_found', locals: { entity_type_name: t('activerecord.models.company.one'),
+                                               id: id,
+                                               button_text: t('companies.list_all_companies'),
+                                               button_path: companies_path},
+           status: 404
+  end
 end

app/policies/company_policy.rb

@@ -1,8 +1,15 @@
 class CompanyPolicy < ApplicationPolicy
   include PoliciesHelper
 
+  # Admin can always see (show) any company
+  # A User in the company can see it
+  #
+  # If the company information is complete, then it can be shown to anyone,
+  #  else (if the information is not complete), it cannot be shown.
   def show?
-    true
+    return true if user.admin? || is_in_company?(record)
+
+    record.information_complete? ? true : false
   end
 
   def index?

features/companies/admin_creates_company.feature

@@ -1,4 +1,6 @@
-Feature: As an admin
+Feature: Admin can create a company
+
+  As an admin
   So that I can view and manage companies
   I need to be able to create them
 
@@ -68,16 +70,23 @@ Feature: As an admin
     Then I should see "No More Snarky Barky"
     And I should see "Bowsers"
 
-  Scenario: User tries to create a company
+  Scenario: User is not allowed to create a company
     Given I am logged in as "applicant_1@happymutts.com"
     And I am on the "create a new company" page
     Then I should see a message telling me I am not allowed to see that page
 
-  Scenario: Visitor tries to create a company
+  Scenario: Visitor is not allowed to create a company
     Given I am Logged out
     And I am on the "create a new company" page
     Then I should see a message telling me I am not allowed to see that page
 
+
+  Scenario: Member is not allowed to create a company
+    Given I am logged in as "member_1@happymutts.com"
+    And I am on the "create a new company" page
+    Then I should see a message telling me I am not allowed to see that page
+
+
   @time_adjust @dinkurs_fetch
   Scenario: Admin creates a company
     Given I am logged in as "admin@shf.se"

features/companies/show-company-complete-info-access.feature

@@ -0,0 +1,123 @@
+Feature: Only show companies with incomplete info to admins and users or members of the company.
+
+  So that visitors and users and members that are not a member of a company
+  do not see a company until all of the necessary information for displaying it is "complete,"
+  only show companies with incomplete information to admins and those that are part of the company.
+  All others should get a 404 message.
+  They should not get a 'you are not authorized' message because we don't want to give
+  any hints that the company does or does not exist.  This is especially important for bots;
+  if they infer that a company (page) does exist, they may try to continue to access it and/or
+  continue to try even more malicious things.
+
+  PivotalTracker: https://www.pivotaltracker.com/story/show/177274707
+
+
+  Background:
+
+    Given the date is set to "2019-06-06"
+    Given the Membership Ethical Guidelines Master Checklist exists
+
+    Given the following regions exist:
+      | name      |
+      | Stockholm |
+
+    Given the following kommuns exist:
+      | name     |
+      | Alingsås |
+
+
+    Given the following companies exist:
+      | name     | company_number | email                               | region    | kommun   | city      | visibility     |
+      |          | 5560360793     | hello@incomplete-info-company-1.com | Stockholm | Alingsås | Harplinge | street_address |
+      | Company2 | 2120000142     | hello@company-2.com                 | Stockholm | Alingsås | Harplinge | street_address |
+
+
+    And the following users exist:
+      | email                            | admin | member |
+      | member-1@addr-all-visible-1.com  |       | true   |
+      | member@company-2.com             |       | true   |
+      | applicant-6@addr-not-visible.com |       | false  |
+      | member-company6@example.com      |       | true   |
+      | member-2@addr-all-visible-1.com  |       | true   |
+      | admin@shf.se                     | true  | false  |
+
+
+    And the following business categories exist
+      | name    |
+      | Groomer |
+
+
+    And the following applications exist:
+      | user_email                      | company_number | categories          | state    |
+      | member-1@addr-all-visible-1.com | 5560360793     | Groomer | accepted |
+      | member@company-2.com            | 2120000142     | Groomer             | accepted |
+      | member-company6@example.com     | 6914762726     | Groomer             | accepted |
+
+    And the following payments exist
+      | user_email                      | start_date | expire_date | payment_type | status | hips_id | company_number |
+      | member-1@addr-all-visible-1.com | 2019-01-01 | 2019-12-31  | member_fee   | betald | none    |                |
+      | member@company-2.com            | 2019-10-1  | 2019-12-31  | member_fee   | betald | none    |                |
+      | member-company6@example.com     | 2019-10-1  | 2019-12-31  | member_fee   | betald | none    |                |
+      | member-2@addr-all-visible-1.com | 2019-01-01 | 2019-12-31  | member_fee   | betald | none    |                |
+      | member-2@addr-all-visible-1.com | 2017-01-01 | 2017-12-31  | branding_fee | betald | none    | 5560360793     |
+      | member@company-2.com            | 2017-01-01 | 2017-12-31  | branding_fee | betald | none    | 2120000142     |
+      | member-company6@example.com     | 2017-01-01 | 2017-12-31  | branding_fee | betald | none    | 6914762726     |
+
+    # -----------------------------------
+  Scenario: Visitor cannot see a company with incomplete info; gets a 404 error.
+    Given I am Logged out
+    When I am the page for company number "5560360793"
+    Then I should not see "5560360793"
+    And I should not see "hello@incomplete-info-company-1.com"
+
+    # The actual entity type will be passed in and shown, but it's beyond the current step to fill it in with a nested t("").
+    #  As long as the error message is being displayed, it is working as it should.
+    And I should see t("activerecord.errors.messages.record_not_found.header", entity_type: '')
+
+
+  Scenario: User that is not a part of the company  cannot see a company with incomplete info; gets a 404 error.
+    Given I am logged in as "applicant-6@addr-not-visible.com"
+    When I am the page for company number "5560360793"
+    Then I should not see "5560360793"
+    And I should not see "hello@incomplete-info-company-1.com"
+
+    # The actual entity type will be passed in and shown, but it's beyond the current step to fill it in with a nested t("").
+    #  As long as the error message is being displayed, it is working as it should.
+    And I should see t("activerecord.errors.messages.record_not_found.header", entity_type: '')
+
+
+  Scenario: Member that is not part of the company cannot see a company with incomplete info; gets a 404 error.
+    Given I am logged in as "member-company6@example.com"
+    And I am the page for company number "5560360793"
+    Then I should not see "5560360793"
+    And I should not see "hello@incomplete-info-company-1.com"
+
+    # The actual entity type will be passed in and shown, but it's beyond the current step to fill it in with a nested t("").
+    #  As long as the error message is being displayed, it is working as it should.
+    And I should see t("activerecord.errors.messages.record_not_found.header", entity_type: '')
+
+
+  Scenario: Member that is part of the company can see a company with incomplete info.
+    Given I am logged in as "member-1@addr-all-visible-1.com"
+    When I am the page for company number "5560360793"
+    Then I should see t("companies.show.this_info_missing")
+    And I should see "Groomer"
+    And I should see "hello@incomplete-info-company-1.com"
+    And I should see "123123123"
+    And I should see "Hundforetagarevägen 1"
+    And I should see "310 40"
+    And I should see "Harplinge"
+    And I should see "http://www.example.com"
+
+
+  Scenario: Admin can see all incomplete companies.
+    Given I am logged in as "admin@shf.se"
+    When I am the page for company number "5560360793"
+    Then I should see t("companies.show.this_info_missing")
+    And I should see "Groomer"
+    And I should see "hello@incomplete-info-company-1.com"
+    And I should see "123123123"
+    And I should see "Hundforetagarevägen 1"
+    And I should see "310 40"
+    And I should see "Harplinge"
+    And I should see "http://www.example.com"

features/companies/show_only_companies_in_good_standing.feature

@@ -8,7 +8,10 @@ Feature: Visitors see only companies in good standing with complete information
   And I only want to see companies with all of the information needed
     so that I can contact them,
 
-  Visitors should only see companies in good standing with complete information.
+  Visitors and users and members should only see companies in good standing with complete information.
+
+  Members and users that are part of a company can view and edit a company that is not in good standing,
+  but will not see the company in the list of all companies.
 
 
   Background:

spec/policies/company_policy_spec.rb

@@ -8,6 +8,7 @@
   let(:admin)  { create(:user, email: 'admin@sfh.com', admin: true) }
   let(:visitor) { build(:visitor) }
   let(:company) { create(:company, company_number: '5712213304')}
+  let(:co_incomplete_info) { create(:company, name: '') }
 
   describe 'For admin' do
     subject { described_class.new(admin, company) }
@@ -38,38 +39,82 @@
   end
 
   describe 'For a member that is not part of a company' do
-    subject { described_class.new(member, company) }
+    context 'company information IS complete' do
+      subject { described_class.new(member, company) }
 
-    it { is_expected.to permit_action :index }
-    it { is_expected.to permit_action :show }
-    it { is_expected.to forbid_action :edit }
-    it { is_expected.to forbid_action :update }
-    it { is_expected.to forbid_action :new }
-    it { is_expected.to permit_action :create }
-    it { is_expected.to forbid_action :edit_payment }
+      it { is_expected.to permit_action :index }
+      it { is_expected.to permit_action :show }
+      it { is_expected.to forbid_action :edit }
+      it { is_expected.to forbid_action :update }
+      it { is_expected.to forbid_action :new }
+      it { is_expected.to permit_action :create }
+      it { is_expected.to forbid_action :edit_payment }
+    end
+
+    context 'company information is NOT complete' do
+      subject { described_class.new(member, co_incomplete_info) }
+
+      it { is_expected.to permit_action :index }
+      it { is_expected.to forbid_action :show }
+      it { is_expected.to forbid_action :edit }
+      it { is_expected.to forbid_action :update }
+      it { is_expected.to forbid_action :new }
+      it { is_expected.to permit_action :create }
+      it { is_expected.to forbid_action :edit_payment }
+    end
   end
 
   describe 'For a user (logged in)' do
-    subject { described_class.new(user_1, company) }
 
-    it { is_expected.to permit_action :index }
-    it { is_expected.to permit_action :show }
-    it { is_expected.to forbid_action :edit }
-    it { is_expected.to forbid_action :update }
-    it { is_expected.to forbid_action :new }
-    it { is_expected.to permit_action :create }
-    it { is_expected.to forbid_action :edit_payment }
+    context 'company information IS complete' do
+      subject { described_class.new(user_1, company) }
+
+      it { is_expected.to permit_action :index }
+      it { is_expected.to permit_action :show }
+      it { is_expected.to forbid_action :edit }
+      it { is_expected.to forbid_action :update }
+      it { is_expected.to forbid_action :new }
+      it { is_expected.to permit_action :create }
+      it { is_expected.to forbid_action :edit_payment }
+    end
+
+    context 'company information is NOT complete' do
+      subject { described_class.new(user_1, co_incomplete_info) }
+
+      it { is_expected.to permit_action :index }
+      it { is_expected.to forbid_action :show }
+      it { is_expected.to forbid_action :edit }
+      it { is_expected.to forbid_action :update }
+      it { is_expected.to forbid_action :new }
+      it { is_expected.to permit_action :create }
+      it { is_expected.to forbid_action :edit_payment }
+    end
   end
 
   describe 'For a visitor (not logged in)' do
-    subject { described_class.new(visitor, company) }
 
-    it { is_expected.to permit_action :index }
-    it { is_expected.to permit_action :show }
-    it { is_expected.to forbid_action :edit }
-    it { is_expected.to forbid_action :update }
-    it { is_expected.to forbid_action :new }
-    it { is_expected.to forbid_action :create }
-    it { is_expected.to forbid_action :edit_payment }
+    context 'company information IS complete' do
+      subject { described_class.new(visitor, company) }
+
+      it { is_expected.to permit_action :index }
+      it { is_expected.to permit_action :show }
+      it { is_expected.to forbid_action :edit }
+      it { is_expected.to forbid_action :update }
+      it { is_expected.to forbid_action :new }
+      it { is_expected.to forbid_action :create }
+      it { is_expected.to forbid_action :edit_payment }
+    end
+
+    context 'company information is NOT complete' do
+      subject { described_class.new(visitor, co_incomplete_info) }
+
+      it { is_expected.to permit_action :index }
+      it { is_expected.to forbid_action :show }
+      it { is_expected.to forbid_action :edit }
+      it { is_expected.to forbid_action :update }
+      it { is_expected.to forbid_action :new }
+      it { is_expected.to forbid_action :create }
+      it { is_expected.to forbid_action :edit_payment }
+    end
   end
 end