From 4c26c4d9bcf8a16c245e69a7a1998e47e3c0d193 Mon Sep 17 00:00:00 2001
From: Paul Kaplan <paul@inventables.com>
Date: Fri, 15 May 2015 12:00:34 -0500
Subject: [PATCH 1/3] Add a SiteSetting to not trust sso emails by default
---
app/models/discourse_single_sign_on.rb | 2 +-
config/locales/server.en.yml | 1 +
config/site_settings.yml | 1 +
spec/models/discourse_single_sign_on_spec.rb | 23 ++++++++++++++++++++
4 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/app/models/discourse_single_sign_on.rb b/app/models/discourse_single_sign_on.rb
index db2387923..8eae077d8 100644
--- a/app/models/discourse_single_sign_on.rb
+++ b/app/models/discourse_single_sign_on.rb
@@ -57,7 +57,7 @@ class DiscourseSingleSignOn < SingleSignOn
change_external_attributes_and_override(sso_record, user)
end
- if sso_record && (user = sso_record.user) && !user.active
+ if sso_record && (user = sso_record.user) && !user.active && SiteSetting.sso_trusts_email
user.active = true
user.save!
user.enqueue_welcome_message('welcome_user') unless suppress_welcome_message
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index 28c064ccf..4f22b37f1 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -885,6 +885,7 @@ en:
enable_sso_provider: "Implement Discourse SSO provider protocol at the /session/sso_provider endpoint, requires sso_secret to be set"
sso_url: "URL of single sign on endpoint"
sso_secret: "Secret string used to cryptographically authenticate SSO information, be sure it is 10 characters or longer"
+ sso_trusts_email: "Allow SSO accounts to skip email verification"
sso_overrides_email: "Overrides local email with external site email from SSO payload (WARNING: discrepancies can occur due to normalization of local emails)"
sso_overrides_username: "Overrides local username with external site username from SSO payload (WARNING: discrepancies can occur due to differences in username length/requirements)"
sso_overrides_name: "Overrides local full name with external site full name from SSO payload"
diff --git a/config/site_settings.yml b/config/site_settings.yml
index c53d526ee..94e21c864 100644
--- a/config/site_settings.yml
+++ b/config/site_settings.yml
@@ -234,6 +234,7 @@ login:
enable_sso_provider: false
sso_url: ''
sso_secret: ''
+ sso_trusts_email: true
sso_overrides_email: false
sso_overrides_username: false
sso_overrides_name: false
diff --git a/spec/models/discourse_single_sign_on_spec.rb b/spec/models/discourse_single_sign_on_spec.rb
index d102e291c..2430e3176 100644
--- a/spec/models/discourse_single_sign_on_spec.rb
+++ b/spec/models/discourse_single_sign_on_spec.rb
@@ -140,6 +140,29 @@ describe DiscourseSingleSignOn do
expect(sso.nonce).to_not be_nil
end
+ context 'trusting emails' do
+ let(:sso) {
+ sso = DiscourseSingleSignOn.new
+ sso.username = "test"
+ sso.name = "test"
+ sso.email = "test@example.com"
+ sso.external_id = "A"
+ sso
+ }
+
+ it 'activates users by default' do
+ user = sso.lookup_or_create_user(ip_address)
+ expect(user.active).to eq(true)
+ end
+
+ it 'does not activate user when asked to' do
+ SiteSetting.sso_trusts_email = false
+ user = sso.lookup_or_create_user(ip_address)
+ expect(user.active).to eq(false)
+ end
+
+ end
+
context 'welcome emails' do
let(:sso) {
sso = DiscourseSingleSignOn.new
From b8a43e153c4080c510af61105736f7bc97a3495e Mon Sep 17 00:00:00 2001
From: Paul Kaplan <paul@inventables.com>
Date: Fri, 15 May 2015 12:01:30 -0500
Subject: [PATCH 2/3] Use session controller to prevent inactive SSO users
---
app/controllers/session_controller.rb | 5 +++
spec/controllers/session_controller_spec.rb | 48 +++++++++++++++++++++
2 files changed, 53 insertions(+)
diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb
index f1c56ba68..8223a95ed 100644
--- a/app/controllers/session_controller.rb
+++ b/app/controllers/session_controller.rb
@@ -73,6 +73,11 @@ class SessionController < ApplicationController
if SiteSetting.must_approve_users? && !user.approved?
render text: I18n.t("sso.account_not_approved"), status: 403
return
+ elsif !user.active?
+ activation = UserActivator.new(user, request, session, cookies)
+ activation.finish
+ session["user_created_message"] = activation.message
+ redirect_to users_account_created_path and return
else
log_on_user user
end
diff --git a/spec/controllers/session_controller_spec.rb b/spec/controllers/session_controller_spec.rb
index 309152e9d..093f4a3c5 100644
--- a/spec/controllers/session_controller_spec.rb
+++ b/spec/controllers/session_controller_spec.rb
@@ -193,6 +193,54 @@ describe SessionController do
expect(logged_on_user.custom_fields["bla"]).to eq(nil)
end
+ context 'when sso emails are not trusted' do
+ before do
+ SiteSetting.sso_trusts_email = false
+ end
+
+ context 'if you have not activated your account' do
+ it 'does not log you in' do
+ sso = get_sso('/a/')
+ sso.external_id = '666' # the number of the beast
+ sso.email = 'bob@bob.com'
+ sso.name = 'Sam Saffron'
+ sso.username = 'sam'
+
+ get :sso_login, Rack::Utils.parse_query(sso.payload)
+
+ logged_on_user = Discourse.current_user_provider.new(request.env).current_user
+ expect(logged_on_user).to eq(nil)
+ end
+
+ it 'sends an activation email' do
+ Jobs.expects(:enqueue).with(:user_email, has_entries(type: :signup))
+ sso = get_sso('/a/')
+ sso.external_id = '666' # the number of the beast
+ sso.email = 'bob@bob.com'
+ sso.name = 'Sam Saffron'
+ sso.username = 'sam'
+ get :sso_login, Rack::Utils.parse_query(sso.payload)
+ end
+ end
+
+ context 'if you have activated your account' do
+ it 'allows you to log in' do
+ sso = get_sso('/hello/world')
+ sso.external_id = '997'
+ sso.sso_url = "http://somewhere.over.com/sso_login"
+
+ user = Fabricate(:user)
+ user.create_single_sign_on_record(external_id: '997', last_payload: '')
+ user.stubs(:active?).returns(true)
+
+ get :sso_login, Rack::Utils.parse_query(sso.payload)
+
+ logged_on_user = Discourse.current_user_provider.new(request.env).current_user
+ expect(user.id).to eq(logged_on_user.id)
+ end
+ end
+ end
+
it 'allows login to existing account with valid nonce' do
sso = get_sso('/hello/world')
sso.external_id = '997'
From 1c34341f31518e369c818ae6b6db415a53bb2250 Mon Sep 17 00:00:00 2001
From: Paul Kaplan <paul@inventables.com>
Date: Tue, 19 May 2015 11:16:02 -0500
Subject: [PATCH 3/3] Replace site setting with a payload attribute
---
app/models/discourse_single_sign_on.rb | 2 +-
config/locales/server.en.yml | 1 -
config/site_settings.yml | 1 -
lib/single_sign_on.rb | 2 +-
spec/controllers/session_controller_spec.rb | 8 ++++----
spec/models/discourse_single_sign_on_spec.rb | 4 ++--
6 files changed, 8 insertions(+), 10 deletions(-)
diff --git a/app/models/discourse_single_sign_on.rb b/app/models/discourse_single_sign_on.rb
index 8eae077d8..4966cf9a1 100644
--- a/app/models/discourse_single_sign_on.rb
+++ b/app/models/discourse_single_sign_on.rb
@@ -57,7 +57,7 @@ class DiscourseSingleSignOn < SingleSignOn
change_external_attributes_and_override(sso_record, user)
end
- if sso_record && (user = sso_record.user) && !user.active && SiteSetting.sso_trusts_email
+ if sso_record && (user = sso_record.user) && !user.active && !require_activation
user.active = true
user.save!
user.enqueue_welcome_message('welcome_user') unless suppress_welcome_message
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index 4f22b37f1..28c064ccf 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -885,7 +885,6 @@ en:
enable_sso_provider: "Implement Discourse SSO provider protocol at the /session/sso_provider endpoint, requires sso_secret to be set"
sso_url: "URL of single sign on endpoint"
sso_secret: "Secret string used to cryptographically authenticate SSO information, be sure it is 10 characters or longer"
- sso_trusts_email: "Allow SSO accounts to skip email verification"
sso_overrides_email: "Overrides local email with external site email from SSO payload (WARNING: discrepancies can occur due to normalization of local emails)"
sso_overrides_username: "Overrides local username with external site username from SSO payload (WARNING: discrepancies can occur due to differences in username length/requirements)"
sso_overrides_name: "Overrides local full name with external site full name from SSO payload"
diff --git a/config/site_settings.yml b/config/site_settings.yml
index 94e21c864..c53d526ee 100644
--- a/config/site_settings.yml
+++ b/config/site_settings.yml
@@ -234,7 +234,6 @@ login:
enable_sso_provider: false
sso_url: ''
sso_secret: ''
- sso_trusts_email: true
sso_overrides_email: false
sso_overrides_username: false
sso_overrides_name: false
diff --git a/lib/single_sign_on.rb b/lib/single_sign_on.rb
index 66285e4dc..f0875a306 100644
--- a/lib/single_sign_on.rb
+++ b/lib/single_sign_on.rb
@@ -1,5 +1,5 @@
class SingleSignOn
- ACCESSORS = [:nonce, :name, :username, :email, :avatar_url, :avatar_force_update,
+ ACCESSORS = [:nonce, :name, :username, :email, :avatar_url, :avatar_force_update, :require_activation,
:about_me, :external_id, :return_sso_url, :admin, :moderator, :suppress_welcome_message]
FIXNUMS = []
BOOLS = [:avatar_force_update, :admin, :moderator, :suppress_welcome_message]
diff --git a/spec/controllers/session_controller_spec.rb b/spec/controllers/session_controller_spec.rb
index 093f4a3c5..683e7ced1 100644
--- a/spec/controllers/session_controller_spec.rb
+++ b/spec/controllers/session_controller_spec.rb
@@ -194,10 +194,6 @@ describe SessionController do
end
context 'when sso emails are not trusted' do
- before do
- SiteSetting.sso_trusts_email = false
- end
-
context 'if you have not activated your account' do
it 'does not log you in' do
sso = get_sso('/a/')
@@ -205,6 +201,7 @@ describe SessionController do
sso.email = 'bob@bob.com'
sso.name = 'Sam Saffron'
sso.username = 'sam'
+ sso.require_activation = true
get :sso_login, Rack::Utils.parse_query(sso.payload)
@@ -219,6 +216,8 @@ describe SessionController do
sso.email = 'bob@bob.com'
sso.name = 'Sam Saffron'
sso.username = 'sam'
+ sso.require_activation = true
+
get :sso_login, Rack::Utils.parse_query(sso.payload)
end
end
@@ -228,6 +227,7 @@ describe SessionController do
sso = get_sso('/hello/world')
sso.external_id = '997'
sso.sso_url = "http://somewhere.over.com/sso_login"
+ sso.require_activation = true
user = Fabricate(:user)
user.create_single_sign_on_record(external_id: '997', last_payload: '')
diff --git a/spec/models/discourse_single_sign_on_spec.rb b/spec/models/discourse_single_sign_on_spec.rb
index 2430e3176..c49b3b44c 100644
--- a/spec/models/discourse_single_sign_on_spec.rb
+++ b/spec/models/discourse_single_sign_on_spec.rb
@@ -155,8 +155,8 @@ describe DiscourseSingleSignOn do
expect(user.active).to eq(true)
end
- it 'does not activate user when asked to' do
- SiteSetting.sso_trusts_email = false
+ it 'does not activate user when asked not to' do
+ sso.require_activation = true
user = sso.lookup_or_create_user(ip_address)
expect(user.active).to eq(false)
end