Skip to content

refactoring object-operation endpoints to use permission descriptors #9416

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

refactoring object-operation endpoints to use permission descriptors #9416

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
da530cf
refactoring object-operation endpoints to use permission descriptors
tkalir Aug 12, 2025
db86f41
changing callback -> onFailure in AuthOpts and authorizeCallback
tkalir Aug 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 29 additions & 56 deletions pkg/api/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,10 @@ type Controller struct {
licenseManager license.Manager
}

type AuthOpts struct {
callback func(w http.ResponseWriter, r *http.Request, code int, v interface{})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add at least one, and possibly all, of these:

  • a type for v
  • documentation for the fields code and v
  • documentation of callback: when is it called? what does it do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently all endpoints except 2 call authorize() which calls authorizeCallback() with writeError as callback. There are 2 endpoints which call authorizeCallback directly with a different callback.
So when a function is supplied here it is used as callback instead of writeError.

I used the same signature with the same parameter names as in authorizeCallback, because I think it's clearer if they match. after your comment I changed the variable names in both places to be more informative, tell me what you think.

}

var usageCounter = stats.NewUsageCounter()

func NewController(cfg config.Config, catalog *catalog.Catalog, authenticator auth.Authenticator, authService auth.Service, authenticationService authentication.Service, blockAdapter block.Adapter, metadataManager auth.MetadataManager, migrator Migrator, collector stats.Collector, actions actionsHandler, auditChecker AuditChecker, logger logging.Logger, sessionStore sessions.Store, pathProvider upload.PathProvider, usageReporter stats.UsageReporterOperations, licenseManager license.Manager) *Controller {
Expand Down Expand Up @@ -3377,12 +3381,7 @@ func (c *Controller) DeleteObject(w http.ResponseWriter, r *http.Request, reposi
}

func (c *Controller) UploadObjectPreflight(w http.ResponseWriter, r *http.Request, repository, branch string, params apigen.UploadObjectPreflightParams) {
if !c.authorize(w, r, permissions.Node{
Permission: permissions.Permission{
Action: permissions.WriteObjectAction,
Resource: permissions.ObjectArn(repository, params.Path),
},
}) {
if !c.authorizeReq(w, r, "UploadObjectPreflight", permissions.PermissionParams{Repository: &repository, Path: &params.Path}, nil) {
return
}

Expand All @@ -3393,12 +3392,7 @@ func (c *Controller) UploadObjectPreflight(w http.ResponseWriter, r *http.Reques
}

func (c *Controller) UploadObject(w http.ResponseWriter, r *http.Request, repository, branch string, params apigen.UploadObjectParams) {
if !c.authorize(w, r, permissions.Node{
Permission: permissions.Permission{
Action: permissions.WriteObjectAction,
Resource: permissions.ObjectArn(repository, params.Path),
},
}) {
if !c.authorizeReq(w, r, "UploadObject", permissions.PermissionParams{Repository: &repository, Path: &params.Path}, nil) {
return
}
ctx := r.Context()
Expand Down Expand Up @@ -3563,12 +3557,7 @@ func (c *Controller) UploadObject(w http.ResponseWriter, r *http.Request, reposi
}

func (c *Controller) StageObject(w http.ResponseWriter, r *http.Request, body apigen.StageObjectJSONRequestBody, repository, branch string, params apigen.StageObjectParams) {
if !c.authorize(w, r, permissions.Node{
Permission: permissions.Permission{
Action: permissions.WriteObjectAction,
Resource: permissions.ObjectArn(repository, params.Path),
},
}) {
if !c.authorizeReq(w, r, "StageObject", permissions.PermissionParams{Repository: &repository, Path: &params.Path}, nil) {
return
}
ctx := r.Context()
Expand Down Expand Up @@ -4383,12 +4372,7 @@ func (c *Controller) RestoreStatus(w http.ResponseWriter, r *http.Request, repos
}

func (c *Controller) CreateSymlinkFile(w http.ResponseWriter, r *http.Request, repository, branch string, params apigen.CreateSymlinkFileParams) {
if !c.authorize(w, r, permissions.Node{
Permission: permissions.Permission{
Action: permissions.WriteObjectAction,
Resource: permissions.ObjectArn(repository, branch),
},
}) {
if !c.authorizeReq(w, r, "CreateSymlinkFile", permissions.PermissionParams{Repository: &repository, Path: &branch}, nil) {
return
}
ctx := r.Context()
Expand Down Expand Up @@ -4582,14 +4566,10 @@ func (c *Controller) LogCommits(w http.ResponseWriter, r *http.Request, reposito
}

func (c *Controller) HeadObject(w http.ResponseWriter, r *http.Request, repository, ref string, params apigen.HeadObjectParams) {
if !c.authorizeCallback(w, r, permissions.Node{
Permission: permissions.Permission{
Action: permissions.ReadObjectAction,
Resource: permissions.ObjectArn(repository, params.Path),
},
}, func(w http.ResponseWriter, r *http.Request, code int, v interface{}) {
writeResponse(w, r, code, nil)
}) {
if !c.authorizeReq(w, r, "HeadObject", permissions.PermissionParams{Repository: &repository, Path: &params.Path},
&AuthOpts{callback: func(w http.ResponseWriter, r *http.Request, code int, v interface{}) {
writeResponse(w, r, code, nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the different callback here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be clear, I didn't change the callback that is used here. but I believe the reason is that authorize() uses writeError as callback, which adds an error message to the response body when authorizeCallback fails. HeadObject is a Head endpoint, so it shouldn't have a body in the response.
I can add this as a comment.

}}) {
return
}
ctx := r.Context()
Expand Down Expand Up @@ -4724,12 +4704,7 @@ func (c *Controller) GetMetadataObject(w http.ResponseWriter, r *http.Request, r
}

func (c *Controller) GetObject(w http.ResponseWriter, r *http.Request, repository, ref string, params apigen.GetObjectParams) {
if !c.authorize(w, r, permissions.Node{
Permission: permissions.Permission{
Action: permissions.ReadObjectAction,
Resource: permissions.ObjectArn(repository, params.Path),
},
}) {
if !c.authorizeReq(w, r, "GetObject", permissions.PermissionParams{Repository: &repository, Path: &params.Path}, nil) {
return
}
ctx := r.Context()
Expand Down Expand Up @@ -4947,12 +4922,7 @@ func (c *Controller) ListObjects(w http.ResponseWriter, r *http.Request, reposit
}

func (c *Controller) StatObject(w http.ResponseWriter, r *http.Request, repository, ref string, params apigen.StatObjectParams) {
if !c.authorize(w, r, permissions.Node{
Permission: permissions.Permission{
Action: permissions.ReadObjectAction,
Resource: permissions.ObjectArn(repository, params.Path),
},
}) {
if !c.authorizeReq(w, r, "StatObject", permissions.PermissionParams{Repository: &repository, Path: &params.Path}, nil) {
return
}
ctx := r.Context()
Expand Down Expand Up @@ -5015,12 +4985,7 @@ func (c *Controller) StatObject(w http.ResponseWriter, r *http.Request, reposito
}

func (c *Controller) UpdateObjectUserMetadata(w http.ResponseWriter, r *http.Request, body apigen.UpdateObjectUserMetadataJSONRequestBody, repository, branch string, params apigen.UpdateObjectUserMetadataParams) {
if !c.authorize(w, r, permissions.Node{
Permission: permissions.Permission{
Action: permissions.WriteObjectAction,
Resource: permissions.ObjectArn(repository, params.Path),
},
}) {
if !c.authorizeReq(w, r, "UpdateObjectUserMetadata", permissions.PermissionParams{Repository: &repository, Path: &params.Path}, nil) {
return
}
ctx := r.Context()
Expand All @@ -5036,12 +5001,7 @@ func (c *Controller) UpdateObjectUserMetadata(w http.ResponseWriter, r *http.Req
}

func (c *Controller) GetUnderlyingProperties(w http.ResponseWriter, r *http.Request, repository, ref string, params apigen.GetUnderlyingPropertiesParams) {
if !c.authorize(w, r, permissions.Node{
Permission: permissions.Permission{
Action: permissions.ReadObjectAction,
Resource: permissions.ObjectArn(repository, params.Path),
},
}) {
if !c.authorizeReq(w, r, "GetUnderlyingProperties", permissions.PermissionParams{Repository: &repository, Path: &params.Path}, nil) {
return
}
ctx := r.Context()
Expand Down Expand Up @@ -5944,6 +5904,19 @@ func (c *Controller) authorize(w http.ResponseWriter, r *http.Request, perms per
return c.authorizeCallback(w, r, perms, writeError)
}

func (c *Controller) authorizeReq(w http.ResponseWriter, r *http.Request, operationId string, params permissions.PermissionParams, opts *AuthOpts) bool {
desc := permissions.GetPermissionDescriptor(operationId)
if desc == nil {
c.Logger.Error(fmt.Sprintf("missing permission descriptor for %s", operationId))
return false
}
callback := writeError
if opts != nil && opts.callback != nil {
callback = opts.callback
}
return c.authorizeCallback(w, r, desc.Permission(params), callback)
}

func (c *Controller) isNameValid(name, nameType string) (bool, string) {
// URLs are % encoded. Allowing % signs in entity names would
// limit the ability to use these entity names in the URL for both
Expand Down
41 changes: 41 additions & 0 deletions pkg/permissions/permission.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,3 +61,44 @@ func PolicyArn(policyID string) string {
func ExternalPrincipalArn(principalID string) string {
return authArnPrefix + "externalPrincipal/" + principalID
}

type PermissionParams struct {
Repository *string
Path *string
}

type PermissionDescriptor interface {
Permission(PermissionParams) Node
}

type ObjectPermission struct {
Action string
}

func (o *ObjectPermission) Permission(params PermissionParams) Node {
return Node{
Permission: Permission{
Action: o.Action,
Resource: ObjectArn(*params.Repository, *params.Path),
},
}
}

var readObjectPermission = ObjectPermission{Action: ReadObjectAction}
var writeObjectPermission = ObjectPermission{Action: WriteObjectAction}

var permissionByOp = map[string]PermissionDescriptor{
"HeadObject": &readObjectPermission,
"GetObject": &readObjectPermission,
"StatObject": &readObjectPermission,
"GetUnderlyingProperties": &readObjectPermission,
"StageObject": &writeObjectPermission,
"CreateSymlinkFile": &writeObjectPermission,
"UpdateObjectUserMetadata": &writeObjectPermission,
"UploadObject": &writeObjectPermission,
"UploadObjectPreflight": &writeObjectPermission,
}

func GetPermissionDescriptor(operationId string) PermissionDescriptor {
return permissionByOp[operationId]
}