fix: improvements after code review; try to fix tests

Signed-off-by: onidoru <onidoru@yahoo.com>

errors/errors.go

@@ -169,13 +169,11 @@ var (
  ErrURLNotFound = errors.New("url not found")
  ErrInvalidSearchQuery = errors.New("invalid search query")
 
- // ErrUserIsNotFound returned if the user is not found.
- ErrUserIsNotFound = errors.New("user is not found")
  // ErrPasswordsDoNotMatch returned if given password does not match existing user's password.
  ErrPasswordsDoNotMatch = errors.New("passwords do not match")
  // ErrOldPasswordIsWrong returned if provided old password for user verification
  // during the password change is wrong.
  ErrOldPasswordIsWrong = errors.New("old password is wrong")
- // ErrPasswordIsEmpty returned if user's new password is empty
+ // ErrPasswordIsEmpty returned if user's new password is empty.
  ErrPasswordIsEmpty = errors.New("password can not be empty")
 )

pkg/api/controller.go

@@ -49,7 +49,7 @@ type Controller struct {
  RelyingParties map[string]rp.RelyingParty
  CookieStore *CookieStore
  taskScheduler *scheduler.Scheduler
- htpasswdClient *HtpasswdClient
+ HtpasswdClient *HtpasswdClient
  // runtime params
  chosenPort int // kernel-chosen port
 }
@@ -99,9 +99,7 @@ func (c *Controller) Run() error {
  return err
  }
 
- if err := c.initHtpasswdClient(); err != nil {
- return err
- }
+ c.StartBackgroundTasks()
 
  // setup HTTP API router
  engine := mux.NewRouter()
@@ -245,6 +243,12 @@ func (c *Controller) Init() error {
 
  c.InitCVEInfo()
 
+ if c.Config.IsHtpasswdAuthEnabled() {
+ if err := c.initHtpasswdClient(); err != nil {
+ return err
+ }
+ }
+
  return nil
 }
 
@@ -284,9 +288,9 @@ func (c *Controller) initCookieStore() error {
 
 func (c *Controller) initHtpasswdClient() error {
  if c.Config.IsHtpasswdAuthEnabled() {
- c.htpasswdClient = NewHtpasswdClient(c.Config.HTTP.Auth.HTPasswd.Path)
+ c.HtpasswdClient = NewHtpasswdClient(c.Config.HTTP.Auth.HTPasswd.Path)
 
- return c.htpasswdClient.Init()
+ return c.HtpasswdClient.Init()
  }
 
  return nil

pkg/api/controller_test.go

@@ -4446,60 +4446,6 @@ func TestAuthorization(t *testing.T) {
  })
 }
 
-func TestChangePassword(t *testing.T) {
- Convey("Make a new controller", t, func() {
- port := test.GetFreePort()
- baseURL := test.GetBaseURL(port)
- conf := config.New()
- conf.HTTP.Port = port
- username, seedUser := test.GenerateRandomString()
- password, seedPass := test.GenerateRandomString()
- htpasswdPath := test.MakeHtpasswdFileFromString(test.GetCredString(username, password))
- defer os.Remove(htpasswdPath)
-
- conf.HTTP.Auth = &config.AuthConfig{
- HTPasswd: config.AuthHTPasswd{
- Path: htpasswdPath,
- },
- }
- conf.HTTP.AccessControl = &config.AccessControlConfig{
- Repositories: config.Repositories{
- test.AuthorizationAllRepos: config.PolicyGroup{
- Policies: []config.Policy{
- {
- Users: []string{},
- Actions: []string{},
- },
- },
- DefaultPolicy: []string{},
- },
- },
- AdminPolicy: config.Policy{
- Users: []string{},
- Actions: []string{},
- },
- }
-
- Convey("with basic auth", func() {
- ctlr := api.NewController(conf)
- ctlr.Config.Storage.RootDirectory = t.TempDir()
-
- err := WriteImageToFileSystem(CreateDefaultImage(), "zot-test", "0.0.1",
- ociutils.GetDefaultStoreController(ctlr.Config.Storage.RootDirectory, ctlr.Log))
- So(err, ShouldBeNil)
-
- cm := test.NewControllerManager(ctlr)
- cm.StartAndWait(port)
- defer cm.StopServer()
-
- client := resty.New()
- client.SetBasicAuth(username, password)
-
- RunAuthorizationTests(t, client, baseURL, username, conf)
- })
- })
-}
-
 func TestGetUsername(t *testing.T) {
  Convey("Make a new controller", t, func() {
  port := test.GetFreePort()

pkg/api/htpasswd.go

@@ -92,7 +92,7 @@ func (hc *HtpasswdClient) Set(login, password string) error {
 func (hc *HtpasswdClient) CheckPassword(login, password string) error {
  passwordHash, ok := hc.Get(login)
  if !ok {
- return zerr.ErrUserIsNotFound
+ return zerr.ErrBadUser
  }
 
  err := bcrypt.CompareHashAndPassword([]byte(passwordHash), []byte(password))
@@ -115,7 +115,7 @@ func (hc *HtpasswdClient) ChangePassword(login, supposedOldPassword, newPassword
  hc.credMap.rw.RUnlock()
 
  if !ok {
- return zerr.ErrUserIsNotFound
+ return zerr.ErrBadUser
  }
 
  // given old password must match actual old password

pkg/api/htpasswd_test.go

@@ -27,7 +27,7 @@ func TestHtpasswdClient_ChangePassword(t *testing.T) {
 
  Convey("change for non-existing login", func() {
  err := client.ChangePassword("non-existing", "old_password", "new_password")
- So(err, ShouldEqual, zerr.ErrUserIsNotFound)
+ So(err, ShouldEqual, zerr.ErrBadUser)
  })
 
  Convey("change with wrong old oldPassword", func() {
@@ -96,7 +96,7 @@ func TestHtpasswdClient_CheckPassword(t *testing.T) {
 
  Convey("check for non-existing login", func() {
  err := client.CheckPassword("non-existing", "password")
- So(err, ShouldEqual, zerr.ErrUserIsNotFound)
+ So(err, ShouldEqual, zerr.ErrBadUser)
  })
 
  Convey("check with wrong password", func() {
@@ -146,38 +146,3 @@ func TestHtpasswdClient_Init(t *testing.T) {
  })
  })
 }
-
-//
-// func Test_credMap_Get(t *testing.T) {
-// credsMap := credMap{
-// m: map[string]string{"testuser": "testpassword"},
-// rw: &sync.RWMutex{},
-// }
-//
-// Convey("test credMap Get", t, func() {
-// Convey("should get existing password", func() {
-// passhprase, ok := credsMap.Get("testuser")
-// So(ok, ShouldBeTrue)
-// So(passhprase, ShouldEqual, "testpassword")
-// })
-//
-// Convey("should not get unexisting password", func() {
-// passhprase, ok := credsMap.Get("non-existing")
-// So(ok, ShouldBeFalse)
-// So(passhprase, ShouldBeBlank)
-// })
-// })
-// }
-//
-// func Test_credMap_Set(t *testing.T) {
-// credsMap := credMap{
-// m: make(map[string]string),
-// rw: &sync.RWMutex{},
-// }
-//
-// Convey("should set password", t, func() {
-// err := credsMap.Set("testuser", "testpassword")
-// So(err, ShouldBeNil)
-// So(credsMap.m["testuser"], ShouldNotBeEmpty)
-// })
-// }

pkg/api/routes.go

@@ -2234,7 +2234,6 @@ func (rh *RouteHandler) ChangePassword(resp http.ResponseWriter, req *http.Reque
  if err != nil {
  rh.c.Log.Error().Msg("failed to read req body")
  resp.WriteHeader(http.StatusInternalServerError)
- _, _ = resp.Write([]byte("internal server error"))
 
  return
  }
@@ -2243,41 +2242,41 @@ func (rh *RouteHandler) ChangePassword(resp http.ResponseWriter, req *http.Reque
  if err := json.Unmarshal(body, &reqBody); err != nil {
  rh.c.Log.Error().Msg("failed to unmarshal req body")
  resp.WriteHeader(http.StatusBadRequest)
- _, _ = resp.Write([]byte("bad req"))
 
  return
  }
 
  userAc, err := reqCtx.UserAcFromContext(req.Context())
  if err != nil {
+ resp.WriteHeader(http.StatusNotFound)
+
  return
  }
 
  username := userAc.GetUsername()
- if err := rh.c.htpasswdClient.ChangePassword(username, reqBody.OldPassword, reqBody.NewPassword); err != nil {
+ if username == "" {
+ resp.WriteHeader(http.StatusNotFound)
+ }
+
+ if err := rh.c.HtpasswdClient.ChangePassword(username, reqBody.OldPassword, reqBody.NewPassword); err != nil {
  rh.c.Log.Error().Err(err).Str("identity", username).Msg("failed to change user password")
  status := http.StatusInternalServerError
- msg := err.Error()
 
  switch {
- case errors.Is(err, zerr.ErrUserIsNotFound):
+ case errors.Is(err, zerr.ErrBadUser):
  status = http.StatusNotFound
  case errors.Is(err, zerr.ErrOldPasswordIsWrong):
  status = http.StatusUnauthorized
  case errors.Is(err, zerr.ErrPasswordIsEmpty):
  status = http.StatusBadRequest
- default:
- msg = "internal server error"
  }
 
  resp.WriteHeader(status)
- _, _ = resp.Write([]byte(msg))
 
  return
  }
 
  resp.WriteHeader(http.StatusOK)
- _, _ = resp.Write([]byte("password changed"))
 }
 
 type ChangePasswordRequest struct {

pkg/api/routes_test.go

@@ -1562,7 +1562,7 @@ func TestRoutes(t *testing.T) {
  NewPassword: "new_password",
  },
  wantCode: http.StatusNotFound,
- wantBody: []byte(zerr.ErrUserIsNotFound.Error()),
+ wantBody: []byte(zerr.ErrBadUser.Error()),
  }))
 
  Convey("old password is wrong", testFn(testCase{

pkg/test/image-utils/upload_test.go

@@ -524,28 +524,28 @@ func TestInjectUploadImageWithBasicAuth(t *testing.T) {
  }
 
  Convey("first marshal", func() {
- injected := inject.InjectFailure(0)
+ injected := inject.InjectFailure(1)
  if injected {
  err := UploadImageWithBasicAuth(img, baseURL, "test", img.DigestStr(), "user", "password")
  So(err, ShouldNotBeNil)
  }
  })
  Convey("CreateBlobUpload POST call", func() {
- injected := inject.InjectFailure(1)
+ injected := inject.InjectFailure(2)
  if injected {
  err := UploadImageWithBasicAuth(img, baseURL, "test", img.DigestStr(), "user", "password")
  So(err, ShouldNotBeNil)
  }
  })
  Convey("UpdateBlobUpload PUT call", func() {
- injected := inject.InjectFailure(3)
+ injected := inject.InjectFailure(4)
  if injected {
  err := UploadImageWithBasicAuth(img, baseURL, "test", img.DigestStr(), "user", "password")
  So(err, ShouldNotBeNil)
  }
  })
  Convey("second marshal", func() {
- injected := inject.InjectFailure(5)
+ injected := inject.InjectFailure(6)
  if injected {
  err := UploadImageWithBasicAuth(img, baseURL, "test", img.DigestStr(), "user", "password")
  So(err, ShouldNotBeNil)

swagger/swagger.json

@@ -1179,7 +1179,7 @@
  }
  },
  "403": {
- "description": "old password is incorrect",
+ "description": "old password is incorrect\".",
  "schema": {
  "type": "string"
  }

swagger/swagger.yaml

@@ -1027,7 +1027,7 @@ paths:
  schema:
  type: string
  "403":
- description: old password is incorrect
+ description: old password is incorrect".
  schema:
  type: string
  "500":