Skip to content

Ability to return 401 and 403 response codes #301#306

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bajtos merged 1 commit into from
Jun 14, 2014
Merged

Ability to return 401 and 403 response codes #301 #306

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions lib/application.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -295,13 +295,20 @@ app.dataSources = app.datasources = {};

app.enableAuth = function() {
var remotes = this.remotes();
var app = this;

remotes.before('**', function(ctx, next, method) {
var req = ctx.req;
var Model = method.ctor;
var modelInstance = ctx.instance;
var modelId = modelInstance && modelInstance.id || req.param('id');

var modelSettings = Model.settings || {};
var errStatusCode = modelSettings.aclErrorStatus || app.get('aclErrorStatus') || 401;
if(!req.accessToken){
errStatusCode = 401;
}

if(Model.checkAccess) {
// Pause the request before checking access
// See https://github.com/strongloop/loopback-storage-service/issues/7
Expand All@@ -319,8 +326,15 @@ app.enableAuth = function() {
} else if(allowed) {
next();
} else {
var e = new Error('Access Denied');
e.statusCode = 401;

var messages = {
403:'Access Denied',
404: ('could not find a model with id ' + modelId),

@bajtosbajtosJun 6, 2014

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How About 'Not Found: ' + Model.modelName + ' #' + modelId to produce a message like this:

Not Found: User #24

Although I am not sure how much benefit there is in including that information, since both model name and model id is usually included in the request URL.

@karlmikkokarlmikkoJun 6, 2014

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

All of these below are different 404 messages. I matched the 404 that would follow if the ACL was allowed. That way someone familiar with loopback would not know it failed at ACL with a 404 and by default know there was a resource there, by way of a different 404 message.

/loopback/lib/models/model.js:57
/loopback/lib/models/data-model.js:69
/loopback/lib/middleware/urlNotFound.js:15
/loopback-datasource-juggler/lib/dao.js:378
/strong-remoting/lib/rest-adapter.js:204
/strong-remoting/lib/rest-adapter.js:214

@bajtosbajtosJun 9, 2014

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Makes sense. Would you mind extracting this code into a shared methods, so that the error message remains the same in the future too? E.g.

Model.notFoundError=function(id){varerr=newError('could not find a model with id '+id);err.statusCode=404;returnerr;}

@karlmikkokarlmikkoJun 10, 2014

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@bajtos I think it would be worth raising this as a separate issue. As I believe errors should be in a separate module to be shared by all loopback modules. This would keep all errors consistent across loopback modules.

@bajtosbajtosJun 12, 2014

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fair enough.

@karlmikkokarlmikkoJun 17, 2014

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@bajtos how do we go about making this new module as it will need to be on strongloop's github. I am happy to start work on this error module, just need to know where to get started from.

@bajtosbajtosJun 25, 2014

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't understand what are the benefits of moving this code to a new module, as opposed to moving the code to a new file like loopback:lib/error, or even implementing the new methods in loopback:lib/models/model.js?

@karlmikkokarlmikkoJun 25, 2014

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

As you can see in one of my above comments a 404 Error is create in at least 3 different loopback modules.

  1. loopback
  2. loopback-datasource-juggler
  3. strong-remoting

I thought if all the errors were in a separate module, it would allow errors to be consistent across modules.

@bajtosbajtosJun 27, 2014

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see. strong-remoting is not aware of models, thus it probably should not share the same error messages with loopback & loopback-datasource-juggler.

AFAIK loopback Model class is inheriting from juggler's ModelBaseClass (source), perhaps the error methods can be defined there?

@karlmikkokarlmikkoJul 15, 2014

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

A problem arises as strong-remoting passes through the errors from loopback and the loopback-data-source-juggler through to the end user. I think in these cases the errors should be the same.

On a side note, the other day I was working on some client side code (Javascript not angular) and had the need to replicate a 404 error that loopback would produce. I had to reproduce this error in my code - I would have loved to just import an errors module.

401:'Authorization Required'
};

var e = new Error(messages[errStatusCode] || messages[403]);
e.statusCode = errStatusCode;
next(e);
}
}
Expand Down
52 changes: 47 additions & 5 deletions test/access-token.test.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -81,13 +81,38 @@ describe('app.enableAuth()', function() {

beforeEach(createTestingToken);

it('should prevent remote method calls if the accessToken doesnt have access', function (done) {
it('prevents remote call with 401 status on denied ACL', function (done) {
createTestAppAndRequest(this.token, done)
.del('/tests/123')
.expect(401)
.set('authorization', this.token.id)
.end(done);
});

it('prevent remote call with app setting status on denied ACL', function (done) {
createTestAppAndRequest(this.token, {app:{aclErrorStatus:403}}, done)
.del('/tests/123')
.expect(403)
.set('authorization', this.token.id)
.end(done);
});

it('prevent remote call with app setting status on denied ACL', function (done) {
createTestAppAndRequest(this.token, {model:{aclErrorStatus:404}}, done)
.del('/tests/123')
.expect(404)
.set('authorization', this.token.id)
.end(done);
});

@bajtosbajtosJun 9, 2014

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's not necessary to duplicate the same test three time, it's enough to check one non-default value.


it('prevent remote call if the accessToken is missing and required', function (done) {
createTestAppAndRequest(null, done)
.del('/tests/123')
.expect(401)
.set('authorization', null)
.end(done);
});

});

function createTestingToken(done) {
Expand All@@ -99,12 +124,19 @@ function createTestingToken(done) {
});
}

function createTestAppAndRequest(testToken, done) {
var app = createTestApp(testToken, done);
function createTestAppAndRequest(testToken, settings, done) {
var app = createTestApp(testToken, settings, done);
return request(app);
}

function createTestApp(testToken, done) {
function createTestApp(testToken, settings, done) {
done = arguments[arguments.length-1];
if(settings == done) settings = {};
settings = settings || {};

var appSettings = settings.app || {};
var modelSettings = settings.model || {};

var app = loopback();

app.use(loopback.cookieParser('secret'));
Expand All@@ -125,7 +157,11 @@ function createTestApp(testToken, done) {
app.use(loopback.rest());
app.enableAuth();

var TestModel = loopback.Model.extend('test', {}, {
Object.keys(appSettings).forEach(function(key){
app.set(key, appSettings[key]);
});

var modelOptions = {
acls: [
{
principalType: "ROLE",
Expand All@@ -135,8 +171,14 @@ function createTestApp(testToken, done) {
property: 'removeById'
}
]
};

Object.keys(modelSettings).forEach(function(key){
modelOptions[key] = modelSettings[key];
});

var TestModel = loopback.Model.extend('test', {}, modelOptions);

TestModel.attachTo(loopback.memory());
app.model(TestModel);

Expand Down