Skip to content
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.

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

Pluggable permissions #43

Open
spion-h4 opened this issue Jun 12, 2019 · 10 comments
Open

Pluggable permissions #43

spion-h4 opened this issue Jun 12, 2019 · 10 comments

Comments

@spion-h4
Copy link
Collaborator

spion-h4 commented Jun 12, 2019

space for final design.

@spion-h4
Copy link
Collaborator Author

spion-h4 commented Jun 12, 2019

Make a permission class that is easy to modify from the outside.

Sketch:

Suppose you have a documents plugin with two types of permissions pertaining to managing those documents

class DocumentPermissions extends AppSingleton {
  list =  new Permission<Document>();
  read = new Permission<Document>();
  create = new Permission<Document>();
  modify = new Permission<Document>();
}

type PermissionResult = 'allow' | 'deny' | 'pass'

getSingleton(DocumentPermissions).list
    .add({
        name: string,
        priority: number,
        check: (doc: Document, ctx: ServiceContext) => Promise<PermisisonResult>
    })

Algorithm:

  • Sort by priority, the first check function that returns “Allow” or “Deny” instead of “Pass” wins.
  • If more than one has equal priority, check both; Deny wins over Allow

Desireables:

  • easy way to query all active rules in all active permissions with their priorities
  • easy recording of which rules were consulted and what their results were in the audit log
  • inheritance? newPermission = this.otherPermission.inherit()

Building a simple role based system on top of this

function makeRolePermission<T extends ObjectId>(actionName: string) {
  return { 
    get nameAsync() {
      let rolesForThisAction = await ctx.getService(Roles).getAllRolesWithAction(actionName);
      //return rolesForthisAction.map(r => ({name: `User has role: ${r}`, results: ['allow', 'pass']}));
      return `Allow user with one of the roles: ${rolesForThisAction.join(' ')}`
   },
    priority: 10, 
    check: async (object:T, ctx) =>  { 
      let userId = this.getService(UserId).id;
      let roleCheck = await ctx.getService(Roles).checkRole(actionName, userId, object.id);
      return roleCheck ? 'allow' : 'deny'
    }
  } 
}

documentPermissions.read.addRule(makeRolePermission('document.read'));

Considerations:

  • It seems to be flexible enough for this
  • Do we need the check function to be able to return additional description (for audit log) ?

@spion-h4
Copy link
Collaborator Author

spion-h4 commented Jun 12, 2019

Building simple permissions like cancan and pundit:

documentPermissions.read.addRule({
  name: 'Superadmin can access everything'
  priority: 100,
  check: (_, ctx) => ctx.getService(UserAdminInfo).isSuperAdmin ? 'allow' : 'pass'
}

Pass means the rule will defer to other rules if the user is not super admin.

Considerations

  • Do we want to add the superadmin rule to everything?
  • Do we use inherits a lot to avoid adding the same rule to everything?
  • Will the permissions be hard to follow with inherits? (a system to query all installed rules would be helpful)

@spion-h4
Copy link
Collaborator Author

To be able to keep track of all permissions, you could have a singleton:

app.getSingleton(Permissions).create(name: string)

You could also plug into this now

app.getSingleton(Permissions).onPermissionQueried((result, ctx) => {
  ctx.getService(AuditLogBuilder).record({permissionCheckResult: result.checkedRules})
})

@spion-h4
Copy link
Collaborator Author

spion-h4 commented Jun 12, 2019

More rule analysis: we can protect the check function from being directly assigned, instead create a checking object from a function with allowed return results

check: checkFn(['allow', 'deny'], fn)

Now fn is not allowed to return "pass"

That way you can detect anomalies like:

  • a high priority rule is "cutting off" an entire tree of low priority ones by never returning 'pass'
  • a terminal rule doesn't have a default value (end result can theoretically be "pass")

@spion-h4
Copy link
Collaborator Author

spion-h4 commented Jun 12, 2019

Rule ordering alternative: Instead of having numbers, create subrules as "slots" in other rules and export the slot only. Then the order is strictly determined.

export class ConfigurablePermissions extends AppSingleton {
  read = permission.create()
}

// not exported
class RealPermissions extends AppSingleton {
  private exported = app.getSingleton(ConfigurablePermissions);
  read = permission.create().add(
    allowAdmins, allowTrusted, denyDeactivatedAccounts, this.exported.read, defaultDeny
  );
  write = permission.create().add(
    allowAdmins, allowTrusted, denyDeactivatedAccounts, this.exported.write, defaultDeny
  );
}

// exported
export class ConfigurablePermissions extends AppSingleton {
  public read = permission.create();
  public write = permission.create();
}

edit: Conviniently this doubles as inheritance too.

@spion-h4
Copy link
Collaborator Author

spion-h4 commented Jun 12, 2019

Here is how a report tree might look like for a single permission

Permission name: document.read

"User is super admin" -> Allow
| (pass)
"Request coming from other app, not user" -> Allow
| (pass)
"Account has been deactivated" -> Deny
| (pass)
Slot 
    "User is member of library" -> Allow
     | (pass)
| (pass)
"Default deny" -> Deny

Here is an example report where the tree seems to have been "cut off"

Permission name: document.write

"User is super admin" -> Allow
| (pass)
"Request coming from other app, not user" -> Allow
| (pass)
"Account has been deactivated" -> Deny
| (pass)
Slot 
    "User is member of library" -> Allow
     -> Deny
------------- WARNING: TREE SECTION CUT OFF ------------
| (pass)
"Default deny" -> Deny

@wh4everest
Copy link
Contributor

wh4everest commented Jun 12, 2019

Consideration about graying out buttons on the frontend...

Currently, our frontends are littered with code that performs the same checks that the backend does, in order to disable some input and prevent a request from happening. This is ugly for obvious reasons, like forgetting to sync the checks on both ends.

For example:

if (!roles.user.includes(['editor']) { makeUploadButtonGray() }`

Instead, it would be nice if you can ask a backend service whether I can perform the action and receive a boolean answer.

myService.checkPermissions.then(perms => { if (!perms.read) makeUploadButtonGray() }`

Behind the scenes, the service would calculate the outcome of permission.create(ctx) for each method and return me the object full of boolean results.

Of course, this won't work if the answer depends on the params ("can I delete this particular set of documents?") but that's fine, since you'd get 403 as a response -- rarely enough not to annoy the users.

@spion
Copy link
Contributor

spion commented Jun 14, 2019

The role system can provide (in the rule) more details by returning

allowDescription: Promise<string>
denyDescription: Promise<string>

The results would probably look like
Allow for roles: X, Y, Z
Deny for roles: A, B

@spion
Copy link
Contributor

spion commented Jun 14, 2019

How to get a typesafe permission installation for RPC methods

class MyService {
  method1(...) { ... }
  method2(...) { ... }
}

setPermissions(MyService, {
  method1: permission1
  method2: permission2
});

Advantages to decorators:

  • typesafe params

Advantages to checking in method body

  • don't have to invoke the method to find permissions
  • can automatically build an asking service "svcName.method.askPermission`
  • can associate installed permissions with methods

Disadvantages

  • permissions a bit far away from method

@spion-h4
Copy link
Collaborator Author

Simplified "slot" design:

function basePermission(additionalRules) {
  return [startRules].concat(additionRules).concat(defaultDeny);
}

function check(ruleList) {
  return x;
}

function describe(ruleList) {
  return x;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants