Add promise support to built-in model ACL by ebarault · Pull Request #3163 · strongloop/loopback

ebarault · 2017-01-30T16:08:20Z

Description

None of the functions from built-in acl.js currently support promises. this PR adds promise support by using loopback utils helper

Related issues

related issue: #418
related PR (similar for built-in role model) : #3146

None

Checklist

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

slnode · 2017-01-30T16:08:30Z

Can one of the admins verify this patch?

slnode · 2017-01-30T16:08:30Z

Can one of the admins verify this patch?

slnode · 2017-01-30T16:08:30Z

Can one of the admins verify this patch?

ebarault · 2017-01-30T16:09:18Z

@bajtos @superkhau please review so we can land this before #2971

superkhau · 2017-01-30T20:51:51Z

common/models/acl.js

-        if (err) return cb(err);
+        if (err) {
+          return cb(err);
+        }


this change is not required

superkhau · 2017-01-30T20:51:59Z

common/models/acl.js

-          if (err || !role) return cb(err, role);
+          if (err || !role) {
+            return cb(err, role);
+          }


superkhau · 2017-01-30T20:54:07Z

test/acl.test.js


 describe('security ACLs', function() {
+  it('supports checkPermission() returning a promise', function(done) {
+    ACL.create({


return ACL.create({ principalType: ACL.USER, principalId: 'u001', model: 'testModel', property: ACL.ALL, accessType: ACL.ALL, permission: ACL.ALLOW, }) .then(function() { return ACL.checkPermission(ACL.USER, 'u001', 'testModel', 'name', ACL.ALL); }) .then(function(access) { assert(access.permission === ACL.ALLOW); });

See http://loopback.io/doc/en/contrib/style-guide.html for more info on promise style tests in mocha

hi @superkhau : i am aware of mocha with promise, i thought you'd like to stick to the origin test file style. as per recent @bajtos PR where the callback style is still used (e.g. here in role.test.js)
i'll modify to use mocha with promises

I vaguely remember there was a reason for using callback-style tests when verifying Promise-based API, but I cannot recall what was it. As I was reviewing the code in this patch, I didn't see why the new style should not work, so let's stick to what our style guide says.

superkhau · 2017-01-30T20:56:16Z

test/acl.test.js

+    .catch(done);
+  });
+
+  it('should support checkAccessForContext() returning a promise', function(done) {


remove should

superkhau · 2017-01-30T20:58:34Z

test/acl.test.js

+
+    ACL.checkAccessForContext({
+      principals: [
+          {type: ACL.USER, id: 'u001'},


indent is off

superkhau · 2017-01-30T20:58:53Z

test/acl.test.js

+      ],
+    });
+
+    ACL.checkAccessForContext({


return ACL.checkAccess...

superkhau · 2017-01-30T20:59:05Z

test/role.test.js

+        expect(u.id).to.eql(user.id);
+        done();
+      })
+      .catch(done);


remove catch

superkhau · 2017-01-30T20:59:38Z

test/acl.test.js


 describe('security ACLs', function() {
+  it('supports checkPermission() returning a promise', function(done) {
+    ACL.create({


See http://loopback.io/doc/en/contrib/style-guide.html for more info on promise style tests in mocha

superkhau · 2017-01-30T20:59:59Z

test/role.test.js

      });
    });

+    it('should support ACL.resolvePrincipal() returning a promise', function(done) {


remove should and done

superkhau · 2017-01-30T21:00:52Z

test/role.test.js

      });
    });

+    it('should support ACL.isMappedToRole() returning a promise', function(done) {


remove should and done

superkhau · 2017-01-30T21:01:10Z

test/role.test.js

    });

+    it('should support ACL.isMappedToRole() returning a promise', function(done) {
+      ACL.isMappedToRole(ACL.USER, user.username, 'admin')


return ACL.isMapped...

superkhau · 2017-01-30T21:01:16Z

test/role.test.js

+      ACL.isMappedToRole(ACL.USER, user.username, 'admin')
+      .then(function(flag) {
+        expect(flag).to.eql(true);
+        done();


remove done

superkhau · 2017-01-30T21:01:33Z

test/role.test.js

+    it('should support ACL.isMappedToRole() returning a promise', function(done) {
+      ACL.isMappedToRole(ACL.USER, user.username, 'admin')
+      .then(function(flag) {
+        expect(flag).to.eql(true);


expect(flag).to.be.true();

superkhau · 2017-01-30T21:01:43Z

test/role.test.js

+        expect(flag).to.eql(true);
+        done();
+      })
+      .catch(done);


remove catch

ebarault · 2017-01-30T22:04:20Z

@superkhau : ready for review + rebased

superkhau · 2017-01-31T05:20:27Z

@ebarault Thanks for the contribution. LGTM. @bajtos For final review/input though.

bajtos

The code changes LGTM, thank you @ebarault for the contribution!

I changed the commit message to follow our style, see http://loopback.io/doc/en/contrib/git-commit-messages.html.

bajtos · 2017-01-31T11:54:35Z

@superkhau thank you for the detailed review!

bajtos · 2017-01-31T12:35:34Z

@slnode test please

bajtos · 2017-01-31T13:30:08Z

@slnode test please

bajtos · 2017-01-31T13:54:56Z

Landed, thank you for the contribution!

And sorry for the comment spam, our CI was acting up a bit today 😕

ebarault mentioned this pull request Jan 30, 2017

Promise-ify built-in Role model #3146

Merged

2 tasks

superkhau reviewed Jan 30, 2017

View reviewed changes

common/models/acl.js Outdated

if (err) return cb(err);

if (err) {

return cb(err);

}

Copy link

Contributor

superkhau Jan 30, 2017

Choose a reason for hiding this comment

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

this change is not required

superkhau reviewed Jan 30, 2017

View reviewed changes

common/models/acl.js Outdated

if (err || !role) return cb(err, role);

if (err || !role) {

return cb(err, role);

}

Copy link

Contributor

superkhau Jan 30, 2017

Choose a reason for hiding this comment

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

ditto

superkhau reviewed Jan 30, 2017

View reviewed changes

superkhau suggested changes Jan 30, 2017

View reviewed changes

ebarault force-pushed the feature/promisify-acl branch 4 times, most recently from 49ed5bb to 31a9591 Compare January 30, 2017 21:55

ebarault force-pushed the feature/promisify-acl branch from 31a9591 to 57e844f Compare January 30, 2017 22:09

superkhau approved these changes Jan 31, 2017

View reviewed changes

superkhau requested a review from bajtos January 31, 2017 05:18

superkhau added the feature label Jan 31, 2017

superkhau added this to the Sprint 28 milestone Jan 31, 2017

bajtos force-pushed the feature/promisify-acl branch from 57e844f to 7e7e393 Compare January 31, 2017 11:51

bajtos approved these changes Jan 31, 2017

View reviewed changes

Add promise support to built-in model ACL

a63fad4

bajtos force-pushed the feature/promisify-acl branch from 7e7e393 to a63fad4 Compare January 31, 2017 13:09

bajtos changed the title ~~adding promise support in built-in model ACL~~ Add promise support to built-in model ACL Jan 31, 2017

bajtos merged commit c099d8c into strongloop:master Jan 31, 2017

ebarault mentioned this pull request Jan 31, 2017

Enable multiple user models inheriting from base class User #2971

Merged

2 tasks

bajtos added the loopback-auth label Feb 15, 2017

Conversation

ebarault commented Jan 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Checklist

Uh oh!

slnode commented Jan 30, 2017

Uh oh!

slnode commented Jan 30, 2017

Uh oh!

slnode commented Jan 30, 2017

Uh oh!

ebarault commented Jan 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

superkhau Jan 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebarault Jan 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebarault commented Jan 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

superkhau commented Jan 31, 2017

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

bajtos commented Jan 31, 2017

Uh oh!

bajtos commented Jan 31, 2017

Uh oh!

bajtos commented Jan 31, 2017

Uh oh!

bajtos commented Jan 31, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ebarault commented Jan 30, 2017 •

edited

Loading

superkhau Jan 30, 2017 •

edited

Loading

ebarault Jan 30, 2017 •

edited

Loading

ebarault commented Jan 30, 2017 •

edited

Loading