From 411829fdb5f24f97ce17e05f5fd018d47075b216 Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Fri, 3 Jul 2015 14:54:50 +0300 Subject: [PATCH] Audit log for user authentication --- CHANGELOG | 3 ++- .../omniauth_callbacks_controller.rb | 8 ++++++ app/controllers/profiles_controller.rb | 7 ++++-- app/controllers/sessions_controller.rb | 7 ++++++ app/models/audit_event.rb | 19 ++++++++++++++ app/models/security_event.rb | 2 ++ app/services/audit_event_service.rb | 25 +++++++++++++++++++ app/views/layouts/nav/_profile.html.haml | 6 ++--- app/views/profiles/_event_table.html.haml | 16 ++++++++++++ app/views/profiles/audit_log.html.haml | 5 ++++ app/views/profiles/history.html.haml | 11 -------- config/routes.rb | 2 +- db/migrate/20141118150935_add_audit_event.rb | 22 ++++++++++++++++ db/schema.rb | 18 +++++++++++-- features/profile/active_tab.feature | 6 ++--- features/profile/profile.feature | 2 +- features/steps/profile/active_tab.rb | 4 +-- features/steps/profile/profile.rb | 2 +- features/steps/shared/paths.rb | 4 +-- spec/features/security/profile_access_spec.rb | 4 +-- spec/routing/routing_spec.rb | 4 +-- 21 files changed, 144 insertions(+), 33 deletions(-) create mode 100644 app/models/audit_event.rb create mode 100644 app/models/security_event.rb create mode 100644 app/services/audit_event_service.rb create mode 100644 app/views/profiles/_event_table.html.haml create mode 100644 app/views/profiles/audit_log.html.haml delete mode 100644 app/views/profiles/history.html.haml create mode 100644 db/migrate/20141118150935_add_audit_event.rb diff --git a/CHANGELOG b/CHANGELOG index 616b41a4269..ca193485149 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -28,7 +28,8 @@ v 7.13.0 (unreleased) - Users with guest access level can not set assignee, labels or milestones for issue and merge request - Reporter role can manage issue tracker now: edit any issue, set assignee or milestone and manage labels - Better performance for pages with events list, issues list and commits list - - Faster automerge check and merge itself when source and target branches are in same repository + - Faster automerge check and merge itself when source and target branches are in same repository + - Audit log for user authentication v 7.12.1 - Fix error when deleting a user who has projects (Stan Hu) diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index 765adaf2128..fd51b380da2 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -28,6 +28,7 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController # Do additional LDAP checks for the user filter and EE features if @user.allowed? + log_audit_event(gl_user, with: :ldap) sign_in_and_redirect(gl_user) else flash[:alert] = "Access denied for your LDAP account." @@ -47,6 +48,7 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController if current_user # Add new authentication method current_user.identities.find_or_create_by(extern_uid: oauth['uid'], provider: oauth['provider']) + log_audit_event(current_user, with: oauth['provider']) redirect_to profile_account_path, notice: 'Authentication method updated' else @user = Gitlab::OAuth::User.new(oauth) @@ -54,6 +56,7 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController # Only allow properly saved users to login. if @user.persisted? && @user.valid? + log_audit_event(@user.gl_user, with: oauth['provider']) sign_in_and_redirect(@user.gl_user) else error_message = @@ -83,4 +86,9 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController def oauth @oauth ||= request.env['omniauth.auth'] end + + def log_audit_event(user, options = {}) + AuditEventService.new(user, user, options). + for_authentication.security_event + end end diff --git a/app/controllers/profiles_controller.rb b/app/controllers/profiles_controller.rb index b4af9e490ed..c42b2caf2b8 100644 --- a/app/controllers/profiles_controller.rb +++ b/app/controllers/profiles_controller.rb @@ -37,8 +37,11 @@ class ProfilesController < Profiles::ApplicationController redirect_to profile_account_path end - def history - @events = current_user.recent_events.page(params[:page]).per(PER_PAGE) + def audit_log + @events = AuditEvent.where(entity_type: "User", entity_id: current_user.id). + order("created_at DESC"). + page(params[:page]). + per(PER_PAGE) end def update_username diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 7577fc96d6d..89629bc0581 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -37,6 +37,8 @@ class SessionsController < Devise::SessionsController resource.update_attributes(reset_password_token: nil, reset_password_sent_at: nil) end + authenticated_with = user_params[:otp_attempt] ? "two-factor" : "standard" + log_audit_event(current_user, with: authenticated_with) end end @@ -95,4 +97,9 @@ class SessionsController < Devise::SessionsController user.valid_otp?(user_params[:otp_attempt]) || user.invalidate_otp_backup_code!(user_params[:otp_attempt]) end + + def log_audit_event(user, options = {}) + AuditEventService.new(user, user, options). + for_authentication.security_event + end end diff --git a/app/models/audit_event.rb b/app/models/audit_event.rb new file mode 100644 index 00000000000..967ffd46db0 --- /dev/null +++ b/app/models/audit_event.rb @@ -0,0 +1,19 @@ +class AuditEvent < ActiveRecord::Base + serialize :details, Hash + + belongs_to :user, foreign_key: :author_id + + validates :author_id, presence: true + validates :entity_id, presence: true + validates :entity_type, presence: true + + after_initialize :initialize_details + + def initialize_details + self.details = {} if details.nil? + end + + def author_name + self.user.name + end +end diff --git a/app/models/security_event.rb b/app/models/security_event.rb new file mode 100644 index 00000000000..d131c11cb6c --- /dev/null +++ b/app/models/security_event.rb @@ -0,0 +1,2 @@ +class SecurityEvent < AuditEvent +end diff --git a/app/services/audit_event_service.rb b/app/services/audit_event_service.rb new file mode 100644 index 00000000000..a7f090655e1 --- /dev/null +++ b/app/services/audit_event_service.rb @@ -0,0 +1,25 @@ +class AuditEventService + def initialize(author, entity, details = {}) + @author, @entity, @details = author, entity, details + end + + def for_authentication + @details = { + with: @details[:with], + target_id: @author.id, + target_type: "User", + target_details: @author.name, + } + + self + end + + def security_event + SecurityEvent.create( + author_id: @author.id, + entity_id: @entity.id, + entity_type: @entity.class.name, + details: @details + ) + end +end diff --git a/app/views/layouts/nav/_profile.html.haml b/app/views/layouts/nav/_profile.html.haml index 914e1b83d1f..de5544268a1 100644 --- a/app/views/layouts/nav/_profile.html.haml +++ b/app/views/layouts/nav/_profile.html.haml @@ -44,8 +44,8 @@ = icon('image fw') %span Preferences - = nav_link(path: 'profiles#history') do - = link_to history_profile_path, title: 'History', data: {placement: 'right'} do + = nav_link(path: 'profiles#audit_log') do + = link_to audit_log_profile_path, title: 'Audit Log', data: {placement: 'right'} do = icon('history fw') %span - History + Audit Log diff --git a/app/views/profiles/_event_table.html.haml b/app/views/profiles/_event_table.html.haml new file mode 100644 index 00000000000..c19ac429d52 --- /dev/null +++ b/app/views/profiles/_event_table.html.haml @@ -0,0 +1,16 @@ +%table.table#audits + %thead + %tr + %th Action + %th When + + %tbody + - events.each do |event| + %tr + %td + %span + Signed in with + %b= event.details[:with] + authentication + %td #{time_ago_in_words event.created_at} ago += paginate events, theme: "gitlab" diff --git a/app/views/profiles/audit_log.html.haml b/app/views/profiles/audit_log.html.haml new file mode 100644 index 00000000000..698d6037428 --- /dev/null +++ b/app/views/profiles/audit_log.html.haml @@ -0,0 +1,5 @@ +- page_title "Audit Log" +%h3.page-title Audit Log +%p.light History of authentications + += render 'event_table', events: @events \ No newline at end of file diff --git a/app/views/profiles/history.html.haml b/app/views/profiles/history.html.haml deleted file mode 100644 index b414fb69f4e..00000000000 --- a/app/views/profiles/history.html.haml +++ /dev/null @@ -1,11 +0,0 @@ -- page_title "History" -%h3.page-title - Your Account History -%p.light - All events created by your account are listed below. -%hr -.profile_history - = render @events -%hr -= paginate @events, theme: "gitlab" - diff --git a/config/routes.rb b/config/routes.rb index 33f55dde476..b2ac8dac36a 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -207,7 +207,7 @@ Gitlab::Application.routes.draw do # resource :profile, only: [:show, :update] do member do - get :history + get :audit_log get :applications put :reset_private_token diff --git a/db/migrate/20141118150935_add_audit_event.rb b/db/migrate/20141118150935_add_audit_event.rb new file mode 100644 index 00000000000..07383c6bbc7 --- /dev/null +++ b/db/migrate/20141118150935_add_audit_event.rb @@ -0,0 +1,22 @@ +class AddAuditEvent < ActiveRecord::Migration + def change + create_table :audit_events do |t| + t.integer :author_id, null: false + t.string :type, null: false + + # "Namespace" where the change occurs + # eg. On a project, group or user + t.integer :entity_id, null: false + t.string :entity_type, null: false + + # Details for the event + t.text :details + + t.timestamps + end + + add_index :audit_events, :author_id + add_index :audit_events, :type + add_index :audit_events, [:entity_id, :entity_type] + end +end diff --git a/db/schema.rb b/db/schema.rb index 3a5af6a76d4..8736d1e0df5 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -28,16 +28,30 @@ ActiveRecord::Schema.define(version: 20150620233230) do t.integer "default_branch_protection", default: 2 t.boolean "twitter_sharing_enabled", default: true t.text "restricted_visibility_levels" - t.boolean "version_check_enabled", default: true t.integer "max_attachment_size", default: 10, null: false t.integer "default_project_visibility" t.integer "default_snippet_visibility" t.text "restricted_signup_domains" + t.boolean "version_check_enabled", default: true t.boolean "user_oauth_applications", default: true t.string "after_sign_out_path" t.integer "session_expire_delay", default: 10080, null: false end + create_table "audit_events", force: true do |t| + t.integer "author_id", null: false + t.string "type", null: false + t.integer "entity_id", null: false + t.string "entity_type", null: false + t.text "details" + t.datetime "created_at" + t.datetime "updated_at" + end + + add_index "audit_events", ["author_id"], name: "index_audit_events_on_author_id", using: :btree + add_index "audit_events", ["entity_id", "entity_type"], name: "index_audit_events_on_entity_id_and_entity_type", using: :btree + add_index "audit_events", ["type"], name: "index_audit_events_on_type", using: :btree + create_table "broadcast_messages", force: true do |t| t.text "message", null: false t.datetime "starts_at" @@ -496,12 +510,12 @@ ActiveRecord::Schema.define(version: 20150620233230) do t.string "bitbucket_access_token" t.string "bitbucket_access_token_secret" t.string "location" + t.string "public_email", default: "", null: false t.string "encrypted_otp_secret" t.string "encrypted_otp_secret_iv" t.string "encrypted_otp_secret_salt" t.boolean "otp_required_for_login", default: false, null: false t.text "otp_backup_codes" - t.string "public_email", default: "", null: false t.integer "dashboard", default: 0 end diff --git a/features/profile/active_tab.feature b/features/profile/active_tab.feature index 1fa4ac88ddc..788b7895d72 100644 --- a/features/profile/active_tab.feature +++ b/features/profile/active_tab.feature @@ -23,7 +23,7 @@ Feature: Profile Active Tab Then the active main tab should be Preferences And no other main tabs should be active - Scenario: On Profile History - Given I visit profile history page - Then the active main tab should be History + Scenario: On Profile Audit Log + Given I visit Audit Log page + Then the active main tab should be Audit Log And no other main tabs should be active diff --git a/features/profile/profile.feature b/features/profile/profile.feature index 0dd0afde8b1..7a1345f2b37 100644 --- a/features/profile/profile.feature +++ b/features/profile/profile.feature @@ -63,7 +63,7 @@ Feature: Profile Scenario: I visit history tab Given I have activity - When I visit profile history page + When I visit Audit Log page Then I should see my activity Scenario: I visit my user page diff --git a/features/steps/profile/active_tab.rb b/features/steps/profile/active_tab.rb index 79e3b55f6e1..4724a326277 100644 --- a/features/steps/profile/active_tab.rb +++ b/features/steps/profile/active_tab.rb @@ -19,7 +19,7 @@ class Spinach::Features::ProfileActiveTab < Spinach::FeatureSteps ensure_active_main_tab('Preferences') end - step 'the active main tab should be History' do - ensure_active_main_tab('History') + step 'the active main tab should be Audit Log' do + ensure_active_main_tab('Audit Log') end end diff --git a/features/steps/profile/profile.rb b/features/steps/profile/profile.rb index 11e1163c352..2b6b8b167f6 100644 --- a/features/steps/profile/profile.rb +++ b/features/steps/profile/profile.rb @@ -115,7 +115,7 @@ class Spinach::Features::Profile < Spinach::FeatureSteps end step 'I should see my activity' do - expect(page).to have_content "#{current_user.name} closed issue" + expect(page).to have_content "Signed in with standard authentication" end step 'my password is expired' do diff --git a/features/steps/shared/paths.rb b/features/steps/shared/paths.rb index 4cc01443c8b..fe651e81dac 100644 --- a/features/steps/shared/paths.rb +++ b/features/steps/shared/paths.rb @@ -127,8 +127,8 @@ module SharedPaths visit profile_preferences_path end - step 'I visit profile history page' do - visit history_profile_path + step 'I visit Audit Log page' do + visit audit_log_profile_path end # ---------------------------------------- diff --git a/spec/features/security/profile_access_spec.rb b/spec/features/security/profile_access_spec.rb index 8f7a9606262..bcabc2d53ac 100644 --- a/spec/features/security/profile_access_spec.rb +++ b/spec/features/security/profile_access_spec.rb @@ -45,8 +45,8 @@ describe "Profile access", feature: true do it { is_expected.to be_denied_for :visitor } end - describe "GET /profile/history" do - subject { history_profile_path } + describe "GET /profile/audit_log" do + subject { audit_log_profile_path } it { is_expected.to be_allowed_for @u1 } it { is_expected.to be_allowed_for :admin } diff --git a/spec/routing/routing_spec.rb b/spec/routing/routing_spec.rb index 0fda6202a11..dd045826692 100644 --- a/spec/routing/routing_spec.rb +++ b/spec/routing/routing_spec.rb @@ -108,8 +108,8 @@ describe ProfilesController, "routing" do expect(get("/profile/account")).to route_to('profiles/accounts#show') end - it "to #history" do - expect(get("/profile/history")).to route_to('profiles#history') + it "to #audit_log" do + expect(get("/profile/audit_log")).to route_to('profiles#audit_log') end it "to #reset_private_token" do