Return 401 if authentication is not provided by sadiqkhoja · Pull Request #1488 · getodk/central-backend

sadiqkhoja · 2025-05-02T05:32:32Z

What has been done to verify that this works as intended?

All tests are passing.

Some tests are updated to reflect the correct requirement.

Why is this the best possible solution? Were any other approaches considered?

It is necessary to distinguish between 401 and 403, previously we were relying on resources to perform authorization even when authentication failed.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

This PR shouldn't be merged until corresponding frontend changes are ready. There are some places where frontend code expects 403, that needs to be updated.

Does this change require updates to the API documentation? If so, please update docs/api.yaml as part of this PR.

No

Before submitting this PR, please make sure you have:

run make test and confirmed all checks still pass OR confirm CircleCI build passes
verified that any code from external sources are properly credited in comments or that everything is internally sourced

alxndrsn · 2025-05-02T07:41:24Z

test/integration/api/assignments.js

      service.login('bob', (asBob) =>
        Promise.all([
-          service.get('/v1/roles/manager').expect(200).then(({ body }) => body.id),
+          asBob.get('/v1/roles/manager').expect(200).then(({ body }) => body.id),


This endpoint does not currently require authentication. How does that expectation changing relate to 401 vs 403 errors?

Those two changes (more 401 errors + requiring auth for /v1/roles) both touch on the theme of anonymity. However, I agree that they're also conceptually distinct. If it's easy to pull the two changes apart, that'd make this PR a little smaller, which sounds nice. I'm also OK with reviewing both changes as part of this PR. I like the idea of having the /v1/roles endpoints check auth.

alxndrsn · 2025-05-02T07:54:00Z

lib/http/preprocessors.js

    // 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;


Where is an "empty" session now handled?

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

Then shouldn't authBySessionToken() be returning the session itself, rather than an Option? And L111 would change from:

-const maybeSession = authBySessionToken(token, () => reject(Problem.user.authenticationFailed())); +const session = authBySessionToken(token, () => reject(Problem.user.authenticationFailed()));

lib/http/service.js

lib/resources/submissions.js

alxndrsn · 2025-05-02T08:10:27Z

lib/resources/submissions.js

+    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()))


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

Besides checking for other errors, calling getForm here performs authorization for this endpoint. Currently if an user calls HEAD/GET /v1/projects/1/forms/restricted-form and project 1 exists then they will always get 200 response, even if they don't have any authorization on the Form or even if Form doesn't exist.

I am open to revert this change as well and create a separate PR

I do think it'd be nice to separate it out if that's not too difficult. I was trying to think through the implications of the change. I don't even remember how this endpoint is used. Is there maybe a reason why it needs to be ultra-performant and shouldn't be doing as many checks? Though I guess if it's checking the project, it should probably check the form as well. 🤔 Anyway, I feel like discussing in a separate PR could be nice!

test/integration/api/app-users.js

alxndrsn · 2025-05-02T08:18:11Z

Does this change require updates to the API documentation? If so, please update docs/api.yaml as part of this PR.

No

Checking one endpoint at random, it looks like some API docs might need updates:

central-backend/docs/api.yaml

Lines 1965 to 1970 in 889d208

    
           403: 
        
             description: Forbidden 
        
             content:             
        
               application/json: 
        
                 schema: 
        
                   $ref: '#/components/schemas/Error403'

matthew-white · 2025-05-02T19:00:25Z

lib/resources/users.js

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.

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

matthew-white · 2025-05-02T19:16:17Z

lib/resources/users.js

            .then((token) => mail(savedUser.email, 'accountCreated', { token })))
          .then(always(savedUser)))));

+    const endpointByInvalidateQuery = (cb) => (req, ...rest) =>


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. 👍

matthew-white · 2025-05-02T19:27:35Z

test/integration/api/assignments.js

+      service.login('alice', (asAlice) =>
+        service.login('bob', (asBob) =>


Doesn't need to change, but just FYI, you can pass an array to service.login():

service.login(['alice', 'bob'], (asAlice, asBob) =>

matthew-white · 2025-05-02T19:44:36Z

test/integration/api/sessions.js

    it('should return a 403 if the token does not exist', testService((service) =>
      service.delete('/v1/sessions/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa')
-        .expect(403)));
+        .expect(401)));


I feel like this test is looking at nonexistence, not lack of authentication. I think the test should have Alice send the request, then assert that it's a 403. This endpoint is unusual in that it returns a 403 in the nonexistence case instead of a 404.

matthew-white · 2025-05-02T19:46:44Z

test/integration/api/sessions.js

@@ -162,7 +162,7 @@ describe('api: /sessions', () => {
      service.get('/v1/sessions/restore')
        .set('X-Forwarded-Proto', 'https')
        .set('Cookie', 'session: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa')


This cookie looks unusual, shouldn't it be = instead of :?

matthew-white · 2025-05-02T19:57:10Z

test/integration/api/sessions.js

        .set('X-Forwarded-Proto', 'https')
        .set('Cookie', 'session: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa')
-        .expect(404)));
+        .expect(401)));


This is probably on your radar already, but the change from 404 to 401 will require a change to restoreSession() in Frontend.

created getodk/central-frontend#1267

matthew-white · 2025-05-02T20:01:28Z

test/unit/http/preprocessors.js

          )).should.be.rejectedWith(Problem, { problemCode: 401.2 }));

-        it('should do nothing on cookie auth with incorrect session token for non-GET requests', () =>
+        it('should reject cookie auth with incorrect session token for non-GET requests', () =>


Maybe we can remove the part of the test description about "for non-GET requests". We will now fail even GET requests.

matthew-white

The approach that's being taken here seems like a good one to me. I left some questions/thoughts above, but I think this will be a nice change for Central.

matthew-white · 2025-05-19T19:16:34Z

Even though I know getodk/central#1001 is a large part of the motivation for this PR, I think it'd be easiest to track this change on its own. I'll go ahead and add it to the project.

matthew-white

Notes from reviewing changes to lib/, now onward to test/.

matthew-white · 2025-05-19T14:28:44Z

lib/http/preprocessors.js

    // fail the request unless we are under HTTPS.
    if ((context.protocol !== 'https') && (context.headers['x-forwarded-proto'] !== 'https'))
-      return;
+      return reject(Problem.user.httpsOnly());


How about updating the comment above:

Cookie Auth, which is more relaxed about not doing anything on failures.

I guess cookie auth is now not as relaxed.

matthew-white · 2025-05-19T14:44:51Z

lib/resources/forms.js

I think we can probably get rid of this line now. I don't think there are any conditions where the preprocessor would pass, but the endpoint would reject here:

central-backend/lib/resources/forms.js

Line 74 in 4ea6121

.then(rejectIf((() => auth.actor.isEmpty()), noargs(Problem.user.openRosaAuthenticationRequired)))

Today, most endpoints return a 403 if no auth is provided. However, IIRC we specifically needed this endpoint to return a 401. I think a 401 causes Collect and/or Enketo to do something special like prompt for credentials or show a special error.

But now we will return a 401 from the preprocessor, so we don't need to do anything special here.

matthew-white · 2025-05-19T15:02:10Z

lib/resources/users.js

Now that the preprocessor ensures that auth is present, what do you think of the following pattern in /v1/users/current? I feel like the .orElse() is impossible now, but I'm not totally sure how we'd do things differently.

// Returns the currently authed actor. service.get('/users/current', endpoint(({ Auth, Users, UserPreferences }, { auth, queryOptions }) => auth.actor.map((actor) => { /* some code */ }) .orElse(Problem.user.notFound())));

I have refactored this function completely on async/await pattern.

matthew-white · 2025-05-19T19:36:25Z

lib/resources/users.js

Also related to /v1/users/current, I was remembering that pyODK uses that endpoint to check whether the session token is still active. If the session token isn't active, the endpoint would return a 404 before, but now it will return a 401. I took a look at the pyODK code to make sure that it wasn't looking specifically for a 404 (it's not): https://github.com/getodk/pyodk/blob/07a6fb568bd60a5c7657f05be3f9ab016fdfbf3e/pyodk/_endpoints/auth.py#L25-L34

sadiqkhoja

I have removed checkAuth from user-preferences.js as well.

And added couple of additional tests for /users/current

sadiqkhoja · 2025-05-21T21:30:02Z

lib/resources/users.js

I have refactored this function completely on async/await pattern.

matthew-white · 2025-05-26T05:23:00Z

test/integration/other/authentication.js

+  ];
+
+  describe('anonymous endpoints', () => {
+    anonymousEndpoints.filter(e => !e.skipForOidc).forEach(({ method, url }) => {


I may be missing something, but I don't see how the tests for which skipForOidc is true would ever be run. They're filtered out here, and I don't see anywhere else where they are used. I feel like process.env.TEST_AUTH needs to be used somehow, something like:

const endpointsToTest = process.env.TEST_AUTH === 'oidc' ? anonymousEndpoints.filter(e => !e.skipForOidc) : anonymousEndpoints;

matthew-white · 2025-05-26T05:31:35Z

test/integration/api/users.js

        it('should fail the request if invalidation is requested but not allowed', testService((service) =>
          service.post('/v1/users/reset/initiate?invalidate=true')
            .send({ email: 'alice@getodk.org' })
-            .expect(403)));
+            .expect(401)));


How about adding a test that Chelsa gets a 403 if she specifies invalidate=true to try to invalidate Alice's password? I took a quick look, but I didn't see an existing test that was exactly like that.

matthew-white · 2025-05-26T05:32:48Z

test/integration/other/authentication.js

+    { method: 'PUT',    url: '/v1/users/1/password', skipForOidc: true },
+    { method: 'GET',    url: '/v1/users/current' },
+    { method: 'POST',   url: '/v1/users/reset/verify', skipForOidc: true }
+  ];


How about adding url: '/v1/users/reset/initiate?invalidate=true to this list? Unless it's already covered sufficiently elsewhere?

matthew-white · 2025-05-26T05:42:10Z

test/integration/api/sessions.js

-    it('should return a 404 if no token was specified', testService(service =>
+    it('should return a 401 if no token was specified', testService(service =>
      service.delete('/v1/sessions/current')
-        .expect(404)));
+        .expect(401)));


Looking at the endpoint, I feel like it's still possible for a 404 to happen. If the user authenticates using Basic auth, then auth.actor will exist, but auth.session will not. In that case, it won't be a 401 (because there is auth), but it will be a 404 (because there's no current session).

matthew-white · 2025-07-29T17:37:38Z

I feel like there are a few more places where auth.actor.map() could go away, but that could also happen in a follow-up PR (feel free just to file an issue if that seems better).

central-backend/lib/resources/app-users.js

Line 28 in 20d7e72

.with({ createdBy: auth.actor.map((actor) => actor.id).orNull() });

central-backend/lib/resources/public-links.js

Line 28 in 20d7e72

.with({ createdBy: auth.actor.map((actor) => actor.id).orNull() });

central-backend/lib/resources/comments.js

Line 35 in 20d7e72

Comments.create(auth.actor.map((actor) => actor.id).orNull(),

matthew-white · 2025-07-29T17:40:12Z

I guess I'm just proposing auth.actor.get().id. I was thinking of this related comment: https://github.com/getodk/central-backend/pull/1488/files#r2095918985. Not a big deal the more I think about it, doesn't even need to change.

matthew-white

LGTM 🚀

Thank you for making this change @sadiqkhoja, it's definitely a great improvement to the codebase.

matthew-white · 2025-07-29T14:21:54Z

test/integration/other/authentication.js

Have any new endpoints been committed to master since this PR was created? If so, they could be added to the list below.

no new endpoint has been added since then

matthew-white · 2025-07-29T17:28:44Z

lib/resources/forms.js

  service.get('/projects/:projectId/formList', endpoint.openRosa(({ Forms, Projects, env }, { auth, params, originalUrl, queryOptions }) =>
    Projects.getById(params.projectId)
      .then(getOrNotFound)
-      .then(rejectIf((() => auth.actor.isEmpty()), noargs(Problem.user.openRosaAuthenticationRequired)))


How about removing this Problem from lib/util/problem.js? I guess there's no harm in leaving it, but it doesn't look like it's used anywhere else.

matthew-white · 2025-07-29T17:43:06Z

lib/resources/users.js

          .then(always(savedUser)))));

+    const endpointByInvalidateQuery = (cb) => (req, ...rest) =>
+      (req.query.invalidate ? endpoint(cb) : anonymousEndpoint(cb))(req, ...rest);


Suggested change

(req.query.invalidate ? endpoint(cb) : anonymousEndpoint(cb))(req, ...rest);

(isTrue(req.query.invalidate) ? endpoint(cb) : anonymousEndpoint(cb))(req, ...rest);

I think we should use isTrue() here to exactly match how query.invalidate is used elsewhere in the endpoint. To avoid subtle differences between truthiness vs. isTrue().

matthew-white · 2025-07-29T18:01:34Z

test/integration/api/forms/forms.js

      await service.get(`/v1/form-links/${enketoId}/form`)
-        .expect(404);
+        .expect(401);


I don't 100% understand why it was 404 before and not 403. I think that enketoId must be null here, because we create the simple form without an enketoId (fixtures/02-forms.js). Maybe the test would be better if it created a new form that does have an enketoId. It's an existing test though, so I don't think we need to change that in this PR. And most importantly, 401 is the correct error code going forward.

matthew-white · 2025-07-29T18:16:03Z

test/integration/api/users.js

 const should = require('should');
 const { getOrNotFound } = require(appRoot + '/lib/util/promise');
 const { testService } = require('../setup');
+const { describe } = require('mocha');


Since these tests existed before and use describe(), I feel like this addition probably isn't needed?

matthew-white · 2025-07-29T18:27:40Z

test/integration/other/authentication.js

+
+describe('authentication', () => {
+  const anonymousEndpoints = [
+    { method: 'POST', url: '/v1/sessions', skipForOidc: true },


I don't feel like it's especially helpful to include /v1/sessions in this list. The test is asserting that a request to the endpoint doesn't return a 401. But the endpoint is capable of returning a 401 if the email or password is incorrect.

the anonymousEndpoints array lists endpoints that don't look for any authentication

matthew-white · 2025-07-29T18:37:28Z

lib/http/preprocessors.js

-      return;
-    // actually try to authenticate with it. no Problem on failure.
-    const maybeSession = authBySessionToken(token);
+      return reject(Problem.user.authenticationFailed());


I don't think I see a test of this change, where:

There is a Cookie header (context.headers.cookie != null)

But there is no session cookie (context.cookies[SESSION_COOKIE].token == null)

In that case, it should be a 401.

I see similar tests:

The Cookie header is invalid

The token is invalid

But I'm not sure I see a test that gets at the sequence I described. I guess I want tests that get at the difference between these two:

There's no cookie of any kind at all

There is some Cookie header, just not a session cookie

Could you check that tests verify that? If they don't, could you add a test along those lines?

alxndrsn · 2025-07-30T09:00:05Z

lib/resources/submissions.js

  // SUBMISSIONS (OPENROSA)

-  const openRosaSubmission = (path, draft, getForm) => {
+  const openRosaSubmission = (path, endpointToUse, draft, getForm) => {


nits: arg order, arg name

Suggested change

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

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

I have renamed endpointToUse to _endpoint but I would like to keep path as the first arg, it's easy to scan.

* Return 401 if authentication is not provided * Make /oidc/login anonymous * Incorporated PR Feedback * Added tests for authentication in one place * More PR Feedback * PR feedback related to tests * Remove check for Form existence from GET /submissions openrosa endpoint * Incorporated PR Feedback

sadiqkhoja force-pushed the fixes/return-401-on-cookie-auth branch from 3f2926d to 82217e7 Compare May 2, 2025 05:53

sadiqkhoja requested review from alxndrsn and matthew-white May 2, 2025 05:57

alxndrsn reviewed May 2, 2025

View reviewed changes

lib/http/service.js Outdated Show resolved Hide resolved

alxndrsn reviewed May 2, 2025

View reviewed changes

lib/resources/submissions.js Outdated Show resolved Hide resolved

alxndrsn reviewed May 2, 2025

View reviewed changes

test/integration/api/app-users.js Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

matthew-white reviewed May 2, 2025

View reviewed changes

sadiqkhoja added 3 commits May 15, 2025 14:28

Return 401 if authentication is not provided

b51d17c

Make /oidc/login anonymous

5bb2556

Incorporated PR Feedback

21c2d15

sadiqkhoja force-pushed the fixes/return-401-on-cookie-auth branch from 82217e7 to f1d3be0 Compare May 19, 2025 13:10

sadiqkhoja mentioned this pull request May 19, 2025

Update restoreSession() getodk/central-frontend#1267

Closed

Added tests for authentication in one place

9523d9e

sadiqkhoja force-pushed the fixes/return-401-on-cookie-auth branch from f1d3be0 to 9523d9e Compare May 19, 2025 13:51

sadiqkhoja requested a review from matthew-white May 19, 2025 13:56

sadiqkhoja mentioned this pull request May 19, 2025

Fixes #1267: Update restoreSession() to check for 401 error getodk/central-frontend#1268

Merged

2 tasks

matthew-white moved this from 🕒 backlog to ✏️ in progress in ODK Central May 19, 2025

matthew-white reviewed May 19, 2025

View reviewed changes

More PR Feedback

f456cbe

sadiqkhoja commented May 21, 2025

View reviewed changes

lib/resources/users.js

Copy link

Contributor Author

sadiqkhoja May 21, 2025

Choose a reason for hiding this comment

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

I have refactored this function completely on async/await pattern.

matthew-white reviewed May 26, 2025

View reviewed changes

sadiqkhoja force-pushed the fixes/return-401-on-cookie-auth branch 2 times, most recently from 30e59d0 to dcc9bb4 Compare May 27, 2025 14:16

sadiqkhoja added 2 commits May 27, 2025 10:17

PR feedback related to tests

5628427

Remove check for Form existence from GET /submissions openrosa endpoint

e8c16d0

sadiqkhoja force-pushed the fixes/return-401-on-cookie-auth branch from dcc9bb4 to e8c16d0 Compare May 27, 2025 14:18

lognaturel requested a review from matthew-white June 5, 2025 22:04

matthew-white moved this from ✏️ in progress to 🕒 backlog in ODK Central Jun 12, 2025

lognaturel moved this from 🕒 backlog to ✏️ in progress in ODK Central Jul 1, 2025

matthew-white approved these changes Jul 29, 2025

View reviewed changes

alxndrsn reviewed Jul 30, 2025

View reviewed changes

This was referenced Jul 31, 2025

lib/resources: standardise usage of xmlFormId in path params #1512

Merged

Submission geometry extraction + API #1555

Closed

Incorporated PR Feedback

ca6904a

This was referenced Aug 8, 2025

Refactor: Replace auth.actor.map((actor) => actor.id).orNull() with auth.actor.get().id getodk/central#1210

Open

Refactor: Test of /v1/form-links/:enketoId/form getodk/central#1211

Open

sadiqkhoja merged commit 0b39288 into getodk:master Aug 8, 2025
6 checks passed

github-project-automation bot moved this from ✏️ in progress to ✅ done in ODK Central Aug 8, 2025

This comment was marked as resolved.

Sign in to view

This was referenced Aug 8, 2025

http/preprocessors: simplify authBySessionToken() signature #1582

Merged

Update docs for auth failures getodk/central#1213

Closed

matthew-white mentioned this pull request Sep 3, 2025

No information about changed access in WF submission page after session expires getodk/central#1001

Closed

matthew-white mentioned this pull request Nov 3, 2025

update submodules for v2025.3 getodk/central#1474

Merged

		service.login('alice', (asAlice) =>
		service.login('bob', (asBob) =>

	(req.query.invalidate ? endpoint(cb) : anonymousEndpoint(cb))(req, ...rest);
	(isTrue(req.query.invalidate) ? endpoint(cb) : anonymousEndpoint(cb))(req, ...rest);

	const openRosaSubmission = (path, endpointToUse, draft, getForm) => {
	const openRosaSubmission = (_endpoint, path, draft, getForm) => {

Conversation

sadiqkhoja commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What has been done to verify that this works as intended?

Why is this the best possible solution? Were any other approaches considered?

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Does this change require updates to the API documentation? If so, please update docs/api.yaml as part of this PR.

Before submitting this PR, please make sure you have:

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!

Uh oh!

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!

Uh oh!

This comment was marked as resolved.

alxndrsn commented May 2, 2025

Does this change require updates to the API documentation? If so, please update docs/api.yaml as part of this PR.

Uh oh!

This comment was marked as resolved.

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!

matthew-white May 2, 2025 • 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!

matthew-white May 2, 2025 • 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!

sadiqkhoja May 16, 2025 • 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!

matthew-white left a comment

Choose a reason for hiding this comment

Uh oh!

matthew-white commented May 19, 2025

Uh oh!

matthew-white 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!

matthew-white May 19, 2025 • 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!

sadiqkhoja left a comment

sadiqkhoja commented May 2, 2025 •

edited

Loading

matthew-white May 2, 2025 •

edited

Loading

matthew-white May 2, 2025 •

edited

Loading

sadiqkhoja May 16, 2025 •

edited

Loading

matthew-white May 19, 2025 •

edited

Loading