Fix 401 error handling, add tests #507

dmitrizagidulin · 2017-05-30T19:58:25Z

No description provided.

dmitrizagidulin · 2017-05-31T14:29:32Z

cc @dan-f for review

dan-f · 2017-05-31T15:15:40Z

test/integration/errors-oidc.js

+
+  describe('Unauthenticated requests to protected resources', () => {
+    describe('accepting json only', () => {
+      it('should return 401 Unauthorized with www-auth header', done => {


FWIW if this is mocha, you can just return promises from the test rather than explicitly call done.

True - this version of supertest doesn't support promises I think, hence the explicit done(). I was planning on upgrading to the latest soon anyway, though.

dan-f · 2017-05-31T15:22:18Z

test/integration/errors-oidc.js

+          .set('Accept', 'text/html')
+          .expect('Content-Type', 'text/html; charset=utf-8')
+          .expect(res => {
+            expect(res.text).to.match(/<meta http-equiv="refresh"/)


Where are we expected to be redirected to? I think that belongs with the test.

dan-f · 2017-05-31T15:25:36Z

test/integration/errors-oidc.js

+    })
+
+    describe('with an expired bearer token', () => {
+      const expiredToken = 'eyJhbGciOiJSUzI1NiIsImtpZCI6ImxOWk9CLURQRTFrIn0.eyJpc3MiOiJodHRwczovL2xvY2FsaG9zdDozNDU3Iiwic3ViIjoiaHR0cHM6Ly9sb2NhbGhvc3Q6MzQ1Ny9wcm9maWxlL2NhcmQjbWUiLCJhdWQiOiJodHRwczovL2xvY2FsaG9zdDozNDU3IiwiZXhwIjoxNDk2MjM5ODY1LCJpYXQiOjE0OTYyMzk4NjUsImp0aSI6IjliN2MwNGQyNDY3MjQ1ZWEiLCJub25jZSI6IklXaUpMVFNZUmktVklSSlhjejVGdU9CQTFZR1lZNjFnRGRlX2JnTEVPMDAiLCJhdF9oYXNoIjoiRFpES3I0RU1xTGE1Q0x1elV1WW9pdyJ9.uBTLy_wG5rr4kxM0hjXwIC-NwGYrGiiiY9IdOk5hEjLj2ECc767RU7iZ5vZa0pSrGy0V2Y3BiZ7lnYIA7N4YUAuS077g_4zavoFWyu9xeq6h70R8yfgFUNPo91PGpODC9hgiNbEv2dPBzTYYHqf7D6_-3HGnnDwiX7TjWLTkPLRvPLTcsCUl7G7y-EedjcVRk3Jyv8TNSoBMeTwOR3ewuzNostmCjUuLsr73YpVid6HE55BBqgSCDCNtS-I7nYmO_lRqIWJCydjdStSMJgxzSpASvoeCJ_lwZF6FXmZOQNNhmstw69fU85J1_QsS78cRa76-SnJJp6JCWHFBUAolPQ'


I think this is too opaque. The code should explicitly generate an expired token for the purposes of a test like this.

dan-f · 2017-05-31T15:29:00Z

lib/handlers/error-pages.js

-    debug('401 error - redirect to Select Provider')
-    res.status(err.status)
-    redirectToLogin(req, res, next)
+  if (ldp.auth === 'oidc') {


Is there a way of including this in the OIDC code rather than in the global error handler?

WWW-Authenticate is meant to be used for all 401s, so it should be here for WebID-TLS as well.

dan-f · 2017-05-31T15:29:41Z

lib/handlers/error-pages.js

-    res.status(err.status)
-    redirectToLogin(req, res, next)
+  if (ldp.auth === 'oidc') {
+    let statusCode = err.status || err.statusCode || 500


I think we've talked about this before, but I can't think of an instance in which, in application code, a 500 error code should be explicitly set.

I remember. The reason I added it here is that this is the last place to catch it (if some other module forgot to specify a status code for their error), otherwise it gets a '0 is an invalid status code' error. I can take it out tho.

Well I guess 500 is better than 0, hah.

dan-f · 2017-05-31T15:30:57Z

lib/handlers/error-pages.js

+      .map(key => `${key}="${errorParams[key]}"`)
+      .join(', ')
+
+    res.set('WWW-Authenticate', 'Bearer ' + challengeParams)


This header is meant to be used with 401s, but it's being used here regardless of the error code.

Thanks. (forgot to move it.)

dan-f · 2017-05-31T15:32:12Z

lib/handlers/error-pages.js

+
+    res.set('WWW-Authenticate', 'Bearer ' + challengeParams)
+
+    if (statusCode === 401 && req.accepts('text/html')) {


And if status code is anything else? what does the return at the end of this function actually do?

dmitrizagidulin · 2017-06-02T18:24:33Z

@dan-f I rewrote the error-pages handler to reflect feedback, and added tests.

dan-f

Looking better. I still have a few points of review

dan-f · 2017-06-05T14:58:48Z

lib/handlers/error-pages.js

-    return
-  }
+  let locals = req.app.locals
+  let authMethod = locals.authMethod


Where does locals.authMethod come from? Does the client specify the auth method, or is it in the server configuration?

Server configuration.

dan-f · 2017-06-05T15:03:18Z

test/integration/errors-oidc.js

+  })
+
+  describe('Authenticated responses to protected resources', () => {
+    describe('with an empty bearer token', () => {


Is there anything in the error telling the client that they messed up the Authorization header? I'd expect a 400 in this case, since I think it could count as malformed syntax.

The Bearer Token spec dictates treating an empty bearer token as a plain 401 Unauthorized (with no error). And a malformed token as a 401 with a invalid_token error (which is tested below).

Huh. According to section 3.1 of the spec, a missing bearer token seems to satisfy the invalid_request/400 scenario:

invalid_request
The request is missing a required parameter, includes an
unsupported parameter or parameter value, repeats the same
parameter, uses more than one method for including an access
token, or is otherwise malformed. The resource server SHOULD
respond with the HTTP 400 (Bad Request) status code.

Not quite - the bearer token is not a required parameter.

The bearer token in bearer token authentication is not a required parameter?

@dan-f Changed to return an invalid_token / Empty access token error instead.

Still needs to be 400, not 401.

I completely disagree with your interpretation, but since this seems to be an important issue for you, I will change it to a 400.

invalid_request
The request is missing a required parameter, includes an
unsupported parameter or parameter value, repeats the same
parameter, uses more than one method for including an access
token, or is otherwise malformed. The resource server SHOULD
respond with the HTTP 400 (Bad Request) status code.

@dan-f now returning a 400 on an empty bearer token header.

dan-f · 2017-06-05T15:07:45Z

lib/handlers/error-pages.js

-    res.header('Content-Type', 'text/html')
-    res.send(text)
+      res.status(statusCode)
+      res.header('Content-Type', 'text/html')


This should really obey the Accept header.

That would be nice, agreed. I've opened issue #508 to implement support for more error formats (other than text/html) based on the Accept header.

dmitrizagidulin · 2017-06-06T15:09:58Z

@dan-f any other issues on these 401 tests, or can I merge this?

* Add WWW-Authenticate to exposed CORS headers * Fix 401 error handling, add tests * Add a test for expired token error * Update to supertest 3.0.0 * Convert 401 tests to Promise interface * Rewrite error-pages handler, add tests * Change 401 error logic on empty bearer token * Return a 400 error on empty bearer token header

Add WWW-Authenticate to exposed CORS headers

8d70903

dmitrizagidulin force-pushed the www-auth-header branch from e1d967d to 63d737b Compare May 30, 2017 20:04

Fix 401 error handling, add tests

42095fc

dmitrizagidulin force-pushed the www-auth-header branch from 63d737b to 42095fc Compare May 30, 2017 20:26

Add a test for expired token error

e71b0b8

dan-f reviewed May 31, 2017

View reviewed changes

dmitrizagidulin added 3 commits June 1, 2017 10:30

Update to supertest 3.0.0

9650bed

Convert 401 tests to Promise interface

3fde14f

Rewrite error-pages handler, add tests

2cb4c58

dmitrizagidulin force-pushed the www-auth-header branch from 61c7b97 to 2cb4c58 Compare June 2, 2017 18:13

dan-f reviewed Jun 5, 2017

View reviewed changes

Change 401 error logic on empty bearer token

670f319

Return a 400 error on empty bearer token header

9c915e9

dmitrizagidulin merged commit 2af40a0 into dz_oidc Jun 7, 2017

dmitrizagidulin deleted the www-auth-header branch June 7, 2017 15:21


		res.set('WWW-Authenticate', 'Bearer ' + challengeParams)

		if (statusCode === 401 && req.accepts('text/html')) {

Fix 401 error handling, add tests #507

Fix 401 error handling, add tests #507

Uh oh!

Conversation

dmitrizagidulin commented May 30, 2017

Uh oh!

dmitrizagidulin commented May 31, 2017

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!

dmitrizagidulin commented Jun 2, 2017

Uh oh!

dan-f 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!

dan-f Jun 6, 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!

dmitrizagidulin commented Jun 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dan-f Jun 6, 2017 •

edited

Loading