New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX: Importing user avatar when new user login by SSO #4213

Merged
merged 1 commit into from Sep 2, 2016

Conversation

5 participants
@fantasticfears
Copy link
Contributor

fantasticfears commented May 7, 2016

As per: https://meta.discourse.org/t/official-single-sign-on-for-discourse/13045/292?u=fantasticfears

This PR contains a rather big refactor. But essentially it's a fix to ensure overrides semantics. Now the code structures like setup non-persistent model, update attrs, persistent models and overrides.
Integration tests build several rounds to ensure the order.

@mention-bot

This comment has been minimized.

Copy link

mention-bot commented May 7, 2016

By analyzing the blame information on this pull request, we identified @techAPJ and @SamSaffron to be potential reviewers

@discoursebot

This comment has been minimized.

Copy link

discoursebot commented May 7, 2016

You've signed the CLA, fantasticfears. Thank you! This pull request is ready for review.

@fantasticfears fantasticfears force-pushed the fantasticfears:sso branch May 7, 2016

@fantasticfears

View changes

spec/controllers/session_controller_spec.rb Outdated
@@ -264,63 +278,6 @@ def sso_for_ip_specs
expect(response.code).to eq('419')
end

describe 'can act as an SSO provider' do

This comment has been minimized.

@fantasticfears

fantasticfears May 7, 2016

Contributor

this was just rename to .sso_provider

@fantasticfears fantasticfears force-pushed the fantasticfears:sso branch 3 times, most recently to 9d5ec54 May 17, 2016

if SiteSetting.sso_overrides_email && user.email != email
user.email = email
def override_user_attributes
if SiteSetting.sso_overrides_email && @user_record.email != email

This comment has been minimized.

@fantasticfears

fantasticfears May 17, 2016

Contributor

no presents check here. Don't know what's the original intention. Keep as-is.

@fantasticfears

This comment has been minimized.

Copy link
Contributor

fantasticfears commented May 17, 2016

I don't know why I forget to check in the fix for importing avatar for new user in the first place...This is included now.

Rebased and checks by integration spec.

@fantasticfears fantasticfears changed the title Ensure SSO flow FEATURE: Importing user avatar when new user login by SSO May 24, 2016

@fantasticfears fantasticfears changed the title FEATURE: Importing user avatar when new user login by SSO FIX: Importing user avatar when new user login by SSO May 24, 2016

@coding-horror

This comment has been minimized.

Copy link
Member

coding-horror commented May 28, 2016

It is up to @sam to review this and pull it in

@SamSaffron

This comment has been minimized.

Copy link
Member

SamSaffron commented Jun 29, 2016

This really worries me ... why is so much code being changed its very hard to digest it all and make sure there are zero regressions

@fantasticfears

This comment has been minimized.

Copy link
Contributor

fantasticfears commented Jun 30, 2016

@sam SSO is quite complicated in the core... One reason is to clean up process since it's getting complicated. Some changes depend on whether a record is save or not. The other one is the bug fix. I simply failed to find where is the problem in the beginning.

I can still find a place to enforce the sso_overrides_avatar in the old code if you'd like. Or I can write many more specs to address the possible problems?

@fantasticfears fantasticfears force-pushed the fantasticfears:sso branch 2 times, most recently Aug 29, 2016

@fantasticfears

This comment has been minimized.

Copy link
Contributor

fantasticfears commented Aug 29, 2016

Now this PR only fixes the avatar stuff.

@SamSaffron

This comment has been minimized.

Copy link
Member

SamSaffron commented Aug 29, 2016

travis failures seem legit, can you have a look @fantasticfears ?

@fantasticfears fantasticfears force-pushed the fantasticfears:sso branch Aug 29, 2016

@fantasticfears

This comment has been minimized.

Copy link
Contributor

fantasticfears commented Aug 29, 2016

Fixed!

@fantasticfears fantasticfears force-pushed the fantasticfears:sso branch to 0217973 Aug 29, 2016

@SamSaffron

This comment has been minimized.

Copy link
Member

SamSaffron commented Sep 2, 2016

cool, we just had this reported by a customer :) merging in

@SamSaffron SamSaffron merged commit 211c374 into discourse:master Sep 2, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment