From 5aed8b3af1ebac7e11aa60fdec40d5bdeb1ffc3d Mon Sep 17 00:00:00 2001 From: Darren Shepherd Date: Sat, 31 Mar 2018 00:13:30 -0700 Subject: [PATCH] Refactor access control to return error not bool --- api/server.go | 23 ++++++++++---------- api/writer/json.go | 10 ++++----- authorization/all.go | 36 ++++++++++++++++++++++--------- httperror/error.go | 40 +++++++++++++++++------------------ httperror/handler.go | 40 ----------------------------------- httperror/handler/handler.go | 41 ++++++++++++++++++++++++++++++++++++ parse/builder/builder.go | 2 ++ store/schema/schema_store.go | 12 +++++------ types/reflection.go | 4 ++-- types/schema_funcs.go | 36 ++++++++++++++++++++++--------- types/server_types.go | 10 ++++----- 11 files changed, 145 insertions(+), 109 deletions(-) delete mode 100644 httperror/handler.go create mode 100644 httperror/handler/handler.go diff --git a/api/server.go b/api/server.go index d473920c5..d785fa43a 100644 --- a/api/server.go +++ b/api/server.go @@ -11,6 +11,7 @@ import ( "github.com/rancher/norman/api/writer" "github.com/rancher/norman/authorization" "github.com/rancher/norman/httperror" + ehandler "github.com/rancher/norman/httperror/handler" "github.com/rancher/norman/parse" "github.com/rancher/norman/store/wrapper" "github.com/rancher/norman/types" @@ -64,7 +65,7 @@ func NewAPIServer() *Server { LinkHandler: func(*types.APIContext, types.RequestHandler) error { return httperror.NewAPIError(httperror.NotFound, "Link not found") }, - ErrorHandler: httperror.ErrorHandler, + ErrorHandler: ehandler.ErrorHandler, }, StoreWrapper: wrapper.Wrap, URLParser: parse.DefaultURLParser, @@ -187,31 +188,31 @@ func (s *Server) handle(rw http.ResponseWriter, req *http.Request) (*types.APICo switch apiRequest.Method { case http.MethodGet: if apiRequest.ID == "" { - if !apiRequest.AccessControl.CanList(apiRequest, apiRequest.Schema) { - return apiRequest, httperror.NewAPIError(httperror.PermissionDenied, "Can not list "+apiRequest.Schema.ID) + if err := apiRequest.AccessControl.CanList(apiRequest, apiRequest.Schema); err != nil { + return apiRequest, err } } else { - if !apiRequest.AccessControl.CanGet(apiRequest, apiRequest.Schema) { - return apiRequest, httperror.NewAPIError(httperror.PermissionDenied, "Can not get "+apiRequest.Schema.ID) + if err := apiRequest.AccessControl.CanGet(apiRequest, apiRequest.Schema); err != nil { + return apiRequest, err } } handler = apiRequest.Schema.ListHandler nextHandler = s.Defaults.ListHandler case http.MethodPost: - if !apiRequest.AccessControl.CanCreate(apiRequest, apiRequest.Schema) { - return apiRequest, httperror.NewAPIError(httperror.PermissionDenied, "Can not create "+apiRequest.Schema.ID) + if err := apiRequest.AccessControl.CanCreate(apiRequest, apiRequest.Schema); err != nil { + return apiRequest, err } handler = apiRequest.Schema.CreateHandler nextHandler = s.Defaults.CreateHandler case http.MethodPut: - if !apiRequest.AccessControl.CanUpdate(apiRequest, nil, apiRequest.Schema) { - return apiRequest, httperror.NewAPIError(httperror.PermissionDenied, "Can not update "+apiRequest.Schema.ID) + if err := apiRequest.AccessControl.CanUpdate(apiRequest, nil, apiRequest.Schema); err != nil { + return apiRequest, err } handler = apiRequest.Schema.UpdateHandler nextHandler = s.Defaults.UpdateHandler case http.MethodDelete: - if !apiRequest.AccessControl.CanDelete(apiRequest, nil, apiRequest.Schema) { - return apiRequest, httperror.NewAPIError(httperror.PermissionDenied, "Can not delete "+apiRequest.Schema.ID) + if err := apiRequest.AccessControl.CanDelete(apiRequest, nil, apiRequest.Schema); err != nil { + return apiRequest, err } handler = apiRequest.Schema.DeleteHandler nextHandler = s.Defaults.DeleteHandler diff --git a/api/writer/json.go b/api/writer/json.go index 90029e5f3..6a784eb6d 100644 --- a/api/writer/json.go +++ b/api/writer/json.go @@ -140,16 +140,16 @@ func (j *JSONResponseWriter) addLinks(b *builder.Builder, schema *types.Schema, self := context.URLBuilder.ResourceLink(rawResource) rawResource.Links["self"] = self - if context.AccessControl.CanUpdate(context, input, schema) { + if context.AccessControl.CanUpdate(context, input, schema) == nil { rawResource.Links["update"] = self } - if context.AccessControl.CanDelete(context, input, schema) { + if context.AccessControl.CanDelete(context, input, schema) == nil { rawResource.Links["remove"] = self } subContextVersion := context.Schemas.SubContextVersionForSchema(schema) for _, backRef := range context.Schemas.References(schema) { - if !backRef.Schema.CanList(context) { + if backRef.Schema.CanList(context) != nil { continue } @@ -162,7 +162,7 @@ func (j *JSONResponseWriter) addLinks(b *builder.Builder, schema *types.Schema, if subContextVersion != nil { for _, subSchema := range context.Schemas.SchemasForVersion(*subContextVersion) { - if subSchema.CanList(context) { + if subSchema.CanList(context) == nil { rawResource.Links[subSchema.PluralName] = context.URLBuilder.SubContextCollection(schema, rawResource.ID, subSchema) } } @@ -184,7 +184,7 @@ func newCollection(apiContext *types.APIContext) *types.GenericCollection { } if apiContext.Method == http.MethodGet { - if apiContext.AccessControl.CanCreate(apiContext, apiContext.Schema) { + if apiContext.AccessControl.CanCreate(apiContext, apiContext.Schema) == nil { result.CreateTypes[apiContext.Schema.ID] = apiContext.URLBuilder.Collection(apiContext.Schema, apiContext.Version) } } diff --git a/authorization/all.go b/authorization/all.go index a99cc0762..7e3cc7d57 100644 --- a/authorization/all.go +++ b/authorization/all.go @@ -3,6 +3,7 @@ package authorization import ( "net/http" + "github.com/rancher/norman/httperror" "github.com/rancher/norman/types" "github.com/rancher/norman/types/slice" ) @@ -10,24 +11,39 @@ import ( type AllAccess struct { } -func (*AllAccess) CanCreate(apiContext *types.APIContext, schema *types.Schema) bool { - return slice.ContainsString(schema.CollectionMethods, http.MethodPost) +func (*AllAccess) CanCreate(apiContext *types.APIContext, schema *types.Schema) error { + if slice.ContainsString(schema.CollectionMethods, http.MethodPost) { + return nil + } + return httperror.NewAPIError(httperror.PermissionDenied, "can not create "+schema.ID) } -func (*AllAccess) CanGet(apiContext *types.APIContext, schema *types.Schema) bool { - return slice.ContainsString(schema.ResourceMethods, http.MethodGet) +func (*AllAccess) CanGet(apiContext *types.APIContext, schema *types.Schema) error { + if slice.ContainsString(schema.ResourceMethods, http.MethodGet) { + return nil + } + return httperror.NewAPIError(httperror.PermissionDenied, "can not get "+schema.ID) } -func (*AllAccess) CanList(apiContext *types.APIContext, schema *types.Schema) bool { - return slice.ContainsString(schema.CollectionMethods, http.MethodGet) +func (*AllAccess) CanList(apiContext *types.APIContext, schema *types.Schema) error { + if slice.ContainsString(schema.CollectionMethods, http.MethodGet) { + return nil + } + return httperror.NewAPIError(httperror.PermissionDenied, "can not list "+schema.ID) } -func (*AllAccess) CanUpdate(apiContext *types.APIContext, obj map[string]interface{}, schema *types.Schema) bool { - return slice.ContainsString(schema.ResourceMethods, http.MethodPut) +func (*AllAccess) CanUpdate(apiContext *types.APIContext, obj map[string]interface{}, schema *types.Schema) error { + if slice.ContainsString(schema.ResourceMethods, http.MethodPut) { + return nil + } + return httperror.NewAPIError(httperror.PermissionDenied, "can not update "+schema.ID) } -func (*AllAccess) CanDelete(apiContext *types.APIContext, obj map[string]interface{}, schema *types.Schema) bool { - return slice.ContainsString(schema.ResourceMethods, http.MethodDelete) +func (*AllAccess) CanDelete(apiContext *types.APIContext, obj map[string]interface{}, schema *types.Schema) error { + if slice.ContainsString(schema.ResourceMethods, http.MethodDelete) { + return nil + } + return httperror.NewAPIError(httperror.PermissionDenied, "can not delete "+schema.ID) } func (*AllAccess) Filter(apiContext *types.APIContext, schema *types.Schema, obj map[string]interface{}, context map[string]string) map[string]interface{} { diff --git a/httperror/error.go b/httperror/error.go index c721dfd92..090724cb9 100644 --- a/httperror/error.go +++ b/httperror/error.go @@ -34,40 +34,40 @@ var ( ) type ErrorCode struct { - code string + Code string Status int } func (e ErrorCode) String() string { - return fmt.Sprintf("%s %d", e.code, e.Status) + return fmt.Sprintf("%s %d", e.Code, e.Status) } type APIError struct { - code ErrorCode - message string + Code ErrorCode + Message string Cause error - fieldName string + FieldName string } func NewAPIErrorLong(status int, code, message string) error { return NewAPIError(ErrorCode{ - code: code, + Code: code, Status: status, }, message) } func NewAPIError(code ErrorCode, message string) error { return &APIError{ - code: code, - message: message, + Code: code, + Message: message, } } func NewFieldAPIError(code ErrorCode, fieldName, message string) error { return &APIError{ - code: code, - message: message, - fieldName: fieldName, + Code: code, + Message: message, + FieldName: fieldName, } } @@ -76,9 +76,9 @@ func NewFieldAPIError(code ErrorCode, fieldName, message string) error { func WrapFieldAPIError(err error, code ErrorCode, fieldName, message string) error { return &APIError{ Cause: err, - code: code, - message: message, - fieldName: fieldName, + Code: code, + Message: message, + FieldName: fieldName, } } @@ -86,17 +86,17 @@ func WrapFieldAPIError(err error, code ErrorCode, fieldName, message string) err // err WILL NOT be in the API response func WrapAPIError(err error, code ErrorCode, message string) error { return &APIError{ - code: code, - message: message, + Code: code, + Message: message, Cause: err, } } func (a *APIError) Error() string { - if a.fieldName != "" { - return fmt.Sprintf("%s=%s: %s", a.fieldName, a.code, a.message) + if a.FieldName != "" { + return fmt.Sprintf("%s=%s: %s", a.FieldName, a.Code, a.Message) } - return fmt.Sprintf("%s: %s", a.code, a.message) + return fmt.Sprintf("%s: %s", a.Code, a.Message) } func IsAPIError(err error) bool { @@ -106,7 +106,7 @@ func IsAPIError(err error) bool { func IsConflict(err error) bool { if apiError, ok := err.(*APIError); ok { - return apiError.code.Status == 409 + return apiError.Code.Status == 409 } return false diff --git a/httperror/handler.go b/httperror/handler.go deleted file mode 100644 index 3db377dcf..000000000 --- a/httperror/handler.go +++ /dev/null @@ -1,40 +0,0 @@ -package httperror - -import ( - "github.com/rancher/norman/types" - "github.com/sirupsen/logrus" -) - -func ErrorHandler(request *types.APIContext, err error) { - var error *APIError - if apiError, ok := err.(*APIError); ok { - if apiError.Cause != nil { - logrus.Errorf("API error response %v for %v %v. Cause: %v", apiError.code.Status, request.Request.Method, - request.Request.RequestURI, apiError.Cause) - } - error = apiError - } else { - logrus.Errorf("Unknown error: %v", err) - error = &APIError{ - code: ServerError, - message: err.Error(), - } - } - - data := toError(error) - request.WriteResponse(error.code.Status, data) -} - -func toError(apiError *APIError) map[string]interface{} { - e := map[string]interface{}{ - "type": "/meta/schemas/error", - "status": apiError.code.Status, - "code": apiError.code.code, - "message": apiError.message, - } - if apiError.fieldName != "" { - e["fieldName"] = apiError.fieldName - } - - return e -} diff --git a/httperror/handler/handler.go b/httperror/handler/handler.go new file mode 100644 index 000000000..8bad04a36 --- /dev/null +++ b/httperror/handler/handler.go @@ -0,0 +1,41 @@ +package handler + +import ( + "github.com/rancher/norman/httperror" + "github.com/rancher/norman/types" + "github.com/sirupsen/logrus" +) + +func ErrorHandler(request *types.APIContext, err error) { + var error *httperror.APIError + if apiError, ok := err.(*httperror.APIError); ok { + if apiError.Cause != nil { + logrus.Errorf("API error response %v for %v %v. Cause: %v", apiError.Code.Status, request.Request.Method, + request.Request.RequestURI, apiError.Cause) + } + error = apiError + } else { + logrus.Errorf("Unknown error: %v", err) + error = &httperror.APIError{ + Code: httperror.ServerError, + Message: err.Error(), + } + } + + data := toError(error) + request.WriteResponse(error.Code.Status, data) +} + +func toError(apiError *httperror.APIError) map[string]interface{} { + e := map[string]interface{}{ + "type": "/meta/schemas/error", + "status": apiError.Code.Status, + "code": apiError.Code.Code, + "message": apiError.Message, + } + if apiError.FieldName != "" { + e["fieldName"] = apiError.FieldName + } + + return e +} diff --git a/parse/builder/builder.go b/parse/builder/builder.go index d06a6b994..e5dd16b98 100644 --- a/parse/builder/builder.go +++ b/parse/builder/builder.go @@ -137,6 +137,8 @@ func (b *Builder) checkDefaultAndRequired(schema *types.Schema, input map[string if op.IsList() && fieldMatchesOp(field, List) && definition.IsReferenceType(field.Type) && !hasKey { result[fieldName] = nil + } else if op.IsList() && fieldMatchesOp(field, List) && !hasKey && field.Default != nil { + result[fieldName] = field.Default } } diff --git a/store/schema/schema_store.go b/store/schema/schema_store.go index 8892fb638..d86572b48 100644 --- a/store/schema/schema_store.go +++ b/store/schema/schema_store.go @@ -38,21 +38,21 @@ func (s *Store) ByID(apiContext *types.APIContext, schema *types.Schema, id stri func (s *Store) modifyForAccessControl(context *types.APIContext, schema types.Schema) *types.Schema { var resourceMethods []string - if slice.ContainsString(schema.ResourceMethods, http.MethodPut) && schema.CanUpdate(context) { + if slice.ContainsString(schema.ResourceMethods, http.MethodPut) && schema.CanUpdate(context) == nil { resourceMethods = append(resourceMethods, http.MethodPut) } - if slice.ContainsString(schema.ResourceMethods, http.MethodDelete) && schema.CanDelete(context) { + if slice.ContainsString(schema.ResourceMethods, http.MethodDelete) && schema.CanDelete(context) == nil { resourceMethods = append(resourceMethods, http.MethodDelete) } - if slice.ContainsString(schema.ResourceMethods, http.MethodGet) && schema.CanGet(context) { + if slice.ContainsString(schema.ResourceMethods, http.MethodGet) && schema.CanGet(context) == nil { resourceMethods = append(resourceMethods, http.MethodGet) } var collectionMethods []string - if slice.ContainsString(schema.CollectionMethods, http.MethodPost) && schema.CanCreate(context) { + if slice.ContainsString(schema.CollectionMethods, http.MethodPost) && schema.CanCreate(context) == nil { collectionMethods = append(collectionMethods, http.MethodPost) } - if slice.ContainsString(schema.CollectionMethods, http.MethodGet) && schema.CanList(context) { + if slice.ContainsString(schema.CollectionMethods, http.MethodGet) && schema.CanList(context) == nil { collectionMethods = append(collectionMethods, http.MethodGet) } @@ -78,7 +78,7 @@ func (s *Store) List(apiContext *types.APIContext, schema *types.Schema, opt *ty continue } - if schema.CanList(apiContext) || schema.CanGet(apiContext) { + if schema.CanList(apiContext) == nil || schema.CanGet(apiContext) == nil { schemas = s.addSchema(apiContext, schema, schemaMap, schemas, included) } } diff --git a/types/reflection.go b/types/reflection.go index 1935ab7aa..263566b02 100644 --- a/types/reflection.go +++ b/types/reflection.go @@ -151,7 +151,7 @@ func (s *Schemas) importType(version *APIVersion, t reflect.Type, overrides ...r mappers := s.mapper(&schema.Version, schema.ID) if s.DefaultMappers != nil { - if schema.CanList(nil) { + if schema.CanList(nil) == nil { mappers = append(s.DefaultMappers(), mappers...) } } @@ -175,7 +175,7 @@ func (s *Schemas) importType(version *APIVersion, t reflect.Type, overrides ...r mapper := &typeMapper{ Mappers: mappers, - root: schema.CanList(nil), + root: schema.CanList(nil) == nil, } if err := mapper.ModifySchema(schema, s); err != nil { diff --git a/types/schema_funcs.go b/types/schema_funcs.go index a6f26b9de..f104728af 100644 --- a/types/schema_funcs.go +++ b/types/schema_funcs.go @@ -3,6 +3,7 @@ package types import ( "net/http" + "github.com/rancher/norman/httperror" "github.com/rancher/norman/types/slice" ) @@ -21,37 +22,52 @@ func (v *APIVersion) Equals(other *APIVersion) bool { v.Path == other.Path } -func (s *Schema) CanList(context *APIContext) bool { +func (s *Schema) CanList(context *APIContext) error { if context == nil { - return slice.ContainsString(s.CollectionMethods, http.MethodGet) + if slice.ContainsString(s.CollectionMethods, http.MethodGet) { + return nil + } + return httperror.NewAPIError(httperror.PermissionDenied, "can not list "+s.ID) } return context.AccessControl.CanList(context, s) } -func (s *Schema) CanGet(context *APIContext) bool { +func (s *Schema) CanGet(context *APIContext) error { if context == nil { - return slice.ContainsString(s.ResourceMethods, http.MethodGet) + if slice.ContainsString(s.ResourceMethods, http.MethodGet) { + return nil + } + return httperror.NewAPIError(httperror.PermissionDenied, "can not get "+s.ID) } return context.AccessControl.CanGet(context, s) } -func (s *Schema) CanCreate(context *APIContext) bool { +func (s *Schema) CanCreate(context *APIContext) error { if context == nil { - return slice.ContainsString(s.CollectionMethods, http.MethodPost) + if slice.ContainsString(s.CollectionMethods, http.MethodPost) { + return nil + } + return httperror.NewAPIError(httperror.PermissionDenied, "can not create "+s.ID) } return context.AccessControl.CanCreate(context, s) } -func (s *Schema) CanUpdate(context *APIContext) bool { +func (s *Schema) CanUpdate(context *APIContext) error { if context == nil { - return slice.ContainsString(s.ResourceMethods, http.MethodPut) + if slice.ContainsString(s.ResourceMethods, http.MethodPut) { + return nil + } + return httperror.NewAPIError(httperror.PermissionDenied, "can not update "+s.ID) } return context.AccessControl.CanUpdate(context, nil, s) } -func (s *Schema) CanDelete(context *APIContext) bool { +func (s *Schema) CanDelete(context *APIContext) error { if context == nil { - return slice.ContainsString(s.ResourceMethods, http.MethodDelete) + if slice.ContainsString(s.ResourceMethods, http.MethodDelete) { + return nil + } + return httperror.NewAPIError(httperror.PermissionDenied, "can not delete "+s.ID) } return context.AccessControl.CanDelete(context, nil, s) } diff --git a/types/server_types.go b/types/server_types.go index 41adaf53f..f58c9915e 100644 --- a/types/server_types.go +++ b/types/server_types.go @@ -69,11 +69,11 @@ type ResponseWriter interface { } type AccessControl interface { - CanCreate(apiContext *APIContext, schema *Schema) bool - CanList(apiContext *APIContext, schema *Schema) bool - CanGet(apiContext *APIContext, schema *Schema) bool - CanUpdate(apiContext *APIContext, obj map[string]interface{}, schema *Schema) bool - CanDelete(apiContext *APIContext, obj map[string]interface{}, schema *Schema) bool + CanCreate(apiContext *APIContext, schema *Schema) error + CanList(apiContext *APIContext, schema *Schema) error + CanGet(apiContext *APIContext, schema *Schema) error + CanUpdate(apiContext *APIContext, obj map[string]interface{}, schema *Schema) error + CanDelete(apiContext *APIContext, obj map[string]interface{}, schema *Schema) error Filter(apiContext *APIContext, schema *Schema, obj map[string]interface{}, context map[string]string) map[string]interface{} FilterList(apiContext *APIContext, schema *Schema, obj []map[string]interface{}, context map[string]string) []map[string]interface{}