Return 401 if authentication is not provided #1488

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

@@ -499,7 +507,7 @@ describe('/projects/:id/assignments', () => {
    it('should assign the actor by role numeric id', testService((service) =>
      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

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


Where is an "empty" session now handled?

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

alxndrsn · 2025-05-02T07:55:16Z

lib/http/service.js

-  const preprocessors = [ emptyAuthInjector, authHandler, queryOptionsHandler, userAgentHandler ];
-  const endpoint = builder(container, preprocessors);
-  const anonymousEndpoint = builder(container, without([authHandler], preprocessors));
+  const preprocessors = [ queryOptionsHandler, userAgentHandler ];


Suggested change

const preprocessors = [ queryOptionsHandler, userAgentHandler ];

const commonPreprocessors = [ queryOptionsHandler, userAgentHandler ];

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

lib/resources/submissions.js


  ////////////////////////////////////////////////////////////////////////////////
  // SUBMISSIONS (OPENROSA)

  const openRosaSubmission = (path, draft, getForm) => {
+    const endpointByTestPath = path.startsWith('/test/:key') ? anonymousEndpoint : endpoint;


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) => {

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

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

lib/resources/submissions.js

      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?

alxndrsn · 2025-05-02T08:12:01Z

test/integration/api/app-users.js

      service.post('/v1/projects/1/app-users')
        .send({ displayName: 'test1' })
-        .expect(403)));
+        .expect(401)));


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

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.

alxndrsn · 2025-05-02T08:15:12Z

From https://www.rfc-editor.org/rfc/rfc9110#status.401:

The server generating a 401 response MUST send a WWW-Authenticate header field (Section 11.6.1) containing at least one challenge applicable to the target resource.

Is this relevant, and if so is it covered by this PR?

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-02T18:51:43Z

The server generating a 401 response MUST send a WWW-Authenticate header field …

Is this relevant, and if so is it covered by this PR?

I don't think we ever send that header, so I don't think this PR needs to do so. How about we continue discussing that header in #851?

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

@@ -54,8 +54,11 @@ module.exports = (service, endpoint) => {
            .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

@@ -181,7 +181,7 @@ describe('api: /sessions', () => {
  describe('/:token DELETE', () => {
    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

@@ -162,7 +162,7 @@ describe('api: /sessions', () => {
      service.get('/v1/sessions/restore')
        .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.

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

test/unit/http/preprocessors.js

@@ -301,7 +289,7 @@ describe('preprocessors', () => {
            )
          )).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.

sadiqkhoja added 2 commits May 2, 2025 01:26

Return 401 if authentication is not provided

733585b

Make /oidc/login anonymous

82217e7

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 matthew-white and alxndrsn May 2, 2025 05:57

alxndrsn reviewed May 2, 2025

View reviewed changes

matthew-white reviewed May 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return 401 if authentication is not provided #1488

Return 401 if authentication is not provided #1488

sadiqkhoja commented May 2, 2025 •

edited

Loading

alxndrsn May 2, 2025

matthew-white May 2, 2025

alxndrsn May 2, 2025

matthew-white May 2, 2025

alxndrsn May 2, 2025

alxndrsn May 2, 2025

matthew-white May 2, 2025

alxndrsn May 2, 2025

alxndrsn May 2, 2025

matthew-white May 2, 2025

alxndrsn commented May 2, 2025

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.

matthew-white commented May 2, 2025

matthew-white May 2, 2025

matthew-white May 2, 2025

matthew-white May 2, 2025

matthew-white May 2, 2025

matthew-white May 2, 2025 •

edited

Loading

matthew-white May 2, 2025

matthew-white May 2, 2025 •

edited

Loading

matthew-white May 2, 2025

matthew-white left a comment

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

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

Return 401 if authentication is not provided #1488

Are you sure you want to change the base?

Return 401 if authentication is not provided #1488

Conversation

sadiqkhoja commented May 2, 2025 • edited Loading

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:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alxndrsn commented May 2, 2025

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.

matthew-white commented May 2, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthew-white May 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthew-white May 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthew-white left a comment

Choose a reason for hiding this comment

sadiqkhoja commented May 2, 2025 •

edited

Loading

matthew-white May 2, 2025 •

edited

Loading

matthew-white May 2, 2025 •

edited

Loading