SSO avatars not being overridden

windy · 2016-12-21 17:40:38 UTC

I know there are a few other posts about this, but none of them were able to solve my problem. Everything else in our SSO implementation works fine; this is the only thing that’s not working as expected. I’ve tried setting avatar_force_update to both true and 1 to no avail.

Code snippet w/ verbose log output: SSO.scala · GitHub

Any insight is much appreciated, thanks.

alehandrof · 2016-12-21 18:24:31 UTC

If you’re in development, maybe it’s this:

Edit: nevermind, you’re using non-local URLs.

windy · 2016-12-23 16:14:11 UTC

Bump (sorry)…

Falco · 2016-12-23 16:39:29 UTC

DId you enable sso overrides avatar ?

windy · 2016-12-23 16:51:40 UTC

Yes I did. Sorry should have mentioned that.

windy · 2016-12-27 22:16:27 UTC

@Falco Any other possible fixes you can think of? Kind of important I work this out.

Falco · 2016-12-27 22:47:18 UTC

In your gist avatar_force_update isn’t set, did you manage to make it true in verbose logging?

windy · 2016-12-27 23:49:52 UTC

Yes sorry, I tried both with and without, I had another instance where it was set to true and still no update.

Falco · 2016-12-28 00:00:52 UTC

So even with verbose_logging showing avatar_force_update = true the avatar isn’t updated?

Are you runiing against a production docker instance?

windy · 2016-12-28 00:16:22 UTC

Yeah, I tested on both production and a dev instance on OS X.

EDIT: Just realized I haven’t tested avatar_force_update on prod, will try that and report back.

windy · 2016-12-28 00:47:47 UTC

Ok just tested on our production docker instance @Falco and still no update with avatar_force_update: true in verbose logging.

Falco · 2016-12-28 01:24:07 UTC

The user already exists or is he being created as part of the SSO log in?

Also is /sidekiq with a queue? Any errors regarding DownloadAvatarFromUrl?

Also try this in a rails console in a production instance:

user = User.find_by_email(AUSEREMAILHERE)
Jobs.enqueue(:download_avatar_from_url, url: 'http://mycoolavatar.png', user_id: user.id)

and watch logs.

lukegb · 2017-01-17 01:19:41 UTC

For what it’s worth, it works fine. The main problem is that sso_overrides_avatar is misleadingly named, because it won’t, in fact, override a Gravatar set on a user.

The call to download_avatar_from_url here:

github.com

discourse/discourse/blob/v1.8.0.beta2/app/models/discourse_single_sign_on.rb#L183


  if SiteSetting.sso_overrides_name && user.name != name && name.present?
    user.name = name || User.suggest_name(username.blank? ? email : username)
  end


  avatar_missing = user.uploaded_avatar_id.nil? || !Upload.exists?(user.uploaded_avatar_id)


  if (avatar_missing || avatar_force_update || SiteSetting.sso_overrides_avatar) && avatar_url.present?
    avatar_changed = sso_record.external_avatar_url != avatar_url


    if avatar_force_update || avatar_changed || avatar_missing
      Jobs.enqueue(:download_avatar_from_url, url: avatar_url, user_id: user.id)
    end
  end


  # change external attributes for sso record
  sso_record.external_username = username
  sso_record.external_email = email
  sso_record.external_name = name
  sso_record.external_avatar_url = avatar_url
end
end

doesn’t pass override_gravatar, so:

github.com

discourse/discourse/blob/v1.8.0.beta2/app/jobs/regular/download_avatar_from_url.rb#L15




  def execute(args)
    url = args[:url]
    user_id = args[:user_id]


    raise Discourse::InvalidParameters.new(:url) if url.blank?
    raise Discourse::InvalidParameters.new(:user_id) if user_id.blank?


    return unless user = User.find_by(id: user_id)


    UserAvatar.import_url_for_user(url, user, override_gravatar: args[:override_gravatar])
  end


end


end

doesn’t pass on override_gravatar, so:

github.com

discourse/discourse/blob/63a88ee6e7bd9c1cabbea48afd745e20be370275/app/models/user_avatar.rb#L76


  ext = FastImage.type(tempfile).to_s
  tempfile.rewind


  upload = Upload.create_for(user.id, tempfile, "external-avatar." + ext, File.size(tempfile.path), origin: avatar_url, image_type: "avatar")


  user.create_user_avatar unless user.user_avatar


  if !user.user_avatar.contains_upload?(upload.id)
    user.user_avatar.update_columns(custom_upload_id: upload.id)


    override_gravatar = !options || options[:override_gravatar]


    if user.uploaded_avatar_id.nil? ||
        !user.user_avatar.contains_upload?(user.uploaded_avatar_id) ||
        override_gravatar
      user.update_columns(uploaded_avatar_id: upload.id)
    end
  end


rescue => e
  # skip saving, we are not connected to the net

won’t set it to the avatar from SSO.

Is this intentional behavior?

zogstrip · 2017-01-17 09:38:10 UTC

No it’s not. Feel free to do a pull request

techAPJ · 2017-01-19 05:26:10 UTC

Fixed via:

lukegb · 2017-01-20 21:23:36 UTC

Thanks! I wasn’t sure if there was an obvious edgecase I’d missed

codinghorror · 2017-01-20 21:29:44 UTC