From ce04db861033c906830fd07ae98b3d0cb375c753 Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Tue, 2 Mar 2021 15:13:04 +0800 Subject: [PATCH] FEATURE: Allow invites redemption with Omniauth providers. --- .../discourse/app/controllers/invites-show.js | 84 +++++++- .../app/initializers/auth-complete.js | 8 +- .../templates/components/login-buttons.hbs | 2 +- .../discourse/app/templates/invites/show.hbs | 125 ++++++----- .../app/templates/modal/create-account.hbs | 2 +- .../tests/acceptance/invite-accept-test.js | 203 +++++++++++++++++- app/assets/stylesheets/common/base/login.scss | 11 +- app/controllers/invites_controller.rb | 10 +- .../users/omniauth_callbacks_controller.rb | 7 +- app/controllers/users_controller.rb | 2 - app/models/invite.rb | 15 +- app/models/invite_redeemer.rb | 28 ++- app/services/user_authenticator.rb | 9 +- config/locales/client.en.yml | 1 + config/locales/server.en.yml | 3 +- lib/guardian.rb | 5 +- spec/components/guardian_spec.rb | 6 +- spec/requests/invites_controller_spec.rb | 112 ++++++++-- .../omniauth_callbacks_controller_spec.rb | 43 ++++ spec/requests/users_controller_spec.rb | 26 ++- spec/services/user_authenticator_spec.rb | 58 +++-- 21 files changed, 621 insertions(+), 139 deletions(-) diff --git a/app/assets/javascripts/discourse/app/controllers/invites-show.js b/app/assets/javascripts/discourse/app/controllers/invites-show.js index 1b5eb23e0b4..3890643ee69 100644 --- a/app/assets/javascripts/discourse/app/controllers/invites-show.js +++ b/app/assets/javascripts/discourse/app/controllers/invites-show.js @@ -1,5 +1,5 @@ import { alias, notEmpty, or, readOnly } from "@ember/object/computed"; -import Controller from "@ember/controller"; +import Controller, { inject as controller } from "@ember/controller"; import DiscourseURL from "discourse/lib/url"; import EmberObject from "@ember/object"; import I18n from "I18n"; @@ -14,6 +14,7 @@ import { emailValid } from "discourse/lib/utilities"; import { findAll as findLoginMethods } from "discourse/models/login-method"; import getUrl from "discourse-common/lib/get-url"; import { isEmpty } from "@ember/utils"; +import { wavingHandURL } from "discourse/lib/waving-hand-url"; export default Controller.extend( PasswordValidation, @@ -21,6 +22,8 @@ export default Controller.extend( NameValidation, UserFieldsValidation, { + createAccount: controller(), + invitedBy: readOnly("model.invited_by"), email: alias("model.email"), accountUsername: alias("model.username"), @@ -28,6 +31,7 @@ export default Controller.extend( successMessage: null, errorMessage: null, userFields: null, + authOptions: null, inviteImageUrl: getUrl("/images/envelope.svg"), isInviteLink: readOnly("model.is_invite_link"), submitDisabled: or( @@ -45,6 +49,20 @@ export default Controller.extend( this.rejectedEmails = []; }, + authenticationComplete(options) { + const props = { + accountUsername: options.username, + accountName: options.name, + authOptions: EmberObject.create(options), + }; + + if (this.isInviteLink) { + props.email = options.email; + } + + this.setProperties(props); + }, + @discourseComputed welcomeTitle() { return I18n.t("invites.welcome_to", { @@ -62,6 +80,25 @@ export default Controller.extend( return findLoginMethods().length > 0; }, + @discourseComputed + externalAuthsOnly() { + return ( + !this.siteSettings.enable_local_logins && this.externalAuthsEnabled + ); + }, + + @discourseComputed( + "externalAuthsOnly", + "authOptions", + "emailValidation.failed" + ) + shouldDisplayForm(externalAuthsOnly, authOptions, emailValidationFailed) { + return ( + this.siteSettings.enable_local_logins || + (externalAuthsOnly && authOptions && !emailValidationFailed) + ); + }, + @discourseComputed fullnameRequired() { return ( @@ -69,8 +106,18 @@ export default Controller.extend( ); }, - @discourseComputed("email", "rejectedEmails.[]") - emailValidation(email, rejectedEmails) { + @discourseComputed( + "email", + "rejectedEmails.[]", + "authOptions.email", + "authOptions.email_valid" + ) + emailValidation( + email, + rejectedEmails, + externalAuthEmail, + externalAuthEmailValid + ) { // If blank, fail without a reason if (isEmpty(email)) { return EmberObject.create({ @@ -85,6 +132,28 @@ export default Controller.extend( }); } + if (externalAuthEmail) { + const provider = this.createAccount.authProviderDisplayName( + this.get("authOptions.auth_provider") + ); + + if (externalAuthEmail === email && externalAuthEmailValid) { + return EmberObject.create({ + ok: true, + reason: I18n.t("user.email.authenticated", { + provider, + }), + }); + } else { + return EmberObject.create({ + failed: true, + reason: I18n.t("user.email.invite_auth_email_invalid", { + provider, + }), + }); + } + } + if (emailValid(email)) { return EmberObject.create({ ok: true, @@ -98,6 +167,9 @@ export default Controller.extend( }); }, + @discourseComputed + wavingHandURL: () => wavingHandURL(), + actions: { submit() { const userFields = this.userFields; @@ -158,6 +230,12 @@ export default Controller.extend( this.set("errorMessage", extractError(error)); }); }, + + externalLogin(provider) { + provider.doLogin({ + origin: window.location.href, + }); + }, }, } ); diff --git a/app/assets/javascripts/discourse/app/initializers/auth-complete.js b/app/assets/javascripts/discourse/app/initializers/auth-complete.js index 261f51e41b9..13029e40442 100644 --- a/app/assets/javascripts/discourse/app/initializers/auth-complete.js +++ b/app/assets/javascripts/discourse/app/initializers/auth-complete.js @@ -13,10 +13,14 @@ export default { if (lastAuthResult) { const router = container.lookup("router:main"); + router.one("didTransition", () => { + const controllerName = + router.currentPath === "invites.show" ? "invites-show" : "login"; + next(() => { - let loginController = container.lookup("controller:login"); - loginController.authenticationComplete(JSON.parse(lastAuthResult)); + let controller = container.lookup(`controller:${controllerName}`); + controller.authenticationComplete(JSON.parse(lastAuthResult)); }); }); } diff --git a/app/assets/javascripts/discourse/app/templates/components/login-buttons.hbs b/app/assets/javascripts/discourse/app/templates/components/login-buttons.hbs index 07a10e5c124..bc2bc0aba98 100644 --- a/app/assets/javascripts/discourse/app/templates/components/login-buttons.hbs +++ b/app/assets/javascripts/discourse/app/templates/components/login-buttons.hbs @@ -9,4 +9,4 @@ {{/if}} {{b.title}} -{{/each}} \ No newline at end of file +{{/each}} diff --git a/app/assets/javascripts/discourse/app/templates/invites/show.hbs b/app/assets/javascripts/discourse/app/templates/invites/show.hbs index 8739fc39246..db455bb7f55 100644 --- a/app/assets/javascripts/discourse/app/templates/invites/show.hbs +++ b/app/assets/javascripts/discourse/app/templates/invites/show.hbs @@ -1,5 +1,9 @@
-

{{welcomeTitle}}

+
@@ -19,71 +23,84 @@ {{#unless isInviteLink}}

{{html-safe yourEmailMessage}} - {{#if externalAuthsEnabled}} + {{#unless externalAuthsOnly}} {{i18n "invites.social_login_available"}} - {{/if}} + {{/unless}}

{{/unless}} -
- - {{#if isInviteLink}} - + {{/unless}} + {{else}} + {{login-buttons externalLogin=(action "externalLogin")}} {{/if}} + {{/if}} -
- - {{input value=accountUsername id="new-account-username" name="username" maxlength=maxUsernameLength autocomplete="discourse"}} - {{input-tip validation=usernameValidation id="username-validation"}} -
{{i18n "user.username.instructions"}}
-
- - {{#if fullnameRequired}} -
- - {{input value=accountName id="new-account-name" name="name"}} -
{{nameInstructions}}
-
- {{/if}} - -
- - {{password-field value=accountPassword type="password" id="new-account-password" capsLockOn=capsLockOn}} - {{input-tip validation=passwordValidation}} -
- {{passwordInstructions}} {{i18n "invites.optional_description"}} -
- {{d-icon "exclamation-triangle"}} {{i18n "login.caps_lock_warning"}} + {{#if shouldDisplayForm}} + + {{#if isInviteLink}} + + {{/if}} + +
+ + {{input value=accountUsername id="new-account-username" name="username" maxlength=maxUsernameLength autocomplete="discourse"}} + {{input-tip validation=usernameValidation id="username-validation"}} +
{{i18n "user.username.instructions"}}
-
- {{#if userFields}} -
- {{#each userFields as |f|}} - {{user-field field=f.field value=f.value}} - {{/each}} -
- {{/if}} + {{#if fullnameRequired}} +
+ + {{input value=accountName id="new-account-name" name="name"}} +
{{nameInstructions}}
+
+ {{/if}} - {{d-button - class="btn-primary" - action=(action "submit") - type="submit" - disabled=submitDisabled - label="invites.accept_invite" - }} + {{#unless externalAuthsOnly}} +
+ + {{password-field value=accountPassword type="password" id="new-account-password" capsLockOn=capsLockOn}} + {{input-tip validation=passwordValidation}} +
+ {{passwordInstructions}} {{i18n "invites.optional_description"}} +
+ {{d-icon "exclamation-triangle"}} {{i18n "login.caps_lock_warning"}} +
+
+
+ {{/unless}} - {{#if errorMessage}} -

-
{{errorMessage}}
- {{/if}} - + {{#if userFields}} +
+ {{#each userFields as |f|}} + {{user-field field=f.field value=f.value}} + {{/each}} +
+ {{/if}} + + {{d-button + class="btn-primary" + action=(action "submit") + type="submit" + disabled=submitDisabled + label="invites.accept_invite" + }} + + {{#if errorMessage}} +

+
{{errorMessage}}
+ {{/if}} + + {{/if}} {{/if}}
diff --git a/app/assets/javascripts/discourse/app/templates/modal/create-account.hbs b/app/assets/javascripts/discourse/app/templates/modal/create-account.hbs index c746fa409da..f309fd6b6de 100644 --- a/app/assets/javascripts/discourse/app/templates/modal/create-account.hbs +++ b/app/assets/javascripts/discourse/app/templates/modal/create-account.hbs @@ -173,4 +173,4 @@ {{plugin-outlet name="create-account-after-modal-footer" tagName=""}} {{/if}} {{/unless}} -{{/create-account}} \ No newline at end of file +{{/create-account}} diff --git a/app/assets/javascripts/discourse/tests/acceptance/invite-accept-test.js b/app/assets/javascripts/discourse/tests/acceptance/invite-accept-test.js index 7004ee6e14c..7a8f9ea517b 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/invite-accept-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/invite-accept-test.js @@ -5,12 +5,39 @@ import { } from "discourse/tests/helpers/qunit-helpers"; import { fillIn, visit } from "@ember/test-helpers"; import PreloadStore from "discourse/lib/preload-store"; +import I18n from "I18n"; import { test } from "qunit"; -acceptance("Invite Accept", function (needs) { +acceptance("Invite accept", function (needs) { needs.settings({ full_name_required: true }); - test("Invite Acceptance Page", async function (assert) { + test("email invite link", async function (assert) { + PreloadStore.store("invite_info", { + invited_by: { + id: 123, + username: "foobar", + avatar_template: "/user_avatar/localhost/neil/{size}/25_1.png", + name: "foobar", + title: "team", + }, + email: "foobar@example.com", + username: "invited", + is_invite_link: false, + }); + + await visit("/invites/myvalidinvitetoken"); + + assert.ok( + queryAll(".col-form") + .text() + .includes(I18n.t("invites.social_login_available")), + "shows social login hint" + ); + + assert.ok(!exists("#new-account-email"), "hides the email input"); + }); + + test("invite link", async function (assert) { PreloadStore.store("invite_info", { invited_by: { id: 123, @@ -84,3 +111,175 @@ acceptance("Invite Accept", function (needs) { ); }); }); + +acceptance("Invite accept when local login is disabled", function (needs) { + needs.settings({ enable_local_logins: false }); + + const preloadStore = function (isInviteLink) { + const info = { + invited_by: { + id: 123, + username: "foobar", + avatar_template: "/user_avatar/localhost/neil/{size}/25_1.png", + name: "foobar", + title: "team", + }, + username: "invited", + }; + + if (isInviteLink) { + info.email = "null"; + info.is_invite_link = true; + } else { + info.email = "foobar@example.com"; + info.is_invite_link = false; + } + + PreloadStore.store("invite_info", info); + }; + + test("invite link", async function (assert) { + preloadStore(true); + + await visit("/invites/myvalidinvitetoken"); + + assert.ok(exists(".btn-social.facebook"), "shows Facebook login button"); + assert.ok(!exists("form"), "does not display the form"); + }); + + test("invite link with authentication data", async function (assert) { + preloadStore(true); + + // Simulate authticated with Facebook + const node = document.createElement("meta"); + node.dataset.authenticationData = JSON.stringify({ + auth_provider: "facebook", + email: "blah@example.com", + email_valid: true, + username: "foobar", + name: "barfoo", + }); + node.id = "data-authentication"; + document.querySelector("head").appendChild(node); + + await visit("/invites/myvalidinvitetoken"); + + assert.ok( + !exists(".btn-social.facebook"), + "does not show Facebook login button" + ); + + assert.ok(!exists("#new-account-password"), "does not show password field"); + + assert.ok( + exists("#new-account-email[disabled]"), + "email field is disabled" + ); + + assert.equal( + queryAll("#account-email-validation").text().trim(), + I18n.t("user.email.authenticated", { provider: "Facebook" }) + ); + + assert.equal( + queryAll("#new-account-username").val(), + "foobar", + "username is prefilled" + ); + + assert.equal( + queryAll("#new-account-name").val(), + "barfoo", + "name is prefilled" + ); + + document + .querySelector("head") + .removeChild(document.getElementById("data-authentication")); + }); + + test("email invite link", async function (assert) { + preloadStore(false); + + await visit("/invites/myvalidinvitetoken"); + + assert.ok(exists(".btn-social.facebook"), "shows Facebook login button"); + assert.ok(!exists("form"), "does not display the form"); + }); + + test("email invite link with authentication data when email does not match", async function (assert) { + preloadStore(false); + + // Simulate authticated with Facebook + const node = document.createElement("meta"); + node.dataset.authenticationData = JSON.stringify({ + auth_provider: "facebook", + email: "blah@example.com", + email_valid: true, + username: "foobar", + name: "barfoo", + }); + node.id = "data-authentication"; + document.querySelector("head").appendChild(node); + + await visit("/invites/myvalidinvitetoken"); + + assert.equal( + queryAll("#account-email-validation").text().trim(), + I18n.t("user.email.invite_auth_email_invalid", { provider: "Facebook" }) + ); + + assert.ok(!exists("form"), "does not display the form"); + + document + .querySelector("head") + .removeChild(document.getElementById("data-authentication")); + }); + + test("email invite link with authentication data", async function (assert) { + preloadStore(false); + + // Simulate authticated with Facebook + const node = document.createElement("meta"); + node.dataset.authenticationData = JSON.stringify({ + auth_provider: "facebook", + email: "foobar@example.com", + email_valid: true, + username: "foobar", + name: "barfoo", + }); + node.id = "data-authentication"; + document.querySelector("head").appendChild(node); + + await visit("/invites/myvalidinvitetoken"); + + assert.ok( + !exists(".btn-social.facebook"), + "does not show Facebook login button" + ); + + assert.ok(!exists("#new-account-password"), "does not show password field"); + assert.ok(!exists("#new-account-email"), "does not show email field"); + + assert.equal( + queryAll("#account-email-validation").text().trim(), + I18n.t("user.email.authenticated", { provider: "Facebook" }) + ); + + assert.equal( + queryAll("#new-account-username").val(), + "foobar", + "username is prefilled" + ); + + assert.equal( + queryAll("#new-account-name").val(), + "barfoo", + "name is prefilled" + ); + + document + .querySelector("head") + .removeChild(document.getElementById("data-authentication")); + }); +}); diff --git a/app/assets/stylesheets/common/base/login.scss b/app/assets/stylesheets/common/base/login.scss index 637df401453..b70d0f9544c 100644 --- a/app/assets/stylesheets/common/base/login.scss +++ b/app/assets/stylesheets/common/base/login.scss @@ -33,7 +33,8 @@ // Create Account + Login .d-modal.create-account, -.d-modal.login-modal { +.d-modal.login-modal, +.invites-show { .modal-inner-container { position: relative; } @@ -267,6 +268,14 @@ .two-col { position: relative; display: flex; + margin-top: 5px; + } + + #login-buttons { + .btn { + background-color: var(--primary-low); + color: var(--primary); + } } .col-image { diff --git a/app/controllers/invites_controller.rb b/app/controllers/invites_controller.rb index 0c3c615f5bd..9cb6c02a511 100644 --- a/app/controllers/invites_controller.rb +++ b/app/controllers/invites_controller.rb @@ -8,6 +8,7 @@ class InvitesController < ApplicationController skip_before_action :preload_json, except: [:show] skip_before_action :redirect_to_login_if_required + before_action :ensure_invites_allowed, only: [:show, :perform_accept_invitation] before_action :ensure_new_registrations_allowed, only: [:show, :perform_accept_invitation] before_action :ensure_not_logged_in, only: [:show, :perform_accept_invitation] @@ -168,7 +169,8 @@ class InvitesController < ApplicationController name: params[:name], password: params[:password], user_custom_fields: params[:user_custom_fields], - ip_address: request.remote_ip + ip_address: request.remote_ip, + session: session } attrs[:email] = @@ -284,6 +286,12 @@ class InvitesController < ApplicationController private + def ensure_invites_allowed + if SiteSetting.enable_discourse_connect || (!SiteSetting.enable_local_logins && Discourse.enabled_auth_providers.count == 0) + raise Discourse::NotFound + end + end + def ensure_new_registrations_allowed unless SiteSetting.allow_new_registrations flash[:error] = I18n.t('login.new_registrations_disabled') diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index 12e021e9edf..b12852b273b 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -119,7 +119,12 @@ class Users::OmniauthCallbacksController < ApplicationController end def invite_required? - SiteSetting.invite_only? + if SiteSetting.invite_only? + path = Discourse.route_for(@origin) + return true unless path + return true if path[:controller] != "invites" && path[:action] != "show" + !Invite.exists?(invite_key: path[:id]) + end end def user_found(user) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index efed469d0eb..945d50a5a51 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -447,8 +447,6 @@ class UsersController < ApplicationController elsif current_user&.staff? message = if SiteSetting.enable_discourse_connect I18n.t("invite.disabled_errors.discourse_connect_enabled") - elsif !SiteSetting.enable_local_logins - I18n.t("invite.disabled_errors.local_logins_disabled") end render_invite_error(message) diff --git a/app/models/invite.rb b/app/models/invite.rb index 53d4293a5b0..7aa89062d54 100644 --- a/app/models/invite.rb +++ b/app/models/invite.rb @@ -162,11 +162,20 @@ class Invite < ActiveRecord::Base invite.reload end - def redeem(email: nil, username: nil, name: nil, password: nil, user_custom_fields: nil, ip_address: nil) + def redeem(email: nil, username: nil, name: nil, password: nil, user_custom_fields: nil, ip_address: nil, session: nil) if !expired? && !destroyed? && link_valid? raise UserExists.new I18n.t("invite_link.email_taken") if is_invite_link? && UserEmail.exists?(email: email) email = self.email if email.blank? && !is_invite_link? - InviteRedeemer.new(invite: self, email: email, username: username, name: name, password: password, user_custom_fields: user_custom_fields, ip_address: ip_address).redeem + InviteRedeemer.new( + invite: self, + email: email, + username: username, + name: name, + password: password, + user_custom_fields: user_custom_fields, + ip_address: ip_address, + session: session + ).redeem end end @@ -251,8 +260,6 @@ class Invite < ActiveRecord::Base if SiteSetting.enable_discourse_connect? errors.add(:email, I18n.t("invite.disabled_errors.discourse_connect_enabled")) - elsif !SiteSetting.enable_local_logins? - errors.add(:email, I18n.t("invite.disabled_errors.local_logins_disabled")) end end end diff --git a/app/models/invite_redeemer.rb b/app/models/invite_redeemer.rb index 3b9edabd213..8c7b4aad303 100644 --- a/app/models/invite_redeemer.rb +++ b/app/models/invite_redeemer.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -InviteRedeemer = Struct.new(:invite, :email, :username, :name, :password, :user_custom_fields, :ip_address, keyword_init: true) do +InviteRedeemer = Struct.new(:invite, :email, :username, :name, :password, :user_custom_fields, :ip_address, :session, keyword_init: true) do def redeem Invite.transaction do @@ -14,7 +14,7 @@ InviteRedeemer = Struct.new(:invite, :email, :username, :name, :password, :user_ end # extracted from User cause it is very specific to invites - def self.create_user_from_invite(email:, invite:, username: nil, name: nil, password: nil, user_custom_fields: nil, ip_address: nil) + def self.create_user_from_invite(email:, invite:, username: nil, name: nil, password: nil, user_custom_fields: nil, ip_address: nil, session: nil) user = User.where(staged: true).with_email(email.strip.downcase).first user.unstage! if user @@ -61,7 +61,20 @@ InviteRedeemer = Struct.new(:invite, :email, :username, :name, :password, :user_ user.password_required! end + authenticator = UserAuthenticator.new(user, session, require_password: false) + + if !authenticator.has_authenticator? && !SiteSetting.enable_local_logins + raise ActiveRecord::RecordNotSaved.new(I18n.t("login.incorrect_username_email_or_password")) + end + + authenticator.start + + if authenticator.email_valid? && !authenticator.authenticated? + raise ActiveRecord::RecordNotSaved.new(I18n.t("login.incorrect_username_email_or_password")) + end + user.save! + authenticator.finish if invite.emailed_status != Invite.emailed_status_types[:not_required] && email == invite.email user.email_tokens.create!(email: user.email) @@ -110,7 +123,16 @@ InviteRedeemer = Struct.new(:invite, :email, :username, :name, :password, :user_ def get_invited_user result = get_existing_user - result ||= InviteRedeemer.create_user_from_invite(email: email, invite: invite, username: username, name: name, password: password, user_custom_fields: user_custom_fields, ip_address: ip_address) + result ||= InviteRedeemer.create_user_from_invite( + email: email, + invite: invite, + username: username, + name: name, + password: password, + user_custom_fields: user_custom_fields, + ip_address: ip_address, + session: session + ) result.send_welcome_message = false result end diff --git a/app/services/user_authenticator.rb b/app/services/user_authenticator.rb index 822c9b5d1f2..48d0b8038a9 100644 --- a/app/services/user_authenticator.rb +++ b/app/services/user_authenticator.rb @@ -2,20 +2,21 @@ class UserAuthenticator - def initialize(user, session, authenticator_finder = Users::OmniauthCallbacksController) + def initialize(user, session, authenticator_finder: Users::OmniauthCallbacksController, require_password: true) @user = user @session = session - if session[:authentication] && session[:authentication].is_a?(Hash) + if session&.dig(:authentication) && session[:authentication].is_a?(Hash) @auth_result = Auth::Result.from_session_data(session[:authentication], user: user) end @authenticator_finder = authenticator_finder + @require_password = require_password end def start if authenticated? @user.active = true @auth_result.apply_user_attributes! - else + elsif @require_password @user.password_required! end @@ -31,7 +32,7 @@ class UserAuthenticator authenticator.after_create_account(@user, @auth_result) confirm_email end - @session[:authentication] = @auth_result = nil if @session[:authentication] + @session[:authentication] = @auth_result = nil if @session&.dig(:authentication) end def email_valid? diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 614c2cf8d09..cf7330e4e54 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -1293,6 +1293,7 @@ en: required: "Please enter an email address" invalid: "Please enter a valid email address" authenticated: "Your email has been authenticated by %{provider}" + invite_auth_email_invalid: "Your invitation email does not match the email authenticated by %{provider}" frequency_immediately: "We'll email you immediately if you haven't read the thing we're emailing you about." frequency: one: "We'll only email you if we haven't seen you in the last minute." diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 64df440feed..8bf8828d7f5 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -241,7 +241,6 @@ en: cant_invite_to_group: "You are not allowed to invite users to specified group(s). Make sure you are owner of the group(s) you are trying to invite to." disabled_errors: discourse_connect_enabled: "Invites are disabled because DiscourseConnect is enabled." - local_logins_disabled: "Invites are disabled because the 'enable local logins' setting is disabled." invalid_access: "You are not permitted to view the requested resource." bulk_invite: @@ -1681,7 +1680,7 @@ en: discourse_connect_not_approved_url: "Redirect unapproved DiscourseConnect accounts to this URL" discourse_connect_allows_all_return_paths: "Do not restrict the domain for return_paths provided by DiscourseConnect (by default return path must be on current site)" - enable_local_logins: "Enable local username and password login based accounts. This must be enabled for invites to work. WARNING: if disabled, you may be unable to log in if you have not previously configured at least one alternate login method." + enable_local_logins: "Enable local username and password login based accounts. WARNING: if disabled, you may be unable to log in if you have not previously configured at least one alternate login method." enable_local_logins_via_email: "Allow users to request a one-click login link to be sent to them via email." allow_new_registrations: "Allow new user registrations. Uncheck this to prevent anyone from creating a new account." enable_signup_cta: "Show a notice to returning anonymous users prompting them to sign up for an account." diff --git a/lib/guardian.rb b/lib/guardian.rb index 779148222a2..f65029bd1b7 100644 --- a/lib/guardian.rb +++ b/lib/guardian.rb @@ -354,7 +354,6 @@ class Guardian authenticated? && (SiteSetting.max_invites_per_day.to_i > 0 || is_staff?) && !SiteSetting.enable_discourse_connect && - SiteSetting.enable_local_logins && ( (!SiteSetting.must_approve_users? && @user.has_trust_level?(SiteSetting.min_trust_level_to_allow_invite.to_i)) || is_staff? @@ -395,9 +394,7 @@ class Guardian end def can_bulk_invite_to_forum?(user) - user.admin? && - !SiteSetting.enable_discourse_connect && - SiteSetting.enable_local_logins + user.admin? && !SiteSetting.enable_discourse_connect end def can_resend_all_invites?(user) diff --git a/spec/components/guardian_spec.rb b/spec/components/guardian_spec.rb index c292582360c..04a051ad3e5 100644 --- a/spec/components/guardian_spec.rb +++ b/spec/components/guardian_spec.rb @@ -511,8 +511,10 @@ describe Guardian do expect(Guardian.new(user).can_invite_to_forum?).to be_falsey end - it 'returns false when the local logins are disabled' do - SiteSetting.enable_local_logins = false + it 'returns false when DiscourseConnect is enabled' do + SiteSetting.discourse_connect_url = "https://www.example.com/sso" + SiteSetting.enable_discourse_connect = true + expect(Guardian.new(user).can_invite_to_forum?).to be_falsey expect(Guardian.new(moderator).can_invite_to_forum?).to be_falsey end diff --git a/spec/requests/invites_controller_spec.rb b/spec/requests/invites_controller_spec.rb index de057c726ef..942bb6046ad 100644 --- a/spec/requests/invites_controller_spec.rb +++ b/spec/requests/invites_controller_spec.rb @@ -374,6 +374,97 @@ describe InvitesController do expect(invite.redeemed?).to be_truthy end + it 'returns the right response when local login is disabled and no external auth is configured' do + SiteSetting.enable_local_logins = false + + put "/invites/show/#{invite.invite_key}.json" + + expect(response.status).to eq(404) + end + + it 'returns the right response when DiscourseConnect is enabled' do + invite + SiteSetting.discourse_connect_url = "https://www.example.com/sso" + SiteSetting.enable_discourse_connect = true + + put "/invites/show/#{invite.invite_key}.json" + + expect(response.status).to eq(404) + end + + describe 'with authentication session' do + let(:authenticated_email) { "foobar@example.com" } + + before do + OmniAuth.config.test_mode = true + + OmniAuth.config.mock_auth[:google_oauth2] = OmniAuth::AuthHash.new( + provider: 'google_oauth2', + uid: '12345', + info: OmniAuth::AuthHash::InfoHash.new( + email: authenticated_email, + name: 'First Last' + ), + extra: { + raw_info: OmniAuth::AuthHash.new( + email_verified: true, + email: authenticated_email, + family_name: "Last", + given_name: "First", + gender: "male", + name: "First Last", + ) + }, + ) + + Rails.application.env_config["omniauth.auth"] = OmniAuth.config.mock_auth[:google_oauth2] + SiteSetting.enable_google_oauth2_logins = true + + get "/auth/google_oauth2/callback.json" + expect(response.status).to eq(302) + end + + after do + Rails.application.env_config["omniauth.auth"] = OmniAuth.config.mock_auth[:google_oauth2] = nil + OmniAuth.config.test_mode = false + end + + it 'should associate the invited user with authenticator records' do + invite.update!(email: authenticated_email) + SiteSetting.auth_overrides_name = true + + expect do + put "/invites/show/#{invite.invite_key}.json", + params: { name: 'somename' } + + expect(response.status).to eq(200) + end.to change { User.with_email(authenticated_email).exists? }.to(true) + + user = User.find_by_email(authenticated_email) + + expect(user.name).to eq('First Last') + + expect(user.user_associated_accounts.first.provider_name) + .to eq("google_oauth2") + end + + it 'returns the right response even if local logins has been disabled' do + SiteSetting.enable_local_logins = false + + invite.update!(email: authenticated_email) + + put "/invites/show/#{invite.invite_key}.json" + + expect(response.status).to eq(200) + end + + it 'returns the right response if authenticated email does not match invite email' do + put "/invites/show/#{invite.invite_key}.json" + + expect(response.status).to eq(412) + end + end + context 'when redeem returns a user' do fab!(:user) { Fabricate(:coding_horror) } @@ -447,27 +538,6 @@ describe InvitesController do expect(Jobs::InvitePasswordInstructionsEmail.jobs.size).to eq(1) expect(Jobs::CriticalUserEmail.jobs.size).to eq(0) end - - it "does not send password reset email if sso is enabled" do - invite # create the invite before enabling SSO - SiteSetting.discourse_connect_url = "https://www.example.com/sso" - SiteSetting.enable_discourse_connect = true - put "/invites/show/#{invite.invite_key}.json" - expect(response.status).to eq(200) - - expect(Jobs::InvitePasswordInstructionsEmail.jobs.size).to eq(0) - expect(Jobs::CriticalUserEmail.jobs.size).to eq(0) - end - - it "does not send password reset email if local login is disabled" do - invite # create the invite before enabling SSO - SiteSetting.enable_local_logins = false - put "/invites/show/#{invite.invite_key}.json" - expect(response.status).to eq(200) - - expect(Jobs::InvitePasswordInstructionsEmail.jobs.size).to eq(0) - expect(Jobs::CriticalUserEmail.jobs.size).to eq(0) - end end context "with password" do diff --git a/spec/requests/omniauth_callbacks_controller_spec.rb b/spec/requests/omniauth_callbacks_controller_spec.rb index cec61b35df4..60245307178 100644 --- a/spec/requests/omniauth_callbacks_controller_spec.rb +++ b/spec/requests/omniauth_callbacks_controller_spec.rb @@ -12,6 +12,7 @@ RSpec.describe Users::OmniauthCallbacksController do after do Rails.application.env_config["omniauth.auth"] = OmniAuth.config.mock_auth[:google_oauth2] = nil + Rails.application.env_config["omniauth.origin"] = nil OmniAuth.config.test_mode = false end @@ -221,6 +222,48 @@ RSpec.describe Users::OmniauthCallbacksController do data = JSON.parse(cookies[:authentication_data]) expect(data["destination_url"]).to eq(destination_url) end + + describe 'when site is invite_only' do + before do + SiteSetting.invite_only = true + end + + it 'should return the right response without any origin' do + get "/auth/google_oauth2/callback.json" + + expect(response.status).to eq(302) + + data = JSON.parse(response.cookies["authentication_data"]) + + expect(data["requires_invite"]).to eq(true) + end + + it 'returns the right response for an invalid origin' do + Rails.application.env_config["omniauth.origin"] = "/invitesinvites" + + get "/auth/google_oauth2/callback.json" + + expect(response.status).to eq(302) + end + + it 'should return the right response when origin is invites page' do + origin = Rails.application.routes.url_helpers.invite_url( + Fabricate(:invite).invite_key, + host: Discourse.base_url + ) + + Rails.application.env_config["omniauth.origin"] = origin + + get "/auth/google_oauth2/callback.json" + + expect(response.status).to eq(302) + expect(response).to redirect_to(origin) + + data = JSON.parse(response.cookies["authentication_data"]) + + expect(data["requires_invite"]).to eq(nil) + end + end end describe 'when user has been verified' do diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index dca299f508c..a4a948aced8 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -1755,20 +1755,24 @@ describe UsersController do expect(response.status).to eq(403) end end + end - context 'when local logins are disabled' do - it 'explains why invites are disabled to staff users' do - SiteSetting.enable_local_logins = false - inviter = sign_in(Fabricate(:admin)) - Fabricate(:invite, invited_by: inviter, email: nil, max_redemptions_allowed: 5, expires_at: 1.month.from_now, emailed_status: Invite.emailed_status_types[:not_required]) + context 'when DiscourseConnect has been enabled' do + before do + SiteSetting.discourse_connect_url = "https://www.example.com/sso" + SiteSetting.enable_discourse_connect = true + end - get "/u/#{inviter.username}/invited/pending.json" - expect(response.status).to eq(200) + it 'explains why invites are disabled to staff users' do + inviter = sign_in(Fabricate(:admin)) + Fabricate(:invite, invited_by: inviter, email: nil, max_redemptions_allowed: 5, expires_at: 1.month.from_now, emailed_status: Invite.emailed_status_types[:not_required]) - expect(response.parsed_body['error']).to include(I18n.t( - 'invite.disabled_errors.local_logins_disabled' - )) - end + get "/u/#{inviter.username}/invited/pending.json" + expect(response.status).to eq(200) + + expect(response.parsed_body['error']).to include(I18n.t( + 'invite.disabled_errors.discourse_connect_enabled' + )) end end diff --git a/spec/services/user_authenticator_spec.rb b/spec/services/user_authenticator_spec.rb index 07cce902857..8f2b86b686c 100644 --- a/spec/services/user_authenticator_spec.rb +++ b/spec/services/user_authenticator_spec.rb @@ -2,30 +2,48 @@ require 'rails_helper' -def github_auth(email_valid) - { - email: "user53@discourse.org", - username: "joedoe546", - email_valid: email_valid, - omit_username: nil, - name: "Joe Doe 546", - authenticator_name: "github", - extra_data: { - provider: "github", - uid: "100" - }, - skip_email_validation: false - } -end - describe UserAuthenticator do + def github_auth(email_valid) + { + email: "user53@discourse.org", + username: "joedoe546", + email_valid: email_valid, + omit_username: nil, + name: "Joe Doe 546", + authenticator_name: "github", + extra_data: { + provider: "github", + uid: "100" + }, + skip_email_validation: false + } + end + + before do + SiteSetting.enable_github_logins = true + end + + describe "#start" do + describe 'without authentication session' do + it "should apply the right user attributes" do + user = User.new + UserAuthenticator.new(user, {}).start + + expect(user.password_required?).to eq(true) + end + + it "allows password requirement to be skipped" do + user = User.new + UserAuthenticator.new(user, {}, require_password: false).start + + expect(user.password_required?).to eq(false) + end + end + end + context "#finish" do fab!(:group) { Fabricate(:group, automatic_membership_email_domains: "discourse.org") } - before do - SiteSetting.enable_github_logins = true - end - it "confirms email and adds the user to appropraite groups based on email" do user = Fabricate(:user, email: "user53@discourse.org") expect(group.usernames).not_to include(user.username)