Skip to content

Return 401 if authentication is not provided #1488

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Return 401 if authentication is not provided #1488

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
733585b
Return 401 if authentication is not provided
sadiqkhoja May 2, 2025
82217e7
Make /oidc/login anonymous
sadiqkhoja May 2, 2025
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
19 changes: 8 additions & 11 deletions lib/http/preprocessors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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:
Expand All @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is an "empty" session now handled?

Copy link
Member

Choose a reason for hiding this comment

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

I think the callback passed to authBySessionToken() handles that case.


const csrf = urlDecode(cxt.body.__csrf);
if (csrf.isEmpty() || isBlank(csrf.get()) || (cxt.auth.session.get().csrf !== csrf.get())) {
return reject(Problem.user.authenticationFailed());
Expand All @@ -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());
}
};

Expand Down
15 changes: 7 additions & 8 deletions lib/http/service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')();
Expand Down Expand Up @@ -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 ];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const preprocessors = [ queryOptionsHandler, userAgentHandler ];
const commonPreprocessors = [ queryOptionsHandler, userAgentHandler ];

const endpoint = builder(container, [authHandler, ...preprocessors]);
const anonymousEndpoint = builder(container, [emptyAuthInjector, ...preprocessors]);


////////////////////////////////////////////////////////////////////////////////
Expand All @@ -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);
Expand All @@ -90,7 +89,7 @@ module.exports = (container) => {
require('../resources/analytics')(service, endpoint);
require('../resources/datasets')(service, endpoint);
require('../resources/entities')(service, endpoint);
require('../resources/oidc')(service, endpoint);
require('../resources/oidc')(service, endpoint, anonymousEndpoint);
require('../resources/user-preferences')(service, endpoint);

////////////////////////////////////////////////////////////////////////////////
Expand Down
10 changes: 5 additions & 5 deletions lib/resources/forms.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }) =>
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand Down
6 changes: 3 additions & 3 deletions lib/resources/oidc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,10 +100,10 @@ const nextFrom = state => {
if (state) return Buffer.from(state.split(':')[1], 'base64url').toString();
};

module.exports = (service, endpoint) => {
module.exports = (service, __, anonymousEndpoint) => {
if (!isEnabled()) return;

service.get('/oidc/login', endpoint.html(async ({ Sentry }, _, req, res) => {
service.get('/oidc/login', anonymousEndpoint.html(async ({ Sentry }, _, req, res) => {
try {
const client = await getClient();
const code_verifier = generators.codeVerifier(); // eslint-disable-line camelcase
Expand Down Expand Up @@ -135,7 +135,7 @@ module.exports = (service, endpoint) => {
}
}));

service.get('/oidc/callback', endpoint.html(async (container, _, req, res) => {
service.get('/oidc/callback', anonymousEndpoint.html(async (container, _, req, res) => {
try {
const code_verifier = req.cookies[CODE_VERIFIER_COOKIE]; // eslint-disable-line camelcase
const state = req.cookies[STATE_COOKIE]; // eslint-disable-line no-multi-spaces
Expand Down
11 changes: 7 additions & 4 deletions lib/resources/submissions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this code be more intuitive if the endpoint implementation was passed in rather than being derived from path?

e.g.

const openRosaSubmission = (endpoint, path, draft, getForm) => {

or

const openRosaSubmission = (path, { isDraft, requiresAuth }, getForm) => {

Copy link
Member

Choose a reason for hiding this comment

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

I like the idea of making this an explicit argument (either of the suggestions above). 👍


// 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
Expand All @@ -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()))
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not obvious how this change is related to 401 responses; any pointers?

.then(always({ code: 204, body: '' }))));

// Temporary solution to https://github.com/expressjs/multer/issues/1104
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
7 changes: 5 additions & 2 deletions lib/resources/users.js
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

For the GET /users endpoint, could you remove the auth.isAuthenticated check? Since the endpoint uses endpoint, we can be sure that it's authenticated.

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe it's a nice safeguard? It sort of feels redundant to me now.

Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -54,8 +54,11 @@ module.exports = (service, endpoint) => {
.then((token) => mail(savedUser.email, 'accountCreated', { token })))
.then(always(savedUser)))));

const endpointByInvalidateQuery = (cb) => (req, ...rest) =>
Copy link
Member

Choose a reason for hiding this comment

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

endpointByInvalidateQuery and endpointByTestPath above make me think that if/when we update endpoint to accept an options object, we should make the anonymous option take a callback.

Maybe it'd be reasonable to do something like that now? What if anonymousEndpoint accepted a function returning true/false, and then anonymousEndpoint either called authHandler or not based on the result? We could even make the function required, since Ramda F would be easy to pass in.

Just a thought, I'm also happy with the approach here. 👍

(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))
Expand Down
12 changes: 6 additions & 6 deletions test/integration/api/app-users.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Would there be value in a corresponding test case for authenticated users who are unauthorised?

Copy link
Member

Choose a reason for hiding this comment

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

Similar question from me. I'd expect every endpoint that calls auth.canOrReject() to have a test asserting a 403. This test now never reaches the actual endpoint logic: it's just being stopped by the preprocessor, which we test elsewhere.


it('should return the created key', testService((service) =>
service.login('alice', (asAlice) =>
Expand Down Expand Up @@ -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) =>
Expand Down Expand Up @@ -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) =>
Expand Down
Loading