From cc8a4537b8505648bf203d8b8a0061c95aa73fac Mon Sep 17 00:00:00 2001 From: Xiaoguang Wang Date: Wed, 22 Sep 2021 20:49:45 +0800 Subject: [PATCH 01/17] avatar-refactor --- integrations/api_user_orgs_test.go | 4 +- integrations/user_avatar_test.go | 2 +- models/{ => avatars}/avatar.go | 116 +++++++++++++++++----------- models/{ => avatars}/avatar_test.go | 6 +- models/repo_activity.go | 4 +- models/user_avatar.go | 60 ++++++-------- modules/convert/convert.go | 2 +- modules/convert/user.go | 2 +- modules/repository/commits.go | 7 +- modules/templates/helper.go | 13 ++-- routers/web/repo/issue.go | 2 +- routers/web/user/avatar.go | 79 +++---------------- routers/web/user/oauth.go | 4 +- routers/web/user/oauth_test.go | 2 +- routers/web/web.go | 2 +- services/auth/sspi_windows.go | 3 +- 16 files changed, 133 insertions(+), 175 deletions(-) rename models/{ => avatars}/avatar.go (55%) rename models/{ => avatars}/avatar_test.go (89%) diff --git a/integrations/api_user_orgs_test.go b/integrations/api_user_orgs_test.go index 36b11335c054c..2a5a1e1885122 100644 --- a/integrations/api_user_orgs_test.go +++ b/integrations/api_user_orgs_test.go @@ -32,7 +32,7 @@ func TestUserOrgs(t *testing.T) { ID: 3, UserName: user3.Name, FullName: user3.FullName, - AvatarURL: user3.AvatarLink(), + AvatarURL: user3.AvatarLinkDefaultSize(), Description: "", Website: "", Location: "", @@ -88,7 +88,7 @@ func TestMyOrgs(t *testing.T) { ID: 3, UserName: user3.Name, FullName: user3.FullName, - AvatarURL: user3.AvatarLink(), + AvatarURL: user3.AvatarLinkDefaultSize(), Description: "", Website: "", Location: "", diff --git a/integrations/user_avatar_test.go b/integrations/user_avatar_test.go index c909fd6851c6c..c1808e931424f 100644 --- a/integrations/user_avatar_test.go +++ b/integrations/user_avatar_test.go @@ -74,7 +74,7 @@ func TestUserAvatar(t *testing.T) { user2 = db.AssertExistsAndLoadBean(t, &models.User{ID: 2}).(*models.User) // owner of the repo3, is an org - req = NewRequest(t, "GET", user2.AvatarLink()) + req = NewRequest(t, "GET", user2.AvatarLinkDefaultSize()) resp := session.MakeRequest(t, req, http.StatusFound) location := resp.Header().Get("Location") if !strings.HasPrefix(location, "/avatars") { diff --git a/models/avatar.go b/models/avatars/avatar.go similarity index 55% rename from models/avatar.go rename to models/avatars/avatar.go index e81b876667abc..231fd871da2d6 100644 --- a/models/avatar.go +++ b/models/avatars/avatar.go @@ -2,11 +2,9 @@ // Use of this source code is governed by a MIT-style // license that can be found in the LICENSE file. -package models +package avatars import ( - "crypto/md5" - "fmt" "net/url" "path" "strconv" @@ -19,7 +17,16 @@ import ( "code.gitea.io/gitea/modules/setting" ) -// EmailHash represents a pre-generated hash map +// DefaultAvatarSize is a sentinel value for the default avatar size, as determined by the avatar-hosting service. +const DefaultAvatarSize = -1 + +// DefaultAvatarPixelSize is the default size in pixels of a rendered avatar +const DefaultAvatarPixelSize = 28 + +// AvatarRenderedSizeFactor is the factor by which the default size is increased for finer rendering +const AvatarRenderedSizeFactor = 4 + +// EmailHash represents a pre-generated hash map (mainly used by LibravatarURL, it queries email server's DNS records) type EmailHash struct { Hash string `xorm:"pk varchar(32)"` Email string `xorm:"UNIQUE NOT NULL"` @@ -41,18 +48,7 @@ func DefaultAvatarLink() string { return u.String() } -// DefaultAvatarSize is a sentinel value for the default avatar size, as -// determined by the avatar-hosting service. -const DefaultAvatarSize = -1 - -// DefaultAvatarPixelSize is the default size in pixels of a rendered avatar -const DefaultAvatarPixelSize = 28 - -// AvatarRenderedSizeFactor is the factor by which the default size is increased for finer rendering -const AvatarRenderedSizeFactor = 4 - -// HashEmail hashes email address to MD5 string. -// https://en.gravatar.com/site/implement/hash/ +// HashEmail hashes email address to MD5 string. https://en.gravatar.com/site/implement/hash/ func HashEmail(email string) string { return base.EncodeMD5(strings.ToLower(strings.TrimSpace(email))) } @@ -69,8 +65,8 @@ func GetEmailForHash(md5Sum string) (string, error) { }) } -// LibravatarURL returns the URL for the given email. This function should only -// be called if a federated avatar service is enabled. +// LibravatarURL returns the URL for the given email. Slow due to the DNS lookup. +// 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 { @@ -85,14 +81,15 @@ func LibravatarURL(email string) (*url.URL, error) { return u, nil } -// HashedAvatarLink returns an avatar link for a provided email -func HashedAvatarLink(email string, size int) string { +// saveEmailHash returns an avatar link for a provided email, +// the email and hash are saved into database, which will be used by GetEmailForHash later +func saveEmailHash(email string) string { lowerEmail := strings.ToLower(strings.TrimSpace(email)) - sum := fmt.Sprintf("%x", md5.Sum([]byte(lowerEmail))) - _, _ = cache.GetString("Avatar:"+sum, func() (string, error) { + emailHash := HashEmail(lowerEmail) + _, _ = cache.GetString("Avatar:"+emailHash, func() (string, error) { emailHash := &EmailHash{ Email: lowerEmail, - Hash: sum, + Hash: emailHash, } // OK we're going to open a session just because I think that that might hide away any problems with postgres reporting errors if err := db.WithTx(func(ctx *db.Context) error { @@ -109,39 +106,68 @@ func HashedAvatarLink(email string, size int) string { } return lowerEmail, nil }) - if size > 0 { - return setting.AppSubURL + "/avatar/" + url.PathEscape(sum) + "?size=" + strconv.Itoa(size) - } - return setting.AppSubURL + "/avatar/" + url.PathEscape(sum) + return emailHash } -// MakeFinalAvatarURL constructs the final avatar URL string -func MakeFinalAvatarURL(u *url.URL, size int) string { - vals := u.Query() - vals.Set("d", "identicon") - if size != DefaultAvatarSize { - vals.Set("s", strconv.Itoa(size)) +// GenerateUserAvatarFastLink returns a fast link to the user's avatar via the local explore page. +func GenerateUserAvatarFastLink(userName string, size int) string { + if size < 0 { + size = 0 } - u.RawQuery = vals.Encode() - return u.String() + link := setting.AppSubURL + "/user/avatar/" + userName + "/" + strconv.Itoa(size) + return setting.AppURL + strings.TrimPrefix(link, setting.AppSubURL)[1:] } -// SizedAvatarLink returns a sized link to the avatar for the given email address. -func SizedAvatarLink(email string, size int) string { +// generateEmailAvatarLink returns a email avatar link. +// if final is true, it may use a slow path (eg: query DNS). +// if final is false, it always uses a fast path. +func generateEmailAvatarLink(email string, size int, final bool) string { + if size <= 0 { + size = DefaultAvatarSize + } + + email = strings.TrimSpace(email) + if email == "" { + return DefaultAvatarLink() + } + var avatarURL *url.URL + var err error + if setting.EnableFederatedAvatar && setting.LibravatarService != nil { - // This is the slow path that would need to call LibravatarURL() which - // does DNS lookups. Avoid it by issuing a redirect so we don't block - // the template render with network requests. - return HashedAvatarLink(email, size) + emailHash := saveEmailHash(email) + if final { + if avatarURL, err = LibravatarURL(email); err != nil { + return DefaultAvatarLink() + } + } else { + if size > 0 { + return setting.AppSubURL + "/avatar/" + emailHash + "?size=" + strconv.Itoa(size) + } + return setting.AppSubURL + "/avatar/" + emailHash + } } else if !setting.DisableGravatar { - // copy GravatarSourceURL, because we will modify its Path. - copyOfGravatarSourceURL := *setting.GravatarSourceURL - avatarURL = ©OfGravatarSourceURL + avatarURLDummy := *setting.GravatarSourceURL // copy GravatarSourceURL, because we will modify its Path. + avatarURL = &avatarURLDummy avatarURL.Path = path.Join(avatarURL.Path, HashEmail(email)) } else { return DefaultAvatarLink() } - return MakeFinalAvatarURL(avatarURL, size) + avatarURL.Query().Set("d", "identicon") + if size > 0 { + avatarURL.Query().Set("s", strconv.Itoa(size)) + } + avatarURL.RawQuery = avatarURL.Query().Encode() + return avatarURL.String() +} + +//GenerateEmailAvatarFastLink returns a avatar link (fast, the link may be a delegated one) +func GenerateEmailAvatarFastLink(email string, size int) string { + return generateEmailAvatarLink(email, size, false) +} + +//GenerateEmailAvatarFinalLink returns a avatar final link (maybe slow) +func GenerateEmailAvatarFinalLink(email string, size int) string { + return generateEmailAvatarLink(email, size, true) } diff --git a/models/avatar_test.go b/models/avatars/avatar_test.go similarity index 89% rename from models/avatar_test.go rename to models/avatars/avatar_test.go index 09f1a8066da58..4d6255ca5fefb 100644 --- a/models/avatar_test.go +++ b/models/avatars/avatar_test.go @@ -2,7 +2,7 @@ // Use of this source code is governed by a MIT-style // license that can be found in the LICENSE file. -package models +package avatars import ( "net/url" @@ -44,11 +44,11 @@ func TestSizedAvatarLink(t *testing.T) { disableGravatar() assert.Equal(t, "/testsuburl/assets/img/avatar_default.png", - SizedAvatarLink("gitea@example.com", 100)) + GenerateEmailAvatarFastLink("gitea@example.com", 100)) enableGravatar(t) assert.Equal(t, "https://secure.gravatar.com/avatar/353cbad9b58e69c96154ad99f92bedc7?d=identicon&s=100", - SizedAvatarLink("gitea@example.com", 100), + GenerateEmailAvatarFastLink("gitea@example.com", 100), ) } diff --git a/models/repo_activity.go b/models/repo_activity.go index cfbda21411d89..0692693a71d1e 100644 --- a/models/repo_activity.go +++ b/models/repo_activity.go @@ -94,7 +94,7 @@ func GetActivityStatsTopAuthors(repo *Repository, timeFrom time.Time, count int) } users := make(map[int64]*ActivityAuthorData) var unknownUserID int64 - unknownUserAvatarLink := NewGhostUser().AvatarLink() + unknownUserAvatarLink := NewGhostUser().AvatarLinkDefaultSize() for _, v := range code.Authors { if len(v.Email) == 0 { continue @@ -116,7 +116,7 @@ func GetActivityStatsTopAuthors(repo *Repository, timeFrom time.Time, count int) users[u.ID] = &ActivityAuthorData{ Name: u.DisplayName(), Login: u.LowerName, - AvatarLink: u.AvatarLink(), + AvatarLink: u.AvatarLinkDefaultSize(), HomeLink: u.HomeLink(), Commits: v.Commits, } diff --git a/models/user_avatar.go b/models/user_avatar.go index 99e533758841e..2e032d58a12ec 100644 --- a/models/user_avatar.go +++ b/models/user_avatar.go @@ -10,8 +10,8 @@ import ( "image/png" "io" "strconv" - "strings" + "code.gitea.io/gitea/models/avatars" "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/modules/avatar" "code.gitea.io/gitea/modules/log" @@ -40,7 +40,7 @@ func (u *User) generateRandomAvatar(e db.Engine) error { return fmt.Errorf("RandomImage: %v", err) } - u.Avatar = HashEmail(seed) + u.Avatar = avatars.HashEmail(seed) // Don't share the images so that we can delete them easily if err := storage.SaveFrom(storage.Avatars, u.CustomAvatarRelativePath(), func(w io.Writer) error { @@ -60,61 +60,45 @@ func (u *User) generateRandomAvatar(e db.Engine) error { return nil } -// SizedRelAvatarLink returns a link to the user's avatar via -// the local explore page. Function returns immediately. -// When applicable, the link is for an avatar of the indicated size (in pixels). -func (u *User) SizedRelAvatarLink(size int) string { - return setting.AppSubURL + "/user/avatar/" + u.Name + "/" + strconv.Itoa(size) -} - -// RealSizedAvatarLink returns a link to the user's avatar. When -// applicable, the link is for an avatar of the indicated size (in pixels). -// -// This function make take time to return when federated avatars -// are in use, due to a DNS lookup need -// -func (u *User) RealSizedAvatarLink(size int) string { +// AvatarLinkWithSize returns a link to the user's avatar with size +func (u *User) AvatarLinkWithSize(size int) string { if u.ID == -1 { - return DefaultAvatarLink() + // ghost user + return avatars.DefaultAvatarLink() } + var useLocalAvatar bool + switch { case u.UseCustomAvatar: - if u.Avatar == "" { - return DefaultAvatarLink() - } - if size > 0 { - return setting.AppSubURL + "/avatars/" + u.Avatar + "?size=" + strconv.Itoa(size) - } - return setting.AppSubURL + "/avatars/" + u.Avatar + useLocalAvatar = true case setting.DisableGravatar, setting.OfflineMode: + useLocalAvatar = true if u.Avatar == "" { if err := u.GenerateRandomAvatar(); err != nil { log.Error("GenerateRandomAvatar: %v", err) } } + default: + useLocalAvatar = false + } + + if useLocalAvatar { + if u.Avatar == "" { + return avatars.DefaultAvatarLink() + } if size > 0 { return setting.AppSubURL + "/avatars/" + u.Avatar + "?size=" + strconv.Itoa(size) } return setting.AppSubURL + "/avatars/" + u.Avatar } - return 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(DefaultAvatarSize) + return avatars.GenerateEmailAvatarFastLink(u.AvatarEmail, size) } -// AvatarLink returns user avatar absolute link. -func (u *User) AvatarLink() string { - link := u.RelAvatarLink() - if link[0] == '/' && link[1] != '/' { - return setting.AppURL + strings.TrimPrefix(link, setting.AppSubURL)[1:] - } - return link +// AvatarLinkDefaultSize returns a avatar link with default size +func (u *User) AvatarLinkDefaultSize() string { + return u.AvatarLinkWithSize(avatars.DefaultAvatarSize) } // UploadAvatar saves custom avatar for user. diff --git a/modules/convert/convert.go b/modules/convert/convert.go index 404786ec9c9ad..50c6718f68550 100644 --- a/modules/convert/convert.go +++ b/modules/convert/convert.go @@ -277,7 +277,7 @@ func ToDeployKey(apiLink string, key *models.DeployKey) *api.DeployKey { func ToOrganization(org *models.User) *api.Organization { return &api.Organization{ ID: org.ID, - AvatarURL: org.AvatarLink(), + AvatarURL: org.AvatarLinkDefaultSize(), UserName: org.Name, FullName: org.FullName, Description: org.Description, diff --git a/modules/convert/user.go b/modules/convert/user.go index 164ffb71fd2e7..898a9d4aae19d 100644 --- a/modules/convert/user.go +++ b/modules/convert/user.go @@ -51,7 +51,7 @@ func toUser(user *models.User, signed, authed bool) *api.User { UserName: user.Name, FullName: markup.Sanitize(user.FullName), Email: user.GetEmail(), - AvatarURL: user.AvatarLink(), + AvatarURL: user.AvatarLinkDefaultSize(), Created: user.CreatedUnix.AsTime(), Restricted: user.IsRestricted, Location: user.Location, diff --git a/modules/repository/commits.go b/modules/repository/commits.go index 1558d8563905f..c86f0d570be4f 100644 --- a/modules/repository/commits.go +++ b/modules/repository/commits.go @@ -9,6 +9,7 @@ import ( "time" "code.gitea.io/gitea/models" + "code.gitea.io/gitea/models/avatars" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/log" api "code.gitea.io/gitea/modules/structs" @@ -139,14 +140,14 @@ func (pc *PushCommits) AvatarLink(email string) string { return avatar } - size := models.DefaultAvatarPixelSize * models.AvatarRenderedSizeFactor + size := avatars.DefaultAvatarPixelSize * avatars.AvatarRenderedSizeFactor u, ok := pc.emailUsers[email] if !ok { var err error u, err = models.GetUserByEmail(email) if err != nil { - pc.avatars[email] = models.SizedAvatarLink(email, size) + pc.avatars[email] = avatars.GenerateEmailAvatarFastLink(email, size) if !models.IsErrUserNotExist(err) { log.Error("GetUserByEmail: %v", err) return "" @@ -156,7 +157,7 @@ func (pc *PushCommits) AvatarLink(email string) string { } } if u != nil { - pc.avatars[email] = u.RealSizedAvatarLink(size) + pc.avatars[email] = u.AvatarLinkWithSize(size) } return pc.avatars[email] diff --git a/modules/templates/helper.go b/modules/templates/helper.go index 27d81ec9d9b94..b935eb6cc097f 100644 --- a/modules/templates/helper.go +++ b/modules/templates/helper.go @@ -23,6 +23,7 @@ import ( "unicode" "code.gitea.io/gitea/models" + "code.gitea.io/gitea/models/avatars" "code.gitea.io/gitea/modules/base" "code.gitea.io/gitea/modules/emoji" "code.gitea.io/gitea/modules/git" @@ -550,16 +551,16 @@ func SVG(icon string, others ...interface{}) template.HTML { // Avatar renders user avatars. args: user, size (int), class (string) func Avatar(item interface{}, others ...interface{}) template.HTML { - size, class := parseOthers(models.DefaultAvatarPixelSize, "ui avatar image", others...) + size, class := parseOthers(avatars.DefaultAvatarPixelSize, "ui avatar image", others...) if user, ok := item.(*models.User); ok { - src := user.RealSizedAvatarLink(size * models.AvatarRenderedSizeFactor) + src := user.AvatarLinkWithSize(size * avatars.AvatarRenderedSizeFactor) if src != "" { return AvatarHTML(src, size, class, user.DisplayName()) } } if user, ok := item.(*models.Collaborator); ok { - src := user.RealSizedAvatarLink(size * models.AvatarRenderedSizeFactor) + src := user.AvatarLinkWithSize(size * avatars.AvatarRenderedSizeFactor) if src != "" { return AvatarHTML(src, size, class, user.DisplayName()) } @@ -575,7 +576,7 @@ func AvatarByAction(action *models.Action, others ...interface{}) template.HTML // RepoAvatar renders repo avatars. args: repo, size(int), class (string) func RepoAvatar(repo *models.Repository, others ...interface{}) template.HTML { - size, class := parseOthers(models.DefaultAvatarPixelSize, "ui avatar image", others...) + size, class := parseOthers(avatars.DefaultAvatarPixelSize, "ui avatar image", others...) src := repo.RelAvatarLink() if src != "" { @@ -586,8 +587,8 @@ func RepoAvatar(repo *models.Repository, others ...interface{}) template.HTML { // AvatarByEmail renders avatars by email address. args: email, name, size (int), class (string) func AvatarByEmail(email string, name string, others ...interface{}) template.HTML { - size, class := parseOthers(models.DefaultAvatarPixelSize, "ui avatar image", others...) - src := models.SizedAvatarLink(email, size*models.AvatarRenderedSizeFactor) + size, class := parseOthers(avatars.DefaultAvatarPixelSize, "ui avatar image", others...) + src := avatars.GenerateEmailAvatarFastLink(email, size*avatars.AvatarRenderedSizeFactor) if src != "" { return AvatarHTML(src, size, class, name) diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 013286f2de90a..f15c798ccfe11 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -2606,5 +2606,5 @@ func handleTeamMentions(ctx *context.Context) { ctx.Data["MentionableTeams"] = ctx.Repo.Owner.Teams ctx.Data["MentionableTeamsOrg"] = ctx.Repo.Owner.Name - ctx.Data["MentionableTeamsOrgAvatar"] = ctx.Repo.Owner.RelAvatarLink() + ctx.Data["MentionableTeamsOrgAvatar"] = ctx.Repo.Owner.AvatarLinkDefaultSize() } diff --git a/routers/web/user/avatar.go b/routers/web/user/avatar.go index 2df5c148f7904..6e6e4e70ab6aa 100644 --- a/routers/web/user/avatar.go +++ b/routers/web/user/avatar.go @@ -5,17 +5,12 @@ package user import ( - "errors" - "net/url" - "path" - "strconv" "strings" "code.gitea.io/gitea/models" + "code.gitea.io/gitea/models/avatars" "code.gitea.io/gitea/modules/context" "code.gitea.io/gitea/modules/httpcache" - "code.gitea.io/gitea/modules/log" - "code.gitea.io/gitea/modules/setting" ) func cacheableRedirect(ctx *context.Context, location string) { @@ -23,82 +18,32 @@ func cacheableRedirect(ctx *context.Context, location string) { ctx.Redirect(location) } -// Avatar redirect browser to user avatar of requested size -func Avatar(ctx *context.Context) { +// AvatarByUserName redirect browser to user avatar of requested size +func AvatarByUserName(ctx *context.Context) { userName := ctx.Params(":username") - size, err := strconv.Atoi(ctx.Params(":size")) - if err != nil { - ctx.ServerError("Invalid avatar size", err) - return - } - - log.Debug("Asked avatar for user %v and size %v", userName, size) + size := ctx.FormInt("size") var user *models.User if strings.ToLower(userName) != "ghost" { - user, err = models.GetUserByName(userName) - if err != nil { - if models.IsErrUserNotExist(err) { - ctx.ServerError("Requested avatar for invalid user", err) - } else { - ctx.ServerError("Retrieving user by name", err) - } - return + var err error + if user, err = models.GetUserByName(userName); err != nil { + ctx.ServerError("Invalid user: "+userName, err) } } else { user = models.NewGhostUser() } - cacheableRedirect(ctx, user.RealSizedAvatarLink(size)) + cacheableRedirect(ctx, user.AvatarLinkWithSize(size)) } -// AvatarByEmailHash redirects the browser to the appropriate Avatar link +// AvatarByEmailHash redirects the browser to the email avatar link func AvatarByEmailHash(ctx *context.Context) { - var err error - hash := ctx.Params(":hash") - if len(hash) == 0 { - ctx.ServerError("invalid avatar hash", errors.New("hash cannot be empty")) - return - } - - var email string - email, err = models.GetEmailForHash(hash) + email, err := avatars.GetEmailForHash(hash) if err != nil { - ctx.ServerError("invalid avatar hash", err) - return - } - if len(email) == 0 { - cacheableRedirect(ctx, models.DefaultAvatarLink()) + ctx.ServerError("invalid avatar hash: "+hash, err) return } size := ctx.FormInt("size") - if size == 0 { - size = models.DefaultAvatarSize - } - - var avatarURL *url.URL - - if setting.EnableFederatedAvatar && setting.LibravatarService != nil { - avatarURL, err = models.LibravatarURL(email) - if err != nil { - avatarURL, err = url.Parse(models.DefaultAvatarLink()) - if err != nil { - ctx.ServerError("invalid default avatar url", err) - return - } - } - } else if !setting.DisableGravatar { - copyOfGravatarSourceURL := *setting.GravatarSourceURL - avatarURL = ©OfGravatarSourceURL - avatarURL.Path = path.Join(avatarURL.Path, hash) - } else { - avatarURL, err = url.Parse(models.DefaultAvatarLink()) - if err != nil { - ctx.ServerError("invalid default avatar url", err) - return - } - } - - cacheableRedirect(ctx, models.MakeFinalAvatarURL(avatarURL, size)) + cacheableRedirect(ctx, avatars.GenerateEmailAvatarFinalLink(email, size)) } diff --git a/routers/web/user/oauth.go b/routers/web/user/oauth.go index cec6a92bbea45..22b3bd83bd6dc 100644 --- a/routers/web/user/oauth.go +++ b/routers/web/user/oauth.go @@ -197,7 +197,7 @@ func newAccessTokenResponse(grant *models.OAuth2Grant, serverKey, clientKey oaut idToken.Name = user.FullName idToken.PreferredUsername = user.Name idToken.Profile = user.HTMLURL() - idToken.Picture = user.AvatarLink() + idToken.Picture = user.AvatarLinkDefaultSize() idToken.Website = user.Website idToken.Locale = user.Language idToken.UpdatedAt = user.UpdatedUnix @@ -245,7 +245,7 @@ func InfoOAuth(ctx *context.Context) { Name: ctx.User.FullName, Username: ctx.User.Name, Email: ctx.User.Email, - Picture: ctx.User.AvatarLink(), + Picture: ctx.User.AvatarLinkDefaultSize(), } ctx.JSON(http.StatusOK, response) } diff --git a/routers/web/user/oauth_test.go b/routers/web/user/oauth_test.go index 09abf1ee2a687..5ac4a5c38fcd9 100644 --- a/routers/web/user/oauth_test.go +++ b/routers/web/user/oauth_test.go @@ -67,7 +67,7 @@ func TestNewAccessTokenResponse_OIDCToken(t *testing.T) { assert.Equal(t, user.FullName, oidcToken.Name) assert.Equal(t, user.Name, oidcToken.PreferredUsername) assert.Equal(t, user.HTMLURL(), oidcToken.Profile) - assert.Equal(t, user.AvatarLink(), oidcToken.Picture) + assert.Equal(t, user.AvatarLinkDefaultSize(), oidcToken.Picture) assert.Equal(t, user.Website, oidcToken.Website) assert.Equal(t, user.UpdatedUnix, oidcToken.UpdatedAt) assert.Equal(t, user.Email, oidcToken.Email) diff --git a/routers/web/web.go b/routers/web/web.go index 8d984abcf2ed9..9f5a2556baedf 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -363,7 +363,7 @@ func RegisterRoutes(m *web.Route) { m.Get("/activate", user.Activate, reqSignIn) m.Post("/activate", user.ActivatePost, reqSignIn) m.Any("/activate_email", user.ActivateEmail) - m.Get("/avatar/{username}/{size}", user.Avatar) + m.Get("/avatar/{username}/{size}", user.AvatarByUserName) m.Get("/email2user", user.Email2User) m.Get("/recover_account", user.ResetPasswd) m.Post("/recover_account", user.ResetPasswdPost) diff --git a/services/auth/sspi_windows.go b/services/auth/sspi_windows.go index d7e0f55242aa7..ec224d9ddf48a 100644 --- a/services/auth/sspi_windows.go +++ b/services/auth/sspi_windows.go @@ -10,6 +10,7 @@ import ( "strings" "code.gitea.io/gitea/models" + "code.gitea.io/gitea/models/avatars" "code.gitea.io/gitea/modules/base" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" @@ -192,7 +193,7 @@ func (s *SSPI) newUser(username string, cfg *sspi.Source) (*models.User, error) IsActive: cfg.AutoActivateUsers, Language: cfg.DefaultLanguage, UseCustomAvatar: true, - Avatar: models.DefaultAvatarLink(), + Avatar: avatars.DefaultAvatarLink(), EmailNotificationsPreference: models.EmailNotificationsDisabled, } if err := models.CreateUser(user); err != nil { From c258ca3ebab9114db2f0d1bda021a3e4b4da3804 Mon Sep 17 00:00:00 2001 From: Xiaoguang Wang Date: Wed, 22 Sep 2021 22:19:44 +0800 Subject: [PATCH 02/17] fix unit test --- integrations/api_user_orgs_test.go | 4 ++-- integrations/user_avatar_test.go | 2 +- models/avatars/avatar.go | 7 ++++--- models/repo_activity.go | 4 ++-- models/user_avatar.go | 4 ++-- modules/convert/convert.go | 2 +- modules/convert/user.go | 2 +- routers/web/repo/issue.go | 2 +- routers/web/user/oauth.go | 4 ++-- routers/web/user/oauth_test.go | 2 +- 10 files changed, 17 insertions(+), 16 deletions(-) diff --git a/integrations/api_user_orgs_test.go b/integrations/api_user_orgs_test.go index 2a5a1e1885122..36b11335c054c 100644 --- a/integrations/api_user_orgs_test.go +++ b/integrations/api_user_orgs_test.go @@ -32,7 +32,7 @@ func TestUserOrgs(t *testing.T) { ID: 3, UserName: user3.Name, FullName: user3.FullName, - AvatarURL: user3.AvatarLinkDefaultSize(), + AvatarURL: user3.AvatarLink(), Description: "", Website: "", Location: "", @@ -88,7 +88,7 @@ func TestMyOrgs(t *testing.T) { ID: 3, UserName: user3.Name, FullName: user3.FullName, - AvatarURL: user3.AvatarLinkDefaultSize(), + AvatarURL: user3.AvatarLink(), Description: "", Website: "", Location: "", diff --git a/integrations/user_avatar_test.go b/integrations/user_avatar_test.go index c1808e931424f..c909fd6851c6c 100644 --- a/integrations/user_avatar_test.go +++ b/integrations/user_avatar_test.go @@ -74,7 +74,7 @@ func TestUserAvatar(t *testing.T) { user2 = db.AssertExistsAndLoadBean(t, &models.User{ID: 2}).(*models.User) // owner of the repo3, is an org - req = NewRequest(t, "GET", user2.AvatarLinkDefaultSize()) + req = NewRequest(t, "GET", user2.AvatarLink()) resp := session.MakeRequest(t, req, http.StatusFound) location := resp.Header().Get("Location") if !strings.HasPrefix(location, "/avatars") { diff --git a/models/avatars/avatar.go b/models/avatars/avatar.go index 231fd871da2d6..9e98f63530738 100644 --- a/models/avatars/avatar.go +++ b/models/avatars/avatar.go @@ -154,11 +154,12 @@ func generateEmailAvatarLink(email string, size int, final bool) string { return DefaultAvatarLink() } - avatarURL.Query().Set("d", "identicon") + urlQuery := avatarURL.Query() + urlQuery.Set("d", "identicon") if size > 0 { - avatarURL.Query().Set("s", strconv.Itoa(size)) + urlQuery.Set("s", strconv.Itoa(size)) } - avatarURL.RawQuery = avatarURL.Query().Encode() + avatarURL.RawQuery = urlQuery.Encode() return avatarURL.String() } diff --git a/models/repo_activity.go b/models/repo_activity.go index 0692693a71d1e..cfbda21411d89 100644 --- a/models/repo_activity.go +++ b/models/repo_activity.go @@ -94,7 +94,7 @@ func GetActivityStatsTopAuthors(repo *Repository, timeFrom time.Time, count int) } users := make(map[int64]*ActivityAuthorData) var unknownUserID int64 - unknownUserAvatarLink := NewGhostUser().AvatarLinkDefaultSize() + unknownUserAvatarLink := NewGhostUser().AvatarLink() for _, v := range code.Authors { if len(v.Email) == 0 { continue @@ -116,7 +116,7 @@ func GetActivityStatsTopAuthors(repo *Repository, timeFrom time.Time, count int) users[u.ID] = &ActivityAuthorData{ Name: u.DisplayName(), Login: u.LowerName, - AvatarLink: u.AvatarLinkDefaultSize(), + AvatarLink: u.AvatarLink(), HomeLink: u.HomeLink(), Commits: v.Commits, } diff --git a/models/user_avatar.go b/models/user_avatar.go index 2e032d58a12ec..1810f6583af78 100644 --- a/models/user_avatar.go +++ b/models/user_avatar.go @@ -96,8 +96,8 @@ func (u *User) AvatarLinkWithSize(size int) string { return avatars.GenerateEmailAvatarFastLink(u.AvatarEmail, size) } -// AvatarLinkDefaultSize returns a avatar link with default size -func (u *User) AvatarLinkDefaultSize() string { +// AvatarLink returns a avatar link with default size +func (u *User) AvatarLink() string { return u.AvatarLinkWithSize(avatars.DefaultAvatarSize) } diff --git a/modules/convert/convert.go b/modules/convert/convert.go index 50c6718f68550..404786ec9c9ad 100644 --- a/modules/convert/convert.go +++ b/modules/convert/convert.go @@ -277,7 +277,7 @@ func ToDeployKey(apiLink string, key *models.DeployKey) *api.DeployKey { func ToOrganization(org *models.User) *api.Organization { return &api.Organization{ ID: org.ID, - AvatarURL: org.AvatarLinkDefaultSize(), + AvatarURL: org.AvatarLink(), UserName: org.Name, FullName: org.FullName, Description: org.Description, diff --git a/modules/convert/user.go b/modules/convert/user.go index 898a9d4aae19d..164ffb71fd2e7 100644 --- a/modules/convert/user.go +++ b/modules/convert/user.go @@ -51,7 +51,7 @@ func toUser(user *models.User, signed, authed bool) *api.User { UserName: user.Name, FullName: markup.Sanitize(user.FullName), Email: user.GetEmail(), - AvatarURL: user.AvatarLinkDefaultSize(), + AvatarURL: user.AvatarLink(), Created: user.CreatedUnix.AsTime(), Restricted: user.IsRestricted, Location: user.Location, diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index f15c798ccfe11..a7315dc1c2978 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -2606,5 +2606,5 @@ func handleTeamMentions(ctx *context.Context) { ctx.Data["MentionableTeams"] = ctx.Repo.Owner.Teams ctx.Data["MentionableTeamsOrg"] = ctx.Repo.Owner.Name - ctx.Data["MentionableTeamsOrgAvatar"] = ctx.Repo.Owner.AvatarLinkDefaultSize() + ctx.Data["MentionableTeamsOrgAvatar"] = ctx.Repo.Owner.AvatarLink() } diff --git a/routers/web/user/oauth.go b/routers/web/user/oauth.go index 22b3bd83bd6dc..cec6a92bbea45 100644 --- a/routers/web/user/oauth.go +++ b/routers/web/user/oauth.go @@ -197,7 +197,7 @@ func newAccessTokenResponse(grant *models.OAuth2Grant, serverKey, clientKey oaut idToken.Name = user.FullName idToken.PreferredUsername = user.Name idToken.Profile = user.HTMLURL() - idToken.Picture = user.AvatarLinkDefaultSize() + idToken.Picture = user.AvatarLink() idToken.Website = user.Website idToken.Locale = user.Language idToken.UpdatedAt = user.UpdatedUnix @@ -245,7 +245,7 @@ func InfoOAuth(ctx *context.Context) { Name: ctx.User.FullName, Username: ctx.User.Name, Email: ctx.User.Email, - Picture: ctx.User.AvatarLinkDefaultSize(), + Picture: ctx.User.AvatarLink(), } ctx.JSON(http.StatusOK, response) } diff --git a/routers/web/user/oauth_test.go b/routers/web/user/oauth_test.go index 5ac4a5c38fcd9..09abf1ee2a687 100644 --- a/routers/web/user/oauth_test.go +++ b/routers/web/user/oauth_test.go @@ -67,7 +67,7 @@ func TestNewAccessTokenResponse_OIDCToken(t *testing.T) { assert.Equal(t, user.FullName, oidcToken.Name) assert.Equal(t, user.Name, oidcToken.PreferredUsername) assert.Equal(t, user.HTMLURL(), oidcToken.Profile) - assert.Equal(t, user.AvatarLinkDefaultSize(), oidcToken.Picture) + assert.Equal(t, user.AvatarLink(), oidcToken.Picture) assert.Equal(t, user.Website, oidcToken.Website) assert.Equal(t, user.UpdatedUnix, oidcToken.UpdatedAt) assert.Equal(t, user.Email, oidcToken.Email) From fa167dda05a99d1b7a7f9a25185e35630b344fe9 Mon Sep 17 00:00:00 2001 From: Xiaoguang Wang Date: Wed, 22 Sep 2021 22:40:12 +0800 Subject: [PATCH 03/17] fix avatar link --- templates/base/head.tmpl | 4 ++-- templates/repo/issue/view_content/comments.tmpl | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/templates/base/head.tmpl b/templates/base/head.tmpl index 328661eb9dec5..15f2826abf928 100644 --- a/templates/base/head.tmpl +++ b/templates/base/head.tmpl @@ -48,11 +48,11 @@ tributeValues: Array.from(new Map([ {{ range .Participants }} ['{{.Name}}', {key: '{{.Name}} {{.FullName}}', value: '{{.Name}}', - name: '{{.Name}}', fullname: '{{.FullName}}', avatar: '{{.RelAvatarLink}}'}], + name: '{{.Name}}', fullname: '{{.FullName}}', avatar: '{{.AvatarLink}}'}], {{ end }} {{ range .Assignees }} ['{{.Name}}', {key: '{{.Name}} {{.FullName}}', value: '{{.Name}}', - name: '{{.Name}}', fullname: '{{.FullName}}', avatar: '{{.RelAvatarLink}}'}], + name: '{{.Name}}', fullname: '{{.FullName}}', avatar: '{{.AvatarLink}}'}], {{ end }} {{ range .MentionableTeams }} ['{{$.MentionableTeamsOrg}}/{{.Name}}', {key: '{{$.MentionableTeamsOrg}}/{{.Name}}', value: '{{$.MentionableTeamsOrg}}/{{.Name}}', diff --git a/templates/repo/issue/view_content/comments.tmpl b/templates/repo/issue/view_content/comments.tmpl index dabba7bc7480e..c7d7574231ed5 100644 --- a/templates/repo/issue/view_content/comments.tmpl +++ b/templates/repo/issue/view_content/comments.tmpl @@ -746,7 +746,7 @@
- + {{svg "octicon-x" 16}} From bb5319882c1a0192fd0fe4875140efa3ab5fc8c3 Mon Sep 17 00:00:00 2001 From: Xiaoguang Wang Date: Wed, 22 Sep 2021 23:34:16 +0800 Subject: [PATCH 04/17] fix unit test --- integrations/user_avatar_test.go | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/integrations/user_avatar_test.go b/integrations/user_avatar_test.go index c909fd6851c6c..6de2914f7f2e2 100644 --- a/integrations/user_avatar_test.go +++ b/integrations/user_avatar_test.go @@ -11,7 +11,6 @@ import ( "mime/multipart" "net/http" "net/url" - "strings" "testing" "code.gitea.io/gitea/models" @@ -75,14 +74,8 @@ func TestUserAvatar(t *testing.T) { user2 = db.AssertExistsAndLoadBean(t, &models.User{ID: 2}).(*models.User) // owner of the repo3, is an org req = NewRequest(t, "GET", user2.AvatarLink()) - resp := session.MakeRequest(t, req, http.StatusFound) - location := resp.Header().Get("Location") - if !strings.HasPrefix(location, "/avatars") { - assert.Fail(t, "Avatar location is not local: %s", location) - } - req = NewRequest(t, "GET", location) - session.MakeRequest(t, req, http.StatusOK) + _ = session.MakeRequest(t, req, http.StatusOK) - // Can't test if the response matches because the image is regened on upload but checking that this at least doesn't give a 404 should be enough. + // Can't test if the response matches because the image is re-generated on upload but checking that this at least doesn't give a 404 should be enough. }) } From a74873a7b28d77ebc9f441852124e49a1b62990a Mon Sep 17 00:00:00 2001 From: Xiaoguang Wang Date: Thu, 23 Sep 2021 12:41:14 +0800 Subject: [PATCH 05/17] optimize http cache header --- modules/httpcache/httpcache.go | 18 +++++++++++------- routers/web/user/avatar.go | 6 +++++- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/modules/httpcache/httpcache.go b/modules/httpcache/httpcache.go index f5e3906be65c4..7e778acc85735 100644 --- a/modules/httpcache/httpcache.go +++ b/modules/httpcache/httpcache.go @@ -16,12 +16,16 @@ import ( "code.gitea.io/gitea/modules/setting" ) -// GetCacheControl returns a suitable "Cache-Control" header value -func GetCacheControl() string { - if !setting.IsProd() { - return "no-store" +// AddCacheControlToHeader adds suitable cache-control headers to response +func AddCacheControlToHeader(h http.Header, d time.Duration) { + if setting.IsProd() { + h.Set("Cache-Control", "private, max-age="+strconv.Itoa(int(d.Seconds()))) + } else { + h.Set("Cache-Control", "no-store") + // to remind users they are using non-prod setting. + // some users may be confused by "Cache-Control: no-store" in their setup if they did wrong to `RUN_MODE` in `app.ini`. + h.Set("X-Gitea-Cache-Control", "not-prod") } - return "private, max-age=" + strconv.FormatInt(int64(setting.StaticCacheTime.Seconds()), 10) } // generateETag generates an ETag based on size, filename and file modification time @@ -32,7 +36,7 @@ func generateETag(fi os.FileInfo) string { // HandleTimeCache handles time-based caching for a HTTP request func HandleTimeCache(req *http.Request, w http.ResponseWriter, fi os.FileInfo) (handled bool) { - w.Header().Set("Cache-Control", GetCacheControl()) + AddCacheControlToHeader(w.Header(), setting.StaticCacheTime) ifModifiedSince := req.Header.Get("If-Modified-Since") if ifModifiedSince != "" { @@ -63,7 +67,7 @@ func HandleGenericETagCache(req *http.Request, w http.ResponseWriter, etag strin return true } } - w.Header().Set("Cache-Control", GetCacheControl()) + AddCacheControlToHeader(w.Header(), setting.StaticCacheTime) return false } diff --git a/routers/web/user/avatar.go b/routers/web/user/avatar.go index 6e6e4e70ab6aa..eccd24ec7a165 100644 --- a/routers/web/user/avatar.go +++ b/routers/web/user/avatar.go @@ -6,6 +6,7 @@ package user import ( "strings" + "time" "code.gitea.io/gitea/models" "code.gitea.io/gitea/models/avatars" @@ -14,7 +15,10 @@ import ( ) func cacheableRedirect(ctx *context.Context, location string) { - ctx.Resp.Header().Set("Cache-Control", httpcache.GetCacheControl()) + // here we should not use `setting.StaticCacheTime`, it is pretty long (default: 6 hours) + // we must make sure the redirection cache time is short enough, otherwise a user won't see the updated avatar in 6 hours + // it's OK to make the cache time short, it is only a redirection, and doesn't cost much to make a new request + httpcache.AddCacheControlToHeader(ctx.Resp.Header(), time.Minute) ctx.Redirect(location) } From 239f5b87469e5c358d249dc3a53ec7cab00076a4 Mon Sep 17 00:00:00 2001 From: Xiaoguang Wang Date: Thu, 23 Sep 2021 12:48:35 +0800 Subject: [PATCH 06/17] fix avatar size --- models/avatars/avatar.go | 3 ++- routers/web/user/avatar.go | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/models/avatars/avatar.go b/models/avatars/avatar.go index 9e98f63530738..2cdcbf88240d9 100644 --- a/models/avatars/avatar.go +++ b/models/avatars/avatar.go @@ -18,7 +18,8 @@ import ( ) // DefaultAvatarSize is a sentinel value for the default avatar size, as determined by the avatar-hosting service. -const DefaultAvatarSize = -1 +// in history the value was "-1", it's not handy as "0", because the int value of empty param is always "0" +const DefaultAvatarSize = 0 // DefaultAvatarPixelSize is the default size in pixels of a rendered avatar const DefaultAvatarPixelSize = 28 diff --git a/routers/web/user/avatar.go b/routers/web/user/avatar.go index eccd24ec7a165..a8c026d04dfe7 100644 --- a/routers/web/user/avatar.go +++ b/routers/web/user/avatar.go @@ -25,7 +25,7 @@ func cacheableRedirect(ctx *context.Context, location string) { // AvatarByUserName redirect browser to user avatar of requested size func AvatarByUserName(ctx *context.Context) { userName := ctx.Params(":username") - size := ctx.FormInt("size") + size := int(ctx.ParamsInt64(":size")) var user *models.User if strings.ToLower(userName) != "ghost" { From 758b2a64cda7cd5b971c4923d13716ee0ef97133 Mon Sep 17 00:00:00 2001 From: Xiaoguang Wang Date: Thu, 23 Sep 2021 12:51:26 +0800 Subject: [PATCH 07/17] fix avatar size --- models/avatars/avatar.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/models/avatars/avatar.go b/models/avatars/avatar.go index 2cdcbf88240d9..19571ff33dc56 100644 --- a/models/avatars/avatar.go +++ b/models/avatars/avatar.go @@ -123,10 +123,6 @@ func GenerateUserAvatarFastLink(userName string, size int) string { // if final is true, it may use a slow path (eg: query DNS). // if final is false, it always uses a fast path. func generateEmailAvatarLink(email string, size int, final bool) string { - if size <= 0 { - size = DefaultAvatarSize - } - email = strings.TrimSpace(email) if email == "" { return DefaultAvatarLink() From 4ec07df636c65205eba3216c4fece3dc019c6681 Mon Sep 17 00:00:00 2001 From: Xiaoguang Wang Date: Thu, 23 Sep 2021 13:13:46 +0800 Subject: [PATCH 08/17] avatar-refactor --- models/avatars/avatar.go | 18 +++++++++++------- models/user_avatar.go | 25 ++++++++++--------------- 2 files changed, 21 insertions(+), 22 deletions(-) diff --git a/models/avatars/avatar.go b/models/avatars/avatar.go index 19571ff33dc56..9e9c627e598ec 100644 --- a/models/avatars/avatar.go +++ b/models/avatars/avatar.go @@ -1,4 +1,4 @@ -// Copyright 2020 The Gitea Authors. All rights reserved. +// Copyright 2021 The Gitea Authors. All rights reserved. // Use of this source code is governed by a MIT-style // license that can be found in the LICENSE file. @@ -17,10 +17,6 @@ import ( "code.gitea.io/gitea/modules/setting" ) -// DefaultAvatarSize is a sentinel value for the default avatar size, as determined by the avatar-hosting service. -// in history the value was "-1", it's not handy as "0", because the int value of empty param is always "0" -const DefaultAvatarSize = 0 - // DefaultAvatarPixelSize is the default size in pixels of a rendered avatar const DefaultAvatarPixelSize = 28 @@ -110,7 +106,7 @@ func saveEmailHash(email string) string { return emailHash } -// GenerateUserAvatarFastLink returns a fast link to the user's avatar via the local explore page. +// GenerateUserAvatarFastLink returns a fast link (302) to the user's avatar: "/user/avatar/${User.Name}/${size}" func GenerateUserAvatarFastLink(userName string, size int) string { if size < 0 { size = 0 @@ -119,6 +115,14 @@ func GenerateUserAvatarFastLink(userName string, size int) string { return setting.AppURL + strings.TrimPrefix(link, setting.AppSubURL)[1:] } +// GenerateUserAvatarImageLink returns a link for `User.Avatar` image file: "/avatars/${User.Avatar}" +func GenerateUserAvatarImageLink(userAvatar string, size int) string { + if size > 0 { + return setting.AppSubURL + "/avatars/" + userAvatar + "?size=" + strconv.Itoa(size) + } + return setting.AppSubURL + "/avatars/" + userAvatar +} + // generateEmailAvatarLink returns a email avatar link. // if final is true, it may use a slow path (eg: query DNS). // if final is false, it always uses a fast path. @@ -160,7 +164,7 @@ func generateEmailAvatarLink(email string, size int, final bool) string { return avatarURL.String() } -//GenerateEmailAvatarFastLink returns a avatar link (fast, the link may be a delegated one) +//GenerateEmailAvatarFastLink returns a avatar link (fast, the link may be a delegated one: "/avatar/${hash}") func GenerateEmailAvatarFastLink(email string, size int) string { return generateEmailAvatarLink(email, size, false) } diff --git a/models/user_avatar.go b/models/user_avatar.go index 1810f6583af78..77921ebfae1a0 100644 --- a/models/user_avatar.go +++ b/models/user_avatar.go @@ -9,7 +9,6 @@ import ( "fmt" "image/png" "io" - "strconv" "code.gitea.io/gitea/models/avatars" "code.gitea.io/gitea/models/db" @@ -60,45 +59,41 @@ func (u *User) generateRandomAvatar(e db.Engine) error { return nil } -// AvatarLinkWithSize returns a link to the user's avatar with size +// AvatarLinkWithSize returns a link to the user's avatar with size. size <= 0 means default size func (u *User) AvatarLinkWithSize(size int) string { if u.ID == -1 { // ghost user return avatars.DefaultAvatarLink() } - var useLocalAvatar bool + useLocalAvatar := false + autoGenerateAvatar := false switch { case u.UseCustomAvatar: useLocalAvatar = true case setting.DisableGravatar, setting.OfflineMode: useLocalAvatar = true - if u.Avatar == "" { + autoGenerateAvatar = true + } + + if useLocalAvatar { + if u.Avatar == "" && autoGenerateAvatar { if err := u.GenerateRandomAvatar(); err != nil { log.Error("GenerateRandomAvatar: %v", err) } } - default: - useLocalAvatar = false - } - - if useLocalAvatar { if u.Avatar == "" { return avatars.DefaultAvatarLink() } - if size > 0 { - return setting.AppSubURL + "/avatars/" + u.Avatar + "?size=" + strconv.Itoa(size) - } - return setting.AppSubURL + "/avatars/" + u.Avatar + return avatars.GenerateUserAvatarImageLink(u.Avatar, size) } - return avatars.GenerateEmailAvatarFastLink(u.AvatarEmail, size) } // AvatarLink returns a avatar link with default size func (u *User) AvatarLink() string { - return u.AvatarLinkWithSize(avatars.DefaultAvatarSize) + return u.AvatarLinkWithSize(0) } // UploadAvatar saves custom avatar for user. From d4e9afb536c2107b60035520dd7d2de7fdf8e111 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 23 Sep 2021 14:09:53 +0800 Subject: [PATCH 09/17] fix unit test --- modules/httpcache/httpcache.go | 3 ++- modules/httpcache/httpcache_test.go | 28 ++++++++++++++++++++-------- 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/modules/httpcache/httpcache.go b/modules/httpcache/httpcache.go index 7e778acc85735..fa8ccfc41e71a 100644 --- a/modules/httpcache/httpcache.go +++ b/modules/httpcache/httpcache.go @@ -24,7 +24,8 @@ func AddCacheControlToHeader(h http.Header, d time.Duration) { h.Set("Cache-Control", "no-store") // to remind users they are using non-prod setting. // some users may be confused by "Cache-Control: no-store" in their setup if they did wrong to `RUN_MODE` in `app.ini`. - h.Set("X-Gitea-Cache-Control", "not-prod") + h.Add("X-Gitea-Debug", "RUN_MODE="+setting.RunMode) + h.Add("X-Gitea-Debug", "CacheControl=not-prod") } } diff --git a/modules/httpcache/httpcache_test.go b/modules/httpcache/httpcache_test.go index fe5ca179560ff..aeb0ae35445a1 100644 --- a/modules/httpcache/httpcache_test.go +++ b/modules/httpcache/httpcache_test.go @@ -8,6 +8,7 @@ import ( "net/http" "net/http/httptest" "os" + "strings" "testing" "time" @@ -24,6 +25,17 @@ func (m mockFileInfo) ModTime() time.Time { return time.Time{} } func (m mockFileInfo) IsDir() bool { return false } func (m mockFileInfo) Sys() interface{} { return nil } +func countFormalHeaders(h http.Header) (c int) { + for k, _ := range h { + // ignore our headers for internal usage + if strings.HasPrefix(k, "X-Gitea-") { + continue + } + c++ + } + return c +} + func TestHandleFileETagCache(t *testing.T) { fi := mockFileInfo{} etag := `"MTBnaXRlYS50ZXN0TW9uLCAwMSBKYW4gMDAwMSAwMDowMDowMCBHTVQ="` @@ -35,7 +47,7 @@ func TestHandleFileETagCache(t *testing.T) { handled := HandleFileETagCache(req, w, fi) assert.False(t, handled) - assert.Len(t, w.Header(), 2) + assert.Equal(t, countFormalHeaders(w.Header()), 2) assert.Contains(t, w.Header(), "Cache-Control") assert.Contains(t, w.Header(), "Etag") assert.Equal(t, etag, w.Header().Get("Etag")) @@ -49,7 +61,7 @@ func TestHandleFileETagCache(t *testing.T) { handled := HandleFileETagCache(req, w, fi) assert.False(t, handled) - assert.Len(t, w.Header(), 2) + assert.Equal(t, countFormalHeaders(w.Header()), 2) assert.Contains(t, w.Header(), "Cache-Control") assert.Contains(t, w.Header(), "Etag") assert.Equal(t, etag, w.Header().Get("Etag")) @@ -63,7 +75,7 @@ func TestHandleFileETagCache(t *testing.T) { handled := HandleFileETagCache(req, w, fi) assert.True(t, handled) - assert.Len(t, w.Header(), 1) + assert.Equal(t, countFormalHeaders(w.Header()), 1) assert.Contains(t, w.Header(), "Etag") assert.Equal(t, etag, w.Header().Get("Etag")) assert.Equal(t, http.StatusNotModified, w.Code) @@ -80,7 +92,7 @@ func TestHandleGenericETagCache(t *testing.T) { handled := HandleGenericETagCache(req, w, etag) assert.False(t, handled) - assert.Len(t, w.Header(), 2) + assert.Equal(t, countFormalHeaders(w.Header()), 2) assert.Contains(t, w.Header(), "Cache-Control") assert.Contains(t, w.Header(), "Etag") assert.Equal(t, etag, w.Header().Get("Etag")) @@ -94,7 +106,7 @@ func TestHandleGenericETagCache(t *testing.T) { handled := HandleGenericETagCache(req, w, etag) assert.False(t, handled) - assert.Len(t, w.Header(), 2) + assert.Equal(t, countFormalHeaders(w.Header()), 2) assert.Contains(t, w.Header(), "Cache-Control") assert.Contains(t, w.Header(), "Etag") assert.Equal(t, etag, w.Header().Get("Etag")) @@ -108,7 +120,7 @@ func TestHandleGenericETagCache(t *testing.T) { handled := HandleGenericETagCache(req, w, etag) assert.True(t, handled) - assert.Len(t, w.Header(), 1) + assert.Equal(t, countFormalHeaders(w.Header()), 1) assert.Contains(t, w.Header(), "Etag") assert.Equal(t, etag, w.Header().Get("Etag")) assert.Equal(t, http.StatusNotModified, w.Code) @@ -122,7 +134,7 @@ func TestHandleGenericETagCache(t *testing.T) { handled := HandleGenericETagCache(req, w, etag) assert.False(t, handled) - assert.Len(t, w.Header(), 2) + assert.Equal(t, countFormalHeaders(w.Header()), 2) assert.Contains(t, w.Header(), "Cache-Control") assert.Contains(t, w.Header(), "Etag") assert.Equal(t, etag, w.Header().Get("Etag")) @@ -136,7 +148,7 @@ func TestHandleGenericETagCache(t *testing.T) { handled := HandleGenericETagCache(req, w, etag) assert.True(t, handled) - assert.Len(t, w.Header(), 1) + assert.Equal(t, countFormalHeaders(w.Header()), 1) assert.Contains(t, w.Header(), "Etag") assert.Equal(t, etag, w.Header().Get("Etag")) assert.Equal(t, http.StatusNotModified, w.Code) From 9eb3043bce4cd00be3079e75d002fd0510ceda13 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 23 Sep 2021 14:12:57 +0800 Subject: [PATCH 10/17] fix unit test --- modules/httpcache/httpcache_test.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/modules/httpcache/httpcache_test.go b/modules/httpcache/httpcache_test.go index aeb0ae35445a1..55926bec54953 100644 --- a/modules/httpcache/httpcache_test.go +++ b/modules/httpcache/httpcache_test.go @@ -47,7 +47,7 @@ func TestHandleFileETagCache(t *testing.T) { handled := HandleFileETagCache(req, w, fi) assert.False(t, handled) - assert.Equal(t, countFormalHeaders(w.Header()), 2) + assert.Equal(t, 2, countFormalHeaders(w.Header())) assert.Contains(t, w.Header(), "Cache-Control") assert.Contains(t, w.Header(), "Etag") assert.Equal(t, etag, w.Header().Get("Etag")) @@ -61,7 +61,7 @@ func TestHandleFileETagCache(t *testing.T) { handled := HandleFileETagCache(req, w, fi) assert.False(t, handled) - assert.Equal(t, countFormalHeaders(w.Header()), 2) + assert.Equal(t, 2, countFormalHeaders(w.Header())) assert.Contains(t, w.Header(), "Cache-Control") assert.Contains(t, w.Header(), "Etag") assert.Equal(t, etag, w.Header().Get("Etag")) @@ -75,7 +75,7 @@ func TestHandleFileETagCache(t *testing.T) { handled := HandleFileETagCache(req, w, fi) assert.True(t, handled) - assert.Equal(t, countFormalHeaders(w.Header()), 1) + assert.Equal(t, 1, countFormalHeaders(w.Header())) assert.Contains(t, w.Header(), "Etag") assert.Equal(t, etag, w.Header().Get("Etag")) assert.Equal(t, http.StatusNotModified, w.Code) @@ -92,7 +92,7 @@ func TestHandleGenericETagCache(t *testing.T) { handled := HandleGenericETagCache(req, w, etag) assert.False(t, handled) - assert.Equal(t, countFormalHeaders(w.Header()), 2) + assert.Equal(t, 2, countFormalHeaders(w.Header())) assert.Contains(t, w.Header(), "Cache-Control") assert.Contains(t, w.Header(), "Etag") assert.Equal(t, etag, w.Header().Get("Etag")) @@ -106,7 +106,7 @@ func TestHandleGenericETagCache(t *testing.T) { handled := HandleGenericETagCache(req, w, etag) assert.False(t, handled) - assert.Equal(t, countFormalHeaders(w.Header()), 2) + assert.Equal(t, 2, countFormalHeaders(w.Header())) assert.Contains(t, w.Header(), "Cache-Control") assert.Contains(t, w.Header(), "Etag") assert.Equal(t, etag, w.Header().Get("Etag")) @@ -120,7 +120,7 @@ func TestHandleGenericETagCache(t *testing.T) { handled := HandleGenericETagCache(req, w, etag) assert.True(t, handled) - assert.Equal(t, countFormalHeaders(w.Header()), 1) + assert.Equal(t, 1, countFormalHeaders(w.Header())) assert.Contains(t, w.Header(), "Etag") assert.Equal(t, etag, w.Header().Get("Etag")) assert.Equal(t, http.StatusNotModified, w.Code) @@ -134,7 +134,7 @@ func TestHandleGenericETagCache(t *testing.T) { handled := HandleGenericETagCache(req, w, etag) assert.False(t, handled) - assert.Equal(t, countFormalHeaders(w.Header()), 2) + assert.Equal(t, 2, countFormalHeaders(w.Header())) assert.Contains(t, w.Header(), "Cache-Control") assert.Contains(t, w.Header(), "Etag") assert.Equal(t, etag, w.Header().Get("Etag")) @@ -148,7 +148,7 @@ func TestHandleGenericETagCache(t *testing.T) { handled := HandleGenericETagCache(req, w, etag) assert.True(t, handled) - assert.Equal(t, countFormalHeaders(w.Header()), 1) + assert.Equal(t, 1, countFormalHeaders(w.Header())) assert.Contains(t, w.Header(), "Etag") assert.Equal(t, etag, w.Header().Get("Etag")) assert.Equal(t, http.StatusNotModified, w.Code) From 60d0dfd277dda78041feeb09c0f04c27040992c2 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 23 Sep 2021 14:24:40 +0800 Subject: [PATCH 11/17] fix appsuburl --- models/avatars/avatar.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/models/avatars/avatar.go b/models/avatars/avatar.go index 9e9c627e598ec..89d33a10df7e9 100644 --- a/models/avatars/avatar.go +++ b/models/avatars/avatar.go @@ -111,8 +111,8 @@ func GenerateUserAvatarFastLink(userName string, size int) string { if size < 0 { size = 0 } - link := setting.AppSubURL + "/user/avatar/" + userName + "/" + strconv.Itoa(size) - return setting.AppURL + strings.TrimPrefix(link, setting.AppSubURL)[1:] + //AppSubURL: It is either "" or starts with '/' and ends without '/', such as '/{subpath}'. question: should we use AppURL or StaticURLPrefix? + return setting.AppSubURL + "/user/avatar/" + userName + "/" + strconv.Itoa(size) } // GenerateUserAvatarImageLink returns a link for `User.Avatar` image file: "/avatars/${User.Avatar}" From 851167a2c49b114bbc263c31fb1d06e2502a40ca Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 23 Sep 2021 14:32:58 +0800 Subject: [PATCH 12/17] fix unit test --- modules/httpcache/httpcache_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/httpcache/httpcache_test.go b/modules/httpcache/httpcache_test.go index 55926bec54953..68ac892c91e7f 100644 --- a/modules/httpcache/httpcache_test.go +++ b/modules/httpcache/httpcache_test.go @@ -26,7 +26,7 @@ func (m mockFileInfo) IsDir() bool { return false } func (m mockFileInfo) Sys() interface{} { return nil } func countFormalHeaders(h http.Header) (c int) { - for k, _ := range h { + for k := range h { // ignore our headers for internal usage if strings.HasPrefix(k, "X-Gitea-") { continue From 91151ede1561a268556e742daa12c9fb044457f7 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 24 Sep 2021 00:03:46 +0800 Subject: [PATCH 13/17] fix merge, set cacheableRedirect cache time to 5m --- models/avatars/avatar.go | 1 + routers/web/user/avatar.go | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/models/avatars/avatar.go b/models/avatars/avatar.go index 13242d1107e42..00f36c5b6c663 100644 --- a/models/avatars/avatar.go +++ b/models/avatars/avatar.go @@ -5,6 +5,7 @@ package avatars import ( + "context" "net/url" "path" "strconv" diff --git a/routers/web/user/avatar.go b/routers/web/user/avatar.go index a8c026d04dfe7..d9663d1c8c72d 100644 --- a/routers/web/user/avatar.go +++ b/routers/web/user/avatar.go @@ -18,7 +18,7 @@ func cacheableRedirect(ctx *context.Context, location string) { // here we should not use `setting.StaticCacheTime`, it is pretty long (default: 6 hours) // we must make sure the redirection cache time is short enough, otherwise a user won't see the updated avatar in 6 hours // it's OK to make the cache time short, it is only a redirection, and doesn't cost much to make a new request - httpcache.AddCacheControlToHeader(ctx.Resp.Header(), time.Minute) + httpcache.AddCacheControlToHeader(ctx.Resp.Header(), 5 * time.Minute) ctx.Redirect(location) } From 2b39ae59a2107443474161eaf3a5f682766193e1 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 24 Sep 2021 01:30:58 +0800 Subject: [PATCH 14/17] fmt --- routers/web/user/avatar.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/web/user/avatar.go b/routers/web/user/avatar.go index d9663d1c8c72d..8d6b30b4571d4 100644 --- a/routers/web/user/avatar.go +++ b/routers/web/user/avatar.go @@ -18,7 +18,7 @@ func cacheableRedirect(ctx *context.Context, location string) { // here we should not use `setting.StaticCacheTime`, it is pretty long (default: 6 hours) // we must make sure the redirection cache time is short enough, otherwise a user won't see the updated avatar in 6 hours // it's OK to make the cache time short, it is only a redirection, and doesn't cost much to make a new request - httpcache.AddCacheControlToHeader(ctx.Resp.Header(), 5 * time.Minute) + httpcache.AddCacheControlToHeader(ctx.Resp.Header(), 5*time.Minute) ctx.Redirect(location) } From ef27c5599f0cdd690168fadc584726aaede07beb Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 30 Sep 2021 12:11:15 +0800 Subject: [PATCH 15/17] clean up --- models/avatars/avatar.go | 46 ++++++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 21 deletions(-) diff --git a/models/avatars/avatar.go b/models/avatars/avatar.go index 00f36c5b6c663..0a1445d2f2d09 100644 --- a/models/avatars/avatar.go +++ b/models/avatars/avatar.go @@ -112,7 +112,6 @@ func GenerateUserAvatarFastLink(userName string, size int) string { if size < 0 { size = 0 } - //AppSubURL: It is either "" or starts with '/' and ends without '/', such as '/{subpath}'. question: should we use AppURL or StaticURLPrefix? return setting.AppSubURL + "/user/avatar/" + userName + "/" + strconv.Itoa(size) } @@ -124,6 +123,17 @@ func GenerateUserAvatarImageLink(userAvatar string, size int) string { return setting.AppSubURL + "/avatars/" + userAvatar } +// generateRecognizedAvatarURL generate a recognized avatar (Gravatar/Libravatar) URL, it modifies the URL so the parameter is passed by a copy +func generateRecognizedAvatarURL(u url.URL, size int) string { + urlQuery := u.Query() + urlQuery.Set("d", "identicon") + if size > 0 { + urlQuery.Set("s", strconv.Itoa(size)) + } + u.RawQuery = urlQuery.Encode() + return u.String() +} + // generateEmailAvatarLink returns a email avatar link. // if final is true, it may use a slow path (eg: query DNS). // if final is false, it always uses a fast path. @@ -133,36 +143,30 @@ func generateEmailAvatarLink(email string, size int, final bool) string { return DefaultAvatarLink() } - var avatarURL *url.URL var err error - if setting.EnableFederatedAvatar && setting.LibravatarService != nil { emailHash := saveEmailHash(email) if final { + // for final link, we can spend more time on slow external query + var avatarURL *url.URL if avatarURL, err = LibravatarURL(email); err != nil { return DefaultAvatarLink() } - } else { - if size > 0 { - return setting.AppSubURL + "/avatar/" + emailHash + "?size=" + strconv.Itoa(size) - } - return setting.AppSubURL + "/avatar/" + emailHash + return generateRecognizedAvatarURL(*avatarURL, size) + } + // for non-final link, we should return fast (use a 302 redirection link) + urlStr := setting.AppSubURL + "/avatar/" + emailHash + if size > 0 { + urlStr += "?size=" + strconv.Itoa(size) } + return urlStr } else if !setting.DisableGravatar { - avatarURLDummy := *setting.GravatarSourceURL // copy GravatarSourceURL, because we will modify its Path. - avatarURL = &avatarURLDummy - avatarURL.Path = path.Join(avatarURL.Path, HashEmail(email)) - } else { - return DefaultAvatarLink() - } - - urlQuery := avatarURL.Query() - urlQuery.Set("d", "identicon") - if size > 0 { - urlQuery.Set("s", strconv.Itoa(size)) + // copy GravatarSourceURL, because we will modify its Path. + avatarURLCopy := *setting.GravatarSourceURL + avatarURLCopy.Path = path.Join(avatarURLCopy.Path, HashEmail(email)) + return generateRecognizedAvatarURL(avatarURLCopy, size) } - avatarURL.RawQuery = urlQuery.Encode() - return avatarURL.String() + return DefaultAvatarLink() } //GenerateEmailAvatarFastLink returns a avatar link (fast, the link may be a delegated one: "/avatar/${hash}") From f594a5f52c832c8b589ae45f19511ade4fe90782 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 30 Sep 2021 16:17:02 +0800 Subject: [PATCH 16/17] fix --- routers/web/user/avatar.go | 1 + 1 file changed, 1 insertion(+) diff --git a/routers/web/user/avatar.go b/routers/web/user/avatar.go index 8d6b30b4571d4..f39bcc36d343b 100644 --- a/routers/web/user/avatar.go +++ b/routers/web/user/avatar.go @@ -32,6 +32,7 @@ func AvatarByUserName(ctx *context.Context) { var err error if user, err = models.GetUserByName(userName); err != nil { ctx.ServerError("Invalid user: "+userName, err) + return } } else { user = models.NewGhostUser() From 43a757c516e9d477465218fe8a96c6882fc73005 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Tue, 5 Oct 2021 21:46:54 +0200 Subject: [PATCH 17/17] Update modules/httpcache/httpcache.go --- modules/httpcache/httpcache.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/httpcache/httpcache.go b/modules/httpcache/httpcache.go index fa8ccfc41e71a..35d4e6dfd82e8 100644 --- a/modules/httpcache/httpcache.go +++ b/modules/httpcache/httpcache.go @@ -25,7 +25,7 @@ func AddCacheControlToHeader(h http.Header, d time.Duration) { // to remind users they are using non-prod setting. // some users may be confused by "Cache-Control: no-store" in their setup if they did wrong to `RUN_MODE` in `app.ini`. h.Add("X-Gitea-Debug", "RUN_MODE="+setting.RunMode) - h.Add("X-Gitea-Debug", "CacheControl=not-prod") + h.Add("X-Gitea-Debug", "CacheControl=no-store") } }