Return 401 if authentication is not provided

Author: sadiqkhoja

lib/http/preprocessors.js

@@ -19,14 +19,11 @@ const { SESSION_COOKIE } = require('./sessions');
 // injects an empty/anonymous auth object into the request context.
 const emptyAuthInjector = ({ Auth }, context) => context.with({ auth: Auth.by(null) });
 
-// if one of (Bearer|Basic|Cookie) credentials are provided in the correct conditions
+// if one of (FieldKey|Bearer|Basic|Cookie) credentials are provided in the correct conditions
 // then authHandler injects the appropriate auth information to the context
 // given the appropriate credentials. if credentials are given but don't match a
 // session or user, aborts the request with a 401.
 //
-// otherwise, nothing is done. n.b. this means you must use the emptyAuthInjector
-// in conjunction with this function!
-//
 // TODO?: repetitive, but deduping it makes it even harder to understand.
 const authHandler = ({ Sessions, Users, Auth }, context) => {
   const authBySessionToken = (token, onFailure = noop) => Sessions.getByBearerToken(token)
@@ -105,14 +102,14 @@ const authHandler = ({ Sessions, Users, Auth }, context) => {
   } else if (context.headers.cookie != null) {
     // fail the request unless we are under HTTPS.
     if ((context.protocol !== 'https') && (context.headers['x-forwarded-proto'] !== 'https'))
-      return;
+      return reject(Problem.user.httpsOnly());
 
     // otherwise get the cookie contents.
     const token = context.cookies[SESSION_COOKIE];
     if (token == null)
-      return;
-    // actually try to authenticate with it. no Problem on failure.
-    const maybeSession = authBySessionToken(token);
+      return reject(Problem.user.authenticationFailed());
+    // actually try to authenticate with it. reject on failure.
+    const maybeSession = authBySessionToken(token, () => reject(Problem.user.authenticationFailed()));
 
     // Following code is for the CSRF protection.
     // Rules:
@@ -128,9 +125,6 @@ const authHandler = ({ Sessions, Users, Auth }, context) => {
 
     // if POST run authentication as usual but we'll have to check CSRF afterwards.
     return maybeSession.then((cxt) => { // we have to use cxt rather than context for the linter
-      // if authentication failed anyway, just do nothing.
-      if ((cxt == null) || cxt.auth.session.isEmpty()) return;
-
       const csrf = urlDecode(cxt.body.__csrf);
       if (csrf.isEmpty() || isBlank(csrf.get()) || (cxt.auth.session.get().csrf !== csrf.get())) {
         return reject(Problem.user.authenticationFailed());
@@ -140,6 +134,9 @@ const authHandler = ({ Sessions, Users, Auth }, context) => {
       // payload expectations.
       return cxt.with({ body: omit([ '__csrf' ], cxt.body) });
     });
+  } else {
+    // Authentication not provided by any means
+    return reject(Problem.user.authenticationFailed());
   }
 };
 

lib/http/service.js

@@ -11,7 +11,6 @@
 // defined elsewhere into an actual Express service. The only thing it needs in
 // order to do this is a valid dependency injection context container.
 const { match } = require('path-to-regexp');
-const { without } = require('ramda');
 
 module.exports = (container) => {
   const service = require('express')();
@@ -64,9 +63,9 @@ module.exports = (container) => {
   const { builder } = require('./endpoint');
   const { emptyAuthInjector, authHandler, queryOptionsHandler, userAgentHandler } = require('./preprocessors');
 
-  const preprocessors = [ emptyAuthInjector, authHandler, queryOptionsHandler, userAgentHandler ];
-  const endpoint = builder(container, preprocessors);
-  const anonymousEndpoint = builder(container, without([authHandler], preprocessors));
+  const preprocessors = [ queryOptionsHandler, userAgentHandler ];
+  const endpoint = builder(container, [authHandler, ...preprocessors]);
+  const anonymousEndpoint = builder(container, [emptyAuthInjector, ...preprocessors]);
 
 
   ////////////////////////////////////////////////////////////////////////////////
@@ -75,10 +74,10 @@ module.exports = (container) => {
   require('../resources/app-users')(service, endpoint);
   require('../resources/odata')(service, endpoint);
   require('../resources/odata-entities')(service, endpoint);
-  require('../resources/forms')(service, endpoint);
-  require('../resources/users')(service, endpoint);
+  require('../resources/forms')(service, endpoint, anonymousEndpoint);
+  require('../resources/users')(service, endpoint, anonymousEndpoint);
   require('../resources/sessions')(service, endpoint, anonymousEndpoint);
-  require('../resources/submissions')(service, endpoint);
+  require('../resources/submissions')(service, endpoint, anonymousEndpoint);
   require('../resources/config')(service, endpoint);
   require('../resources/projects')(service, endpoint);
   require('../resources/roles')(service, endpoint);

lib/resources/forms.js

@@ -58,7 +58,7 @@ const streamAttachment = async (container, attachment, response) => {
   }
 };
 
-module.exports = (service, endpoint) => {
+module.exports = (service, endpoint, anonymousEndpoint) => {
   // This forms list can also be used to get a list of just the soft-deleted forms by adding ?deleted=true
   // TODO: paging.
   service.get('/projects/:projectId/forms', endpoint(({ Forms, Projects }, { auth, params, query, queryOptions }) =>
@@ -418,7 +418,7 @@ module.exports = (service, endpoint) => {
     noargs(Problem.user.notFound)
   );
 
-  service.get('/test/:key/projects/:projectId/forms/:id/draft/formList', endpoint.openRosa(({ Forms, FormAttachments, env }, { params, originalUrl }) =>
+  service.get('/test/:key/projects/:projectId/forms/:id/draft/formList', anonymousEndpoint.openRosa(({ Forms, FormAttachments, env }, { params, originalUrl }) =>
     Forms.getByProjectAndXmlFormId(params.projectId, params.id, false, Form.DraftVersion)
       .then(getOrNotFound)
       .then(ensureDef)
@@ -433,7 +433,7 @@ module.exports = (service, endpoint) => {
             draft: true, forms: [ form.withAux('openRosa', { hasAttachments }) ], basePath: path.resolve(originalUrl, '../../../..'), domain: env.domain
           })))));
 
-  service.get('/test/:key/projects/:projectId/forms/:id/draft/manifest', endpoint.openRosa(({ FormAttachments, Forms, env }, { params, originalUrl }) =>
+  service.get('/test/:key/projects/:projectId/forms/:id/draft/manifest', anonymousEndpoint.openRosa(({ FormAttachments, Forms, env }, { params, originalUrl }) =>
     Forms.getByProjectAndXmlFormId(params.projectId, params.id, false, Form.DraftVersion)
       .then(getOrNotFound)
       .then(ensureDef)
@@ -442,14 +442,14 @@ module.exports = (service, endpoint) => {
         .then((attachments) =>
           formManifest({ attachments, basePath: path.resolve(originalUrl, '..'), domain: env.domain })))));
 
-  service.get('/test/:key/projects/:projectId/forms/:id/draft.xml', endpoint(({ Forms }, { params }) =>
+  service.get('/test/:key/projects/:projectId/forms/:id/draft.xml', anonymousEndpoint(({ Forms }, { params }) =>
     Forms.getByProjectAndXmlFormId(params.projectId, params.id, true, Form.DraftVersion)
       .then(getOrNotFound)
       .then(ensureDef)
       .then(checkFormToken(params.key))
       .then((form) => xml(form.xml))));
 
-  service.get('/test/:key/projects/:projectId/forms/:id/draft/attachments/:name', endpoint((container, { params }, request, response) => {
+  service.get('/test/:key/projects/:projectId/forms/:id/draft/attachments/:name', anonymousEndpoint((container, { params }, request, response) => {
     const { FormAttachments, Forms } = container;
     return Forms.getByProjectAndXmlFormId(params.projectId, params.id, false, Form.DraftVersion)
       .then(getOrNotFound)

lib/resources/submissions.js

@@ -65,12 +65,14 @@ const findMultipart = (files) => {
 };
 const forceAuthFailed = Problem.translate(Problem.user.insufficientRights, Problem.user.authenticationFailed);
 
-module.exports = (service, endpoint) => {
+module.exports = (service, endpoint, anonymousEndpoint) => {
 
   ////////////////////////////////////////////////////////////////////////////////
   // SUBMISSIONS (OPENROSA)
 
   const openRosaSubmission = (path, draft, getForm) => {
+    const endpointByTestPath = path.startsWith('/test/:key') ? anonymousEndpoint : endpoint;
+
     // Nonstandard REST; OpenRosa-specific API.
     // This bit of silliness is to address the fact that the OpenRosa standards
     // specification requires the presence of a HEAD /submission endpoint, but
@@ -81,9 +83,10 @@ module.exports = (service, endpoint) => {
     // there is no body content, and the HTTP spec requires that HEAD should
     // return exactly what GET would, just without a response body, the only thing
     // we can possibly respond with in either case is no body and a 204 code.
-    service.get(path, endpoint.openRosa(({ Projects }, { params }) =>
+    service.get(path, endpointByTestPath.openRosa(({ Projects, Forms }, { params, auth }) =>
       Projects.getById(params.projectId)
         .then(getOrNotFound)
+        .then(() => (params.xmlFormId ? getForm(auth, params, params.xmlFormId, Forms) : resolve()))
         .then(always({ code: 204, body: '' }))));
 
     // Temporary solution to https://github.com/expressjs/multer/issues/1104
@@ -101,7 +104,7 @@ module.exports = (service, endpoint) => {
     };
 
     // Nonstandard REST; OpenRosa-specific API.
-    service.post(path, multipart.any(), multerUtf, multipartErrorHandler, endpoint.openRosa(({ Forms, Submissions, SubmissionAttachments }, { params, files, auth, query, userAgent }) =>
+    service.post(path, multipart.any(), multerUtf, multipartErrorHandler, endpointByTestPath.openRosa(({ Forms, Submissions, SubmissionAttachments }, { params, files, auth, query, userAgent }) =>
       Submission.fromXml(findMultipart(files).buffer)
         .then((partial) => getForm(auth, params, partial.xmlFormId, Forms, partial.def.version)
           .catch(forceAuthFailed)
@@ -267,7 +270,7 @@ module.exports = (service, endpoint) => {
   );
 
   // Create Submission using draftToken
-  service.post(`/test/:key/projects/:projectId/forms/:formId/draft/submissions`, endpoint(({ Forms, Submissions, SubmissionAttachments }, { params, query, userAgent }, request) =>
+  service.post(`/test/:key/projects/:projectId/forms/:formId/draft/submissions`, anonymousEndpoint(({ Forms, Submissions, SubmissionAttachments }, { params, query, userAgent }, request) =>
     Submission.fromXml(request)
       .then((partial) => Forms.getByProjectAndXmlFormId(params.projectId, params.formId, false, Form.DraftVersion)
         .then(getOrNotFound)

lib/resources/users.js

@@ -17,7 +17,7 @@ const { resolve, reject, getOrNotFound } = require('../util/promise');
 const { isPresent } = require('../util/util');
 const oidc = require('../util/oidc');
 
-module.exports = (service, endpoint) => {
+module.exports = (service, endpoint, anonymousEndpoint) => {
 
   // Get a list of user accounts.
   // TODO: paging.
@@ -54,8 +54,11 @@ module.exports = (service, endpoint) => {
             .then((token) => mail(savedUser.email, 'accountCreated', { token })))
           .then(always(savedUser)))));
 
+    const endpointByInvalidateQuery = (cb) => (req, ...rest) =>
+      (req.query.invalidate ? endpoint(cb) : anonymousEndpoint(cb))(req, ...rest);
+
     // TODO/SECURITY: subtle timing attack here.
-    service.post('/users/reset/initiate', endpoint(({ Users, mail }, { auth, body, query }) =>
+    service.post('/users/reset/initiate', endpointByInvalidateQuery(({ Users, mail }, { auth, body, query }) =>
       (!body.email ? Problem.user.missingParameter({ field: 'email' }) : Users.getByEmail(body.email)
         .then((maybeUser) => maybeUser
           .map((user) => ((isTrue(query.invalidate))

test/integration/api/app-users.js

@@ -4,10 +4,10 @@ const testData = require('../../data/xml');
 
 describe('api: /projects/:id/app-users', () => {
   describe('POST', () => {
-    it('should return 403 unless the user is allowed to create', testService((service) =>
+    it('should return 403 if unauthenticated', testService((service) =>
       service.post('/v1/projects/1/app-users')
         .send({ displayName: 'test1' })
-        .expect(403)));
+        .expect(401)));
 
     it('should return the created key', testService((service) =>
       service.login('alice', (asAlice) =>
@@ -63,8 +63,8 @@ describe('api: /projects/:id/app-users', () => {
   });
 
   describe('GET', () => {
-    it('should return 403 unless the user is allowed to list', testService((service) =>
-      service.get('/v1/projects/1/app-users').expect(403)));
+    it('should return 401 if unauthenticated', testService((service) =>
+      service.get('/v1/projects/1/app-users').expect(401)));
 
     it('should return a list of tokens in order with merged data', testService((service) =>
       service.login('alice', (asAlice) =>
@@ -174,11 +174,11 @@ describe('api: /projects/:id/app-users', () => {
   });
 
   describe('/:id DELETE', () => {
-    it('should return 403 unless the user can delete', testService((service) =>
+    it('should return 401 if unauthenticated', testService((service) =>
       service.login('alice', (asAlice) =>
         asAlice.post('/v1/projects/1/app-users').send({ displayName: 'condemned' }).expect(200)
           .then(({ body }) =>
-            service.delete('/v1/projects/1/app-users/' + body.id).expect(403)))));
+            service.delete('/v1/projects/1/app-users/' + body.id).expect(401)))));
 
     it('should delete the token', testService((service) =>
       service.login('alice', (asAlice) =>