[Issue #42] Allow a 'redirect_url' parameter for client registrations by ferjm · Pull Request #43 · mozilla-sensorweb/sensorweb-server

ferjm · 2017-01-09T18:11:39Z

No description provided.

coveralls · 2017-01-09T18:13:15Z

Coverage increased (+0.5%) to 86.066% when pulling 522822a on ferjm:redirect_url into 7c81079 on mozilla-sensorweb:master.

coveralls · 2017-01-09T18:23:31Z

Coverage increased (+0.5%) to 86.066% when pulling dc2a1b4 on ferjm:redirect_url into 7c81079 on mozilla-sensorweb:master.

julienw

Some nits, some questions, some remarks for later.

julienw · 2017-01-10T14:09:59Z

doc/API.md

nit: looks like an indentation mismatch; I don't have any preference with/without, up to you :)

julienw · 2017-01-10T14:11:01Z

src/errors.js

+  ERRNO_UNAUTHORIZED                    : 401,
+  ERRNO_FORBIDDEN                       : 403,
+  ERRNO_RESOURCE_NOT_FOUND              : 404,
+  ERRNO_INTERNAL_ERROR                  : 500


not for this patch, but I'm not a big fan of this indentation mechanism. As a result we need to change all lines when we really need to add 1 line...

Ok, fair enough

julienw · 2017-01-10T14:11:45Z

src/models/clients.js

nit: tell me what you think, I would use "authRedirectUrl" to make it clear, when looking at the database, that this is for the authentication process.

I realize we also need a authFailureRedirectUrl (that could be null)

Sounds good. I'll change the naming and add the failure redirect url

julienw · 2017-01-10T14:14:56Z

src/routes/clients.js

+        errno = ERRNO_INVALID_API_CLIENT_NAME;
+        break;
+    }
+    return ApiError(res, 400, errno, BAD_REQUEST);


(not for this patch either)

I don't really like how the errors are generated currently. For example I don't get why we need to put the numeric error number when errors.js already have it... It's also difficult to remember the order of the parameters.

Here is how I did it in a previous app:

in the main app configuration file: https://github.com/project-abigail/calendar-server/blob/master/app.js#L46-L68

here are my errors: https://github.com/project-abigail/calendar-server/blob/master/utils/errors.js

here is an example about how to use them: https://github.com/project-abigail/calendar-server/blob/master/routes/users.js#L38-L41

I'll file an issue for this.

julienw · 2017-01-10T14:15:23Z

src/routes/clients.js

same remark about the naming: authRedirectUrl ?

So it's optional ?

Yes, as mentioned above, I think it should be optional.

julienw · 2017-01-10T14:19:19Z

src/routes/clients.js

+        break;
+      case 'name':
+        errno = ERRNO_INVALID_API_CLIENT_NAME;
+        break;


nit: I'd find it more readable to use a default: close to set ERRNO_BAD_REQUEST. (+ eslint will be happier when I'll set it up :) )

julienw · 2017-01-10T14:22:34Z

src/models/clients.js

allowNull: false ? Or a null means we can't login ? :)

Another question: do you think a client would want to configure several redirect urls ? (Facebook permits it)

allowNull: false makes the field mandatory. I was thinking that we should probably allow creating new clients without auth redirection urls. Some of these clients might not want to use the APIs that will require user authentication (I think for now we will have only an API to bookmark sensors requiring user authentication). Some clients might only be interested on pushing sensors data to the server. And for sensors authentication we can't use a redirection url (we can't redirect to a sensor) so the authentication process will be different.

Allowing to configure several redirection urls sounds good.

ok for me !

…ng clients registration

coveralls · 2017-01-11T12:39:48Z

Coverage increased (+0.5%) to 86.111% when pulling 642ebf8 on ferjm:redirect_url into 7c81079 on mozilla-sensorweb:master.

julienw

lgtm, thx !

julienw · 2017-01-17T22:21:45Z

src/server.js

+      const validator = expressValidator.validator;
+      return Array.isArray(value) &&
+             value.length > 0 &&
+             value.every(item => validator.isUrl(item, options));


I realize that it is isURL we want here. I don't know why we didn't see it earlier, but now this fails badly for me.

julienw · 2017-01-17T22:30:30Z

test/test_clients_api.js

-            .send({ name: 'clientName' })
+            .send({ name: 'clientName',
+                    authRedirectUrls: ['http://something.com'],
+                    authFailureRedirectUrls: ['http://something.com'] })


this should have found the issue; I don't know why this doesn't :/ (but locally it actually fails for me...)

ah, this is interesting, this works with a clean node_modules. :)

https://github.com/sequelize/sequelize/blob/2f8930f84837722f5a6e2f288a3a20dcaf247d4e/lib/utils/validator-extras.js#L18-L20
oooh bad bad bad sequelize monkey-patching validatorjs, and so the behavior seems to depend on the order of something...

Ugh... thanks for finding the issue.

ferjm requested a review from julienw January 9, 2017 18:11

ferjm force-pushed the redirect_url branch from 522822a to dc2a1b4 Compare January 9, 2017 18:21

julienw reviewed Jan 10, 2017

View reviewed changes

[Issue mozilla-sensorweb#42] Allow setting auth redirection urls duri…

642ebf8

…ng clients registration

ferjm force-pushed the redirect_url branch from dc2a1b4 to 642ebf8 Compare January 11, 2017 12:38

ferjm mentioned this pull request Jan 11, 2017

Change the way API errors are generated #44

Open

ferjm requested a review from julienw January 11, 2017 16:26

julienw approved these changes Jan 11, 2017

View reviewed changes

ferjm merged commit 71b32ca into mozilla-sensorweb:master Jan 11, 2017

ferjm deleted the redirect_url branch January 11, 2017 20:01

ferjm mentioned this pull request Jan 12, 2017

Require a 'redirect_url' parameter for client registrations #42

Closed

julienw reviewed Jan 17, 2017

View reviewed changes

Conversation

ferjm commented Jan 9, 2017

Uh oh!

coveralls commented Jan 9, 2017

Uh oh!

coveralls commented Jan 9, 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!

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 11, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments