From 60d15b77e606931def8d0b1f6cd2a9ceb74f632c Mon Sep 17 00:00:00 2001 From: Richard Frank Date: Mon, 28 Mar 2016 12:38:37 -0600 Subject: [PATCH 1/8] Uses the session.id as an anonymous id for correlating client and server segment identifies --- app/controllers/application_controller.rb | 1 + app/views/shared/_analytics.html.erb | 3 ++- vendor/engines/saas/app/jobs/analytics/identify_user_job.rb | 1 + vendor/engines/saas/app/jobs/analytics/page_job.rb | 1 + vendor/engines/saas/app/jobs/base_login_job.rb | 3 ++- vendor/engines/saas/lib/saas/engine.rb | 3 ++- 6 files changed, 9 insertions(+), 3 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index fb92c82e..7640b948 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -39,6 +39,7 @@ def queue_job job_params['current_user'] = (current_user.attribs || {}).to_h job_params['action_controller.params'] = params + job_params['session_id'] = session.id #TODO: make sure you can safely serialize the params JobResolver.find_jobs(params).each do |job| diff --git a/app/views/shared/_analytics.html.erb b/app/views/shared/_analytics.html.erb index 7712ba33..d884f9ad 100644 --- a/app/views/shared/_analytics.html.erb +++ b/app/views/shared/_analytics.html.erb @@ -6,7 +6,8 @@ window.analytics.identify('<%= current_user.id %>', {}) window.analytics.page({ userId: '<% current_user.id %>' }); <% else %> - window.analytics.page(); + window.analytics.identify({ anonymousId: '<%= session.id %>' }); + window.analytics.page({ anonymousId: '<%= session.id %>' }); <% end %> <% end %> diff --git a/vendor/engines/saas/app/jobs/analytics/identify_user_job.rb b/vendor/engines/saas/app/jobs/analytics/identify_user_job.rb index ac80be6c..248ca643 100644 --- a/vendor/engines/saas/app/jobs/analytics/identify_user_job.rb +++ b/vendor/engines/saas/app/jobs/analytics/identify_user_job.rb @@ -5,6 +5,7 @@ class IdentifyUserJob < AnalyticsJob def payload(params) { user_id: params['current_user']['id'] || "Anonymous", + anonymous_id: params['session_id'], traits: params['data'] } end diff --git a/vendor/engines/saas/app/jobs/analytics/page_job.rb b/vendor/engines/saas/app/jobs/analytics/page_job.rb index 87cc4b6a..c14f609b 100644 --- a/vendor/engines/saas/app/jobs/analytics/page_job.rb +++ b/vendor/engines/saas/app/jobs/analytics/page_job.rb @@ -6,6 +6,7 @@ def payload(params) user = params['current_user'] ? params['current_user']['id'] : "Anonymous" { user_id: user, + anonymous_id: params['session_id'], name: params['url'], properties: { url: params['url'] } } diff --git a/vendor/engines/saas/app/jobs/base_login_job.rb b/vendor/engines/saas/app/jobs/base_login_job.rb index fec4c429..74ac4c90 100644 --- a/vendor/engines/saas/app/jobs/base_login_job.rb +++ b/vendor/engines/saas/app/jobs/base_login_job.rb @@ -32,7 +32,8 @@ def map_user(params) { 'current_user' => params['user'], - 'data' => params['user'] + 'data' => params['user'], + 'session_id' => params['session_id'] } end end diff --git a/vendor/engines/saas/lib/saas/engine.rb b/vendor/engines/saas/lib/saas/engine.rb index d21fcef9..7df432a3 100644 --- a/vendor/engines/saas/lib/saas/engine.rb +++ b/vendor/engines/saas/lib/saas/engine.rb @@ -26,7 +26,8 @@ def page_job auth_request = request.params['code'] && request.params['state'] if request.referer !~ /github\.com/ && !logged_in? && !auth_request Analytics::PageJob.perform_later({ - 'url' => "/login/#{params['action']}" + 'url' => "/login/#{params['action']}", + 'session_id' => request.session.id }) end end From 86b9da0e45f8c357a80035066ad6e1f4b4a73741 Mon Sep 17 00:00:00 2001 From: Richard Frank Date: Mon, 28 Mar 2016 15:26:05 -0600 Subject: [PATCH 2/8] Tracks anonymousId on logged in payloads also --- app/views/shared/_analytics.html.erb | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/app/views/shared/_analytics.html.erb b/app/views/shared/_analytics.html.erb index d884f9ad..34c7e180 100644 --- a/app/views/shared/_analytics.html.erb +++ b/app/views/shared/_analytics.html.erb @@ -3,8 +3,13 @@ window.analytics=window.analytics||[],window.analytics.methods=["identify","group","track","page","pageview","alias","ready","on","once","off","trackLink","trackForm","trackClick","trackSubmit"],window.analytics.factory=function(t){return function(){var a=Array.prototype.slice.call(arguments);return a.unshift(t),window.analytics.push(a),window.analytics}};for(var i=0;i"); <% if logged_in? %> - window.analytics.identify('<%= current_user.id %>', {}) - window.analytics.page({ userId: '<% current_user.id %>' }); + window.analytics.identify('<%= current_user.id %>', { + anonymousId: '<%= session.id %>' + }) + window.analytics.page({ + userId: '<%= current_user.id %>', + anonymousId: '<%= session.id %>' + }); <% else %> window.analytics.identify({ anonymousId: '<%= session.id %>' }); window.analytics.page({ anonymousId: '<%= session.id %>' }); From d8e444d9e2302dde7392545bd959354770109013 Mon Sep 17 00:00:00 2001 From: Richard Frank Date: Mon, 28 Mar 2016 15:32:35 -0600 Subject: [PATCH 3/8] Overrides the Warden::Proxy.set_user method to removing a line that forced session id renewal on login --- config/initializers/warden.rb | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/config/initializers/warden.rb b/config/initializers/warden.rb index 2db9a758..708962a2 100644 --- a/config/initializers/warden.rb +++ b/config/initializers/warden.rb @@ -75,4 +75,26 @@ def call(env) end end +Warden::Proxy.class_eval do + ENV_SESSION_OPTIONS = 'rack.session.options'.freeze + def set_user(user, opts = {}) + scope = (opts[:scope] ||= @config.default_scope) + + # Get the default options from the master configuration for the given scope + opts = (@config[:scope_defaults][scope] || {}).merge(opts) + opts[:event] ||= :set_user + @users[scope] = user + + if opts[:store] != false && opts[:event] != :fetch + options = env[ENV_SESSION_OPTIONS] + session_serializer.store(user, scope) + end + + run_callbacks = opts.fetch(:run_callbacks, true) + manager._run_callbacks(:after_set_user, user, self, opts) if run_callbacks + + @users[scope] + end +end + Rails.application.middleware.insert_before Warden::Manager, Huboard::Warden::AppRedirect From 53367cb254ecdca26f6322c05b4df0cbe3cd6659 Mon Sep 17 00:00:00 2001 From: Richard Frank Date: Wed, 30 Mar 2016 12:23:34 -0600 Subject: [PATCH 4/8] Removes Warden method override as makes us vulernable to session fixation attacks, sets a guid per session as anon id via rack middleware --- app/controllers/application_controller.rb | 2 +- app/views/shared/_analytics.html.erb | 14 +++++++------- config/application.rb | 2 ++ config/initializers/warden.rb | 22 ---------------------- lib/hu_board/middleware.rb | 1 + lib/hu_board/middleware/session.rb | 17 +++++++++++++++++ vendor/engines/saas/lib/saas/engine.rb | 2 +- 7 files changed, 29 insertions(+), 31 deletions(-) create mode 100644 lib/hu_board/middleware.rb create mode 100644 lib/hu_board/middleware/session.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 7640b948..9c5d54cf 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -39,7 +39,7 @@ def queue_job job_params['current_user'] = (current_user.attribs || {}).to_h job_params['action_controller.params'] = params - job_params['session_id'] = session.id + job_params['session_id'] = session['guid'] #TODO: make sure you can safely serialize the params JobResolver.find_jobs(params).each do |job| diff --git a/app/views/shared/_analytics.html.erb b/app/views/shared/_analytics.html.erb index 34c7e180..b078f92c 100644 --- a/app/views/shared/_analytics.html.erb +++ b/app/views/shared/_analytics.html.erb @@ -1,18 +1,18 @@ diff --git a/config/application.rb b/config/application.rb index a9d184c6..a65b210a 100644 --- a/config/application.rb +++ b/config/application.rb @@ -10,6 +10,7 @@ Bundler.require(*Rails.groups) require File.expand_path('../../lib/core_extensions/string', __FILE__) +require File.expand_path('../../lib/hu_board/middleware', __FILE__) Octokit.api_endpoint = ENV["GITHUB_API_ENDPOINT"] if ENV["GITHUB_API_ENDPOINT"] Octokit.web_endpoint = ENV["GITHUB_WEB_ENDPOINT"] if ENV["GITHUB_WEB_ENDPOINT"] @@ -79,6 +80,7 @@ class Application < Rails::Application config.middleware.use Rack::Attack config.middleware.use PDFKit::Middleware, {print_media_type: true}, only: %r[^/settings] + config.middleware.use HuBoard::Middleware::Session # !!! This addresses a Rails Behaviour that coaxes empty arrays in the params hash into nils # see https://github.com/rails/rails/pull/13188 diff --git a/config/initializers/warden.rb b/config/initializers/warden.rb index 708962a2..2db9a758 100644 --- a/config/initializers/warden.rb +++ b/config/initializers/warden.rb @@ -75,26 +75,4 @@ def call(env) end end -Warden::Proxy.class_eval do - ENV_SESSION_OPTIONS = 'rack.session.options'.freeze - def set_user(user, opts = {}) - scope = (opts[:scope] ||= @config.default_scope) - - # Get the default options from the master configuration for the given scope - opts = (@config[:scope_defaults][scope] || {}).merge(opts) - opts[:event] ||= :set_user - @users[scope] = user - - if opts[:store] != false && opts[:event] != :fetch - options = env[ENV_SESSION_OPTIONS] - session_serializer.store(user, scope) - end - - run_callbacks = opts.fetch(:run_callbacks, true) - manager._run_callbacks(:after_set_user, user, self, opts) if run_callbacks - - @users[scope] - end -end - Rails.application.middleware.insert_before Warden::Manager, Huboard::Warden::AppRedirect diff --git a/lib/hu_board/middleware.rb b/lib/hu_board/middleware.rb new file mode 100644 index 00000000..2111f0fb --- /dev/null +++ b/lib/hu_board/middleware.rb @@ -0,0 +1 @@ +require_relative 'middleware/session' diff --git a/lib/hu_board/middleware/session.rb b/lib/hu_board/middleware/session.rb new file mode 100644 index 00000000..15866017 --- /dev/null +++ b/lib/hu_board/middleware/session.rb @@ -0,0 +1,17 @@ +module HuBoard + module Middleware + class Session + def initialize(app) + @app = app + end + + def call(env) + if env['rack.session']['guid'].nil? + env['rack.session']['guid'] = SecureRandom.uuid + end + + @app.call(env) + end + end + end +end diff --git a/vendor/engines/saas/lib/saas/engine.rb b/vendor/engines/saas/lib/saas/engine.rb index 7df432a3..3a162ace 100644 --- a/vendor/engines/saas/lib/saas/engine.rb +++ b/vendor/engines/saas/lib/saas/engine.rb @@ -27,7 +27,7 @@ def page_job if request.referer !~ /github\.com/ && !logged_in? && !auth_request Analytics::PageJob.perform_later({ 'url' => "/login/#{params['action']}", - 'session_id' => request.session.id + 'session_id' => request.session['guid'] }) end end From f7905a55dab486ef073d718c22e31f24384c808f Mon Sep 17 00:00:00 2001 From: Keith Dahlby Date: Thu, 31 Mar 2016 01:15:55 -0500 Subject: [PATCH 5/8] Try different approach to set anonymousId client-side https://segment.com/docs/libraries/analytics.js/#setting-the-anonymous-id --- app/views/shared/_analytics.html.erb | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/app/views/shared/_analytics.html.erb b/app/views/shared/_analytics.html.erb index b078f92c..fe7e41d5 100644 --- a/app/views/shared/_analytics.html.erb +++ b/app/views/shared/_analytics.html.erb @@ -2,17 +2,15 @@ <% if ENV['SEGMENTIO_KEY'] %> window.analytics=window.analytics||[],window.analytics.methods=["identify","group","track","page","pageview","alias","ready","on","once","off","trackLink","trackForm","trackClick","trackSubmit"],window.analytics.factory=function(t){return function(){var a=Array.prototype.slice.call(arguments);return a.unshift(t),window.analytics.push(a),window.analytics}};for(var i=0;i"); + window.analytics.user().anonymousId("<%= session['guid'] %>"); <% if logged_in? %> - window.analytics.identify("<%= current_user.id %>", { - anonymousId: "<%= session['guid'] %>" - }) + window.analytics.identify("<%= current_user.id %>") window.analytics.page({ - userId: "<%= current_user.id %>", - anonymousId: "<%= session['guid'] %>" + userId: "<%= current_user.id %>" }); <% else %> - window.analytics.identify({ anonymousId: "<%= session['guid'] %>" }); - window.analytics.page({ anonymousId: "<%= session['guid'] %>" }); + window.analytics.identify(); + window.analytics.page(); <% end %> <% end %> From 9b0fee9042bc0afbe2394fb43f39711161034094 Mon Sep 17 00:00:00 2001 From: Keith Dahlby Date: Thu, 31 Mar 2016 01:26:10 -0500 Subject: [PATCH 6/8] Revert "Try different approach to set anonymousId client-side" This reverts commit f7905a55dab486ef073d718c22e31f24384c808f. --- app/views/shared/_analytics.html.erb | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/app/views/shared/_analytics.html.erb b/app/views/shared/_analytics.html.erb index fe7e41d5..b078f92c 100644 --- a/app/views/shared/_analytics.html.erb +++ b/app/views/shared/_analytics.html.erb @@ -2,15 +2,17 @@ <% if ENV['SEGMENTIO_KEY'] %> window.analytics=window.analytics||[],window.analytics.methods=["identify","group","track","page","pageview","alias","ready","on","once","off","trackLink","trackForm","trackClick","trackSubmit"],window.analytics.factory=function(t){return function(){var a=Array.prototype.slice.call(arguments);return a.unshift(t),window.analytics.push(a),window.analytics}};for(var i=0;i"); - window.analytics.user().anonymousId("<%= session['guid'] %>"); <% if logged_in? %> - window.analytics.identify("<%= current_user.id %>") + window.analytics.identify("<%= current_user.id %>", { + anonymousId: "<%= session['guid'] %>" + }) window.analytics.page({ - userId: "<%= current_user.id %>" + userId: "<%= current_user.id %>", + anonymousId: "<%= session['guid'] %>" }); <% else %> - window.analytics.identify(); - window.analytics.page(); + window.analytics.identify({ anonymousId: "<%= session['guid'] %>" }); + window.analytics.page({ anonymousId: "<%= session['guid'] %>" }); <% end %> <% end %> From fa328e0ba0dbb5e2500595f22c2e6b11b764e1b2 Mon Sep 17 00:00:00 2001 From: Keith Dahlby Date: Thu, 31 Mar 2016 01:15:55 -0500 Subject: [PATCH 7/8] Try yet another approach to set anonymousId client-side https://segment.com/docs/libraries/analytics.js/#setting-the-anonymous-id --- app/views/shared/_analytics.html.erb | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/app/views/shared/_analytics.html.erb b/app/views/shared/_analytics.html.erb index b078f92c..9bd90d21 100644 --- a/app/views/shared/_analytics.html.erb +++ b/app/views/shared/_analytics.html.erb @@ -3,16 +3,17 @@ window.analytics=window.analytics||[],window.analytics.methods=["identify","group","track","page","pageview","alias","ready","on","once","off","trackLink","trackForm","trackClick","trackSubmit"],window.analytics.factory=function(t){return function(){var a=Array.prototype.slice.call(arguments);return a.unshift(t),window.analytics.push(a),window.analytics}};for(var i=0;i"); <% if logged_in? %> - window.analytics.identify("<%= current_user.id %>", { + window.analytics.identify("<%= current_user.id %>", {}, { anonymousId: "<%= session['guid'] %>" }) window.analytics.page({ - userId: "<%= current_user.id %>", + userId: "<%= current_user.id %>" + }, { anonymousId: "<%= session['guid'] %>" }); <% else %> - window.analytics.identify({ anonymousId: "<%= session['guid'] %>" }); - window.analytics.page({ anonymousId: "<%= session['guid'] %>" }); + window.analytics.identify({}, { anonymousId: "<%= session['guid'] %>" }); + window.analytics.page({}, { anonymousId: "<%= session['guid'] %>" }); <% end %> <% end %> From 264d998d51ced49c21dc56af0c7ae20726a700f4 Mon Sep 17 00:00:00 2001 From: Richard Frank Date: Thu, 31 Mar 2016 14:32:04 -0600 Subject: [PATCH 8/8] Removes logging out on scope change to maintain session creds --- app/controllers/dashboard_controller.rb | 1 - app/controllers/login_controller.rb | 2 -- 2 files changed, 3 deletions(-) diff --git a/app/controllers/dashboard_controller.rb b/app/controllers/dashboard_controller.rb index 80033467..90ee93c0 100644 --- a/app/controllers/dashboard_controller.rb +++ b/app/controllers/dashboard_controller.rb @@ -44,7 +44,6 @@ def private end :protected def login - request.env['warden'].logout if github_authenticated? :default github_authenticate! :private end diff --git a/app/controllers/login_controller.rb b/app/controllers/login_controller.rb index 9bdc3cd2..34036efd 100644 --- a/app/controllers/login_controller.rb +++ b/app/controllers/login_controller.rb @@ -5,14 +5,12 @@ def logout redirect_to "/" end def public - request.env['warden'].logout if github_authenticated? :private github_authenticate! :default @user = gh.user @emails = @user.emails.all redirect_to params[:redirect_to] || "/" end def private - request.env['warden'].logout if github_authenticated? :default github_authenticate! :private @user = gh.user @emails = @user.emails.all