From 25f1f23288e116ef04ce787df803460ed5e51dca Mon Sep 17 00:00:00 2001 From: Sam Saffron Date: Thu, 26 Mar 2020 17:35:32 +1100 Subject: [PATCH] FEATURE: Stricter rules for user presence Previously we would consider a user "present" and "last seen" if the browser window was visible. This has many edge cases, you could be considered present and around for days just by having a window open and no screensaver on. Instead we now also check that you either clicked, transitioned around app or scrolled the page in the last minute in combination with window visibility This will lead to more reliable notifications via email and reduce load of message bus for cases where a user walks away from the terminal --- Gemfile.lock | 2 +- app/assets/javascripts/application.js | 2 +- .../discourse/initializers/message-bus.js | 11 ++--- app/assets/javascripts/discourse/lib/ajax.js | 6 +-- .../discourse/lib/user-presence.js | 45 +++++++++++++++++++ .../javascripts/discourse/routes/discourse.js | 5 +++ config/initializers/004-message_bus.rb | 2 +- config/initializers/008-rack-cors.rb | 2 +- lib/auth/default_current_user_provider.rb | 2 +- .../default_current_user_provider_spec.rb | 8 ++-- spec/components/hijack_spec.rb | 2 +- 11 files changed, 69 insertions(+), 18 deletions(-) create mode 100644 app/assets/javascripts/discourse/lib/user-presence.js diff --git a/Gemfile.lock b/Gemfile.lock index 471bb0f956c..08056d633de 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -182,7 +182,7 @@ GEM mini_mime (>= 0.1.1) maxminddb (0.1.22) memory_profiler (0.9.14) - message_bus (2.2.3) + message_bus (2.2.4) rack (>= 1.1.3) metaclass (0.0.4) method_source (0.9.2) diff --git a/app/assets/javascripts/application.js b/app/assets/javascripts/application.js index 35cc29acce0..9addddf9ef4 100644 --- a/app/assets/javascripts/application.js +++ b/app/assets/javascripts/application.js @@ -11,7 +11,7 @@ // Stuff we need to load first //= require ./discourse/lib/to-markdown //= require ./discourse/lib/utilities -//= require ./discourse/lib/page-visible +//= require ./discourse/lib/user-presence //= require ./discourse/lib/logout //= require ./discourse/mixins/singleton //= require ./discourse/models/rest diff --git a/app/assets/javascripts/discourse/initializers/message-bus.js b/app/assets/javascripts/discourse/initializers/message-bus.js index 960aaaaf1c9..61905e42a0c 100644 --- a/app/assets/javascripts/discourse/initializers/message-bus.js +++ b/app/assets/javascripts/discourse/initializers/message-bus.js @@ -1,5 +1,5 @@ // Initialize the message bus to receive messages. -import pageVisible from "discourse/lib/page-visible"; +import userPresent from "discourse/lib/user-presence"; import { handleLogoff } from "discourse/lib/ajax"; function ajax(opts) { @@ -31,6 +31,7 @@ export default { siteSettings = container.lookup("site-settings:main"); messageBus.alwaysLongPoll = Discourse.Environment === "development"; + messageBus.shouldLongPollCallback = userPresent; // we do not want to start anything till document is complete messageBus.stop(); @@ -65,16 +66,16 @@ export default { opts.headers["X-Shared-Session-Key"] = $( "meta[name=shared_session_key]" ).attr("content"); - if (pageVisible()) { - opts.headers["Discourse-Visible"] = "true"; + if (userPresent()) { + opts.headers["Discourse-Present"] = "true"; } return ajax(opts); }; } else { messageBus.ajax = function(opts) { opts.headers = opts.headers || {}; - if (pageVisible()) { - opts.headers["Discourse-Visible"] = "true"; + if (userPresent()) { + opts.headers["Discourse-Present"] = "true"; } return ajax(opts); }; diff --git a/app/assets/javascripts/discourse/lib/ajax.js b/app/assets/javascripts/discourse/lib/ajax.js index 54de18d4c8f..37052a4d793 100644 --- a/app/assets/javascripts/discourse/lib/ajax.js +++ b/app/assets/javascripts/discourse/lib/ajax.js @@ -1,5 +1,5 @@ import { run } from "@ember/runloop"; -import pageVisible from "discourse/lib/page-visible"; +import userPresent from "discourse/lib/user-presence"; import logout from "discourse/lib/logout"; import Session from "discourse/models/session"; import { Promise } from "rsvp"; @@ -92,8 +92,8 @@ export function ajax() { args.headers["Discourse-Track-View"] = "true"; } - if (pageVisible()) { - args.headers["Discourse-Visible"] = "true"; + if (userPresent()) { + args.headers["Discourse-Present"] = "true"; } args.success = (data, textStatus, xhr) => { diff --git a/app/assets/javascripts/discourse/lib/user-presence.js b/app/assets/javascripts/discourse/lib/user-presence.js new file mode 100644 index 00000000000..67d332981ff --- /dev/null +++ b/app/assets/javascripts/discourse/lib/user-presence.js @@ -0,0 +1,45 @@ +// for android we test webkit +const hiddenProperty = + document.hidden !== undefined + ? "hidden" + : document.webkitHidden !== undefined + ? "webkitHidden" + : undefined; + +const MAX_UNSEEN_TIME = 60000; + +let seenUserTime = Date.now(); + +export default function() { + const now = Date.now(); + + if (seenUserTime + MAX_UNSEEN_TIME < now) { + return false; + } + + if (hiddenProperty !== undefined) { + return !document[hiddenProperty]; + } else { + return document && document.hasFocus; + } +} + +export function seenUser() { + seenUserTime = Date.now(); +} + +// We could piggieback on the Scroll mixin, but it is not applied +// consistently to all pages +// +// We try to keep this as cheap as possible by performing absolute minimal +// amount of work when the event handler is fired +// +// An alternative would be to use a timer that looks at the scroll position +// however this will not work as message bus can issue page updates and scroll +// page around when user is not present +// +// We avoid tracking mouse move which would be very expensive + +$(document).bind("touchmove.discourse-track-presence", seenUser); +$(document).bind("click.discourse-track-presence", seenUser); +$(window).bind("scroll.discourse-track-presence", seenUser); diff --git a/app/assets/javascripts/discourse/routes/discourse.js b/app/assets/javascripts/discourse/routes/discourse.js index 980e986cd9c..91be0b43312 100644 --- a/app/assets/javascripts/discourse/routes/discourse.js +++ b/app/assets/javascripts/discourse/routes/discourse.js @@ -3,10 +3,15 @@ import Composer from "discourse/models/composer"; import { getOwner } from "discourse-common/lib/get-owner"; import Route from "@ember/routing/route"; import deprecated from "discourse-common/lib/deprecated"; +import { seenUser } from "discourse/lib/user-presence"; const DiscourseRoute = Route.extend({ showFooter: false, + willTransition() { + seenUser(); + }, + // Set to true to refresh a model without a transition if a query param // changes resfreshQueryWithoutTransition: false, diff --git a/config/initializers/004-message_bus.rb b/config/initializers/004-message_bus.rb index da12f03bed6..924fe6dc711 100644 --- a/config/initializers/004-message_bus.rb +++ b/config/initializers/004-message_bus.rb @@ -30,7 +30,7 @@ def setup_message_bus_env(env) extra_headers = { "Access-Control-Allow-Origin" => Discourse.base_url_no_prefix, "Access-Control-Allow-Methods" => "GET, POST", - "Access-Control-Allow-Headers" => "X-SILENCE-LOGGER, X-Shared-Session-Key, Dont-Chunk, Discourse-Visible" + "Access-Control-Allow-Headers" => "X-SILENCE-LOGGER, X-Shared-Session-Key, Dont-Chunk, Discourse-Present" } user = nil diff --git a/config/initializers/008-rack-cors.rb b/config/initializers/008-rack-cors.rb index 5f1c6f40685..cc5bb8cd960 100644 --- a/config/initializers/008-rack-cors.rb +++ b/config/initializers/008-rack-cors.rb @@ -39,7 +39,7 @@ class Discourse::Cors end headers['Access-Control-Allow-Origin'] = origin || cors_origins[0] - headers['Access-Control-Allow-Headers'] = 'Content-Type, Cache-Control, X-Requested-With, X-CSRF-Token, Discourse-Visible, User-Api-Key, User-Api-Client-Id, Authorization' + headers['Access-Control-Allow-Headers'] = 'Content-Type, Cache-Control, X-Requested-With, X-CSRF-Token, Discourse-Present, User-Api-Key, User-Api-Client-Id, Authorization' headers['Access-Control-Allow-Credentials'] = 'true' headers['Access-Control-Allow-Methods'] = 'POST, PUT, GET, OPTIONS, DELETE' end diff --git a/lib/auth/default_current_user_provider.rb b/lib/auth/default_current_user_provider.rb index ca8ec4d1971..3e293dcb77e 100644 --- a/lib/auth/default_current_user_provider.rb +++ b/lib/auth/default_current_user_provider.rb @@ -251,7 +251,7 @@ class Auth::DefaultCurrentUserProvider api = !!(@env[API_KEY_ENV]) || !!(@env[USER_API_KEY_ENV]) if @request.xhr? || api - @env["HTTP_DISCOURSE_VISIBLE".freeze] == "true".freeze + @env["HTTP_DISCOURSE_PRESENT"] == "true" else true end diff --git a/spec/components/auth/default_current_user_provider_spec.rb b/spec/components/auth/default_current_user_provider_spec.rb index 2fd165c848e..f0c6fbc90e5 100644 --- a/spec/components/auth/default_current_user_provider_spec.rb +++ b/spec/components/auth/default_current_user_provider_spec.rb @@ -462,18 +462,18 @@ describe Auth::DefaultCurrentUserProvider do expect(provider("/topic/anything/goes", :method => "POST", "HTTP_X_REQUESTED_WITH" => "XMLHttpRequest", - "HTTP_DISCOURSE_VISIBLE" => "true" + "HTTP_DISCOURSE_PRESENT" => "true" ).should_update_last_seen?).to eq(true) end - it "should not update last seen for ajax calls without Discourse-Visible header" do + it "should not update last seen for ajax calls without Discourse-Present header" do expect(provider("/topic/anything/goes", :method => "POST", "HTTP_X_REQUESTED_WITH" => "XMLHttpRequest" ).should_update_last_seen?).to eq(false) end - it "should update last seen for API calls with Discourse-Visible header" do + it "should update last seen for API calls with Discourse-Present header" do user = Fabricate(:user) api_key = ApiKey.create!(user_id: user.id, created_by_id: -1) params = { :method => "POST", @@ -482,7 +482,7 @@ describe Auth::DefaultCurrentUserProvider do } expect(provider("/topic/anything/goes", params).should_update_last_seen?).to eq(false) - expect(provider("/topic/anything/goes", params.merge("HTTP_DISCOURSE_VISIBLE" => "true")).should_update_last_seen?).to eq(true) + expect(provider("/topic/anything/goes", params.merge("HTTP_DISCOURSE_PRESENT" => "true")).should_update_last_seen?).to eq(true) end it "correctly rotates tokens" do diff --git a/spec/components/hijack_spec.rb b/spec/components/hijack_spec.rb index 7e30a5439e1..93bf9c98bf3 100644 --- a/spec/components/hijack_spec.rb +++ b/spec/components/hijack_spec.rb @@ -108,7 +108,7 @@ describe Hijack do expected = { "Access-Control-Allow-Origin" => "www.rainbows.com", - "Access-Control-Allow-Headers" => "Content-Type, Cache-Control, X-Requested-With, X-CSRF-Token, Discourse-Visible, User-Api-Key, User-Api-Client-Id, Authorization", + "Access-Control-Allow-Headers" => "Content-Type, Cache-Control, X-Requested-With, X-CSRF-Token, Discourse-Present, User-Api-Key, User-Api-Client-Id, Authorization", "Access-Control-Allow-Credentials" => "true", "Access-Control-Allow-Methods" => "POST, PUT, GET, OPTIONS, DELETE" }