Skip to content

propagate resolved roles in remoting context#2957

Merged
bajtos merged 1 commit into
strongloop:masterfrom
ebarault:propagate-resolved-roles-in-context
Mar 20, 2017
Merged

propagate resolved roles in remoting context#2957
bajtos merged 1 commit into
strongloop:masterfrom
ebarault:propagate-resolved-roles-in-context

Conversation

@ebarault

@ebarault ebarault commented Nov 19, 2016

Copy link
Copy Markdown
Contributor

Description

For some advanced CRUD access control in remote methods, it is often required to get/check roles resolved (i.e. allowed) for a user against a method with Role.getRoles(context) or Role.isInRole(role, context).
In addition to clutter the remote methods / hooks code, it forces the app to run again through all static roles and registered dynamic role resolvers, inducing performance loss and extra calls to db.

Meanwhile, the exact same course of action is performed right when the Model.checkAccess() method is called to verify if a principal is allowed to access a protected resource :

Model.checkAccess(context) -> ACL.checkAccessForContext(context) -> acls.forEach(acl, ...) ->roleModel.isInRole(acl.principalId, context)

This PR proposes to add a new param (Array) in remoting context (name proposed: resolvedRoles, to be discussed) which is filled during Model Access Permission Checking phase with the roles for which the user is allowed to run the remote method.

an example of resolvedRoles is: ['admin', '$owner'] where admin is a custom role statically mapped to a principal, and $owner is a built-in smart role

resolvedRoles param can then be accessed in remote method and remote method hooks as ctx.req.remotingContext.resolvedRoles

To be noted: the resolvedRoles param is set in a way that the calling user cannot temper the value to fool the additional access control policies added by user in remote methods

Reviewers: please review/discuss the general idea before i provide the required tests

Related issues

  • None

Checklist

  • Code conforms with the style
    guide
  • New tests added or existing tests modified to cover all changes

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants