Remove default enabling of gravatar…

[simonpoole]

Remove default enabling of gravatar, check on initial confirmation of e-mail address and on any changes afterwards if a gravatar exists and enable then if the user hasn't uploaded a picture.

Works as is, however probably needs some refining: for example displaying messages what we are doing and so on.

[HolgerJeromin]

The migration files seems to be named after the current date.
But I did not checked in detail.

[tomhughes]

Hang on, this turns off all the existing gravatars!

[tomhughes]

Oh no I see, it's just changing the default value.

[simonpoole]

@tomhughes this fix will requires the frontend to make an outbound connection to gravatar.com/automattic.com, will that cause any issues? It does reduce the information leakage to gravatar.com a bit since it makes the actually client a bit less discoverable.

[tomhughes]

What sort of issue would you expect it to cause? It's not something that bothers me...

[simonpoole]

@tomhughes I was thinking of outbound FW rules etc.

In any case, the other part of this would be a script that (probably slowly) goes through all the existing accounts and disables gravatar for those without one, any preference/suggestion how that should be done, if at all?

[tomhughes]

So by my reckoning that function is about ten times longer than it needs to be... Something like this should be all you need I think:

def gravatar_enable(user)
  return if user.image.present?
  hash = Digest::MD5.hexdigest(user.email.downcase)
  url = "https://www.gravatar.com/avatar/#{hash}?d=404"
  response - OSM.http_client.get(URI.parse(url))
  user.image_use_gravatar = response.success?
end

Obviously the tests also need to pass...

My only other comment would be, if the current behaviour violates the privacy policy then does this actually fix it? We are still automatically enabling it for anybody that has a gravatar after alll...

[simonpoole]

@tomhughes the PR reduces exposure of the hash of the e-mail address for users that don't have a gravatar to third parties to zero, it further reduces the ability of automattic to fingerprint new OSM users to zero (because the check is done from the frontends), it further limits the ability of automattic to fingerprint and track third parties to those viewing the profile of a user that actually -has- a gravatar (instead of all and every profile). We still expose the hash of the e-mail address of new signups to automattic once, but that is the compromise to make the whole thing user friendly.

It simply does what should have been done from the beginning.

WRT verbosity: I didn't claim anything else, specifically as I mentioned it would be nice to give the user feedback on what has actually been set/changed and thats why I left some code to be able to do that. You will unlikely be able to get around the resuce though.

I believe the tests fail because there is a expectation that an image exists, which is likely no longer the case now.

[tomhughes]

Why? We don't use the rescue anywhere else...

[simonpoole]

@tomhughes at least on my local server I ended up with a rather ugly strack trace etc on the screen if there are network/DNS/server related issues that raise an exception which will likely happen in real life now and then, but I admit to having no idea what would happen running in a production env.

[tomhughes]

@simonpoole But you were using a completely different HTTP client!

[simonpoole]

@TomH wrt tests .... a proper test of this needs a new account without a gravatar enabled (which I believe we create during the tests) and one with, or put differently is it (with reasonable effort) possible to setup a dummy e-mail address in one of the OSM domains that we can use for obtaining a gravatar?

[tomhughes]

@simonpoole I don't really want to test against the live gravatar servers, but if you use OSM.http_client for the web queries then you can mock the responses using with_http_stubs as the geocoder tests do.

[simonpoole]

I've added two tests that cover the case of users changing their email, for what ever reason coverage has supposedly decreased, however in files which are not actually changed in this PR ........

In any case to make this work I had to rejig the http_stubs stuff so that you can specify the http status code in the config.

[simonpoole]

Note wrt testing. While I use stubs for the test of the new functionality, all the other tests naturally actually hit the gravatar servers.

[simonpoole]

Rebased on current master.

Pro memoria: https://docs.google.com/document/d/12jJw7qNGhQCIE4z0h33CH9yrtTIWnpNksAd86Pxiu1o/edit?usp=sharing item 7

[tomhughes]

I think :notice would be more appropriate here - it's an informational message rather than a notification that anything is wrong as such.

[simonpoole]

IIRC (its been a bit long) the problem was that there may be a notice already displayed which causes issues, but outside of that, I agree.

[simonpoole]

Just jogged my memory a bit and retested on my instance.

The check if a gravatar is available or not takes place when the user uses the link from the confirmation mail, that in itself shows a notice that the e-mail has been changed which hides the one from changing the gravatar state. I suspect a work around would be to merge the messages / create two additional ones for the case when the gravatar enabled status has changed.