From 786454a100c61cbe571d9ee0ef688ff7e63f9793 Mon Sep 17 00:00:00 2001 From: Steffen Vogel Date: Tue, 19 Oct 2021 15:30:27 +0200 Subject: [PATCH] use proper error types instead of using strings.Contains() --- database/database.go | 4 +- routes/healthz/healthz_endpoint.go | 5 +-- .../infrastructure-component/ic_amqpclient.go | 3 +- routes/infrastructure-component/ic_errors.go | 32 +++++++++++++++ routes/infrastructure-component/ic_methods.go | 3 +- routes/user/user_endpoints.go | 5 +-- routes/user/user_errors.go | 40 +++++++++++++++++++ routes/user/user_methods.go | 2 +- routes/user/user_test.go | 8 ++-- routes/user/user_validators.go | 12 +++--- 10 files changed, 91 insertions(+), 23 deletions(-) create mode 100644 routes/infrastructure-component/ic_errors.go create mode 100644 routes/user/user_errors.go diff --git a/database/database.go b/database/database.go index b8b48b5..e0bc95f 100644 --- a/database/database.go +++ b/database/database.go @@ -49,8 +49,8 @@ func InitDB(cfg *config.Config, clear bool) error { return err } - user, err := cfg.String("db.user") - if err != nil && !strings.Contains(err.Error(), "Required setting 'db.user' not set") { + user, err := cfg.StringOr("db.user", "") + if err != nil { return err } diff --git a/routes/healthz/healthz_endpoint.go b/routes/healthz/healthz_endpoint.go index eb5f65e..d26c263 100644 --- a/routes/healthz/healthz_endpoint.go +++ b/routes/healthz/healthz_endpoint.go @@ -24,7 +24,6 @@ package healthz import ( "log" "net/http" - "strings" "git.rwth-aachen.de/acs/public/villas/web-backend-go/configuration" "git.rwth-aachen.de/acs/public/villas/web-backend-go/database" @@ -55,8 +54,8 @@ func getHealth(c *gin.Context) { } // check if connection to AMQP broker is alive if backend was started with AMQP client - url, err := configuration.GlobalConfig.String("amqp.host") - if err != nil && strings.Contains(err.Error(), "Required setting 'amqp.host' not set") { + url, err := configuration.GlobalConfig.StringOr("amqp.host", "not-set") + if err != nil && url == "not-set" { c.JSON(http.StatusOK, gin.H{}) return } else if err != nil { diff --git a/routes/infrastructure-component/ic_amqpclient.go b/routes/infrastructure-component/ic_amqpclient.go index ee81693..cc071c9 100644 --- a/routes/infrastructure-component/ic_amqpclient.go +++ b/routes/infrastructure-component/ic_amqpclient.go @@ -25,7 +25,6 @@ import ( "encoding/json" "fmt" "log" - "strings" "git.rwth-aachen.de/acs/public/villas/web-backend-go/helper" "github.com/gin-gonic/gin" @@ -197,7 +196,7 @@ func (s *InfrastructureComponent) updateExternalIC(payload ICUpdate, body []byte if err != nil { // if component could not be deleted there are still configurations using it in the DB // continue with the update to save the new state of the component and get back to the deletion later - if strings.Contains(err.Error(), "postponed") { + if _, ok := err.(*DeletionPostponed); ok { log.Println(err) // print log message } else { return err // return upon DB error diff --git a/routes/infrastructure-component/ic_errors.go b/routes/infrastructure-component/ic_errors.go new file mode 100644 index 0000000..1a05fb8 --- /dev/null +++ b/routes/infrastructure-component/ic_errors.go @@ -0,0 +1,32 @@ +/** User package, methods. +* +* @author Sonja Happ +* @copyright 2014-2019, Institute for Automation of Complex Power Systems, EONERC +* @license GNU General Public License (version 3) +* +* VILLASweb-backend-go +* +* This program is free software: you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation, either version 3 of the License, or +* any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program. If not, see . +*********************************************************************************/ +package infrastructure_component + +import "fmt" + +type DeletionPostponed struct { + References int +} + +func (e *DeletionPostponed) Error() string { + return fmt.Sprintf("deletion of IC postponed, %d config(s) associated to it", e.References) +} diff --git a/routes/infrastructure-component/ic_methods.go b/routes/infrastructure-component/ic_methods.go index 7c9c307..35389ab 100644 --- a/routes/infrastructure-component/ic_methods.go +++ b/routes/infrastructure-component/ic_methods.go @@ -22,7 +22,6 @@ package infrastructure_component import ( - "fmt" "git.rwth-aachen.de/acs/public/villas/web-backend-go/database" ) @@ -68,7 +67,7 @@ func (s *InfrastructureComponent) delete() error { noConfigs := db.Model(s).Association("ComponentConfigurations").Count() if noConfigs > 0 { - return fmt.Errorf("deletion of IC postponed, %v config(s) associated to it", noConfigs) + return &DeletionPostponed{References: noConfigs} } // delete InfrastructureComponent from DB (does NOT remain as dangling) diff --git a/routes/user/user_endpoints.go b/routes/user/user_endpoints.go index 3e0b03a..21f6a9c 100644 --- a/routes/user/user_endpoints.go +++ b/routes/user/user_endpoints.go @@ -24,7 +24,6 @@ package user import ( "fmt" "net/http" - "strings" "git.rwth-aachen.de/acs/public/villas/web-backend-go/helper" @@ -188,9 +187,9 @@ func updateUser(c *gin.Context) { // case that the request updates the role of the old user) updatedUser, err := req.updatedUser(callerID, callerRole, oldUser) if err != nil { - if strings.Contains(err.Error(), "Admin") || strings.Contains(err.Error(), "pw not changed") { + if _, ok := err.(*ForbiddenError); ok { helper.ForbiddenError(c, err.Error()) - } else if strings.Contains(err.Error(), "Username") || strings.Contains(err.Error(), "old or admin password") { + } else if _, ok := err.(*UsernameAlreadyTaken); ok { helper.BadRequestError(c, err.Error()) } else { // password encryption failed helper.InternalServerError(c, err.Error()) diff --git a/routes/user/user_errors.go b/routes/user/user_errors.go new file mode 100644 index 0000000..f523a36 --- /dev/null +++ b/routes/user/user_errors.go @@ -0,0 +1,40 @@ +/** User package, methods. +* +* @author Sonja Happ +* @copyright 2014-2019, Institute for Automation of Complex Power Systems, EONERC +* @license GNU General Public License (version 3) +* +* VILLASweb-backend-go +* +* This program is free software: you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation, either version 3 of the License, or +* any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program. If not, see . +*********************************************************************************/ +package user + +import "fmt" + +type UsernameAlreadyTaken struct { + Username string +} + +func (e *UsernameAlreadyTaken) Error() string { + return fmt.Sprintf("username is already taken: %s", e.Username) +} + +type ForbiddenError struct { + Reason string +} + +func (e *ForbiddenError) Error() string { + return fmt.Sprintf("permission denied: %s", e.Reason) +} diff --git a/routes/user/user_methods.go b/routes/user/user_methods.go index 9396f07..c0ea4d6 100644 --- a/routes/user/user_methods.go +++ b/routes/user/user_methods.go @@ -43,7 +43,7 @@ func NewUser(username, password, mail, role string, active bool) (User, error) { // Check that the username is NOT taken err := newUser.ByUsername(username) if err == nil { - return newUser, fmt.Errorf("username is already taken") + return newUser, &UsernameAlreadyTaken{Username: username} } newUser.Username = username diff --git a/routes/user/user_test.go b/routes/user/user_test.go index 97fdcec..984f084 100644 --- a/routes/user/user_test.go +++ b/routes/user/user_test.go @@ -54,11 +54,11 @@ type UserRequest struct { func TestMain(m *testing.M) { err := configuration.InitConfig() if err != nil { - panic(m) + panic(err) } err = database.InitDB(configuration.GlobalConfig, true) if err != nil { - panic(m) + panic(err) } defer database.DBpool.Close() @@ -541,7 +541,7 @@ func TestModifyAddedUserAsUser(t *testing.T) { fmt.Sprintf("/api/v2/users/%v", newUserID), "PUT", helper.KeyModels{"user": modRequest}) assert.NoError(t, err) - assert.Equalf(t, 400, code, "Response body: \n%v\n", resp) + assert.Equalf(t, 403, code, "Response body: \n%v\n", resp) // modify newUser's password with wring old password modRequest = UserRequest{ @@ -725,7 +725,7 @@ func TestModifyAddedUserAsAdmin(t *testing.T) { fmt.Sprintf("/api/v2/users/%v", newUserID), "PUT", helper.KeyModels{"user": modRequest}) assert.NoError(t, err) - assert.Equalf(t, 400, code, "Response body: \n%v\n", resp) + assert.Equalf(t, 403, code, "Response body: \n%v\n", resp) // modify newUser's password, requires admin password modRequest = UserRequest{ diff --git a/routes/user/user_validators.go b/routes/user/user_validators.go index 18fe56b..2bf5bf1 100644 --- a/routes/user/user_validators.go +++ b/routes/user/user_validators.go @@ -82,7 +82,7 @@ func (r *updateUserRequest) updatedUser(callerID interface{}, role interface{}, // Only the Admin must be able to update user's role if role != "Admin" && r.User.Role != "" { if r.User.Role != u.Role { - return u, fmt.Errorf("only Admin can update user's Role") + return u, &ForbiddenError{Reason: "only Admin can update user's Role"} } } else if role == "Admin" && r.User.Role != "" { u.Role = r.User.Role @@ -91,7 +91,7 @@ func (r *updateUserRequest) updatedUser(callerID interface{}, role interface{}, // Only the Admin must be able to update users Active state if (r.User.Active == "yes" && !u.Active) || (r.User.Active == "no" && u.Active) { if role != "Admin" { - return u, fmt.Errorf("only Admin can update user's Active state") + return u, &ForbiddenError{Reason: "only Admin can update user's Active state"} } else { u.Active = !u.Active } @@ -100,7 +100,7 @@ func (r *updateUserRequest) updatedUser(callerID interface{}, role interface{}, // Update the username making sure it is NOT taken var testUser User if err := testUser.ByUsername(r.User.Username); err == nil { - return u, fmt.Errorf("username is already taken") + return u, &UsernameAlreadyTaken{Username: r.User.Username} } if r.User.Username != "" { @@ -111,7 +111,7 @@ func (r *updateUserRequest) updatedUser(callerID interface{}, role interface{}, if r.User.Password != "" { if r.User.OldPassword == "" { // admin or old password has to be present for pw change - return u, fmt.Errorf("old or admin password is missing in request") + return u, &ForbiddenError{Reason: "missing old or admin password"} } if role == "Admin" { // admin has to enter admin password @@ -123,14 +123,14 @@ func (r *updateUserRequest) updatedUser(callerID interface{}, role interface{}, err = adminUser.validatePassword(r.User.OldPassword) if err != nil { - return u, fmt.Errorf("admin password not correct, pw not changed") + return u, &ForbiddenError{Reason: "admin password not correct, pw not changed"} } } else { //normal or guest user has to enter old password err := oldUser.validatePassword(r.User.OldPassword) if err != nil { - return u, fmt.Errorf("previous password not correct, pw not changed") + return u, &ForbiddenError{Reason: "previous password not correct, pw not changed"} } }