From 5b3cd3fac9261a24ba021b57dbba0b3b4f349e3f Mon Sep 17 00:00:00 2001 From: Sam Date: Mon, 19 Sep 2016 15:10:02 +1000 Subject: [PATCH] FEATURE: Import facebook avatars when logging in via facebook FIX: warning about popup dimensions when using facebook login Rules are: - On account creation we always import - If you already have an avatar uploaded, nothing is changed - If you have no avatar uploaded, we upload from facebook on login - If you have no avatar uploaded, we select facebook unless gravatar already selected This also fixes SSO issues where on account creation accounts had missing avatar uploads --- .../discourse/controllers/login.js.es6 | 7 ++++- .../discourse/models/login-method.js.es6 | 4 ++- app/models/user_avatar.rb | 18 ++++++++--- ...9003141_add_avatar_url_to_facebook_info.rb | 5 ++++ lib/auth/facebook_authenticator.rb | 26 +++++++++++++++- spec/models/user_avatar_spec.rb | 30 +++++++++++++++++++ 6 files changed, 83 insertions(+), 7 deletions(-) create mode 100644 db/migrate/20160919003141_add_avatar_url_to_facebook_info.rb diff --git a/app/assets/javascripts/discourse/controllers/login.js.es6 b/app/assets/javascripts/discourse/controllers/login.js.es6 index 611a90045..8909993cc 100644 --- a/app/assets/javascripts/discourse/controllers/login.js.es6 +++ b/app/assets/javascripts/discourse/controllers/login.js.es6 @@ -130,7 +130,7 @@ export default Ember.Controller.extend(ModalFunctionality, { if(customLogin){ customLogin(); } else { - const authUrl = loginMethod.get('customUrl') || Discourse.getURL("/auth/" + name); + let authUrl = loginMethod.get('customUrl') || Discourse.getURL("/auth/" + name); if (loginMethod.get("fullScreenLogin")) { document.cookie = "fsl=true"; window.location = authUrl; @@ -141,6 +141,11 @@ export default Ember.Controller.extend(ModalFunctionality, { const height = loginMethod.get("frameHeight") || 400; const width = loginMethod.get("frameWidth") || 800; + + if (loginMethod.get("displayPopup")) { + authUrl = authUrl + "?display=popup"; + } + const w = window.open(authUrl, "_blank", "menubar=no,status=no,height=" + height + ",width=" + width + ",left=" + left + ",top=" + top); const self = this; diff --git a/app/assets/javascripts/discourse/models/login-method.js.es6 b/app/assets/javascripts/discourse/models/login-method.js.es6 index e1cd941a2..c125e7bd8 100644 --- a/app/assets/javascripts/discourse/models/login-method.js.es6 +++ b/app/assets/javascripts/discourse/models/login-method.js.es6 @@ -34,7 +34,9 @@ export function findAll(siteSettings, capabilities, isMobileDevice) { params.frameWidth = 850; params.frameHeight = 500; } else if (name === "facebook") { - params.frameHeight = 450; + params.frameWidth= 580; + params.frameHeight = 400; + params.displayPopup = true; } params.siteSettings = siteSettings; diff --git a/app/models/user_avatar.rb b/app/models/user_avatar.rb index 0669ea6be..239f8a7db 100644 --- a/app/models/user_avatar.rb +++ b/app/models/user_avatar.rb @@ -60,23 +60,33 @@ class UserAvatar < ActiveRecord::Base "#{upload_id}_#{OptimizedImage::VERSION}" end - def self.import_url_for_user(avatar_url, user) + def self.import_url_for_user(avatar_url, user, options=nil) tempfile = FileHelper.download(avatar_url, SiteSetting.max_image_size_kb.kilobytes, "sso-avatar", true) ext = FastImage.type(tempfile).to_s tempfile.rewind upload = Upload.create_for(user.id, tempfile, "external-avatar." + ext, File.size(tempfile.path), origin: avatar_url, image_type: "avatar") - user.uploaded_avatar_id = upload.id unless user.user_avatar - user.build_user_avatar + user.create_user_avatar end if !user.user_avatar.contains_upload?(upload.id) - user.user_avatar.custom_upload_id = upload.id + user.user_avatar.update_columns(custom_upload_id: upload.id) + + override_gravatar = !options || options[:override_gravatar] + + if user.uploaded_avatar_id.nil? || + !user.user_avatar.contains_upload?(user.uploaded_avatar_id) || + override_gravatar + user.update_columns(uploaded_avatar_id: upload.id) + end end + + rescue => e + p e # skip saving, we are not connected to the net Rails.logger.warn "#{e}: Failed to download external avatar: #{avatar_url}, user id #{ user.id }" ensure diff --git a/db/migrate/20160919003141_add_avatar_url_to_facebook_info.rb b/db/migrate/20160919003141_add_avatar_url_to_facebook_info.rb new file mode 100644 index 000000000..65b2a80dc --- /dev/null +++ b/db/migrate/20160919003141_add_avatar_url_to_facebook_info.rb @@ -0,0 +1,5 @@ +class AddAvatarUrlToFacebookInfo < ActiveRecord::Migration + def change + add_column :facebook_user_infos, :avatar_url, :string + end +end diff --git a/lib/auth/facebook_authenticator.rb b/lib/auth/facebook_authenticator.rb index b4c48ae67..fb59d0a64 100644 --- a/lib/auth/facebook_authenticator.rb +++ b/lib/auth/facebook_authenticator.rb @@ -24,6 +24,17 @@ class Auth::FacebookAuthenticator < Auth::Authenticator FacebookUserInfo.create({user_id: result.user.id}.merge(facebook_hash)) end + if user_info + user_info.update_columns(facebook_hash) + end + + user = result.user + if user && (!user.user_avatar || user.user_avatar.custom_upload_id.nil?) + if (avatar_url = facebook_hash[:avatar_url]).present? + UserAvatar.import_url_for_user(avatar_url, user, override_gravatar: false) + end + end + if email.blank? UserHistory.create( action: UserHistory.actions[:facebook_no_email], @@ -37,14 +48,24 @@ class Auth::FacebookAuthenticator < Auth::Authenticator def after_create_account(user, auth) data = auth[:extra_data] FacebookUserInfo.create({user_id: user.id}.merge(data)) + + + if (avatar_url = data[:avatar_url]).present? + UserAvatar.import_url_for_user(avatar_url, user) + user.save + end + + true end def register_middleware(omniauth) + omniauth.provider :facebook, :setup => lambda { |env| strategy = env["omniauth.strategy"] strategy.options[:client_id] = SiteSetting.facebook_app_id strategy.options[:client_secret] = SiteSetting.facebook_app_secret + strategy.options[:info_fields] = 'gender,email,name,bio,first_name,link,last_name' }, :scope => "email" end @@ -54,6 +75,8 @@ class Auth::FacebookAuthenticator < Auth::Authenticator def parse_auth_token(auth_token) raw_info = auth_token["extra"]["raw_info"] + info = auth_token["info"] + email = auth_token["info"][:email] { @@ -65,7 +88,8 @@ class Auth::FacebookAuthenticator < Auth::Authenticator last_name: raw_info["last_name"], email: email, gender: raw_info["gender"], - name: raw_info["name"] + name: raw_info["name"], + avatar_url: info["image"] }, email: email, email_valid: true diff --git a/spec/models/user_avatar_spec.rb b/spec/models/user_avatar_spec.rb index 944a0785c..066cb97ab 100644 --- a/spec/models/user_avatar_spec.rb +++ b/spec/models/user_avatar_spec.rb @@ -17,4 +17,34 @@ describe UserAvatar do temp.unlink expect(avatar.gravatar_upload).not_to eq(nil) end + + context '#import_url_for_user' do + + it 'creates user_avatar record if missing' do + user = Fabricate(:user) + user.user_avatar.destroy + user.reload + + + FileHelper.stubs(:download).returns(file_from_fixtures("logo.png")) + + UserAvatar.import_url_for_user("logo.png", user) + user.reload + + expect(user.uploaded_avatar_id).not_to eq(nil) + expect(user.user_avatar.custom_upload_id).to eq(user.uploaded_avatar_id) + end + + it 'can leave gravatar alone' do + user = Fabricate(:user, uploaded_avatar_id: 1) + user.user_avatar.update_columns(gravatar_upload_id: 1) + + FileHelper.stubs(:download).returns(file_from_fixtures("logo.png")) + UserAvatar.import_url_for_user("logo.png", user, override_gravatar: false) + + user.reload + expect(user.uploaded_avatar_id).to eq(1) + expect(user.user_avatar.custom_upload_id).not_to eq(nil) + end + end end