Merge pull request #5848 from OsamaSayegh/fix-social-login-groups

FIX: apply automatic group rules when using social login providers

app/services/user_authenticator.rb

@@ -21,7 +21,10 @@ class UserAuthenticator
   end
 
   def finish
-    authenticator.after_create_account(@user, @session) if authenticator
+    if authenticator
+      authenticator.after_create_account(@user, @session)
+      confirm_email
+    end
     @session = nil
   end
 
@@ -30,11 +33,18 @@ class UserAuthenticator
   end
 
   def authenticated?
-    @session && @session[:email] == @user.email && @session[:email_valid]
+    @session && @session[:email]&.downcase == @user.email.downcase && @session[:email_valid].to_s == "true"
   end
 
   private
 
+  def confirm_email
+    if authenticated?
+      EmailToken.confirm(@user.email_tokens.first.token)
+      @user.set_automatic_groups
+    end
+  end
+
   def authenticator
     if authenticator_name
       @authenticator ||= @authenticator_finder.find_authenticator(authenticator_name)

lib/auth/google_oauth2_authenticator.rb

@@ -44,10 +44,6 @@ class Auth::GoogleOAuth2Authenticator < Auth::Authenticator
   def after_create_account(user, auth)
     data = auth[:extra_data]
     GoogleUserInfo.create({ user_id: user.id }.merge(data))
-    if auth[:email_valid].to_s == 'true' && data[:email]&.downcase == user.email
-      EmailToken.confirm(user.email_tokens.first.token)
-      user.set_automatic_groups
-    end
   end
 
   def register_middleware(omniauth)

spec/components/auth/google_oauth2_authenticator_spec.rb

@@ -81,35 +81,4 @@ describe Auth::GoogleOAuth2Authenticator do
       expect(result.extra_data[:name]).to eq("Jane Doe")
     end
   end
-
-  context 'after_create_account' do
-    it 'confirms email' do
-      authenticator = Auth::GoogleOAuth2Authenticator.new
-      user = Fabricate(:user, email: 'realgoogleuser@gmail.com')
-      session = {
-        email_valid: "true",
-        extra_data: {
-          google_user_id: 1,
-          email: 'realgoogleuser@gmail.com'
-        }
-      }
-      authenticator.after_create_account(user, session)
-      expect(user.email_confirmed?).to eq(true)
-    end
-
-    it "doesn't confirm email if it was changed" do
-      authenticator = Auth::GoogleOAuth2Authenticator.new
-      user = Fabricate(:user, email: 'changed@gmail.com')
-      session = {
-        email_valid: "true",
-        extra_data: {
-          google_user_id: 1,
-          email: 'realgoogleuser@gmail.com'
-        }
-      }
-      authenticator.after_create_account(user, session)
-      expect(user.email_confirmed?).to eq(false)
-    end
-  end
-
 end

spec/services/user_authenticator_spec.rb

@@ -0,0 +1,60 @@
+require 'rails_helper'
+require_dependency 'user_authenticator'
+
+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: {
+      github_user_id: "100",
+      github_screen_name: "joedoe546"
+    },
+    skip_email_validation: false
+  }
+end
+
+describe UserAuthenticator do
+  context "#finish" do
+    let(:authenticator) { Auth::GithubAuthenticator.new }
+    let(: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)
+
+      authentication = github_auth(true)
+
+      UserAuthenticator.new(user, authentication: authentication).finish
+      expect(user.email_confirmed?).to be_truthy
+      expect(group.usernames).to include(user.username)
+    end
+
+    it "doesn't confirm email if email is invalid" do
+      user = Fabricate(:user, email: "user53@discourse.org")
+
+      authentication = github_auth(false)
+
+      UserAuthenticator.new(user, authentication: authentication).finish
+      expect(user.email_confirmed?).to be_falsey
+      expect(group.usernames).not_to include(user.username)
+    end
+
+    it "doesn't confirm email if it was changed" do
+      user = Fabricate(:user, email: "changed@discourse.org")
+
+      authentication = github_auth(true)
+
+      UserAuthenticator.new(user, authentication: authentication).finish
+      expect(user.email_confirmed?).to be_falsey
+      expect(group.usernames).not_to include(user.username)
+    end
+  end
+end