Discource-C
/
discourse
mirror of https://github.com/discourse/discourse.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

FEATURE: Use full page redirection for all external auth methods (#8092) 

Using popups is becoming increasingly rare. Full page redirects are already used on mobile, and for some providers. This commit removes all logic related to popup authentication, leaving only the full page redirect method.

For more info, see https://meta.discourse.org/t/do-we-need-popups-for-login/127988
This commit is contained in:
David Taylor 2019-10-08 12:10:43 +01:00 committed by GitHub
parent 20514f2e44
commit d2bceff133
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
21 changed files with 92 additions and 271 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

38
app/assets/javascripts/discourse/controllers/login.js.es6
View File

@ -24,7 +24,6 @@ export default Ember.Controller.extend(ModalFunctionality, {
forgotPassword: Ember.inject.controller(),
application: Ember.inject.controller(),
authenticate: null,
loggingIn: false,
loggedIn: false,
processingEmailLink: false,
@ -39,7 +38,6 @@ export default Ember.Controller.extend(ModalFunctionality, {
resetForm() {
this.setProperties({
authenticate: null,
loggingIn: false,
loggedIn: false,
secondFactorRequired: false,
@ -85,12 +83,12 @@ export default Ember.Controller.extend(ModalFunctionality, {
loginDisabled: Ember.computed.or("loggingIn", "loggedIn"),
@computed("loggingIn", "authenticate", "application.canSignUp")
showSignupLink(loggingIn, authenticate, canSignUp) {
return canSignUp && !loggingIn && Ember.isEmpty(authenticate);
@computed("loggingIn", "application.canSignUp")
showSignupLink(loggingIn, canSignUp) {
return canSignUp && !loggingIn;
},
showSpinner: Ember.computed.or("loggingIn", "authenticate"),
showSpinner: Ember.computed.readOnly("loggingIn"),
@computed("canLoginLocalWithEmail", "processingEmailLink")
showLoginWithEmailLink(canLoginLocalWithEmail, processingEmailLink) {
@ -233,20 +231,13 @@ export default Ember.Controller.extend(ModalFunctionality, {
return false;
},
externalLogin(loginMethod, { fullScreenLogin = false } = {}) {
const capabilities = this.capabilities;
// On Mobile, Android or iOS always go with full screen
if (
this.isMobileDevice ||
(capabilities &&
(capabilities.isIOS ||
capabilities.isAndroid ||
capabilities.isSafari))
) {
fullScreenLogin = true;
externalLogin(loginMethod) {
if (this.loginDisabled) {
return;
}
loginMethod.doLogin({ fullScreenLogin });
this.set("loggingIn", true);
loginMethod.doLogin().catch(() => this.set("loggingIn", false));
},
createAccount() {
@ -324,16 +315,6 @@ export default Ember.Controller.extend(ModalFunctionality, {
}
},
@computed("authenticate")
authMessage(authenticate) {
if (Ember.isEmpty(authenticate)) return "";
const method = findAll().findBy("name", authenticate);
if (method) {
return method.message;
}
},
authenticationComplete(options) {
const loginError = (errorMsg, className, callback) => {
showModal("login");
@ -341,7 +322,6 @@ export default Ember.Controller.extend(ModalFunctionality, {
Ember.run.next(() => {
if (callback) callback();
this.flash(errorMsg, className || "success");
this.set("authenticate", null);
});
};

2
app/assets/javascripts/discourse/controllers/preferences/account.js.es6
View File

@ -252,7 +252,7 @@ export default Ember.Controller.extend(
},
connectAccount(method) {
method.doLogin({ reconnect: true, fullScreenLogin: false });
method.doLogin({ reconnect: true });
}
}
}

6
app/assets/javascripts/discourse/initializers/auth-complete.js.es6
View File

@ -4,11 +4,7 @@ export default {
initialize(container) {
let lastAuthResult;
if (window.location.search.indexOf("authComplete=true") !== -1) {
// Happens when a popup social login loses connection to the parent window
lastAuthResult = localStorage.getItem("lastAuthResult");
localStorage.removeItem("lastAuthResult");
} else if (document.getElementById("data-authentication")) {
if (document.getElementById("data-authentication")) {
// Happens for full screen logins
lastAuthResult = document.getElementById("data-authentication").dataset
.authenticationData;

70
app/assets/javascripts/discourse/models/login-method.js.es6
View File

@ -17,60 +17,24 @@ const LoginMethod = Ember.Object.extend({
return this.message_override || I18n.t(`login.${this.name}.message`);
},
doLogin({ reconnect = false, fullScreenLogin = true } = {}) {
const name = this.name;
const customLogin = this.customLogin;
if (customLogin) {
customLogin();
} else {
if (this.custom_url) {
window.location = this.custom_url;
return;
}
let authUrl = Discourse.getURL(`/auth/${name}`);
if (reconnect) {
authUrl += "?reconnect=true";
}
if (reconnect || fullScreenLogin || this.full_screen_login) {
LoginMethod.buildPostForm(authUrl).then(form => {
document.cookie = "fsl=true";
form.submit();
});
} else {
this.set("authenticate", name);
const left = this.lastX - 400;
const top = this.lastY - 200;
const height = this.frame_height || 400;
const width = this.frame_width || 800;
if (name === "facebook") {
authUrl += authUrl.includes("?") ? "&" : "?";
authUrl += "display=popup";
}
LoginMethod.buildPostForm(authUrl).then(form => {
const windowState = window.open(
"about:blank",
"auth_popup",
`menubar=no,status=no,height=${height},width=${width},left=${left},top=${top}`
);
form.target = "auth_popup";
form.submit();
const timer = setInterval(() => {
// If the process is aborted, reset state in this window
if (!windowState || windowState.closed) {
clearInterval(timer);
this.set("authenticate", null);
}
}, 1000);
});
}
doLogin({ reconnect = false } = {}) {
if (this.customLogin) {
this.customLogin();
return Ember.RSVP.resolve();
}
if (this.custom_url) {
window.location = this.custom_url;
return Ember.RSVP.resolve();
}
let authUrl = Discourse.getURL(`/auth/${this.name}`);
if (reconnect) {
authUrl += "?reconnect=true";
}
return LoginMethod.buildPostForm(authUrl).then(form => form.submit());
}
});

4
app/assets/javascripts/discourse/routes/application.js.es6
View File

@ -265,9 +265,7 @@ const ApplicationRoute = Discourse.Route.extend(OpenComposer, {
const methods = findAll();
if (!this.siteSettings.enable_local_logins && methods.length === 1) {
this.controllerFor("login").send("externalLogin", methods[0], {
fullScreenLogin: true
});
this.controllerFor("login").send("externalLogin", methods[0]);
} else {
showModal(modal);
this.controllerFor("modal").set("modalClass", modalClass);

6
app/assets/javascripts/discourse/templates/mobile/modal/login.hbs
View File

@ -62,10 +62,6 @@
{{/d-modal-body}}
<div class="modal-footer">
{{#if authenticate}}
{{i18n "login.authenticating"}}
{{/if}}
{{#if canLoginLocal}}
{{#unless showSecurityKey }}
{{d-button action=(action "login")
@ -82,6 +78,6 @@
{{/if}}
{{/if}}
</div>
<div class="auth-message">{{authMessage}}</div>
<div id='login-alert' class={{alertClass}}>{{alert}}</div>
{{/login-modal}}

5
app/assets/javascripts/discourse/templates/modal/login.hbs
View File

@ -73,13 +73,8 @@
{{/if}}
{{/if}}
{{#if authenticate}}
&nbsp;{{i18n 'login.authenticating'}}
{{/if}}
{{conditional-loading-spinner condition=showSpinner size="small"}}
</div>
<div class="auth-message">{{authMessage}}</div>
<div id='login-alert' class={{alertClass}}>{{alert}}</div>
{{/login-modal}}

18
app/assets/javascripts/omniauth-complete.js.no-module.es6
View File

@ -1,18 +0,0 @@
(function() {
const { authResult, baseUrl } = document.getElementById(
"data-auth-result"
).dataset;
const parsedAuthResult = JSON.parse(authResult);
if (
!window.opener ||
!window.opener.Discourse ||
!window.opener.Discourse.authenticationComplete
) {
localStorage.setItem("lastAuthResult", authResult);
window.location.href = `${baseUrl}?authComplete=true`;
} else {
window.opener.Discourse.authenticationComplete(parsedAuthResult);
window.close();
}
})();

3
app/controllers/application_controller.rb
View File

@ -724,9 +724,6 @@ class ApplicationController < ActionController::Base
session[:destination_url] = destination_url
redirect_to path('/session/sso')
return
elsif params[:authComplete].present?
redirect_to path("/login?authComplete=true")
return
else
# save original URL in a cookie (javascript redirects after login in this case)
cookies[:destination_url] = destination_url

15
app/controllers/users/omniauth_callbacks_controller.rb
View File

@ -71,18 +71,9 @@ class Users::OmniauthCallbacksController < ApplicationController
else
@auth_result.authenticator_name = authenticator.name
complete_response_data
if provider&.full_screen_login || cookies['fsl']
cookies.delete('fsl')
cookies['_bypass_cache'] = true
cookies[:authentication_data] = @auth_result.to_client_hash.to_json
redirect_to @origin
else
respond_to do |format|
format.html
format.json { render json: @auth_result.to_client_hash }
end
end
cookies['_bypass_cache'] = true
cookies[:authentication_data] = @auth_result.to_client_hash.to_json
redirect_to @origin
end
end

8
app/serializers/auth_provider_serializer.rb
View File

@ -3,7 +3,7 @@
class AuthProviderSerializer < ApplicationSerializer
attributes :name, :custom_url, :pretty_name_override, :title_override, :message_override,
:frame_width, :frame_height, :full_screen_login, :can_connect, :can_revoke,
:frame_width, :frame_height, :can_connect, :can_revoke,
:icon
def title_override
@ -16,12 +16,6 @@ class AuthProviderSerializer < ApplicationSerializer
object.pretty_name
end
def full_screen_login
return SiteSetting.get(object.full_screen_login_setting) if object.full_screen_login_setting
return object.full_screen_login if object.full_screen_login
false
end
def message_override
object.message
end

33
app/views/users/omniauth_callbacks/complete.html.erb
View File

@ -1,33 +0,0 @@
<!DOCTYPE html>
<html>
<head>
<title><%= SiteSetting.title %></title>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta name="viewport" content="width=device-width, minimum-scale=1.0, maximum-scale=1.0, user-scalable=no">
<style type="text/css">
body { background-color: #fff; color: #666; text-align: center; font-family: arial, sans-serif; }
div.dialog {
width: 90%;
padding: 0 1em;
margin: 2em auto 0 auto;
border: 1px solid #ccc;
border-right-color: #999;
border-bottom-color: #999;
}
</style>
<%= tag.meta id: 'data-auth-result', data: {
auth_result: @auth_result.to_client_hash,
base_url: Discourse.base_url
} %>
<%= preload_script('omniauth-complete') %>
</head>
<body>
<div class="dialog">
<p>
<%=t "login.auth_complete" %>
<a href="<%= Discourse.base_url %>?authComplete=true"><%= t("login.click_to_continue") %></a>
</p>
</div>
</body>
</html>

1
app/views/users/omniauth_callbacks/confirm_request.html.erb
View File

@ -3,7 +3,6 @@
<br/>
<%= form_tag do %>
<input type="hidden" name="full_screen_login" value="true">
<%= button_tag(type: "submit", class: "btn btn-primary") do %>
<%= SvgSprite.raw_svg('fa-plug') %><%= t 'login.omniauth_confirm_button' %>
<% end %>

6
config/locales/client.en.yml
View File

@ -1497,27 +1497,21 @@ en:
google_oauth2:
name: "Google"
title: "with Google"
message: "Authenticating with Google (make sure pop up blockers are not enabled)"
twitter:
name: "Twitter"
title: "with Twitter"
message: "Authenticating with Twitter (make sure pop up blockers are not enabled)"
instagram:
name: "Instagram"
title: "with Instagram"
message: "Authenticating with Instagram (make sure pop up blockers are not enabled)"
facebook:
name: "Facebook"
title: "with Facebook"
message: "Authenticating with Facebook (make sure pop up blockers are not enabled)"
github:
name: "GitHub"
title: "with GitHub"
message: "Authenticating with GitHub (make sure pop up blockers are not enabled)"
discord:
name: "Discord"
title: "with Discord"
message: "Authenticating with Discord"
second_factor_toggle:
totp: "Use an authenticator app instead"
backup_code: "Use a backup code instead"

12
lib/auth/auth_provider.rb
View File

@ -16,12 +16,20 @@ class Auth::AuthProvider
attr_accessor(*auth_attributes)
def enabled_setting=(val)
Discourse.deprecate("enabled_setting is deprecated. Please define authenticator.enabled? instead")
Discourse.deprecate("(#{authenticator.name}) enabled_setting is deprecated. Please define authenticator.enabled? instead")
@enabled_setting = val
end
def background_color=(val)
Discourse.deprecate("background_color is no longer functional. Please use CSS instead")
Discourse.deprecate("(#{authenticator.name}) background_color is no longer functional. Please use CSS instead")
end
def full_screen_login=(val)
Discourse.deprecate("(#{authenticator.name}) full_screen_login is now forced. The full_screen_login parameter can be removed from the auth_provider.")
end
def full_screen_login_setting=(val)
Discourse.deprecate("(#{authenticator.name}) full_screen_login is now forced. The full_screen_login_setting parameter can be removed from the auth_provider.")
end
def name

2
lib/discourse.rb
View File

@ -260,7 +260,7 @@ module Discourse
Auth::AuthProvider.new(authenticator: Auth::GithubAuthenticator.new, icon: "fab-github"),
Auth::AuthProvider.new(authenticator: Auth::TwitterAuthenticator.new, icon: "fab-twitter"),
Auth::AuthProvider.new(authenticator: Auth::InstagramAuthenticator.new, icon: "fab-instagram"),
Auth::AuthProvider.new(authenticator: Auth::DiscordAuthenticator.new, icon: "fab-discord", full_screen_login: true)
Auth::AuthProvider.new(authenticator: Auth::DiscordAuthenticator.new, icon: "fab-discord")
]
def self.auth_providers

12
spec/components/plugin/instance_spec.rb
View File

@ -161,20 +161,26 @@ describe Plugin::Instance do
end
it 'patches the enabled? function for auth_providers if not defined' do
SimpleAuthenticator = Class.new(Auth::Authenticator) do
def name
"my_authenticator"
end
end
plugin = Plugin::Instance.new
# lets piggy back on another boolean setting, so we don't dirty our SiteSetting object
SiteSetting.enable_badges = false
# No enabled_site_setting
authenticator = Auth::Authenticator.new
authenticator = SimpleAuthenticator.new
plugin.auth_provider(authenticator: authenticator)
plugin.notify_before_auth
expect(authenticator.enabled?).to eq(true)
# With enabled site setting
plugin = Plugin::Instance.new
authenticator = Auth::Authenticator.new
authenticator = SimpleAuthenticator.new
plugin.auth_provider(enabled_setting: 'enable_badges', authenticator: authenticator)
plugin.notify_before_auth
expect(authenticator.enabled?).to eq(false)
@ -183,7 +189,7 @@ describe Plugin::Instance do
plugin = Plugin::Instance.new
SiteSetting.enable_badges = true
authenticator = Class.new(Auth::Authenticator) do
authenticator = Class.new(SimpleAuthenticator) do
def enabled?
false
end

5
spec/requests/application_controller_spec.rb
View File

@ -11,11 +11,6 @@ RSpec.describe ApplicationController do
SiteSetting.login_required = true
end
it "should carry-forward authComplete param to login page redirect" do
get "/?authComplete=true"
expect(response).to redirect_to('/login?authComplete=true')
end
it "should never cache a login redirect" do
get "/"
expect(response.headers["Cache-Control"]).to eq("no-cache, no-store")

76
spec/requests/omniauth_callbacks_controller_spec.rb
View File

@ -197,17 +197,17 @@ RSpec.describe Users::OmniauthCallbacksController do
get "/auth/google_oauth2/callback.json"
expect(response.status).to eq(200)
expect(response.status).to eq(302)
response_body = JSON.parse(response.body)
data = JSON.parse(cookies[:authentication_data])
expect(response_body["email"]).to eq(email)
expect(response_body["username"]).to eq("Some_Name")
expect(response_body["auth_provider"]).to eq("google_oauth2")
expect(response_body["email_valid"]).to eq(true)
expect(response_body["omit_username"]).to eq(false)
expect(response_body["name"]).to eq("Some Name")
expect(response_body["destination_url"]).to eq(destination_url)
expect(data["email"]).to eq(email)
expect(data["username"]).to eq("Some_Name")
expect(data["auth_provider"]).to eq("google_oauth2")
expect(data["email_valid"]).to eq(true)
expect(data["omit_username"]).to eq(false)
expect(data["name"]).to eq("Some Name")
expect(data["destination_url"]).to eq(destination_url)
end
it 'should include destination url in response' do
@ -216,8 +216,8 @@ RSpec.describe Users::OmniauthCallbacksController do
get "/auth/google_oauth2/callback.json"
response_body = JSON.parse(response.body)
expect(response_body["destination_url"]).to eq(destination_url)
data = JSON.parse(cookies[:authentication_data])
expect(data["destination_url"]).to eq(destination_url)
end
end
@ -254,15 +254,15 @@ RSpec.describe Users::OmniauthCallbacksController do
expect(events.map { |event| event[:event_name] }).to include(:user_logged_in, :user_first_logged_in)
expect(response.status).to eq(200)
expect(response.status).to eq(302)
response_body = JSON.parse(response.body)
data = JSON.parse(cookies[:authentication_data])
expect(response_body["authenticated"]).to eq(true)
expect(response_body["awaiting_activation"]).to eq(false)
expect(response_body["awaiting_approval"]).to eq(false)
expect(response_body["not_allowed_from_ip_address"]).to eq(false)
expect(response_body["admin_not_allowed_from_ip_address"]).to eq(false)
expect(data["authenticated"]).to eq(true)
expect(data["awaiting_activation"]).to eq(false)
expect(data["awaiting_approval"]).to eq(false)
expect(data["not_allowed_from_ip_address"]).to eq(false)
expect(data["admin_not_allowed_from_ip_address"]).to eq(false)
user.reload
expect(user.email_confirmed?).to eq(true)
@ -280,7 +280,7 @@ RSpec.describe Users::OmniauthCallbacksController do
expect(events.map { |event| event[:event_name] }).to include(:user_logged_in, :user_first_logged_in)
expect(response.status).to eq(200)
expect(response.status).to eq(302)
user.reload
expect(user.email_confirmed?).to eq(true)
@ -299,7 +299,7 @@ RSpec.describe Users::OmniauthCallbacksController do
expect(events.map { |event| event[:event_name] }).to include(:user_logged_in, :user_first_logged_in)
expect(response.status).to eq(200)
expect(response.status).to eq(302)
user.reload
expect(user.staged).to eq(false)
@ -330,18 +330,18 @@ RSpec.describe Users::OmniauthCallbacksController do
it 'should return the right response' do
get "/auth/google_oauth2/callback.json"
expect(response.status).to eq(200)
expect(response.status).to eq(302)
response_body = JSON.parse(response.body)
data = JSON.parse(cookies[:authentication_data])
expect(response_body["email"]).to eq(user.email)
expect(response_body["omniauth_disallow_totp"]).to eq(true)
expect(data["email"]).to eq(user.email)
expect(data["omniauth_disallow_totp"]).to eq(true)
user.update!(email: 'different@user.email')
get "/auth/google_oauth2/callback.json"
expect(response.status).to eq(200)
expect(JSON.parse(response.body)["email"]).to eq(user.email)
expect(response.status).to eq(302)
expect(JSON.parse(cookies[:authentication_data])["email"]).to eq(user.email)
end
end
@ -383,11 +383,11 @@ RSpec.describe Users::OmniauthCallbacksController do
it 'should return the right response' do
get "/auth/google_oauth2/callback.json"
expect(response.status).to eq(200)
expect(response.status).to eq(302)
response_body = JSON.parse(response.body)
data = JSON.parse(cookies[:authentication_data])
expect(response_body["destination_url"]).to match(/\/session\/sso_provider\?sso\=.*\&sig\=.*/)
expect(data["destination_url"]).to match(/\/session\/sso_provider\?sso\=.*\&sig\=.*/)
end
end
@ -421,13 +421,13 @@ RSpec.describe Users::OmniauthCallbacksController do
it 'should return the right response' do
get "/auth/google_oauth2/callback.json"
expect(response.status).to eq(200)
expect(response.status).to eq(302)
response_body = JSON.parse(response.body)
data = JSON.parse(cookies[:authentication_data])
expect(user.reload.active).to eq(false)
expect(response_body["authenticated"]).to eq(false)
expect(response_body["awaiting_activation"]).to eq(true)
expect(data["authenticated"]).to eq(false)
expect(data["awaiting_activation"]).to eq(true)
end
end
@ -534,7 +534,7 @@ RSpec.describe Users::OmniauthCallbacksController do
expect(session[:auth_reconnect]).to eq(false)
get "/auth/google_oauth2/callback.json"
expect(response.status).to eq(200)
expect(response.status).to eq(302)
expect(session[:current_user_id]).to eq(user.id)
# Log into another user
@ -544,7 +544,7 @@ RSpec.describe Users::OmniauthCallbacksController do
expect(session[:auth_reconnect]).to eq(false)
get "/auth/google_oauth2/callback.json"
expect(response.status).to eq(200)
expect(response.status).to eq(302)
expect(session[:current_user_id]).to eq(user2.id)
expect(UserAssociatedAccount.count).to eq(2)
end
@ -556,7 +556,7 @@ RSpec.describe Users::OmniauthCallbacksController do
expect(session[:auth_reconnect]).to eq(true)
get "/auth/google_oauth2/callback.json"
expect(response.status).to eq(200)
expect(response.status).to eq(302)
expect(session[:current_user_id]).to eq(user.id)
# Clear cookie after login
@ -605,8 +605,8 @@ RSpec.describe Users::OmniauthCallbacksController do
Rails.application.env_config["omniauth.auth"] = OmniAuth.config.mock_auth[:google_oauth2]
get "/auth/google_oauth2/callback.json"
expect(response.status).to eq(200)
JSON.parse(response.body)
expect(response.status).to eq(302)
JSON.parse(cookies[:authentication_data])
end
it 'activates the correct email' do

40
spec/views/omniauth_callbacks/complete.html.erb_spec.rb
View File

@ -1,40 +0,0 @@
# frozen_string_literal: true
require "rails_helper"
require "auth/authenticator"
describe "users/omniauth_callbacks/complete.html.erb" do
let :rendered_data do
JSON.parse(rendered.match(/data-auth-result="([^"]*)"/)[1].gsub('&quot;', '"'))
end
it "renders auth info" do
result = Auth::Result.new
result.user = User.new
assign(:auth_result, result)
render
expect(rendered_data["authenticated"]).to eq(false)
expect(rendered_data["awaiting_activation"]).to eq(false)
expect(rendered_data["awaiting_approval"]).to eq(false)
end
it "renders cas data " do
result = Auth::Result.new
result.email = "xxx@xxx.com"
result.authenticator_name = "CAS"
assign(:auth_result, result)
render
expect(rendered_data["email"]).to eq(result.email)
expect(rendered_data["auth_provider"]).to eq("CAS")
end
end

1
test/javascripts/fixtures/site-fixtures.js.es6
View File

@ -601,7 +601,6 @@ export default {
message_override: null,
frame_width: 580,
frame_height: 400,
full_screen_login: false,
can_connect: true,
can_revoke: true
}