diff --git a/db/filesystem.go b/db/filesystem.go index d94e53f..e03b479 100644 --- a/db/filesystem.go +++ b/db/filesystem.go @@ -49,13 +49,17 @@ type filesStorage struct { func (repository *filesStorage) FindByUsername(username string) (*model.Cape, error) { if username == "" { - return nil, &http.CapeNotFoundError{Who: username} + return nil, nil } capePath := path.Join(repository.path, strings.ToLower(username)+".png") file, err := os.Open(capePath) if err != nil { - return nil, &http.CapeNotFoundError{Who: username} + if os.IsNotExist(err) { + return nil, nil + } + + return nil, err } return &model.Cape{ diff --git a/db/redis.go b/db/redis.go index ec218b6..899eca1 100644 --- a/db/redis.go +++ b/db/redis.go @@ -5,7 +5,6 @@ import ( "compress/zlib" "encoding/json" "fmt" - "github.com/elyby/chrly/http" "io" "strconv" "strings" @@ -15,6 +14,7 @@ import ( "github.com/mediocregopher/radix.v2/redis" "github.com/mediocregopher/radix.v2/util" + "github.com/elyby/chrly/http" "github.com/elyby/chrly/model" "github.com/elyby/chrly/mojangtextures" ) @@ -147,14 +147,10 @@ func (db *redisDb) StoreUuid(username string, uuid string) error { } func findByUsername(username string, conn util.Cmder) (*model.Skin, error) { - if username == "" { - return nil, &http.SkinNotFoundError{Who: username} - } - redisKey := buildUsernameKey(username) response := conn.Cmd("GET", redisKey) - if !response.IsType(redis.Str) { - return nil, &http.SkinNotFoundError{Who: username} + if response.IsType(redis.Nil) { + return nil, nil } encodedResult, err := response.Bytes() @@ -180,11 +176,14 @@ func findByUsername(username string, conn util.Cmder) (*model.Skin, error) { func findByUserId(id int, conn util.Cmder) (*model.Skin, error) { response := conn.Cmd("HGET", accountIdToUsernameKey, id) - if !response.IsType(redis.Str) { - return nil, &http.SkinNotFoundError{Who: "unknown"} + if response.IsType(redis.Nil) { + return nil, nil } - username, _ := response.Str() + username, err := response.Str() + if err != nil { + return nil, err + } return findByUsername(username, conn) } @@ -192,9 +191,7 @@ func findByUserId(id int, conn util.Cmder) (*model.Skin, error) { func removeByUserId(id int, conn util.Cmder) error { record, err := findByUserId(id, conn) if err != nil { - if _, ok := err.(*http.SkinNotFoundError); !ok { - return err - } + return err } conn.Cmd("MULTI") @@ -212,13 +209,13 @@ func removeByUserId(id int, conn util.Cmder) error { func removeByUsername(username string, conn util.Cmder) error { record, err := findByUsername(username, conn) if err != nil { - if _, ok := err.(*http.SkinNotFoundError); ok { - return nil - } - return err } + if record == nil { + return nil + } + conn.Cmd("MULTI") conn.Cmd("DEL", buildUsernameKey(record.Username)) diff --git a/http/api.go b/http/api.go index b533278..75fe9a9 100644 --- a/http/api.go +++ b/http/api.go @@ -73,6 +73,13 @@ func (ctx *Api) postSkinHandler(resp http.ResponseWriter, req *http.Request) { return } + if record == nil { + record = &model.Skin{ + UserId: identityId, + Username: username, + } + } + skinId, _ := strconv.Atoi(req.Form.Get("skinId")) is18, _ := strconv.ParseBool(req.Form.Get("is1_8")) isSlim, _ := strconv.ParseBool(req.Form.Get("isSlim")) @@ -109,13 +116,13 @@ func (ctx *Api) deleteSkinByUsernameHandler(resp http.ResponseWriter, req *http. func (ctx *Api) deleteSkin(skin *model.Skin, err error, resp http.ResponseWriter) { if err != nil { - if _, ok := err.(*SkinNotFoundError); ok { - apiNotFound(resp, "Cannot find record for the requested identifier") - } else { - ctx.Emit("skinsystem:error", fmt.Errorf("unable to find skin info from the repository: %w", err)) - apiServerError(resp) - } + ctx.Emit("skinsystem:error", fmt.Errorf("unable to find skin info from the repository: %w", err)) + apiServerError(resp) + return + } + if skin == nil { + apiNotFound(resp, "Cannot find record for the requested identifier") return } @@ -130,29 +137,38 @@ func (ctx *Api) deleteSkin(skin *model.Skin, err error, resp http.ResponseWriter } func (ctx *Api) findIdentityOrCleanup(identityId int, username string) (*model.Skin, error) { - var record *model.Skin record, err := ctx.SkinsRepo.FindByUserId(identityId) if err != nil { - if _, isSkinNotFound := err.(*SkinNotFoundError); !isSkinNotFound { - return nil, err - } - - record, err = ctx.SkinsRepo.FindByUsername(username) - if err == nil { - _ = ctx.SkinsRepo.RemoveByUsername(username) - record.UserId = identityId - } else { - record = &model.Skin{ - UserId: identityId, - Username: username, - } - } - } else if record.Username != username { - _ = ctx.SkinsRepo.RemoveByUserId(identityId) - record.Username = username + return nil, err } - return record, nil + if record != nil { + // The username may have changed in the external database, + // so we need to remove the old association + if record.Username != username { + _ = ctx.SkinsRepo.RemoveByUserId(identityId) + record.Username = username + } + + return record, nil + } + + // If the requested id was not found, then username was reassigned to another user + // who has not uploaded his data to Chrly yet + record, err = ctx.SkinsRepo.FindByUsername(username) + if err != nil { + return nil, err + } + + // If the target username does exist, clear it as it will be reassigned to the new user + if record != nil { + _ = ctx.SkinsRepo.RemoveByUsername(username) + record.UserId = identityId + + return record, nil + } + + return nil, nil } func validatePostSkinRequest(request *http.Request) map[string][]string { diff --git a/http/api_test.go b/http/api_test.go index db60f13..fe4f1cb 100644 --- a/http/api_test.go +++ b/http/api_test.go @@ -88,8 +88,8 @@ var postSkinTestsCases = []*postSkinTestCase{ "url": {"http://example.com/skin.png"}, }.Encode()), BeforeTest: func(suite *apiTestSuite) { - suite.SkinsRepository.On("FindByUserId", 1).Return(nil, &SkinNotFoundError{Who: "unknown"}) - suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, &SkinNotFoundError{Who: "mock_username"}) + suite.SkinsRepository.On("FindByUserId", 1).Return(nil, nil) + suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, nil) suite.SkinsRepository.On("Save", mock.MatchedBy(func(model *model.Skin) bool { suite.Equal(1, model.UserId) suite.Equal("mock_username", model.Username) @@ -151,7 +151,7 @@ var postSkinTestsCases = []*postSkinTestCase{ "url": {"http://example.com/skin.png"}, }.Encode()), BeforeTest: func(suite *apiTestSuite) { - suite.SkinsRepository.On("FindByUserId", 2).Return(nil, &SkinNotFoundError{Who: "unknown"}) + suite.SkinsRepository.On("FindByUserId", 2).Return(nil, nil) suite.SkinsRepository.On("FindByUsername", "mock_username").Return(createSkinModel("mock_username", false), nil) suite.SkinsRepository.On("RemoveByUsername", "mock_username").Times(1).Return(nil) suite.SkinsRepository.On("Save", mock.MatchedBy(func(model *model.Skin) bool { @@ -368,7 +368,7 @@ func (suite *apiTestSuite) TestDeleteByUserId() { }) suite.RunSubTest("Try to remove not exists identity id", func() { - suite.SkinsRepository.On("FindByUserId", 1).Return(nil, &SkinNotFoundError{Who: "unknown"}) + suite.SkinsRepository.On("FindByUserId", 1).Return(nil, nil) req := httptest.NewRequest("DELETE", "http://chrly/skins/id:1", nil) w := httptest.NewRecorder() @@ -407,7 +407,7 @@ func (suite *apiTestSuite) TestDeleteByUsername() { }) suite.RunSubTest("Try to remove not exists identity username", func() { - suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, &SkinNotFoundError{Who: "mock_username"}) + suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, nil) req := httptest.NewRequest("DELETE", "http://chrly/skins/mock_username", nil) w := httptest.NewRecorder() diff --git a/http/skinsystem.go b/http/skinsystem.go index d8f7335..3a065d4 100644 --- a/http/skinsystem.go +++ b/http/skinsystem.go @@ -25,24 +25,6 @@ type CapesRepository interface { FindByUsername(username string) (*model.Cape, error) } -// TODO: can I get rid of this? -type SkinNotFoundError struct { - Who string -} - -func (e SkinNotFoundError) Error() string { - return "skin data not found" -} - -type CapeNotFoundError struct { - Who string -} - -// TODO: can I get rid of this? -func (e CapeNotFoundError) Error() string { - return "cape file not found" -} - type MojangTexturesProvider interface { GetForUsername(username string) (*mojang.SignedTexturesResponse, error) } @@ -73,7 +55,7 @@ func (ctx *Skinsystem) Handler() *mux.Router { func (ctx *Skinsystem) skinHandler(response http.ResponseWriter, request *http.Request) { username := parseUsername(mux.Vars(request)["username"]) rec, err := ctx.SkinsRepo.FindByUsername(username) - if err == nil && rec.SkinId != 0 { + if err == nil && rec != nil && rec.SkinId != 0 { http.Redirect(response, request, rec.Url, 301) return } @@ -110,7 +92,7 @@ func (ctx *Skinsystem) skinGetHandler(response http.ResponseWriter, request *htt func (ctx *Skinsystem) capeHandler(response http.ResponseWriter, request *http.Request) { username := parseUsername(mux.Vars(request)["username"]) rec, err := ctx.CapesRepo.FindByUsername(username) - if err == nil { + if err == nil && rec != nil { request.Header.Set("Content-Type", "image/png") _, _ = io.Copy(response, rec.File) return @@ -150,11 +132,10 @@ func (ctx *Skinsystem) texturesHandler(response http.ResponseWriter, request *ht var textures *mojang.TexturesResponse skin, skinErr := ctx.SkinsRepo.FindByUsername(username) - _, capeErr := ctx.CapesRepo.FindByUsername(username) - if (skinErr == nil && skin.SkinId != 0) || capeErr == nil { + cape, capeErr := ctx.CapesRepo.FindByUsername(username) + if (skinErr == nil && skin != nil && skin.SkinId != 0) || (capeErr == nil && cape != nil) { textures = &mojang.TexturesResponse{} - - if skinErr == nil && skin.SkinId != 0 { + if skinErr == nil && skin != nil && skin.SkinId != 0 { skinTextures := &mojang.SkinTexturesResponse{ Url: skin.Url, } @@ -168,7 +149,7 @@ func (ctx *Skinsystem) texturesHandler(response http.ResponseWriter, request *ht textures.Skin = skinTextures } - if capeErr == nil { + if capeErr == nil && cape != nil { textures.Cape = &mojang.CapeTexturesResponse{ Url: request.URL.Scheme + "://" + request.Host + "/cloaks/" + username, } @@ -202,7 +183,7 @@ func (ctx *Skinsystem) signedTexturesHandler(response http.ResponseWriter, reque var responseData *mojang.SignedTexturesResponse rec, err := ctx.SkinsRepo.FindByUsername(username) - if err == nil && rec.SkinId != 0 && rec.MojangTextures != "" { + if err == nil && rec != nil && rec.SkinId != 0 && rec.MojangTextures != "" { responseData = &mojang.SignedTexturesResponse{ Id: strings.Replace(rec.Uuid, "-", "", -1), Name: rec.Username, diff --git a/http/skinsystem_test.go b/http/skinsystem_test.go index a02ac16..f0e8cb3 100644 --- a/http/skinsystem_test.go +++ b/http/skinsystem_test.go @@ -163,7 +163,7 @@ var skinsTestsCases = []*skinsystemTestCase{ { Name: "Username doesn't exists on the local storage, but exists on Mojang and has textures", BeforeTest: func(suite *skinsystemTestSuite) { - suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, &SkinNotFoundError{Who: "mock_username"}) + suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, nil) suite.MojangTexturesProvider.On("GetForUsername", "mock_username").Return(createMojangResponse(true, false), nil) }, AfterTest: func(suite *skinsystemTestSuite, response *http.Response) { @@ -174,7 +174,7 @@ var skinsTestsCases = []*skinsystemTestCase{ { Name: "Username doesn't exists on the local storage, but exists on Mojang and has no textures", BeforeTest: func(suite *skinsystemTestSuite) { - suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, &SkinNotFoundError{Who: "mock_username"}) + suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, nil) suite.MojangTexturesProvider.On("GetForUsername", "mock_username").Return(createMojangResponse(false, false), nil) }, AfterTest: func(suite *skinsystemTestSuite, response *http.Response) { @@ -184,7 +184,7 @@ var skinsTestsCases = []*skinsystemTestCase{ { Name: "Username doesn't exists on the local storage and doesn't exists on Mojang", BeforeTest: func(suite *skinsystemTestSuite) { - suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, &SkinNotFoundError{Who: "mock_username"}) + suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, nil) suite.MojangTexturesProvider.On("GetForUsername", "mock_username").Return(nil, nil) }, AfterTest: func(suite *skinsystemTestSuite, response *http.Response) { @@ -267,7 +267,7 @@ var capesTestsCases = []*skinsystemTestCase{ { Name: "Username doesn't exists on the local storage, but exists on Mojang and has textures", BeforeTest: func(suite *skinsystemTestSuite) { - suite.CapesRepository.On("FindByUsername", "mock_username").Return(nil, &CapeNotFoundError{Who: "mock_username"}) + suite.CapesRepository.On("FindByUsername", "mock_username").Return(nil, nil) suite.MojangTexturesProvider.On("GetForUsername", "mock_username").Return(createMojangResponse(true, true), nil) }, AfterTest: func(suite *skinsystemTestSuite, response *http.Response) { @@ -278,7 +278,7 @@ var capesTestsCases = []*skinsystemTestCase{ { Name: "Username doesn't exists on the local storage, but exists on Mojang and has no textures", BeforeTest: func(suite *skinsystemTestSuite) { - suite.CapesRepository.On("FindByUsername", "mock_username").Return(nil, &CapeNotFoundError{Who: "mock_username"}) + suite.CapesRepository.On("FindByUsername", "mock_username").Return(nil, nil) suite.MojangTexturesProvider.On("GetForUsername", "mock_username").Return(createMojangResponse(false, false), nil) }, AfterTest: func(suite *skinsystemTestSuite, response *http.Response) { @@ -288,7 +288,7 @@ var capesTestsCases = []*skinsystemTestCase{ { Name: "Username doesn't exists on the local storage and doesn't exists on Mojang", BeforeTest: func(suite *skinsystemTestSuite) { - suite.CapesRepository.On("FindByUsername", "mock_username").Return(nil, &CapeNotFoundError{Who: "mock_username"}) + suite.CapesRepository.On("FindByUsername", "mock_username").Return(nil, nil) suite.MojangTexturesProvider.On("GetForUsername", "mock_username").Return(nil, nil) }, AfterTest: func(suite *skinsystemTestSuite, response *http.Response) { @@ -362,7 +362,7 @@ var texturesTestsCases = []*skinsystemTestCase{ Name: "Username exists and has skin, no cape", BeforeTest: func(suite *skinsystemTestSuite) { suite.SkinsRepository.On("FindByUsername", "mock_username").Return(createSkinModel("mock_username", false), nil) - suite.CapesRepository.On("FindByUsername", "mock_username").Return(nil, &CapeNotFoundError{Who: "mock_username"}) + suite.CapesRepository.On("FindByUsername", "mock_username").Return(nil, nil) }, AfterTest: func(suite *skinsystemTestSuite, response *http.Response) { suite.Equal(200, response.StatusCode) @@ -379,7 +379,7 @@ var texturesTestsCases = []*skinsystemTestCase{ Name: "Username exists and has slim skin, no cape", BeforeTest: func(suite *skinsystemTestSuite) { suite.SkinsRepository.On("FindByUsername", "mock_username").Return(createSkinModel("mock_username", true), nil) - suite.CapesRepository.On("FindByUsername", "mock_username").Return(nil, &CapeNotFoundError{Who: "mock_username"}) + suite.CapesRepository.On("FindByUsername", "mock_username").Return(nil, nil) }, AfterTest: func(suite *skinsystemTestSuite, response *http.Response) { suite.Equal(200, response.StatusCode) @@ -398,7 +398,7 @@ var texturesTestsCases = []*skinsystemTestCase{ { Name: "Username exists and has cape, no skin", BeforeTest: func(suite *skinsystemTestSuite) { - suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, &SkinNotFoundError{Who: "mock_username"}) + suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, nil) suite.CapesRepository.On("FindByUsername", "mock_username").Return(createCapeModel(), nil) }, AfterTest: func(suite *skinsystemTestSuite, response *http.Response) { @@ -435,8 +435,8 @@ var texturesTestsCases = []*skinsystemTestCase{ { Name: "Username not exists, but Mojang profile available", BeforeTest: func(suite *skinsystemTestSuite) { - suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, &SkinNotFoundError{}) - suite.CapesRepository.On("FindByUsername", "mock_username").Return(nil, &CapeNotFoundError{}) + suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, nil) + suite.CapesRepository.On("FindByUsername", "mock_username").Return(nil, nil) suite.MojangTexturesProvider.On("GetForUsername", "mock_username").Once().Return(createMojangResponse(true, true), nil) }, AfterTest: func(suite *skinsystemTestSuite, response *http.Response) { @@ -456,8 +456,8 @@ var texturesTestsCases = []*skinsystemTestCase{ { Name: "Username not exists and Mojang profile unavailable", BeforeTest: func(suite *skinsystemTestSuite) { - suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, &SkinNotFoundError{}) - suite.CapesRepository.On("FindByUsername", "mock_username").Return(nil, &CapeNotFoundError{}) + suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, nil) + suite.CapesRepository.On("FindByUsername", "mock_username").Return(nil, nil) suite.MojangTexturesProvider.On("GetForUsername", "mock_username").Once().Return(nil, nil) }, AfterTest: func(suite *skinsystemTestSuite, response *http.Response) { @@ -524,7 +524,7 @@ var signedTexturesTestsCases = []*signedTexturesTestCase{ Name: "Username not exists", AllowProxy: false, BeforeTest: func(suite *skinsystemTestSuite) { - suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, &SkinNotFoundError{Who: "mock_username"}) + suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, nil) }, AfterTest: func(suite *skinsystemTestSuite, response *http.Response) { suite.Equal(204, response.StatusCode) @@ -551,7 +551,7 @@ var signedTexturesTestsCases = []*signedTexturesTestCase{ Name: "Username not exists, but Mojang profile is available and proxying is enabled", AllowProxy: true, BeforeTest: func(suite *skinsystemTestSuite) { - suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, &SkinNotFoundError{Who: "mock_username"}) + suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, nil) suite.MojangTexturesProvider.On("GetForUsername", "mock_username").Return(createMojangResponse(true, false), nil) }, AfterTest: func(suite *skinsystemTestSuite, response *http.Response) { @@ -578,7 +578,7 @@ var signedTexturesTestsCases = []*signedTexturesTestCase{ Name: "Username not exists, Mojang profile is unavailable too and proxying is enabled", AllowProxy: true, BeforeTest: func(suite *skinsystemTestSuite) { - suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, &SkinNotFoundError{Who: "mock_username"}) + suite.SkinsRepository.On("FindByUsername", "mock_username").Return(nil, nil) suite.MojangTexturesProvider.On("GetForUsername", "mock_username").Return(nil, nil) }, AfterTest: func(suite *skinsystemTestSuite, response *http.Response) {