From 6486425f4635dba7cd720a3c2399d92764280284 Mon Sep 17 00:00:00 2001 From: Ahsan Barkati Date: Wed, 4 May 2022 14:50:15 +0530 Subject: [PATCH] fix(auth): Return 403 for forbidden requests due to rbac (#1060) * Return error json for user not found * Return 403 for rbac error * Return not_found instead of internal_error for getInvite --- pkg/query-service/app/http_handler.go | 88 ++++++++++++++++++++------- pkg/query-service/auth/jwt.go | 2 +- pkg/query-service/auth/rbac.go | 60 +++--------------- pkg/query-service/model/auth.go | 6 ++ pkg/query-service/model/response.go | 1 + 5 files changed, 82 insertions(+), 75 deletions(-) diff --git a/pkg/query-service/app/http_handler.go b/pkg/query-service/app/http_handler.go index 2ab1845c37..0e5b959a53 100644 --- a/pkg/query-service/app/http_handler.go +++ b/pkg/query-service/app/http_handler.go @@ -142,6 +142,8 @@ func respondError(w http.ResponseWriter, apiErr *model.ApiError, data interface{ code = http.StatusNotImplemented case model.ErrorUnauthorized: code = http.StatusUnauthorized + case model.ErrorForbidden: + code = http.StatusForbidden default: code = http.StatusInternalServerError } @@ -191,10 +193,19 @@ func OpenAccess(f func(http.ResponseWriter, *http.Request)) http.HandlerFunc { func ViewAccess(f func(http.ResponseWriter, *http.Request)) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - if !(auth.IsViewer(r) || auth.IsEditor(r) || auth.IsAdmin(r)) { + user, err := auth.GetUserFromRequest(r) + if err != nil { respondError(w, &model.ApiError{ Typ: model.ErrorUnauthorized, - Err: errors.New("API accessible only to the admins"), + Err: err, + }, nil) + return + } + + if !(auth.IsViewer(user) || auth.IsEditor(user) || auth.IsAdmin(user)) { + respondError(w, &model.ApiError{ + Typ: model.ErrorForbidden, + Err: errors.New("API is not accessible to the viewers."), }, nil) return } @@ -204,10 +215,18 @@ func ViewAccess(f func(http.ResponseWriter, *http.Request)) http.HandlerFunc { func EditAccess(f func(http.ResponseWriter, *http.Request)) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - if !(auth.IsEditor(r) || auth.IsAdmin(r)) { + user, err := auth.GetUserFromRequest(r) + if err != nil { respondError(w, &model.ApiError{ Typ: model.ErrorUnauthorized, - Err: errors.New("API accessible only to the editors"), + Err: err, + }, nil) + return + } + if !(auth.IsEditor(user) || auth.IsAdmin(user)) { + respondError(w, &model.ApiError{ + Typ: model.ErrorForbidden, + Err: errors.New("API is not accessible to the editors."), }, nil) return } @@ -217,10 +236,19 @@ func EditAccess(f func(http.ResponseWriter, *http.Request)) http.HandlerFunc { func SelfAccess(f func(http.ResponseWriter, *http.Request)) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - if !(auth.IsSelfAccessRequest(r) || auth.IsAdmin(r)) { + user, err := auth.GetUserFromRequest(r) + if err != nil { respondError(w, &model.ApiError{ Typ: model.ErrorUnauthorized, - Err: errors.New("API accessible only for self userId"), + Err: err, + }, nil) + return + } + id := mux.Vars(r)["id"] + if !(auth.IsSelfAccessRequest(user, id) || auth.IsAdmin(user)) { + respondError(w, &model.ApiError{ + Typ: model.ErrorForbidden, + Err: errors.New("API accessible only for self userId or admins."), }, nil) return } @@ -230,9 +258,17 @@ func SelfAccess(f func(http.ResponseWriter, *http.Request)) http.HandlerFunc { func AdminAccess(f func(http.ResponseWriter, *http.Request)) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - if !auth.IsAdmin(r) { + user, err := auth.GetUserFromRequest(r) + if err != nil { respondError(w, &model.ApiError{ Typ: model.ErrorUnauthorized, + Err: err, + }, nil) + return + } + if !auth.IsAdmin(user) { + respondError(w, &model.ApiError{ + Typ: model.ErrorForbidden, Err: errors.New("API accessible only to the admins"), }, nil) return @@ -1163,7 +1199,7 @@ func (aH *APIHandler) getInvite(w http.ResponseWriter, r *http.Request) { resp, err := auth.GetInvite(context.Background(), token) if err != nil { - respondError(w, &model.ApiError{Err: err, Typ: model.ErrorInternal}, nil) + respondError(w, &model.ApiError{Err: err, Typ: model.ErrorNotFound}, nil) return } aH.writeJSON(w, r, resp) @@ -1268,8 +1304,8 @@ func (aH *APIHandler) listUsers(w http.ResponseWriter, r *http.Request) { return } // mask the password hash - for _, u := range users { - u.Password = "" + for i := range users { + users[i].Password = "" } aH.writeJSON(w, r, users) } @@ -1284,11 +1320,16 @@ func (aH *APIHandler) getUser(w http.ResponseWriter, r *http.Request) { respondError(w, err, "Failed to get user") return } - // No need to send password hash for the user object. - if user != nil { - user.Password = "" + if user == nil { + respondError(w, &model.ApiError{ + Typ: model.ErrorInternal, + Err: errors.New("User not found"), + }, nil) + return } + // No need to send password hash for the user object. + user.Password = "" aH.writeJSON(w, r, user) } @@ -1336,14 +1377,6 @@ func (aH *APIHandler) editUser(w http.ResponseWriter, r *http.Request) { func (aH *APIHandler) deleteUser(w http.ResponseWriter, r *http.Request) { id := mux.Vars(r)["id"] - if !auth.IsAdmin(r) { - respondError(w, &model.ApiError{ - Typ: model.ErrorUnauthorized, - Err: errors.New("Only admin can delete user"), - }, "Failed to get user") - return - } - // Query for the user's group, and the admin's group. If the user belongs to the admin group // and is the last user then don't let the deletion happen. Otherwise, the system will become // admin less and hence inaccessible. @@ -1353,6 +1386,15 @@ func (aH *APIHandler) deleteUser(w http.ResponseWriter, r *http.Request) { respondError(w, apiErr, "Failed to get user's group") return } + + if user == nil { + respondError(w, &model.ApiError{ + Typ: model.ErrorNotFound, + Err: errors.New("User not found"), + }, nil) + return + } + adminGroup, apiErr := dao.DB().GetGroupByName(ctx, constants.AdminGroup) if apiErr != nil { respondError(w, apiErr, "Failed to get admin group") @@ -1504,8 +1546,8 @@ func (aH *APIHandler) getOrgUsers(w http.ResponseWriter, r *http.Request) { return } // mask the password hash - for _, u := range users { - u.Password = "" + for i := range users { + users[i].Password = "" } aH.writeJSON(w, r, users) } diff --git a/pkg/query-service/auth/jwt.go b/pkg/query-service/auth/jwt.go index f5b3e41335..35ed509048 100644 --- a/pkg/query-service/auth/jwt.go +++ b/pkg/query-service/auth/jwt.go @@ -45,7 +45,7 @@ func validateUser(tok string) (*model.UserPayload, error) { } now := time.Now().Unix() if !claims.VerifyExpiresAt(now, true) { - return nil, errors.Errorf("Token is expired") + return nil, model.ErrorTokenExpired } return &model.UserPayload{ User: model.User{ diff --git a/pkg/query-service/auth/rbac.go b/pkg/query-service/auth/rbac.go index 94bcfcb276..d45a06e5d5 100644 --- a/pkg/query-service/auth/rbac.go +++ b/pkg/query-service/auth/rbac.go @@ -6,11 +6,10 @@ import ( "net/http" "regexp" - "github.com/gorilla/mux" "github.com/pkg/errors" "go.signoz.io/query-service/constants" "go.signoz.io/query-service/dao" - "go.uber.org/zap" + "go.signoz.io/query-service/model" ) type Group struct { @@ -51,65 +50,24 @@ func InitAuthCache(ctx context.Context) error { return nil } -func IsAdmin(r *http.Request) bool { +func GetUserFromRequest(r *http.Request) (*model.UserPayload, error) { accessJwt, err := ExtractJwtFromRequest(r) if err != nil { - zap.S().Debugf("Failed to verify admin access, err: %v", err) - return false + return nil, err } user, err := validateUser(accessJwt) if err != nil { - return false + return nil, err } - return user.GroupId == AuthCacheObj.AdminGroupId + return user, nil } -func IsSelfAccessRequest(r *http.Request) bool { - id := mux.Vars(r)["id"] - accessJwt, err := ExtractJwtFromRequest(r) - if err != nil { - zap.S().Debugf("Failed to verify self access, err: %v", err) - return false - } +func IsSelfAccessRequest(user *model.UserPayload, id string) bool { return user.Id == id } - user, err := validateUser(accessJwt) - if err != nil { - zap.S().Debugf("Failed to verify self access, err: %v", err) - return false - } - zap.S().Debugf("Self access verification, userID: %s, id: %s\n", user.Id, id) - return user.Id == id -} - -func IsViewer(r *http.Request) bool { - accessJwt, err := ExtractJwtFromRequest(r) - if err != nil { - zap.S().Debugf("Failed to verify viewer access, err: %v", err) - return false - } - - user, err := validateUser(accessJwt) - if err != nil { - return false - } - - return user.GroupId == AuthCacheObj.ViewerGroupId -} - -func IsEditor(r *http.Request) bool { - accessJwt, err := ExtractJwtFromRequest(r) - if err != nil { - zap.S().Debugf("Failed to verify editor access, err: %v", err) - return false - } - - user, err := validateUser(accessJwt) - if err != nil { - return false - } - return user.GroupId == AuthCacheObj.EditorGroupId -} +func IsViewer(user *model.UserPayload) bool { return user.GroupId == AuthCacheObj.ViewerGroupId } +func IsEditor(user *model.UserPayload) bool { return user.GroupId == AuthCacheObj.EditorGroupId } +func IsAdmin(user *model.UserPayload) bool { return user.GroupId == AuthCacheObj.AdminGroupId } func ValidatePassword(password string) error { if len(password) < minimumPasswordLength { diff --git a/pkg/query-service/model/auth.go b/pkg/query-service/model/auth.go index df86c43488..e5d3e7bfbf 100644 --- a/pkg/query-service/model/auth.go +++ b/pkg/query-service/model/auth.go @@ -1,5 +1,11 @@ package model +import "github.com/pkg/errors" + +var ( + ErrorTokenExpired = errors.New("Token is expired") +) + type InviteRequest struct { Name string `json:"name"` Email string `json:"email"` diff --git a/pkg/query-service/model/response.go b/pkg/query-service/model/response.go index 408d0ee529..602f8e20be 100644 --- a/pkg/query-service/model/response.go +++ b/pkg/query-service/model/response.go @@ -28,6 +28,7 @@ const ( ErrorNotFound ErrorType = "not_found" ErrorNotImplemented ErrorType = "not_implemented" ErrorUnauthorized ErrorType = "unauthorized" + ErrorForbidden ErrorType = "forbidden" ) type QueryDataV2 struct {