fix: improvements after code review; try to fix tests

Signed-off-by: onidoru <[email protected]>

Author: onidoru

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

@@ -50,7 +50,7 @@ type Controller struct {
 	CookieStore     *CookieStore
 	LDAPClient      *LDAPClient
 	taskScheduler   *scheduler.Scheduler
-	htpasswdClient  *HtpasswdClient
+	HtpasswdClient  *HtpasswdClient
 	// runtime params
 	chosenPort int // kernel-chosen port
 }
@@ -100,9 +100,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()
@@ -246,6 +244,12 @@ func (c *Controller) Init() error {
 
 	c.InitCVEInfo()
 
+	if c.Config.IsHtpasswdAuthEnabled() {
+		if err := c.initHtpasswdClient(); err != nil {
+			return err
+		}
+	}
+
 	return nil
 }
 
@@ -285,9 +289,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

@@ -4629,60 +4629,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/docs.go

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

swagger/swagger.json

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