Skip to:
Content

Opened 19 months ago

Closed 3 months ago

#4571 closed defect (bug) (fixed)

Changes in Gravatar APIs result in failed fallback on default avatars

Reported by: boonebgorges Owned by: boonebgorges
Milestone: 2.0 Priority: normal
Severity: major Version:
Component: Core Keywords: needs-refresh
Cc:

Description

BP allows admins to specify a default avatar, which is to be used when a user both has no Gravatar and has not uploaded a local avatar. We implement this by passing the URL of the default avatar along to Gravatar using the ?d= parameter. I'm not 100% sure how Gravatar handles these params, but it appears that they cache a copy of the local avatar and serve it from their servers.

A change made at gravatar.com last week means that this method no longer works in all cases. For security reasons, your default image must now be "publicly accessible". That means that they can't be behind HTTP authentication or otherwise hidden with Apache rules. See https://twitter.com/beaulebens/statuses/251407787341524992

I first noticed this in a password-protected staging environment, but users are already reporting the issue in their production installations.

Possible solution: Near the end of bp_core_fetch_avatar(), we detect when a Gravatar query has failed, and if so, we serve up the local default directly.

Attachments (1)

4571.01.patch (5.0 KB) - added by r-a-y 19 months ago.

Download all attachments as: .zip

Change History (22)

comment:1 DJPaul19 months ago

I've seen this in a dev environment using a mapped hostname, and today I found a report on the bporg forums. This obviously has backpat concerns but I'm not sure if we can do anything. http://buddypress.org/community/groups/miscellaneous/forum/topic/what-is-i0-wp-com/

Another idea which I'm pretty sure we've discussed before: set a usermeta flag for users who have uploaded a "BP avatar" to avoid the gravatar.com lookup. Seems simpler than making a ton of HEADER requests to gravatar just to see if it would return a valid response or not. I'm not fond of this approach anyway -- a slow internet connection from the server to gravatar could impact page load times.

comment:2 follow-up: boonebgorges19 months ago

But that usermeta tag won't differentiate between users who have legit Gravatars and those who don't.

How about something like this:

  • First time we try to load an avatar for a given user, first check for local avatar (we already do this). Then check gravatar, and examine the headers. Then set a usermeta key 'bp_avatar_type' with value 'local', 'gravatar', or 'default' (or we could just leave it empty in the latter case, whatever).
  • For each subsequent load of that user's avatar, first fetch bp_avatar_type and only pursue the appropriate route: gravatar requests go to gravatar.com, local and default are served locally.
  • We bust the bp_avatar_type cache for a user when (a) the user uploads a local avatar, (b) we get a bad header back from gravatar.com (the user has deleted his gravatar)

This will save a ton of HTTP requests against gravatar.com.

comment:3 in reply to: ↑ 2 DJPaul19 months ago

Replying to boonebgorges:

How about something like this:

If we use usermeta, we wouldn't need to bust the cache. Where in this flow would we detect when we get a bad header back from gravatar.com (the user has deleted his gravatar)?

comment:4 boonebgorges19 months ago

Something like:

function bp_core_fetch_avatar( $args ) {
    // parse args

    $user_avatar_type = get_user_meta( $item_id, 'bp_avatar_type', true );

    // Try 'gravatar' first in case it fails
    if ( 'gravatar' == $user_avatar_type ) {
        $avatar = do some stuff to get the gravatar
        
        if ( we get a bad response from gravatar ) {
            delete_user_meta( $item_id, 'bp_avatar_type' );
            $user_avatar_type = 'default';
        }
    }

    if ( 'local' == $user_avatar_type ) ....

    if ( 'default' == $user_avatar_type ) {
        $avatar = the default avatar;
    }

    return $avatar;
}

What this *doesn't* cover is the case where someone using the default avatar uploads a gravatar. If we're not checking on every page load, we won't detect it right away. We could, perhaps, do a daily check for 'default' users to see if we still get bad results from gravatar, and could also add a button to the Change Avatar screen that says "I've got a Gravatar", which would bust the cache manually.

comment:5 boonebgorges19 months ago

(And what I mean by "bust the cache" is "delete the usermeta value so that it can be recalculated". Not in the sense of wp_cache().)

comment:6 r-a-y19 months ago

A more, conservative approach is to ping Gravatar when a user logs in or out and if this user has a new Gravatar, we bust the cache.

When we move to using meta for avatars, we'll also have to be DB query-conscious. Will probably have to add the avatar query to BP_User_Query.

Also, we can't forget about groups that use Identicon from Gravatar. So it's a can of worms.

I'm guessing that the Gravatar team will address this sooner rather than later though.

comment:7 follow-up: modemlooper19 months ago

Why even use Gravatar? Most people dislike it.

comment:8 in reply to: ↑ 7 ; follow-up: johnjamesjacoby19 months ago

Replying to modemlooper:

Why even use Gravatar? Most people dislike it.

Because it comes baked into WordPress, we get a huge avatar distribution network for free, and it serves billions of images every month.

Per a Skype chat, I think this is an educational issue about keeping the default avatar publicly accessible for 1.7. Later on, we can store the URL to an object's avatar (Gravatar or otherwise) in meta, and perform whatever logic we need in a BP_Avatar class. It's a complicated enough piece of code that it deserves its own architecture.

Since BuddyPress doesn't load avatars without some kind of user/group/blog information available to it, we should have already queried that object, cached it's results, and have nothing to lose by keeping an avatar URL in meta.

Let's move this to future release for now, and revisit it later?

Last edited 19 months ago by johnjamesjacoby (previous) (diff)

comment:9 boonebgorges19 months ago

  • Milestone changed from 1.6.2 to Future Release

Why even use Gravatar? Most people dislike it.

We must continue to use Gravatar because we currently use it. If I were building BP from scratch I would not include it in core. But we have to deal with it for backward compatibility.

When we move to using meta for avatars, we'll also have to be DB query-conscious. Will probably have to add the avatar query to BP_User_Query.

Right. Just so long as we're not creating a new DB query for every avatar on a page. As r-a-y notes, this can be easily done by adding the check to BP_User_Query and BP_Core_User.

Let's definitely go with user education (including pointing users to Gravatar's updated documentation, when it exists), at least for the 1.6.x series. If this turns out to be an enormous problem, we can try for a stopgap solution in 1.7. But I doubt this will be the case - as John noted in chat, this will affect only a small number of users (as vocal as they may be). Let's maybe look at a refactor of avatar stuff for 1.8.

comment:10 r-a-y19 months ago

  • Keywords has-patch added

In 01.patch, I've addressed the problem of Gravatar's default avatars failing by directly using Gravatar's CDN-hosted 'mystery-man' avatar.

The main fix for this is line 344 of the patch:
apply_filters( 'bp_core_mysteryman_src', 'mm', $grav_size );

Changing to 'mm' automatically uses Gravatar's CDN-hosted 'mystery-man' avatar instead of our own, bundled version.

View Gravatar's docs on this:
https://en.gravatar.com/site/implement/images/#default-image

This eliminates the problems with our bundled version being behind any firewalls and on local development sites.

This should also be backwards-compatible with anyone using the 'bp_core_mysteryman_src' filter, unless the avatar is not publicly-accessible.

Was not backwards-compatible! Needed to write a new hook function called bp_core_avatar_mysteryman_src_backpat() to handle cases where BP_AVATAR_DEFAULT was defined. It's not the most elegant solution, but it works.

---

I've also taken the liberty of refactoring parts of the avatar code.

We have way too many hooks that overlap, so I've made some attempts at consolidating them under bp_core_avatar_default() and bp_core_avatar_default_thumb().

I've also switched out our locally-hosted 'mystery-man' avatars with Gravatar's CDN-hosted mystery man as well.

Let me know what you think.

Last edited 19 months ago by r-a-y (previous) (diff)

r-a-y19 months ago

comment:11 boonebgorges19 months ago

We can't remove our packaged version of the mystery man, right? Just in case someone is referencing it directly somewhere.

I think the general strategy here looks good, and will help in lots of cases. (Still doesn't help when someone has supplied a custom default avatar that is non-public, but this will be a subset of a subset.)

comment:12 r-a-y19 months ago

We can't remove our packaged version of the mystery man, right? Just in case someone is referencing it directly somewhere.

We could remove it in a future release, but you're right that we should keep them packaged for now.

(Still doesn't help when someone has supplied a custom default avatar that is non-public, but this will be a subset of a subset.)

Yeah, unfortunately this will have to be a user education thing.

comment:13 in reply to: ↑ 8 rogercoathup19 months ago

Replying to johnjamesjacoby:

Replying to modemlooper:

Why even use Gravatar? Most people dislike it.

Because it comes baked into WordPress, we get a huge avatar distribution network for free, and it serves billions of images every month.

Who do you mean by "we"? The avatars are being displayed on users sites, which they host on their own servers. So any benefit is likely to be to the site owners, rather than to BuddyPress.

By the same score, they (the site owners) should be in control of whether they want gravatar or not ( i.e. it should be easy to opt in or out of on their site ).

[Edit:
ok, see Boone's comment on this: 'We must continue to use Gravatar because we currently use it. If I were building BP from scratch I would not include it in core. But we have to deal with it for backward compatibility.'

How difficult would it be to modify to make Gravatar optional -- keeping it as default if necessary to deal with backward compatibility? ]

Last edited 19 months ago by rogercoathup (previous) (diff)

comment:14 hnla19 months ago

Have also had this happen on a protected staging site & local - pretty annoying!

(Still doesn't help when someone has supplied a custom default avatar that is non-public, but this will be a subset of a subset.)

No it doesn't.

Think the thing I found surprising when I was diverted to having to look at this 'apparent' issue was the fact that unless one had uploaded a user avatar there was no option to not use gravatar!

comment:15 r-a-y19 months ago

How difficult would it be to modify to make Gravatar optional -- keeping it as default if necessary to deal with backward compatibility?

there was no option to not use gravatar!

There's a simple filter to disable Gravatar. (There's no admin option for this at the moment.)

Add this to your /wp-content/plugins/bp-custom.php:

add_filter( 'bp_core_fetch_avatar_no_grav', '__return_true' );

In BP 1.6, to use a custom avatar in this instance, you'll also need to hook into 'bp_core_default_avatar_$object', which would translate to 'bp_core_default_avatar_user' for users.

View the hook here:
https://buddypress.trac.wordpress.org/browser/tags/1.6.1/bp-core/bp-core-avatars.php#L373

comment:16 hnla19 months ago

Cheers for that r-a-y.

filter == simple only if ones aware of them, of course I could have easily gone and delved through the pertinent file to find this which was the plan but not exactly a pressing matter for me. An admin option ought to be simple enough to run up though and personally I dislike and try and avoid adding things to wp-config or bp-custom.

comment:17 DJPaul12 months ago

  • Keywords needs-refresh added; has-patch removed
  • Resolution set to fixed
  • Status changed from new to closed

comment:18 DJPaul12 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:19 boonebgorges3 months ago

  • Milestone changed from Future Release to 2.0

An additional complication has recently come up, related to how Gravatar does (or doesn't) handle default images available only over SSL. See http://teleogistic.net/2014/01/default-gravatar-images-and-ssl/ for more.

I think we should go with r-a-y's original suggestion that we use 'mm' for the default. That, along with his backward compatibility fix for BP_AVATAR_DEFAULT, should cover most cases.
https://buddypress.trac.wordpress.org/ticket/4571#comment:10

comment:20 boonebgorges3 months ago

I studied 4571.01.patch and it looks good, with the exception of one case. When calling bp_core_fetch_avatar() with no_grav set to true, the patch has you fall back on bp_core_avatar_default(). But we shouldn't be sending people to Gravatar if they've specifically asked not to be - this could potentially be a case where we break existing sites that need to force Gravatar off for one reason or other. So I've added a $type param to the _default_ functions that lets you force local (with default to gravatar). Not super elegant, but we've gotta work with what we've got here :)

comment:21 boonebgorges3 months ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from reopened to closed

In 7769:

Improve default avatar handling in bp_core_fetch_avatar()

Changes to Gravatar over the last few years have placed restrictions on the way
that a default gravatar (to be used as a fallback for when no actual Gravatar
is found) can be served to gravatar.com. In particular, Gravatar cannot cache
and serve default images that are not publicly available (as in dev
environments) or are only available over SSL.

Because BuddyPress's default behavior was to provide a local copy of the
mystery-man avatar, switching to Gravatar's hosted mystery-man solves these
problems for sites that are using the default mystery-man as a fallback image.

This changeset also reconfigures the way that the BP_AVATAR_DEFAULT constant
is respected, to ensure that it's possible to override the avatar fallback when
no_grav is set to true.

Fixes #4571

Props r-a-y

Note: See TracTickets for help on using tickets.