Merge branch 'fix/405-master' into 'master'

Fix an issue with methods not accessible in some controllers

Backport of !295 (actually I could have opened it against `master` at that time, I was probably confused by https://gitlab.com/gitlab-org/gitlab-ce/issues/14503#note_4386197).

I also cleaned the routing/controllers for audit_events and added
RSpec features specs.

@DouweM I addressed your remarks from https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/295. ;)

See merge request !298

app/controllers/audit_events_controller.rb

-class AuditEventsController < ApplicationController
-  # Authorize
-  before_action :repository, only: :project_log
-  before_action :authorize_admin_project!, only: :project_log
-  before_action :group, only: :group_log
-  before_action :authorize_admin_group!, only: :group_log
-
-  layout :determine_layout
-
-  def project_log
-    @events = AuditEvent.where(entity_type: "Project", entity_id: project.id).page(params[:page]).per(20)
-  end
-
-  def group_log
-    @events = AuditEvent.where(entity_type: "Group", entity_id: group.id).page(params[:page]).per(20)
-  end
-
-  private
-
-  def group
-    @group ||= Group.find_by(path: params[:group_id])
-  end
-
-  def authorize_admin_group!
-    render_404 unless can?(current_user, :admin_group, group)
-  end
-
-  def determine_layout
-    if @project
-      'project_settings'
-    elsif @group
-      'group_settings'
-    end
-  end
-
-  def audit_events_params
-    params.permit(:project_id, :group_id)
-  end
-end

app/controllers/groups/audit_events_controller.rb

+class Groups::AuditEventsController < Groups::ApplicationController
+  before_action :authorize_admin_group!
+
+  layout 'group_settings'
+
+  def index
+    @events = group.audit_events.page(params[:page])
+  end
+end

app/controllers/projects/audit_events_controller.rb

+class Projects::AuditEventsController < Projects::ApplicationController
+  before_action :authorize_admin_project!
+
+  layout 'project_settings'
+
+  def index
+    @events = project.audit_events.page(params[:page])
+  end
+end

app/models/group.rb

@@ -27,6 +27,9 @@ class Group < Namespace
   has_many :shared_projects, through: :project_group_links, source: :project
   has_many :ldap_group_links, foreign_key: 'group_id', dependent: :destroy
   has_many :hooks, dependent: :destroy, class_name: 'GroupHook'
+  # We cannot simply set `has_many :audit_events, as: :entity, dependent: :destroy`
+  # here since Group inherits from Namespace, the entity_type would be set to `Namespace`.
+  has_many :audit_events, -> { where(entity_type: Group) }, dependent: :destroy, foreign_key: 'entity_id'
 
   validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? }
   validates :avatar, file_size: { maximum: 200.kilobytes.to_i }

app/models/project.rb

@@ -164,6 +164,7 @@ class Project < ActiveRecord::Base
   has_many :invited_groups, through: :project_group_links, source: :group
   has_many :pages_domains, dependent: :destroy
   has_many :todos, dependent: :destroy
+  has_many :audit_events, as: :entity, dependent: :destroy
 
   has_one :import_data, dependent: :destroy, class_name: "ProjectImportData"
 

app/views/audit_events/group_log.html.haml → app/views/groups/audit_events/index.html.haml

@@ -2,4 +2,4 @@
 %h3.page-title Group Audit Events
 %p.light Events in #{@group.name}
 
-= render 'event_table', events: @events
+= render 'shared/audit_events/event_table', events: @events

app/views/audit_events/project_log.html.haml → app/views/projects/audit_events/index.html.haml

@@ -2,4 +2,4 @@
 %h3.page-title Project Audit Events
 %p.light Events in #{@project.path_with_namespace}
 
-= render 'event_table', events: @events
+= render 'shared/audit_events/event_table', events: @events

config/routes.rb

@@ -422,10 +422,9 @@ Rails.application.routes.draw do
 
       resource :avatar, only: [:destroy]
       resources :milestones, constraints: { id: /[^\/]+/ }, only: [:index, :show, :update, :new, :create]
+      resources :audit_events, only: [:index]
     end
 
-    get "/audit_events" => "audit_events#group_log"
-
     resources :hooks, only: [:index, :create, :destroy], constraints: { id: /\d+/ }, module: :groups do
       member do
         get :test
@@ -794,10 +793,8 @@ Rails.application.routes.draw do
             get :build, constraints: { format: /svg/ }
           end
         end
+        resources :audit_events, only: [:index]
       end
-
-      get "/audit_events" => "audit_events#project_log"
-
     end
   end
 

features/project/audit_event.feature

-Feature: Audit Event
-  Background:
-    Given I sign in as a user
-    And I own project "Shop"
-
-  Scenario: I add new deploy key
-    Given I created new depoloy key
-    When I visit audit event page
-    Then I see deploy key event
-    When I remove deploy key
-    And I visit audit event page
-    Then I see remove deploy key event
-
-  @javascript
-  Scenario: I should see audit events
-    And gitlab user "Pete"
-    And "Pete" is "Shop" developer
-    When I visit project "Shop" page
-    And I go to "Members"
-    And I change "Pete" access level to master
-    And I visit project "Shop" settings page
-    And I go to "Audit Events"
-    Then I should see the audit event listed

features/steps/groups.rb

@@ -66,37 +66,6 @@ class Spinach::Features::Groups < Spinach::FeatureSteps
     end
   end
 
-  step 'I change the role to "Developer"' do
-    user = User.find_by(name: "Mary Jane")
-    member = Group.find_by(name: "Owned").members.where(user_id: user.id).first
-
-    page.within "#group_member_#{member.id}" do
-      find(".js-toggle-button").click
-      page.within "#edit_group_member_#{member.id}" do
-        select 'Developer', from: 'group_member_access_level'
-        click_on 'Save'
-      end
-    end
-
-    page.within "#group_member_#{member.id}" do
-      expect(page).to have_content "Developer"
-    end
-  end
-
-  step 'I go to "Audit Events"' do
-    find(:link, 'Audit Events').trigger('click')
-  end
-
-  step 'I should see the audit event listed' do
-    page.within('table#audits') do
-      expect(page).to have_content 'Add user access as reporter'
-      expect(page).to have_content 'Change access level from reporter to developer'
-      expect(page).to have_content 'Remove user access'
-      expect(page).to have_content('John Doe', count: 3)
-      expect(page).to have_content('Mary Jane', count: 3)
-    end
-  end
-
   step 'project from group "Owned" has issues assigned to me' do
     create :issue,
       project: project,

features/steps/project/audit_event.rb

-class Spinach::Features::AuditEvent < Spinach::FeatureSteps
-  include SharedAuthentication
-  include SharedProject
-  include SharedPaths
-
-  step 'I created new depoloy key' do
-    visit new_namespace_project_deploy_key_path(@project.namespace, @project)
-
-    fill_in "deploy_key_title", with: "laptop"
-    fill_in "deploy_key_key", with: "ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEAzrEJUIR6Y03TCE9rIJ+GqTBvgb8t1jI9h5UBzCLuK4VawOmkLornPqLDrGbm6tcwM/wBrrLvVOqi2HwmkKEIecVO0a64A4rIYScVsXIniHRS6w5twyn1MD3sIbN+socBDcaldECQa2u1dI3tnNVcs8wi77fiRe7RSxePsJceGoheRQgC8AZ510UdIlO+9rjIHUdVN7LLyz512auAfYsgx1OfablkQ/XJcdEwDNgi9imI6nAXhmoKUm1IPLT2yKajTIC64AjLOnE0YyCh6+7RFMpiMyu1qiOCpdjYwTgBRiciNRZCH8xIedyCoAmiUgkUT40XYHwLuwiPJICpkAzp7Q== user@laptop"
-
-    click_button "Create"
-  end
-
-  step 'I remove deploy key' do
-    visit namespace_project_deploy_keys_path(@project.namespace, @project)
-    click_link "Remove"
-  end
-
-  step 'I see remove deploy key event' do
-    expect(page).to have_content("Remove deploy key")
-  end
-
-  step 'I see deploy key event' do
-    expect(page).to have_content("Add deploy key")
-  end
-
-  step 'I should see the audit event listed' do
-    page.within('table#audits') do
-      expect(page).to have_content "Change access level from developer to master"
-      expect(page).to have_content(project.owner.name)
-      expect(page).to have_content('Pete')
-    end
-  end
-
-  step 'gitlab user "Pete"' do
-    create(:user, name: "Pete")
-  end
-
-  step '"Pete" is "Shop" developer' do
-    user = User.find_by(name: "Pete")
-    project = Project.find_by(name: "Shop")
-    project.team << [user, :developer]
-  end
-
-  step 'I go to "Members"' do
-    find(:link, 'Members').trigger('click')
-  end
-
-  step 'I visit project "Shop" settings page' do
-    find(:link, 'Settings').trigger('click')
-  end
-
-  step 'I change "Pete" access level to master' do
-    user = User.find_by(name: "Pete")
-    project_member = @project.project_members.find_by(user_id: user)
-
-    page.within "#project_member_#{project_member.id}" do
-      click_button "Edit access level"
-      select "Master", from: "project_member_access_level"
-      click_button "Save"
-    end
-
-    sleep 0.05
-  end
-
-  step 'I go to "Audit Events"' do
-    find(:link, 'Audit Events').trigger('click')
-  end
-end

features/steps/project/project.rb

@@ -122,14 +122,6 @@ class Spinach::Features::Project < Spinach::FeatureSteps
     end
   end
 
-  step 'I visit project "Shop" settings page' do
-    click_link 'Settings'
-  end
-
-  step 'I go to "Members"' do
-    click_link 'Members'
-  end
-
   step 'I add project tags' do
     fill_in 'Tags', with: 'tag1, tag2'
   end

features/steps/shared/paths.rb

@@ -79,10 +79,6 @@ module SharedPaths
     visit edit_group_path(Group.find_by(name: "Guest"))
   end
 
-  step 'I visit audit event page' do
-    visit namespace_project_audit_events_path(@project.namespace, @project)
-  end
-
   # ----------------------------------------
   # Dashboard
   # ----------------------------------------

spec/features/groups/audit_events.rb

+require 'spec_helper'
+
+feature 'Groups > Audit Events', js: true, feature: true do
+  let(:user) { create(:user) }
+  let(:pete) { create(:user, name: 'Pete') }
+  let(:group) { create(:group) }
+
+  before do
+    group.add_owner(user)
+    group.add_developer(pete)
+    login_with(user)
+  end
+
+  describe 'changing a user access level' do
+    it "appears in the group's audit events" do
+      visit group_path(group)
+
+      click_link 'Members'
+
+      group_member = group.members.find_by(user_id: pete)
+      page.within "#group_member_#{group_member.id}" do
+        click_button 'Edit access level'
+        select 'Master', from: 'group_member_access_level'
+        click_button 'Save'
+      end
+
+      # This is to avoid a Capybara::Poltergeist::MouseEventFailed error
+      find('a[title=Settings]').trigger('click')
+
+      click_link 'Audit Events'
+
+      page.within('table#audits') do
+        expect(page).to have_content 'Change access level from developer to master'
+        expect(page).to have_content(user.name)
+        expect(page).to have_content('Pete')
+      end
+    end
+  end
+end

spec/features/projects/audit_events.rb

+require 'spec_helper'
+
+feature 'Projects > Audit Events', js: true, feature: true do
+  let(:user) { create(:user) }
+  let(:pete) { create(:user, name: 'Pete') }
+  let(:project) { create(:project, namespace: user.namespace) }
+
+  before do
+    project.team << [user, :master]
+    login_with(user)
+  end
+
+  describe 'adding an SSH key' do
+    it "appears in the project's audit events" do
+      visit new_namespace_project_deploy_key_path(project.namespace, project)
+
+      fill_in 'deploy_key_title', with: 'laptop'
+      fill_in 'deploy_key_key', with: 'ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEAzrEJUIR6Y03TCE9rIJ+GqTBvgb8t1jI9h5UBzCLuK4VawOmkLornPqLDrGbm6tcwM/wBrrLvVOqi2HwmkKEIecVO0a64A4rIYScVsXIniHRS6w5twyn1MD3sIbN+socBDcaldECQa2u1dI3tnNVcs8wi77fiRe7RSxePsJceGoheRQgC8AZ510UdIlO+9rjIHUdVN7LLyz512auAfYsgx1OfablkQ/XJcdEwDNgi9imI6nAXhmoKUm1IPLT2yKajTIC64AjLOnE0YyCh6+7RFMpiMyu1qiOCpdjYwTgBRiciNRZCH8xIedyCoAmiUgkUT40XYHwLuwiPJICpkAzp7Q== user@laptop'
+
+      click_button 'Create'
+
+      visit namespace_project_audit_events_path(project.namespace, project)
+
+      expect(page).to have_content('Add deploy key')
+
+      visit namespace_project_deploy_keys_path(project.namespace, project)
+      click_link 'Remove'
+
+      visit namespace_project_audit_events_path(project.namespace, project)
+
+      expect(page).to have_content('Remove deploy key')
+    end
+  end
+
+  describe 'changing a user access level' do
+    before do
+      project.team << [pete, :developer]
+    end
+
+    it "appears in the project's audit events" do
+      visit namespace_project_path(project.namespace, project)
+
+      click_link 'Members'
+
+      project_member = project.project_member(pete)
+      page.within "#project_member_#{project_member.id}" do
+        click_button 'Edit access level'
+        select 'Master', from: 'project_member_access_level'
+        click_button 'Save'
+      end
+
+      # This is to avoid a Capybara::Poltergeist::MouseEventFailed error
+      find('a[title=Settings]').trigger('click')
+
+      click_link 'Audit Events'
+
+      page.within('table#audits') do
+        expect(page).to have_content 'Change access level from developer to master'
+        expect(page).to have_content(project.owner.name)
+        expect(page).to have_content('Pete')
+      end
+    end
+  end
+end