diff --git a/app/models/discourse_single_sign_on.rb b/app/models/discourse_single_sign_on.rb index 111a3a295..f97955654 100644 --- a/app/models/discourse_single_sign_on.rb +++ b/app/models/discourse_single_sign_on.rb @@ -39,31 +39,19 @@ class DiscourseSingleSignOn < SingleSignOn "SSO_NONCE_#{nonce}" end - def lookup_or_create_user sso_record = SingleSignOnRecord.where(external_id: external_id).first - if sso_record && sso_record.user + + if sso_record && user = sso_record.user sso_record.last_payload = unsigned_payload - sso_record.save else - user = User.where(email: Email.downcase(email)).first - - user_params = { - email: email, - name: User.suggest_name(name || username || email), - username: UserNameSuggester.suggest(username || name || email), - } - - if user || user = User.create(user_params) - if sso_record = user.single_sign_on_record - sso_record.last_payload = unsigned_payload - sso_record.external_id = external_id - sso_record.save! - else - sso_record = user.create_single_sign_on_record(last_payload: unsigned_payload, - external_id: external_id) - end - end + user = match_email_or_create_user + sso_record = user.single_sign_on_record + end + + # if the user isn't new or it's attached to the SSO record we might be overriding username or email + unless user.new_record? + change_external_attributes_and_override(sso_record, user) end if sso_record && (user = sso_record.user) && !user.active @@ -71,8 +59,62 @@ class DiscourseSingleSignOn < SingleSignOn user.save user.enqueue_welcome_message('welcome_user') end + + # optionally save the user and sso_record if they have changed + user.save! + sso_record.save! sso_record && sso_record.user end -end + + private + + def match_email_or_create_user + user = User.where(email: Email.downcase(email)).first + user_params = { + email: email, + name: User.suggest_name(name || username || email), + username: UserNameSuggester.suggest(username || name || email), + } + + if user || user = User.create(user_params) + if sso_record = user.single_sign_on_record + sso_record.last_payload = unsigned_payload + sso_record.external_id = external_id + else + sso_record = user.create_single_sign_on_record(last_payload: unsigned_payload, + external_id: external_id, + external_username: username, + external_email: email, + external_name: name) + end + end + + user + end + + def change_external_attributes_and_override(sso_record, user) + if SiteSetting.sso_overrides_email && email != sso_record.external_email + # set the user's email to whatever came in the payload + user.email = email + end + + if SiteSetting.sso_overrides_username && username != sso_record.external_username && user.username != username + # we have an external username change, and the user's current username doesn't match + # run it through the UserNameSuggester to override it + user.username = UserNameSuggester.suggest(username || name || email) + end + + if SiteSetting.sso_overrides_name && name != sso_record.external_name && user.name != name + # we have an external name change, and the user's current name doesn't match + # run it through the name suggester to override it + user.name = User.suggest_name(name || username || email) + end + + # change external attributes for sso record + sso_record.external_username = username + sso_record.external_email = email + sso_record.external_name = name + end +end \ No newline at end of file diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 5b2e742ba..157245b64 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -676,6 +676,9 @@ en: enable_sso: "Enable single sign on via an external site" sso_url: "URL of single sign on endpoint" sso_secret: "Secret string used to encrypt/decrypt SSO information, be sure it is 10 chars or longer" + 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 name with external site name from SSO payload (WARNING: discrepancies can occur due to normalization of local names)" enable_local_logins: "Enable traditional, local username and password authentication" enable_local_account_create: "Enable creating new local accounts" diff --git a/config/site_settings.yml b/config/site_settings.yml index e740fdf86..de90a29ba 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -78,6 +78,15 @@ users: default: '' sso_secret: defalt: '' + sso_overrides_email: + client: true + default: false + sso_overrides_username: + client: true + default: false + sso_overrides_name: + client: true + default: false enable_local_logins: client: true default: true diff --git a/db/migrate/20140228005443_add_external_username_to_single_sign_on_record.rb b/db/migrate/20140228005443_add_external_username_to_single_sign_on_record.rb new file mode 100644 index 000000000..61a1a1bb7 --- /dev/null +++ b/db/migrate/20140228005443_add_external_username_to_single_sign_on_record.rb @@ -0,0 +1,5 @@ +class AddExternalUsernameToSingleSignOnRecord < ActiveRecord::Migration + def change + add_column :single_sign_on_records, :external_username, :string + end +end diff --git a/db/migrate/20140228173431_add_external_email_and_external_name_to_single_sign_on_record.rb b/db/migrate/20140228173431_add_external_email_and_external_name_to_single_sign_on_record.rb new file mode 100644 index 000000000..a56a611d8 --- /dev/null +++ b/db/migrate/20140228173431_add_external_email_and_external_name_to_single_sign_on_record.rb @@ -0,0 +1,6 @@ +class AddExternalEmailAndExternalNameToSingleSignOnRecord < ActiveRecord::Migration + def change + add_column :single_sign_on_records, :external_email, :string + add_column :single_sign_on_records, :external_name, :string + end +end diff --git a/lib/single_sign_on.rb b/lib/single_sign_on.rb index 9ba7a2eb7..5e7d3c460 100644 --- a/lib/single_sign_on.rb +++ b/lib/single_sign_on.rb @@ -1,5 +1,5 @@ class SingleSignOn - ACCESSORS = [:nonce, :name, :username, :email, :about_me, :external_id] + ACCESSORS = [:nonce, :name, :username, :email, :about_me, :external_email, :external_username, :external_name, :external_id] FIXNUMS = [] NONCE_EXPIRY_TIME = 10.minutes diff --git a/spec/controllers/session_controller_spec.rb b/spec/controllers/session_controller_spec.rb index 948689a7f..35f256355 100644 --- a/spec/controllers/session_controller_spec.rb +++ b/spec/controllers/session_controller_spec.rb @@ -35,15 +35,17 @@ describe SessionController do sso = get_sso("/") user = Fabricate(:user) sso.email = user.email - sso.external_id = "abc" + sso.external_id = 'abc' + sso.username = 'sam' get :sso_login, Rack::Utils.parse_query(sso.payload) response.should redirect_to('/') logged_on_user = Discourse.current_user_provider.new(request.env).current_user logged_on_user.email.should == user.email - + logged_on_user.single_sign_on_record.external_id.should == "abc" + logged_on_user.single_sign_on_record.external_username.should == 'sam' end it 'allows you to create an account' do @@ -63,11 +65,11 @@ describe SessionController do logged_on_user.username.should == 'sam' logged_on_user.single_sign_on_record.external_id.should == "666" + logged_on_user.single_sign_on_record.external_username.should == 'sam' logged_on_user.active.should == true end - + it 'allows login to existing account with valid nonce' do - sso = get_sso('/hello/world') sso.external_id = '997' @@ -87,8 +89,79 @@ describe SessionController do # nonce is bad now get :sso_login, Rack::Utils.parse_query(sso.payload) response.code.should == '500' - end + + describe 'local attribute ovveride from SSO payload' do + before do + SiteSetting.stubs("sso_overrides_email").returns(true) + SiteSetting.stubs("sso_overrides_username").returns(true) + SiteSetting.stubs("sso_overrides_name").returns(true) + + @user = Fabricate(:user) + + @sso = get_sso('/hello/world') + @sso.external_id = '997' + + @reversed_username = @user.username.reverse + @sso.username = @reversed_username + + @sso.email = "#{@reversed_username}@garbage.org" + @reversed_name = @user.name.reverse + + @sso.name = @reversed_name + + @suggested_username = UserNameSuggester.suggest(@sso.username || @sso.name || @sso.email) + @suggested_name = User.suggest_name(@sso.name || @sso.username || @sso.email) + + @user.create_single_sign_on_record(external_id: '997', last_payload: '') + end + + it 'stores the external attributes' do + get :sso_login, Rack::Utils.parse_query(@sso.payload) + + @user.single_sign_on_record.reload + @user.single_sign_on_record.external_username.should == @sso.username + @user.single_sign_on_record.external_email.should == @sso.email + @user.single_sign_on_record.external_name.should == @sso.name + end + + it 'overrides attributes' do + get :sso_login, Rack::Utils.parse_query(@sso.payload) + + logged_on_user = Discourse.current_user_provider.new(request.env).current_user + + logged_on_user.username.should == @suggested_username + logged_on_user.email.should == "#{@reversed_username}@garbage.org" + logged_on_user.name.should == @suggested_name + end + + it 'does not change matching attributes for an existing account' do + @sso.username = @user.username + @sso.name = @user.name + @sso.email = @user.email + + get :sso_login, Rack::Utils.parse_query(@sso.payload) + + logged_on_user = Discourse.current_user_provider.new(request.env).current_user + logged_on_user.username.should == @user.username + logged_on_user.name.should == @user.name + logged_on_user.email.should == @user.email + end + + it 'does not change attributes for unchanged external attributes' do + @user.single_sign_on_record.external_username = @sso.username + @user.single_sign_on_record.external_email = @sso.email + @user.single_sign_on_record.external_name = @sso.name + @user.single_sign_on_record.save + + get :sso_login, Rack::Utils.parse_query(@sso.payload) + + logged_on_user = Discourse.current_user_provider.new(request.env).current_user + logged_on_user.username.should == @user.username + logged_on_user.email.should == @user.email + logged_on_user.name.should == @user.name + end + end end describe '.create' do