[Issue #70] Allow setting the list of allowed permissions for a client by ferjm · Pull Request #71 · mozilla-sensorweb/sensorweb-server

ferjm · 2017-01-23T18:56:00Z

This will allow us to set a list of permissions a client is allowed to ask when requesting an auth token.

coveralls · 2017-01-23T18:58:15Z

Coverage increased (+1.1%) to 86.732% when pulling d791ffa on ferjm:client.scopes into 47530b7 on mozilla-sensorweb:master.

ferjm · 2017-01-23T18:59:34Z

r? @julienw

julienw

mostly nits
main question is: why should the permissions be in the config ?

julienw · 2017-01-24T14:35:14Z

src/models/clients.js

+      associate: db => {
+        Client.belongsToMany(db.Permissions, { through: 'ClientPermissions' });
+      }
+    }


quite a good idea to have it here instead of defining it below, so that it's closer to the definition.

julienw · 2017-01-24T14:40:02Z

src/models/db.js

    state = READY;
+
+    // Load default permissions.
+    const permissions = config.get('permissions').map(permission => {


should this really belong in config ?

I'm not sure what you have in mind for the future. How will the permissions be required ?

I just though the config would be a good place cause it allows overriding the list of default permissions (for future testing purposes for example) and it provides an easy way to access the list (config.get('permissions').

I don't have a strong about this, so we could use an independent json file if you think is more appropriate.

Yeah I think it's better; so that we can also ship it in git.

Alternatively you can provide the default list in the default property for this, but I find it's clumsy.

That's what we are currently doing :) https://github.com/mozilla-sensorweb/sensorweb-server/pull/71/files/d791ffafbd6b3cbc898c242f35b9a2063c513768#diff-9d9a6cd82f41984872a66a3ab0d440c4R100

Ah yes, I missed it. I still find it clumsy but up to you :) maybe just file an issue if you prefer to move faster.

I'll file a follow-up.

julienw · 2017-01-24T14:45:56Z

src/models/permissions.js

+    timestamps: false,
+    classMethods: {
+      associate: db => {
+        Permission.belongsToMany(db.Clients, { through: 'ClientPermissions' });


nit: do we need the bidierectional association ? Maybe we only need it in Clients.

A permission can be applied to multiple clients and a client can have multiple permissions. And in the future we may want to be able to query all clients with a specific permission. But maybe the single direction is enough for this.

yeah, I'm fine with belongsToMany, but maybe it's not needed to be bidirectional (I mean: both here and in the definition for Clients). Adding it here will add "addClient" and such methods to Permission, and I don't think it's useful. I think only addPermission and such in Clients are useful.

I just tried using a single association here and it does not work for what we need. We need to be able to assign the same permission to multiple clients, so we need a n:m relation. With a Client.hasMany(Permissions) (1:n relation) we get a Permissions table with a Client's foreign key, which is not enough for our needs. For example, with this kind of relation, trying to set the same permission (admin) to two different clients ends up with this result:

[{ "key": "acb91322e69ed852", "name": "anotherClientName", "authRedirectUrls": [ "http://domain.org" ], "authFailureRedirectUrls": [ "http://domain.org" ], "permissions": [ "admin" ] }, { "key": "f8913a3a75deffdf", "name": "clientName", "authRedirectUrls": ["http://domain.org"], "authFailureRedirectUrls": ["http://domain.org"], "permissions": [] } ]

As you can see, the permissions list of the second client is empty.

Sorry if I was not clear. I totally agree with the belongsToMany relation.

What I mean is: I think it's enough to define it in Clients (https://github.com/mozilla-sensorweb/sensorweb-server/pull/71/files#diff-4727579a8668deee0b4103f2a0aa458bR48) and that it's (maybe) not useful to define it here as well.

The relation is already effective with the first command (I tried it myself -- I correctly get the relation table, and tests are passing). The second command only adds methods like addClients to a Permission object, or maybe find all clients that has a permission. I don't know if that's useful, but you know better.

That's all I'm saying here: asking if the bidirectional relation is useful :) Again, I agree with you thinking that belongsToMany is the right relation type.

Ok! Got it :) Sorry I wasn't understanding you properly. I'll remove the bidirectional association.

julienw · 2017-01-24T14:55:53Z

src/routes/clients.js

+          return;
+        }
+        return client.addPermissions(req.body.permissions, { transaction });
+      });


maybe we can simplify with a create with include, see http://docs.sequelizejs.com/en/latest/docs/associations/#creating-elements-of-a-hasmany-or-belongstomany-association
so that we don't need to be explicit with the transaction

This was my first approach, but somehow I failed to apply it... so if you don't mind, I'll stick with the current approach.

julienw · 2017-01-24T14:59:12Z

src/routes/clients.js

  db().then(models => {
-    models.Clients.create(req.body).then(client => {
+    let client;
+    models.sequelize.transaction(transaction => {


nit: should we return the result of transaction ? Currently I think we return the 201 before the data is actually written in the DB.

The transaction is not committed/rolled back until the promise chain returned within the models.sequelize.transaction callback resolves/rejects.

I know, but the function we're in still returns before this, and the next promise in the chain (the one sending the response) runs -- all this possibly before the transaction is commited.

Hmm... that's not how I understood the doc. IIUC line 94 is not executed until the Promise returned at line 86 resolves.

you need to return at line 85 too :) There is nothing to do with sequelize, this is just how promises work !

Maybe I mix the indentations, let me look again.

OK, I was confused with indentation because I thought the response was done on the upper chain. That said, I think it would be better at this location ;)
I mean:

db().then(models => { return transaction(transaction => { return create().then(() => addPermissions()) }); }).then(() => res.status(XXX).send()) .catch(...)

Ok, fair enough.

julienw · 2017-01-24T15:20:45Z

src/routes/clients.js


+const normalizeClient = client => {
+  if (client.Permissions) {
+    client.dataValues.permissions = client.Permissions.map(


Is it really needed to use dataValues here ? Can't you just assign to client.permissions ?

Yes, unfortunately we need to modify dataValues. Modifying client.permissions directly didn't work for me.

julienw · 2017-01-24T15:26:52Z

test/test_clients_api.js

+    return db().then(models => {
+      models.Clients.destroy({ where: {} });
+    });
+  });


You need to return destroy because otherwise the beforeEach finishes before the destroy is done.

using co-mocha you can do it like this:

beforeEach(function*() { const { Clients } = yield db(); yield Clients.destroy({ where: {} }); });

julienw · 2017-01-24T15:33:13Z

test/test_clients_api.js

+              res.status.should.be.equal(401);
+              res.body.code.should.be.equal(401);
+              res.body.errno.should.be.equal(errnos[ERRNO_UNAUTHORIZED]);
+              res.body.error.should.be.equal(errors[UNAUTHORIZED]);


can you use postError ?

We add the auth header within postError, adding yet another parameter to postError to remove the auth header for a single test didn't seem worthy... But it's not a big deal anyway.

julienw · 2017-01-24T15:35:15Z

test/test_clients_api.js

+    };

-    it('should respond 400 BadRequest if name param is missing', done => {
+    const postError = (done, code, error, errno, body) => {


can you use an object as parameter instead of the very long parameter list ?

or maybe (done, body, { code, error, errno }) so that done, body is the same as for postSuccess

julienw · 2017-01-24T15:39:09Z

test/test_clients_api.js

+
  describe('POST ' + endpointPrefix + '/clients', () => {
-    it('should respond 401 Unauthorized if there is no auth header', done => {
+    const postSuccess = (done, body) => {


what about postSuccess -> postAndCheckSuccess (same for postError) ?

coveralls · 2017-01-25T17:23:11Z

Coverage increased (+1.1%) to 86.797% when pulling f25d22f on ferjm:client.scopes into 17797c2 on mozilla-sensorweb:master.

ferjm · 2017-01-25T17:23:44Z

r? @julienw

julienw

The bidirectional relation comment is a nit -- the code is working like this. I still thing it's better with only the one-directional belongsToMany relation, but up to you :)

Also 1 very small nit in the test, and 1 nit in how you send back the client to the client (pun not intended).

julienw · 2017-01-26T10:34:40Z

test/test_clients_api.js

    }].forEach(test => {
-      it('should respond 400 BadRequest if ' + test.reason, done => {
-        postError(done, 400, BAD_REQUEST, test.errno, test.body);
+      it('should respond ' + test.code + ' if ' + test.reason, done => {


nit: you can use template strings

julienw · 2017-01-26T10:34:56Z

src/routes/clients.js

        if (!req.body.permissions) {
          return;
        }
        return client.addPermissions(req.body.permissions, { transaction });


you could do:

return client.addPermissions(req.body.permissions, { transaction }) .then(() => client);

and then below:

.then(client => { res.status(201).send(client); });

So that you don't need to have the client temp variable. The object is just passed along with the promise.

…sions for a client

coveralls · 2017-01-26T17:06:44Z

Coverage increased (+1.09%) to 86.765% when pulling a130ff4 on ferjm:client.scopes into 17797c2 on mozilla-sensorweb:master.

ferjm requested a review from julienw January 23, 2017 18:56

julienw reviewed Jan 24, 2017

View reviewed changes

ferjm mentioned this pull request Jan 25, 2017

Move permissions out of config #74

Open

ferjm force-pushed the client.scopes branch from d791ffa to f25d22f Compare January 25, 2017 17:21

ferjm requested a review from julienw January 25, 2017 17:23

julienw approved these changes Jan 26, 2017

View reviewed changes

[Issue mozilla-sensorweb#70] Allow setting the list of allowed permis…

a130ff4

…sions for a client

ferjm force-pushed the client.scopes branch from f25d22f to a130ff4 Compare January 26, 2017 17:04

ferjm merged commit 7cf93a7 into mozilla-sensorweb:master Jan 26, 2017

ferjm deleted the client.scopes branch January 26, 2017 17:10

ferjm mentioned this pull request Jan 27, 2017

Allow setting the list of allowed permissions for a client #70

Closed

Conversation

ferjm commented Jan 23, 2017

Uh oh!

coveralls commented Jan 23, 2017

Uh oh!

ferjm commented Jan 23, 2017

Uh oh!

julienw left a comment

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!

ferjm Jan 24, 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!

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!

coveralls commented Jan 25, 2017

Uh oh!

ferjm Jan 24, 2017 •

edited

Loading