Role.id and Role.name mixup in resolving ACLs and Role.isInRole

[marcuspl]

The documentation for the ACL class states that the principalId for a Role should be "ID of the principal - such as appId, userId or roleId". When a role is resolved in ACL.checkAccessForContext we see that it is called with roleModel.isInRole(acl.principalId, ...
However, when looking at the documentation for Role.isInRole:

  /**
   * Check if a given principal is in the role
   *
   * @param {String} role The role name
   * @param {Object} context The context object
   * @callback {Function} callback
   * @param {Error} err
   * @param {Boolean} isInRole
   */
  Role.isInRole = function(role, context, callback)

And later on it is used as a name, not as an id: this.findOne({where: {name: role}} ...

I had hooked up all my ACLs to map on IDs, so I changed the code to use findById, and I could not find any other place that referred to it so that worked fine for me. Another option would be to change the calling code to use name, or have the find look for both name OR id.

As I don't know how you would like to have it, I figured I mention it to you guys without trying to provide a patch or something first.

Thanks for the great work and keep it up!

[bajtos]

@raymondfeng you are more familiar with ACLs than I am. Could you PTAL?

[smartmouse]

@marcuspl Thank you for spotting this issue. We confirm with @raymondfeng that it should be role name. This should be a bug in code and not doc issue .

So we should pass in the principalName instead of the principalId
Do you want to provide a patch?
Thanks

       // Check role matches
       if (acl.principalType === ACL.ROLE) {
         inRoleTasks.push(function(done) {
           roleModel.isInRole(acl.principalId, context,
             function(err, inRole) {
               if (!err && inRole) {
                 effectiveACLs.push(acl);
               }
               done(err, acl);
             });
         });

[smartmouse]

I am removing the doc label as this is not really doc issue.

[crandmck]

@superkhau Who should we ping/assign this to now to ensure it doesn't get lost?

[superkhau]

@crandmck I believe @kjdelisle's team is on it according to the Slack convo's in their channel.

[superkhau]

Another option would be to change the calling code to use name...

@marcuspl Would you like to submit a patch instead? I can help get it landed quickly.

[bajtos]

FWIW, this may be related to #2420 and #2975 /cc @ebarault.