From 6a3767cde738863dff6d436d019ae06be93438d3 Mon Sep 17 00:00:00 2001 From: Bianca Nenciu Date: Thu, 25 Oct 2018 12:45:31 +0300 Subject: [PATCH] FEATURE: Warn users via email about suspicious logins. (#6520) * FEATURE: Warn users via email about suspicious logins. * DEV: Move suspicious login check to a job. --- .../admin/email_templates_controller.rb | 1 + app/jobs/regular/suspicious_login.rb | 17 ++++++++++ app/jobs/regular/user_email.rb | 5 +++ app/mailers/user_notifications.rb | 21 ++++++++++++ app/models/user_auth_token.rb | 34 +++++++++++++++++++ config/locales/server.en.yml | 18 ++++++++++ config/site_settings.yml | 2 +- lib/discourse_ip_info.rb | 1 + spec/jobs/suspicious_login_spec.rb | 33 ++++++++++++++++++ spec/models/user_auth_token_spec.rb | 1 + 10 files changed, 132 insertions(+), 1 deletion(-) create mode 100644 app/jobs/regular/suspicious_login.rb create mode 100644 spec/jobs/suspicious_login_spec.rb diff --git a/app/controllers/admin/email_templates_controller.rb b/app/controllers/admin/email_templates_controller.rb index 9a008f48a70..271f3413617 100644 --- a/app/controllers/admin/email_templates_controller.rb +++ b/app/controllers/admin/email_templates_controller.rb @@ -51,6 +51,7 @@ class Admin::EmailTemplatesController < Admin::AdminController "user_notifications.set_password", "user_notifications.signup", "user_notifications.signup_after_approval", + "user_notifications.suspicious_login", "user_notifications.user_invited_to_private_message_pm", "user_notifications.user_invited_to_private_message_pm_group", "user_notifications.user_invited_to_topic", diff --git a/app/jobs/regular/suspicious_login.rb b/app/jobs/regular/suspicious_login.rb new file mode 100644 index 00000000000..afeff845be6 --- /dev/null +++ b/app/jobs/regular/suspicious_login.rb @@ -0,0 +1,17 @@ +module Jobs + + class SuspiciousLogin < Jobs::Base + + def execute(args) + if UserAuthToken.is_suspicious(args[:user_id], args[:client_ip]) + Jobs.enqueue(:critical_user_email, + type: :suspicious_login, + user_id: args[:user_id], + client_ip: args[:client_ip], + user_agent: args[:user_agent]) + end + end + + end + +end diff --git a/app/jobs/regular/user_email.rb b/app/jobs/regular/user_email.rb index cfe8bf96339..4b0ae6b1f50 100644 --- a/app/jobs/regular/user_email.rb +++ b/app/jobs/regular/user_email.rb @@ -146,6 +146,11 @@ module Jobs email_args[:email_token] = email_token if email_token.present? email_args[:new_email] = user.email if type.to_s == "notify_old_email" + if args[:client_ip] && args[:user_agent] + email_args[:client_ip] = args[:client_ip] + email_args[:user_agent] = args[:user_agent] + end + if EmailLog.reached_max_emails?(user, type.to_s) return skip_message(SkippedEmailLog.reason_types[:exceeded_emails_limit]) end diff --git a/app/mailers/user_notifications.rb b/app/mailers/user_notifications.rb index 76be39f1dee..fec948e8be0 100644 --- a/app/mailers/user_notifications.rb +++ b/app/mailers/user_notifications.rb @@ -2,6 +2,8 @@ require_dependency 'markdown_linker' require_dependency 'email/message_builder' require_dependency 'age_words' require_dependency 'rtl' +require_dependency 'discourse_ip_info' +require_dependency 'browser_detection' class UserNotifications < ActionMailer::Base include UserNotificationsHelper @@ -31,6 +33,25 @@ class UserNotifications < ActionMailer::Base new_user_tips: tips) end + def suspicious_login(user, opts = {}) + ipinfo = DiscourseIpInfo.get(opts[:client_ip]) + location = [ipinfo[:city], ipinfo[:region], ipinfo[:country]].reject(&:blank?).join(", ") + browser = BrowserDetection.browser(opts[:user_agent]) + device = BrowserDetection.device(opts[:user_agent]) + os = BrowserDetection.os(opts[:user_agent]) + + build_email( + user.email, + template: "user_notifications.suspicious_login", + locale: user_locale(user), + client_ip: opts[:client_ip], + location: location.present? ? location : I18n.t('staff_action_logs.unknown'), + browser: I18n.t("user_auth_tokens.browser.#{browser}"), + device: I18n.t("user_auth_tokens.device.#{device}"), + os: I18n.t("user_auth_tokens.os.#{os}") + ) + end + def notify_old_email(user, opts = {}) build_email(user.email, template: "user_notifications.notify_old_email", diff --git a/app/models/user_auth_token.rb b/app/models/user_auth_token.rb index 3f6ca49acd2..2c066b4255d 100644 --- a/app/models/user_auth_token.rb +++ b/app/models/user_auth_token.rb @@ -30,6 +30,35 @@ class UserAuthToken < ActiveRecord::Base end end + # Returns the login location as it will be used by the the system to detect + # suspicious login. + # + # This should not be very specific because small variations in location + # (i.e. changes of network, small trips, etc) will be detected as suspicious + # logins. + # + # On the other hand, if this is too broad it will not report any suspicious + # logins at all. + # + # For example, let's choose the country as the only component in login + # locations. In general, this should be a pretty good choce with the + # exception that for users from huge countries it might not be specific + # enoguh. For US users where the real user and the malicious one could + # happen to live both in USA, this will not detect any suspicious activity. + def self.login_location(ip) + DiscourseIpInfo.get(ip)[:country] + end + + def self.is_suspicious(user_id, user_ip) + ips = UserAuthTokenLog.where(user_id: user_id).pluck(:client_ip) + ips.delete_at(ips.index(user_ip) || ips.length) # delete one occurance (current) + ips.uniq! + return false if ips.empty? # first login is never suspicious + + user_location = login_location(user_ip) + ips.none? { |ip| user_location == login_location(ip) } + end + def self.generate!(info) token = SecureRandom.hex(16) hashed_token = hash_token(token) @@ -51,6 +80,11 @@ class UserAuthToken < ActiveRecord::Base path: info[:path], auth_token: hashed_token) + Jobs.enqueue(:suspicious_login, + user_id: info[:user_id], + client_ip: info[:client_ip], + user_agent: info[:user_agent]) + user_auth_token end diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 9b9c0abce7f..b592e968614 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -3225,6 +3225,24 @@ en: If the above link is not clickable, try copying and pasting it into the address bar of your web browser. + suspicious_login: + title: "Suspicious Login Activity Detected" + subject_template: "Suspicious Login Activity Detected" + text_body_template: | + Hello, + + Suspicious login activity has been detected on your account. Check the details below for more information about this login. + + - IP: %{client_ip} + - Location: %{location} + - Browser: %{browser} + - Device: %{device} + - Operating System: %{os} + + If you do not recognize this information, please reset your password to prevent future attacks on your account. + + To strengthen your account's security, please enable second-factor authentication. + page_not_found: title: "Oops! That page doesn’t exist or is private." popular_topics: "Popular" diff --git a/config/site_settings.yml b/config/site_settings.yml index 1f22e1f1799..421eacd67d6 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -334,7 +334,7 @@ login: verbose_sso_logging: false verbose_auth_token_logging: hidden: true - default: false + default: true sso_url: default: '' regex: '^https?:\/\/.+[^\/]$' diff --git a/lib/discourse_ip_info.rb b/lib/discourse_ip_info.rb index 2f627f4c8f4..3623c772559 100644 --- a/lib/discourse_ip_info.rb +++ b/lib/discourse_ip_info.rb @@ -37,6 +37,7 @@ class DiscourseIpInfo def get(ip) return {} unless @mmdb + ip = ip.to_s @cache[ip] ||= lookup(ip) end diff --git a/spec/jobs/suspicious_login_spec.rb b/spec/jobs/suspicious_login_spec.rb new file mode 100644 index 00000000000..836bac3d185 --- /dev/null +++ b/spec/jobs/suspicious_login_spec.rb @@ -0,0 +1,33 @@ +require 'rails_helper' + +describe Jobs::SuspiciousLogin do + + let(:user) { Fabricate(:user) } + + before do + UserAuthToken.stubs(:login_location).with("1.1.1.1").returns("Location 1") + UserAuthToken.stubs(:login_location).with("1.1.1.2").returns("Location 1") + UserAuthToken.stubs(:login_location).with("1.1.2.1").returns("Location 2") + end + + it "will not send an email on first login" do + Jobs.expects(:enqueue).with(:critical_user_email, has_entries(type: :suspicious_login)).never + described_class.new.execute(user_id: user.id, client_ip: "1.1.1.1") + end + + it "will not send an email when user log in from a known location" do + UserAuthTokenLog.create!(action: "generate", user_id: user.id, client_ip: "1.1.1.1") + + Jobs.expects(:enqueue).with(:critical_user_email, has_entries(type: :suspicious_login)).never + described_class.new.execute(user_id: user.id, client_ip: "1.1.1.1") + described_class.new.execute(user_id: user.id, client_ip: "1.1.1.2") + end + + it "will send an email when user logs in from a new location" do + UserAuthTokenLog.create!(action: "generate", user_id: user.id, client_ip: "1.1.1.1") + + Jobs.expects(:enqueue).with(:critical_user_email, has_entries(type: :suspicious_login)) + described_class.new.execute(user_id: user.id, client_ip: "1.1.2.1") + end + +end diff --git a/spec/models/user_auth_token_spec.rb b/spec/models/user_auth_token_spec.rb index dce8eec3248..f3a989a66b5 100644 --- a/spec/models/user_auth_token_spec.rb +++ b/spec/models/user_auth_token_spec.rb @@ -1,4 +1,5 @@ require 'rails_helper' +require 'discourse_ip_info' describe UserAuthToken do