Fix avatar URLs (#3069) (#3143)

* Fix avatar URLs

* import order

Author: ethantkoenig

models/user.go

@@ -315,10 +315,9 @@ func (u *User) generateRandomAvatar(e Engine) error {
 	return nil
 }
 
-// RelAvatarLink returns relative avatar link to the site domain,
-// which includes app sub-url as prefix. However, it is possible
-// to return full URL if user enables Gravatar-like service.
-func (u *User) RelAvatarLink() string {
+// SizedRelAvatarLink returns a relative link to the user's avatar. When
+// applicable, the link is for an avatar of the indicated size (in pixels).
+func (u *User) SizedRelAvatarLink(size int) string {
 	if u.ID == -1 {
 		return base.DefaultAvatarLink()
 	}
@@ -338,7 +337,14 @@ func (u *User) RelAvatarLink() string {
 
 		return setting.AppSubURL + "/avatars/" + u.Avatar
 	}
-	return base.AvatarLink(u.AvatarEmail)
+	return base.SizedAvatarLink(u.AvatarEmail, size)
+}
+
+// RelAvatarLink returns a relative link to the user's avatar. The link
+// may either be a sub-URL to this site, or a full URL to an external avatar
+// service.
+func (u *User) RelAvatarLink() string {
+	return u.SizedRelAvatarLink(base.DefaultAvatarSize)
 }
 
 // AvatarLink returns user avatar absolute link.

modules/base/tool.go

@@ -16,6 +16,8 @@ import (
 	"math"
 	"math/big"
 	"net/http"
+	"net/url"
+	"path"
 	"strconv"
 	"strings"
 	"time"
@@ -197,24 +199,59 @@ func DefaultAvatarLink() string {
 	return setting.AppSubURL + "/img/avatar_default.png"
 }
 
-// AvatarLink returns relative avatar link to the site domain by given email,
-// which includes app sub-url as prefix. However, it is possible
-// to return full URL if user enables Gravatar-like service.
-func AvatarLink(email string) string {
+// DefaultAvatarSize is a sentinel value for the default avatar size, as
+// determined by the avatar-hosting service.
+const DefaultAvatarSize = -1
+
+// libravatarURL returns the URL for the given email. This function should only
+// be called if a federated avatar service is enabled.
+func libravatarURL(email string) (*url.URL, error) {
+	urlStr, err := setting.LibravatarService.FromEmail(email)
+	if err != nil {
+		log.Error(4, "LibravatarService.FromEmail(email=%s): error %v", email, err)
+		return nil, err
+	}
+	u, err := url.Parse(urlStr)
+	if err != nil {
+		log.Error(4, "Failed to parse libravatar url(%s): error %v", urlStr, err)
+		return nil, err
+	}
+	return u, nil
+}
+
+// SizedAvatarLink returns a sized link to the avatar for the given email
+// address.
+func SizedAvatarLink(email string, size int) string {
+	var avatarURL *url.URL
 	if setting.EnableFederatedAvatar && setting.LibravatarService != nil {
-		url, err := setting.LibravatarService.FromEmail(email)
+		var err error
+		avatarURL, err = libravatarURL(email)
 		if err != nil {
-			log.Error(4, "LibravatarService.FromEmail(email=%s): error %v", email, err)
 			return DefaultAvatarLink()
 		}
-		return url
+	} else if !setting.DisableGravatar {
+		// copy GravatarSourceURL, because we will modify its Path.
+		copyOfGravatarSourceURL := *setting.GravatarSourceURL
+		avatarURL = &copyOfGravatarSourceURL
+		avatarURL.Path = path.Join(avatarURL.Path, HashEmail(email))
+	} else {
+		return DefaultAvatarLink()
 	}
 
-	if !setting.DisableGravatar {
-		return setting.GravatarSource + HashEmail(email) + "?d=identicon"
+	vals := avatarURL.Query()
+	vals.Set("d", "identicon")
+	if size != DefaultAvatarSize {
+		vals.Set("s", strconv.Itoa(size))
 	}
+	avatarURL.RawQuery = vals.Encode()
+	return avatarURL.String()
+}
 
-	return DefaultAvatarLink()
+// AvatarLink returns relative avatar link to the site domain by given email,
+// which includes app sub-url as prefix. However, it is possible
+// to return full URL if user enables Gravatar-like service.
+func AvatarLink(email string) string {
+	return SizedAvatarLink(email, DefaultAvatarSize)
 }
 
 // Seconds-based time units

modules/base/tool_test.go

@@ -1,11 +1,13 @@
 package base
 
 import (
+	"net/url"
 	"os"
 	"testing"
 	"time"
 
 	"code.gitea.io/gitea/modules/setting"
+
 	"github.com/Unknwon/i18n"
 	macaroni18n "github.com/go-macaron/i18n"
 	"github.com/stretchr/testify/assert"
@@ -126,16 +128,40 @@ func TestHashEmail(t *testing.T) {
 	)
 }
 
-func TestAvatarLink(t *testing.T) {
+const gravatarSource = "https://secure.gravatar.com/avatar/"
+
+func disableGravatar() {
 	setting.EnableFederatedAvatar = false
 	setting.LibravatarService = nil
 	setting.DisableGravatar = true
+}
 
-	assert.Equal(t, "/img/avatar_default.png", AvatarLink(""))
-
+func enableGravatar(t *testing.T) {
 	setting.DisableGravatar = false
+	var err error
+	setting.GravatarSourceURL, err = url.Parse(gravatarSource)
+	assert.NoError(t, err)
+}
+
+func TestSizedAvatarLink(t *testing.T) {
+	disableGravatar()
+	assert.Equal(t, "/img/avatar_default.png",
+		SizedAvatarLink("[email protected]", 100))
+
+	enableGravatar(t)
+	assert.Equal(t,
+		"https://secure.gravatar.com/avatar/353cbad9b58e69c96154ad99f92bedc7?d=identicon&s=100",
+		SizedAvatarLink("[email protected]", 100),
+	)
+}
+
+func TestAvatarLink(t *testing.T) {
+	disableGravatar()
+	assert.Equal(t, "/img/avatar_default.png", AvatarLink("[email protected]"))
+
+	enableGravatar(t)
 	assert.Equal(t,
-		"353cbad9b58e69c96154ad99f92bedc7?d=identicon",
+		"https://secure.gravatar.com/avatar/353cbad9b58e69c96154ad99f92bedc7?d=identicon",
 		AvatarLink("[email protected]"),
 	)
 }

modules/setting/setting.go

@@ -325,6 +325,7 @@ var (
 	// Picture settings
 	AvatarUploadPath      string
 	GravatarSource        string
+	GravatarSourceURL     *url.URL
 	DisableGravatar       bool
 	EnableFederatedAvatar bool
 	LibravatarService     *libravatar.Libravatar
@@ -1026,18 +1027,22 @@ func NewContext() {
 	if DisableGravatar {
 		EnableFederatedAvatar = false
 	}
+	if EnableFederatedAvatar || !DisableGravatar {
+		GravatarSourceURL, err = url.Parse(GravatarSource)
+		if err != nil {
+			log.Fatal(4, "Failed to parse Gravatar URL(%s): %v",
+				GravatarSource, err)
+		}
+	}
 
 	if EnableFederatedAvatar {
 		LibravatarService = libravatar.New()
-		parts := strings.Split(GravatarSource, "/")
-		if len(parts) >= 3 {
-			if parts[0] == "https:" {
-				LibravatarService.SetUseHTTPS(true)
-				LibravatarService.SetSecureFallbackHost(parts[2])
-			} else {
-				LibravatarService.SetUseHTTPS(false)
-				LibravatarService.SetFallbackHost(parts[2])
-			}
+		if GravatarSourceURL.Scheme == "https" {
+			LibravatarService.SetUseHTTPS(true)
+			LibravatarService.SetSecureFallbackHost(GravatarSourceURL.Host)
+		} else {
+			LibravatarService.SetUseHTTPS(false)
+			LibravatarService.SetFallbackHost(GravatarSourceURL.Host)
 		}
 	}
 

templates/org/header.tmpl

@@ -3,7 +3,7 @@
 		<div class="ui vertically grid head">
 			<div class="column">
 				<div class="ui header">
-					<img class="ui image" src="{{.RelAvatarLink}}?s=100">
+					<img class="ui image" src="{{.SizedRelAvatarLink 100}}">
 					<span class="text thin grey"><a href="{{.HomeLink}}">{{.DisplayName}}</a></span>
 
 					<div class="ui right">

templates/org/home.tmpl

@@ -3,7 +3,7 @@
 	<div class="ui container">
 		<div class="ui grid">
 			<div class="ui sixteen wide column">
-				<img class="ui left" id="org-avatar" src="{{.Org.RelAvatarLink}}?s=140"/>
+				<img class="ui left" id="org-avatar" src="{{.Org.SizedRelAvatarLink 140}}"/>
 				<div id="org-info">
 					<div class="ui header">
 						{{.Org.DisplayName}}

templates/org/member/members.tmpl

@@ -8,7 +8,7 @@
 			{{range .Members}}
 				<div class="item ui grid">
 					<div class="ui one wide column">
-						<img class="ui avatar" src="{{.RelAvatarLink}}?s=48">
+						<img class="ui avatar" src="{{.SizedRelAvatarLink 48}}">
 					</div>
 					<div class="ui three wide column">
 						<div class="meta"><a href="{{.HomeLink}}">{{.Name}}</a></div>

templates/user/profile.tmpl

@@ -6,11 +6,11 @@
 				<div class="ui card">
 					{{if eq .SignedUserName .Owner.Name}}
 						<a class="image poping up" href="{{AppSubUrl}}/user/settings/avatar" id="profile-avatar" data-content="{{.i18n.Tr "user.change_avatar"}}" data-variation="inverted tiny" data-position="bottom center">
-							<img src="{{.Owner.RelAvatarLink}}?s=290" title="{{.Owner.Name}}"/>
+							<img src="{{.Owner.SizedRelAvatarLink 290}}" title="{{.Owner.Name}}"/>
 						</a>
 					{{else}}
 						<span class="image">
-							<img src="{{.Owner.RelAvatarLink}}?s=290" title="{{.Owner.Name}}"/>
+							<img src="{{.Owner.SizedRelAvatarLink 290}}" title="{{.Owner.Name}}"/>
 						</span>
 					{{end}}
 					<div class="content">