Support Single Sign-on with OpenID Connect

[alxndrsn]

Backend portion of getodk/central#449. Requires frontend changes at getodk/central-frontend#819

[sadiqkhoja]

Should we write isolated integration tests for new endpoints defined in lib/resources/oidc.js? That would assert

• Audit logs are created
• Response body and headers are correct

It would be nice to write unit tests for lib/util/oidc.js

[sadiqkhoja]

What if we move this to frontend repository and just redirect from here?

I don't like

1. HTML literals in JS files
2. Duplication of html and css

[matthew-white]

Frontend would also provide i18n for any text.

[alxndrsn]

👍 This should be possible for errors if specific codes are defined and then a corresponding view is implemented in frontend.

For the success path, I think the extra backend step is required to maintain the current session/csrf cookie settings.

[matthew-white]

For the success path, I think the extra backend step is required to maintain the current session/csrf cookie settings.

Could you say more about why this is required? If this requirement is due to the setup in Frontend, maybe we could make changes there. Would it help to make the Frontend request to /v1/oidc/login async?

[alxndrsn]

Session ID and CSRF cookies are both SameSite=Strict:

central-backend/lib/resources/sessions.js

Lines 39 to 42 in facf339

	response.cookie('__Host-session', session.token, { path: '/', expires: session.expiresAt,
	httpOnly: true, secure: true, sameSite: 'strict' });
	response.cookie('__csrf', session.csrf, { expires: session.expiresAt,
	secure: true, sameSite: 'strict' });

This means that if backend redirects to frontend at

central-backend/lib/resources/oidc.js

Lines 130 to 146 in e7da23a

	// This redirect is neat, but breaks SameSite: Secure cookies.
	//return redirect(302, '/'); // REVIEW: internally, the redirect() function throws... which is odd
	// Instead, we need to render a page and then "browse" from that page to the normal frontend:
	// TODO id=cl only set for playwright.. why can't it locate this anchor in any other way?
	// TODO this approach does not work. Either:
	// 1. expose a browse-to-next page on the frontend, and pass `next` value there, or
	// 2. re-implement
	const nextPath = safeNextPathFrom(next);
	return respond(res, page(
	html`<meta http-equiv="refresh" content="0; url=${nextPath}">`,
	html`
	<h1>Authentication Successful</h1>
	<div><a href="${nextPath}" id="cl">Continue to ODK Central</a></div>
	`,
	));

, the browser does not send cookies, so the user cannot be authorised.

There are tests for this at:

• https://github.com/getodk/central-backend/blob/e7da23a3d3ac248f19a179e59a432c756f7c6361/oidc-tester/playwright-tests/src/happy.spec.js
• https://github.com/getodk/central-backend/blob/e7da23a3d3ac248f19a179e59a432c756f7c6361/oidc-tester/playwright-tests/src/happy-next.spec.js

With a 302 redirect, the tests fail like this:

3) [firefox] › happy-next.spec.js:21:1 › can log in ──────────────────────────────────────────────

  AssertionError: No session cookie found!

     at utils.js:55

    53 |   console.log('requestCookies:', JSON.stringify(requestCookies, null, 2));
    54 |
  > 55 |   assert(requestCookies[SESSION_COOKIE], 'No session cookie found!');
       |   ^   
    56 |   assert(requestCookies['__csrf'],       'No CSRF cookie found!');
    57 |   assert.equal(Object.keys(requestCookies).length, 2, 'Unexpected requestCookie count!');
    58 | } 

      at assertLoginSuccessful (/odk-central-backend/oidc-tester/playwright-tests/src/utils.js:55:3)
      at /odk-central-backend/oidc-tester/playwright-tests/src/happy-next.spec.js:25:3

See e.g. https://github.com/alxndrsn/odk-central-backend/actions/runs/5666083718/job/15352099923#step:5:6265

[matthew-white]

OK, that's super helpful, thank you! I think I'm understanding things better now. Just to confirm:

1. An error results in a redirect to Frontend. The redirect specifies an error code as a query parameter, which Frontend uses to show an alert.
2. A success renders an HTML page. The page does a client-side redirect to Frontend and also links to Frontend.

That all sounds good to me. 👍 For the success page, what do you think about leaving that as unstyled as possible and not trying to make it look like Frontend? Since the user will only be on the page for a short time and is automatically redirected, I don't feel like it needs to be styled. I've seen that pattern elsewhere and feel like it wouldn't be overly confusing to users.

[alxndrsn]

There's a parallel discussion at https://github.com/getodk/central-backend/pull/910/files#r1310995967:

I've also seen the pattern of unstyled forwarding pages elsewhere and I'm not a fan! I think a branded forwarding page maintains users' trust that they are inside a legitimate workflow.

[matthew-white]

I've filed a related issue: #971

[matthew-white]

This may be addressed by other changes you're working on, but I'm noticing that when I log in on dev, a request is sent to /csp-report. The violated-directive is style-src-elem.

[sadiqkhoja]

There are bunch of TODOs, to me all of them are about explanation of the code, I recommend that we merge this branch and update comments in a separate PR

[sadiqkhoja]

This doesn't look like a utility function. Where this should reside? maybe under lib/http folder or a new folder lib/resources/shared?

And we can make this arrow function

[alxndrsn]

This doesn't look like a utility function. Where this should reside?

The current location was my best guess. What qualifies something as a utility function?

[alxndrsn]

This has been converted to an anonymous function 👍

[matthew-white]

I agree that it's surprising to see createUserSession() here, since it contains specific logic related to endpoints/resources/responses. Usually such logic is located in lib/resources. What makes this a particularly interesting case is that this logic is shared across multiple resource files (sessions.js and oidc.js). It's hard to think of a similar case, which is why I'm guessing @sadiqkhoja suggested lib/resources/shared.

That said, I can see why it's nice for createUserSession() to be in the same file as SESSION_COOKIE and HTTPS_ENABLED. SESSION_COOKIE is also used in lib/http/preprocessors.js. Since that's in a different directory from lib/resources, that might be a strike against moving this code to inside lib/resources.

lib/http seems like a reasonable location to me. That directory contains endpoint/resource/response logic that's shared across endpoints (in some cases, logic that's shared across all endpoints, and in other cases, logic that's shared across a smaller subset of endpoints). I also understand though that it's sometimes hard to find the exact right place for a function and that sometimes these sorts of distinctions can get blurry or arbitrary. @alxndrsn, I think you should make a decision one way or another for now, then we can continue thinking in general about what goes where.

[sadiqkhoja]

arrow functions would be nice here

[sadiqkhoja]

added few minor comments about the tests. I am happy with this PR 🎉 can't wait to get this merged

[sadiqkhoja]

arrow function would be nice here

[sadiqkhoja]

I don't get this, if user is not there then we are saying service is missing?

[alxndrsn]

When refactoring tests, I found it easy to mistakenly write authenticateUser('alice') instead of authenticateUser(service, 'alice'). This check was very helpful in catching those mistakes - without the check here, the eventual error was nebulous. I think it will be helpful for future test writers to keep this check here. But this branch is passing now, so I can delete it if it's not helpful.

Alternatively, maybe the function could be refactored to be an instance method of service. Would that be preferred?

[sadiqkhoja]

the function could be refactored to be an instance method of service

I like the idea

[matthew-white]

The release criteria suggest that admins still need to be able to update users' email addresses. @sadiqkhoja left a comment about that in the release criteria, but if we do decide to run with that behavior, this logic would need to change.

Also, password isn't a writable field on the frame, so I don't think it needs to be filtered out here: fromApi() should always filter it out. This endpoint can't be used to change a password: that's a different endpoint.

[alxndrsn]

So can this whole block be deleted? (i.e. revert all changes to this route)

[matthew-white]

I think there does need to be something here. If SSO is enabled, we don't want non-admin users to use this endpoint to update their own email. Note that in auth, users can user.update themselves even if they can't user.update other users:

central-backend/lib/model/query/auth.js

Lines 26 to 29 in cedadf2

	if (actor.acteeId === acteeId) {
	// special privileges actors always get on themselves.
	if ((verb === 'user.read') || (verb === 'user.update'))
	return resolve(true);

Non-admin users should still be able to use this endpoint to update their own display name.

I think this means that we should filter out email if the user can't user.update users in general. I think you can check for that by calling auth.can('user.update', User.species), which returns a promise.

[alxndrsn]

I've added a test and updated the existing tests for this case.

With SSO enabled, should an admin be able to change their own email?

[lognaturel]

I personally have no preference. @issa-tseng maybe you have strong feelings?

I think we could say admins can change their own email because they should have a lot more context on the implications. This is also more helpful in contexts in which there’s only one site wide admin.

It would also be very defensible to say admins need to use the command line or talk to another admin to get their email address changed.

Whatever it is, it should be in the docs.

[matthew-white]

Whatever the behavior is, we should also make sure that Frontend matches it. Right now, Frontend will allow an admin to change their own email.

[lognaturel]

I think the way it works now makes a lot of sense! Let's keep it and document it.