Added customizable authentication failure HTTP code and location header #23

ProgramCpp · 2017-09-20T16:09:32Z

Fix for issue #20
Can customize authentication failure HTTP response code and location header for the routes as below
app.get("/secure/customloggedin",cansec.unauthenticated({code:302,location:"/login"}),cansec.restrictToLoggedIn,send200);

deitch · 2017-09-24T07:46:57Z

This looks pretty good, a few comments on the code itself.

deitch · 2017-09-24T07:47:24Z

lib/authorization.js

@@ -3,14 +3,22 @@ const errors = require('./errors'), rparams = require('./param'), sender = requi
  constants = require('./constants').get(),
  csauth = constants.header.AUTH,
  fields = {}, params = {};
+// initialize [TODO: overridable] global default
+var unauthenticatedcode = constants.httpcodes.UNAUTHENTICATED;


This should be a const instead of a var.

Actually wouldn't this just be easier to do:

const HttpStatus = require('http-status-codes'); // and later const checkLoggedIn = (req, res, next, unauthenticatedResponse = {code:HttpStatus.UNAUTHORIZED}) => {

It is var instead of const since it can be overridden in the init function when an option is provided to override the global default is added.

Should we provide that option in the init or should i still make the requested changes?

It is var instead of const since it can be overridden in the init function

Ah, right. So the logic is something like: when returning unauthenticated error, do local override, else init() override, else 401?

Well, we have no init()-driven override now, so why bother?

Let me add that option then :)

deitch · 2017-09-24T07:48:05Z

lib/authorization.js

+      if (unauthenticatedResponse.location != null) {
+        res.header("location", unauthenticatedResponse.location);
+      }
+      sender(res,unauthenticatedResponse.code,errors.unauthenticated(),unauthenticatedResponse);


How does this work? sender() only accepts 3 args: res, code, body. Here you send 4?

deitch · 2017-09-24T07:49:19Z

lib/authorization.js

@@ -101,6 +109,11 @@ const checkLoggedIn = (req, res, next) => {
      }
    },
    indirect: {
+      unauthenticated: (unauthenticatedResponse) => {


So this is middleware equivalent to restrictToLoggedIn but with a custom per-invocation code and/or location?

Yes! Anything else to add or change?

The only thing that comes to mind is that we should make the naming consistent. If this is restrictToLoggedIn() but with custom handler, then we probably should call it restrictToLoggedInCode() or restrictToLoggedInErrorCode() or something like that.

I couldn't come up with a name for unauthenticated() interface. Please do Suggest!
I think a generic name will be better since it can be used with any interface. Also restrictToLoggedIn() is not explicitly called for other interfaces even though it is checked internally.

I couldn't come up with a name for unauthenticated() interface

If you think about it from a user's perspective, it is middleware that we are calling:

Receive a given path app.get("/some/path",

Pass it through first middleware filter, if it passes...

Pass it through second middleware filter, if it passes...

Pass it through third middleware filter, if it passes...
etc.

Each security middleware is a filter that acts like a gate. So restrictToLoggedIn means, "only let someone through this middleware of they are logged in". We probably could have called it requireLogIn or verifyLoggedIn, but all is the same idea.

In that vein:

Why not just call it restrictToLoggedInWithErrorCode or similar? It is long, but is expressive, and function names are cheap, these aren't mainframes, and it doesn't pass to the browser (even if it did, we would minify it which would replace it with something nice and clear like f :-) )

Why do your test and the README call restrictToLoggedIn after calling unauthenticated or whatever we rename it)? unauthenticated already calls restrictToLoggedIn.

restrictToLoggedInWithErrorCode sounds good.

I am seeing unauthenticated as a gate before reaching one of restrictTo* gates. So, I was thinking unauthenticated can be used along with all the other filters. i.e, to expect custom code for all the restrictions. Earlier, user could pick one of the filters without explicitly using restrictToLoggedIn. Now the user will have inconsistent experience. i.e, user will not use restrictToLoggedIn for the default response code and will use restrictToLoggedInWithErrorCode for the custom code.

OPTION 1:
for default response code,

app.get("/secure/loggedin",cansec.restrictToLoggedIn,send200); app.get("/secure/user/:user",cansec.restrictToSelf,send200); app.get("/secure/roles/admin",cansec.restrictToRoles("admin"),send200);

for custom response code,

app.get("/secure/loggedin", cansec.unauthenticated({code:302,location:"/login"}), cansec.restrictToLoggedIn,send200); app.get("/secure/user/:user", cansec.unauthenticated({code:302,location:"/login"}), cansec.restrictToSelf,send200); app.get("/secure/roles/admin", cansec.unauthenticated({code:302,location:"/login"}),cansec.restrictToRoles("admin"),send200);

user will first set authentication rules and then set authorization rules.

OPTION 2:
for default response code,

app.get("/secure/loggedin",cansec.restrictToLoggedIn,send200); app.get("/secure/user/:user",cansec.restrictToSelf,send200); app.get("/secure/roles/admin",cansec.restrictToRoles("admin"),send200);

User does not use restrictToLoggedIn with restrictToSelf or restrictToRoles

for custom response code,

app.get("/secure/loggedin", cansec.restrictToLoggedInWithErrorCode({code:302,location:"/login"}),send200); // I'm using a **different interface** now. app.get("/secure/user/:user", cansec.restrictToLoggedInWithErrorCode({code:302,location:"/login"}), cansec.restrictToSelf,send200); app.get("/secure/roles/admin", cansec.restrictToLoggedInWithErrorCode({code:302,location:"/login"}),cansec.restrictToRoles("admin"),send200);

User uses restrictToLoggedInWithErrorCode with restrictToSelf or restrictToRoles

If we have restrictToLoggedInWithErrorCode, does the user expect restrictTo*WithErrorCode?

Having said that, I do agree that restrictToLoggedIn is unnecessary when unauthenticated is used. A careful user can choose to use only unauthenticated instead.

In case of OPTION 2 custom code, restrictToLoggedInWithErrorCode and restrictToSelf both check for checkLoggedIn

OK, now I see where you are going. This is why I was confused.

the comments implied that unauthenticated was just setting the error code, but not doing the actual restrictToLoggedIn filtering

the code in the PR had unauthenticated actually calling restrictToLoggedIn

The former would look something like:

app.get("/secure/loggedin", cansec.unauthenticated({code:401, location: '/login'}),cansec.restrictToLogged,send200);

While the latter would look like this:

app.get("/secure/loggedin", cansec.restrictToLoggedWithErrorCode({code:401, location: '/login'}),send200);

Is that It? You were suggesting that unauthenticated just set the unauthorized error code and location for future filters in this request chain?

Sorry for the confusion.
Yes, unauthenticated has to just set unauthorized code for future filters in this request chain.

For the implementation, Is it possible to do that? Do the interfaces share their states for each requests?

Ah, now I get it. And I really like it. Yes, 100%, unauthenticated makes more sense, and restrictToLoggedInErrorCode does not..

But then unauthenticated shouldn't call restrictToLoggedIn. It should just set the code on the req and move on, and restrictToLoggedIn should take it from there.

And given that, yes, now a better name makes sense, I would call it setUnauthenticatedCode() or similar. And we could (eventually) have one globally to set the default.

deitch · 2017-09-24T07:56:12Z

lib/authorization.js


-const checkLoggedIn = (req, res, next) => {
+const checkLoggedIn = (req, res, next, unauthenticatedResponse = {code:unauthenticatedcode}) => {


I think we should clean this up a bit.

const checkLoggedIn = (req, res, next, unauthCode = HttpStatus.UNAUTHORIZED, unauthLocation = null) => { // ... // inside becomes simpler, lines 15-17 go away if (unauthLocation !== null { res.header("location", unauthenticatedResponse.location); } sender(res, unauthCode, errors.unauthenticated()); // ... };

deitch

Overall, I like this. Let's do the following:

HttpStatus instead of our own custom
Simplify checkLoggedIn() (and update its caller)
I wonder about cansec.unauthenticated(). It is identical to restrictToLoggedIn(), can we name it closer?
Bump minor version

ProgramCpp · 2017-09-24T14:31:30Z

Will submit all the changes based on your feedback on my comments.
I wonder what can cansec.unauthenticated() be named as! Please suggest :)

deitch · 2017-09-24T14:36:51Z

Will submit all the changes based on your feedback on my comments

Thanks

please bump minor version number.
You have a great sample in the test, can you put that in the README? Pity for you to do all this work and no one knows it is there to use.

ProgramCpp · 2017-09-25T17:07:27Z

Updated README
Bumped minor version
Used HTTP code from package http-status-codes
Cleaned up checkLoggedIn()

ProgramCpp · 2017-09-28T11:35:54Z

Pending changes:

Rename unauthenticated to setUnauthenticatedCode
setUnauthenticatedCode should only set the custom code in the request and move on!

ProgramCpp · 2017-09-30T04:01:04Z

Pending Changes:

Include custom code in the declarative definitions.
[verb,path,default,[test params,] test condition]
Have an option to set the global default.
Use global default for failures in session management?

deitch · 2017-10-01T05:58:03Z

This is great, thank you. And npm publish-ed as 3.2.0

For the other pending changes, we should do a separate PR.

ProgramCpp added 2 commits September 20, 2017 21:26

added customizable authentication failure HTTP code

7f43f12

Added object validation for checkloggedin

12fcfa0

deitch reviewed Sep 24, 2017

View reviewed changes

deitch requested changes Sep 24, 2017

View reviewed changes

made review comment changes

acfd5ef

renamed unauthenticated to setunauthenticatedcode

b3ed0f3

deitch merged commit b3ed0f3 into deitch:master Oct 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added customizable authentication failure HTTP code and location header #23

Added customizable authentication failure HTTP code and location header #23

ProgramCpp commented Sep 20, 2017

deitch commented Sep 24, 2017

deitch Sep 24, 2017

deitch Sep 24, 2017 •

edited

Loading

ProgramCpp Sep 24, 2017 •

edited

Loading

deitch Sep 24, 2017

ProgramCpp Sep 24, 2017

deitch Sep 24, 2017

ProgramCpp Sep 24, 2017

deitch Sep 24, 2017

deitch Sep 24, 2017

ProgramCpp Sep 24, 2017

deitch Sep 24, 2017

ProgramCpp Sep 25, 2017

deitch Sep 27, 2017

ProgramCpp Sep 28, 2017 •

edited by deitch

Loading

deitch Sep 28, 2017

ProgramCpp Sep 28, 2017 •

edited

Loading

deitch Sep 28, 2017 •

edited

Loading

deitch Sep 24, 2017

deitch left a comment

ProgramCpp commented Sep 24, 2017

deitch commented Sep 24, 2017

ProgramCpp commented Sep 25, 2017 •

edited

Loading

ProgramCpp commented Sep 28, 2017 •

edited

Loading

ProgramCpp commented Sep 30, 2017

deitch commented Oct 1, 2017


		const checkLoggedIn = (req, res, next) => {
		const checkLoggedIn = (req, res, next, unauthenticatedResponse = {code:unauthenticatedcode}) => {

Added customizable authentication failure HTTP code and location header #23

Added customizable authentication failure HTTP code and location header #23

Conversation

ProgramCpp commented Sep 20, 2017

deitch commented Sep 24, 2017

Choose a reason for hiding this comment

deitch Sep 24, 2017 • edited Loading

Choose a reason for hiding this comment

ProgramCpp Sep 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ProgramCpp Sep 28, 2017 • edited by deitch Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ProgramCpp Sep 28, 2017 • edited Loading

Choose a reason for hiding this comment

deitch Sep 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deitch left a comment

Choose a reason for hiding this comment

ProgramCpp commented Sep 24, 2017

deitch commented Sep 24, 2017

ProgramCpp commented Sep 25, 2017 • edited Loading

ProgramCpp commented Sep 28, 2017 • edited Loading

ProgramCpp commented Sep 30, 2017

deitch commented Oct 1, 2017

deitch Sep 24, 2017 •

edited

Loading

ProgramCpp Sep 24, 2017 •

edited

Loading

ProgramCpp Sep 28, 2017 •

edited by deitch

Loading

ProgramCpp Sep 28, 2017 •

edited

Loading

deitch Sep 28, 2017 •

edited

Loading

ProgramCpp commented Sep 25, 2017 •

edited

Loading

ProgramCpp commented Sep 28, 2017 •

edited

Loading