Skip to content
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.

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

Support Single Sign-on with OpenID Connect #910

Merged
merged 4 commits into from
Sep 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
21 changes: 21 additions & 0 deletions .dockerignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# Ignore everything
*

# Explicitly whitelist _necessary_ **source files**
!/package.json
!/package-lock.json
!/Makefile
!/lib/
!/config/
!/test/

!/oidc-dev/certs/*.pem
!/oidc-dev/fake-oidc-server/accounts.json
!/oidc-dev/fake-oidc-server/index.js
!/oidc-dev/fake-oidc-server/package.json
!/oidc-dev/fake-oidc-server/package-lock.json
!/oidc-dev/playwright-tests/package.json
!/oidc-dev/playwright-tests/package-lock.json
!/oidc-dev/playwright-tests/playwright.config.js
!/oidc-dev/playwright-tests/src/**/*.js
!/oidc-dev/scripts/*.sh
1 change: 1 addition & 0 deletions .eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
"array-bracket-spacing": "off",
"arrow-parens": "off",
"class-methods-use-this": "off",
"camelcase": [ "error", { "ignoreDestructuring": true, "properties": "never" } ],
"comma-dangle": "off",
"consistent-return": "off",
"curly": "off",
Expand Down
22 changes: 22 additions & 0 deletions .github/workflows/oidc-e2e.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
name: OIDC e2e tests

on: push

jobs:
oidc-e2e-test:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- name: Use Node.js 18
uses: actions/setup-node@v3
with:
node-version: 18.17.0
cache: 'npm'
- run: sudo apt-get install -y curl
- run: make test-oidc-e2e
- name: Archive playwright screenshots
if: failure()
uses: actions/upload-artifact@v3
with:
name: Playwright Screenshots
path: oidc-dev/playwright-results/**/*.png
36 changes: 36 additions & 0 deletions .github/workflows/oidc-integration.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
name: OIDC integration tests

on: push

jobs:
oidc-integration-test:
# TODO should we use the same container as circle & central?
runs-on: ubuntu-latest
services:
# see: https://docs.github.com/en/[email protected]/actions/using-containerized-services/creating-postgresql-service-containers
postgres:
image: postgres:14.6
env:
POSTGRES_PASSWORD: odktest
ports:
- 5432:5432
# Set health checks to wait until postgres has started
options: >-
--health-cmd pg_isready
--health-interval 10s
--health-timeout 5s
--health-retries 5
steps:
- uses: actions/checkout@v3
- name: Use Node.js 18
uses: actions/setup-node@v3
with:
node-version: 18.17.0
cache: 'npm'
- run: npm ci --legacy-peer-deps
- run: make fake-oidc-server-ci > fake-oidc-server.log &
- run: node lib/bin/create-docker-databases.js
- run: make test-oidc-integration
- name: Fake OIDC Server Logs
if: always()
run: "! [[ -f ./fake-oidc-server.log ]] || cat ./fake-oidc-server.log"
32 changes: 32 additions & 0 deletions .github/workflows/standard-suite.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
name: Full Standard Test Suite

on: push

jobs:
standard-tests:
# TODO should we use the same container as circle & central?
runs-on: ubuntu-latest
services:
# see: https://docs.github.com/en/[email protected]/actions/using-containerized-services/creating-postgresql-service-containers
postgres:
image: postgres:14.6
env:
POSTGRES_PASSWORD: odktest
ports:
- 5432:5432
# Set health checks to wait until postgres has started
options: >-
--health-cmd pg_isready
--health-interval 10s
--health-timeout 5s
--health-retries 5
steps:
- uses: actions/checkout@v3
- name: Use Node.js 18
uses: actions/setup-node@v3
with:
node-version: 18.17.0
cache: 'npm'
- run: npm ci --legacy-peer-deps
- run: node lib/bin/create-docker-databases.js
- run: make test-full
6 changes: 6 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,12 @@ If you're looking for help or discussion on _how_ ODK Central Backend works inte

Please see the [project README](https://github.com/getodk/central-backend#setting-up-a-development-environment) for instructions on how to set up your development environment.

### OpenID Connect

If you want to use OpenID Connect instead of username/password for authentication in development:

Instead of `make dev`, run both `make dev-oidc` and `make fake-oidc-server`.

## Guidelines

If you're starting work on an issue ticket, please leave a comment saying so. If you run into trouble or have to stop working on a ticket, please leave a comment as well. As you write code, the usual guidelines apply; please ensure you are following existing conventions:
Expand Down
27 changes: 27 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,33 @@ node_modules: package.json
npm install --legacy-peer-deps
touch node_modules

.PHONY: test-oidc-integration
test-oidc-integration: node_modules
TEST_AUTH=oidc NODE_CONFIG_ENV=oidc-integration-test make test-integration

.PHONY: test-oidc-e2e
test-oidc-e2e: node_modules
cd oidc-dev && \
docker compose down && \
docker compose build && \
docker compose up --exit-code-from odk-central-oidc-tester

.PHONY: dev-oidc
dev-oidc: base
NODE_CONFIG_ENV=oidc-development npx nodemon --watch lib --watch config lib/bin/run-server.js
alxndrsn marked this conversation as resolved.
Show resolved Hide resolved

.PHONY: fake-oidc-server
fake-oidc-server:
cd oidc-dev/fake-oidc-server && \
npm clean-install && \
FAKE_OIDC_ROOT_URL=http://localhost:9898 npx nodemon index.js

.PHONY: fake-oidc-server-ci
fake-oidc-server-ci:
cd oidc-dev/fake-oidc-server && \
npm clean-install && \
FAKE_OIDC_ROOT_URL=http://localhost:9898 node index.js

.PHONY: node_version
node_version: node_modules
node lib/bin/enforce-node-version.js
Expand Down
11 changes: 11 additions & 0 deletions config/oidc-development.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
alxndrsn marked this conversation as resolved.
Show resolved Hide resolved
"default": {
"oidc": {
"_description": "local test server: from https://www.npmjs.com/package/oidc-provider",
"issuerUrl": "http://localhost:9898",
"clientId": "odk-central-backend-dev",
"clientSecret": "super-top-secret",
"enabled": true
}
}
}
11 changes: 11 additions & 0 deletions config/oidc-example-auth0.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"default": {
"oidc": {
"_description": "auth0: https://manage.auth0.com/dashboard/us/odk-oidc-dev/",
"issuerUrl": "https://odk-oidc-dev.us.auth0.com",
"clientId": "ZKKpcW8TpKymVLbD1dbDVExj7SU4Zxbn",
"clientSecret": "7tuVT7OsjRHfmUiwYYyWNT8YArMNlmvvv70tqlChkjtVHW0Xsp0mvVAyKIfCgUn5",
"enabled": true
}
}
}
11 changes: 11 additions & 0 deletions config/oidc-example-broken.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"default": {
"oidc": {
"_description": "broken config: fiddle with this config to test out different init failure modes",
"issuerUrl": "http://example.com",
"clientId": "this is required; should be reported during client init if this line commented out",
"clientSecret": "this is required; should be reported during client init if this line commented out",
"enabled": true
}
}
}
11 changes: 11 additions & 0 deletions config/oidc-example-google.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"default": {
"oidc": {
"_description": "google: from https://console.cloud.google.com/apis/credentials",
"issuerUrl": "https://accounts.google.com",
"clientId": "564021877275-o5q3i8j44190d93d9mldd3rti1fncn3u.apps.googleusercontent.com",
"clientSecret": "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",
"enabled": true
}
}
}
11 changes: 11 additions & 0 deletions config/oidc-integration-test.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"default": {
"oidc": {
"enabled": true,
"issuerUrl": "http://localhost:9898",
"clientId": "odk-central-backend-dev",
"clientSecret": "super-top-secret"
}
}
}

19 changes: 19 additions & 0 deletions config/oidc-tester-docker.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
{
"default": {
"database": {
"host": "odk-central-oidc-tester-postgres",
"database": "oidc-tester",
"user": "odk-central-backend",
"password": "supertopsecret3000"
},
"env": {
"domain": "https://odk-central.example.org:8989"
},
"oidc": {
"issuerUrl": "https://fake-oidc-server.example.net:9898",
"clientId": "odk-central-backend-dev",
"clientSecret": "super-top-secret",
"enabled": true
}
}
}
35 changes: 23 additions & 12 deletions lib/bin/cli.js
alxndrsn marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,7 @@

const { run } = require('../task/task');
const { createUser, promoteUser, setUserPassword } = require('../task/account');

// gets a password interactively if not supplied in cli args.
const prompt = require('prompt');
const withPassword = (f) => {
prompt.start();
prompt.get([{ name: 'password', hidden: true, replace: '*' }], (_, { password }) => f(password));
};
const oidc = require('../util/oidc');

const { Command } = require('commander');
const program = new Command('node lib/bin/cli.js');
Expand All @@ -30,13 +24,30 @@ const email = () => program.opts().email;

program.requiredOption('-u, --email <email-address>');

program.command('user-create')
.action(() => withPassword((password) => run(createUser(email(), password))));
if (oidc.isEnabled()) {
program.command('user-create')
.action(() => run(createUser(email(), null)));

program.command('user-set-password')
.action(() => {
throw new Error(`You cannot set a user's password when OpenID Connect (OIDC) is enabled.`); // eslint-disable-line quotes
});
} else {
// gets a password interactively if not supplied in cli args.
const prompt = require('prompt');
const withPassword = (f) => {
prompt.start();
prompt.get([{ name: 'password', hidden: true, replace: '*' }], (_, { password }) => f(password));
};

program.command('user-create')
.action(() => (withPassword((password) => run(createUser(email(), password)))));

program.command('user-set-password')
.action(() => withPassword((password) => run(setUserPassword(email(), password))));
}

program.command('user-promote')
.action(() => run(promoteUser(email())));

program.command('user-set-password')
.action(() => withPassword((password) => run(setUserPassword(email(), password))));

program.parse();
3 changes: 3 additions & 0 deletions lib/formats/mail.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ const messages = {
// Notifies a user that an account has been created with a predetermined password.
accountCreatedWithPassword: message('ODK Central account created', '<html>Hello!<p>An account has been provisioned for you on an ODK Central server.</p><p>If this message is unexpected, simply ignore it. Your account was created with an assigned password. Please use that password to <a href="{{{domain}}}">sign in</a>.</p><p>If you have not been given the password, or you cannot remember it, you can reset it at any time at this link:</p><p><a href="{{{domain}}}/#/reset-password">{{{domain}}}/#/reset-password</a></p></html>'),

// Notifies a user that an account has been created for login exclusively with OIDC.
accountCreatedForOidc: message('ODK Central account created', '<html>Hello!<p>An account has been provisioned for you on an ODK Central data collection server.</p><p>If this message is unexpected, simply ignore it. Please go to <a href="{{{domain}}}">{{{domain}}}</a> to sign in.</p></html>'),
lognaturel marked this conversation as resolved.
Show resolved Hide resolved

// Notifies a user that their account's email has been changed
accountEmailChanged: message('ODK Central account email changed', '<html>Hello!<p><p>We are emailing because you have an ODK Central account, and somebody has just changed the email address associated with the account from this one you are reading right now ({{oldEmail}}) to a new address ({{newEmail}}).</p><p>If this was you, please feel free to ignore this email. Otherwise, please contact your local ODK system administrator immediately.</p></html>'),

Expand Down
22 changes: 22 additions & 0 deletions lib/http/endpoint.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ const { reduce } = require('ramda');
const { openRosaError } = require('../formats/openrosa');
const { odataXmlError } = require('../formats/odata');
const { noop, isPresent } = require('../util/util');
const { frontendPage, html } = require('../util/html');
const { serialize, redirect } = require('../util/http');
const { resolve, reject } = require('../util/promise');
const { PartialPipe } = require('../util/stream');
Expand Down Expand Up @@ -89,6 +90,7 @@ const getRequestContext = (request) => ({
params: request.params,
query: request.query,
files: request.files,
cookies: request.cookies,

apiVersion: request.apiVersion,
fieldKey: request.fieldKey
Expand Down Expand Up @@ -235,6 +237,25 @@ const defaultEndpoint = endpointBase({
errorWriter: defaultErrorWriter
});

// Render html content in the style of frontend
const htmlEndpoint = endpointBase({
resultWriter: (result, request, response) => {
response.type('text/html');
response.send(frontendPage(result));
},
errorWriter: (error, request, response) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we redirect to frontend here as well?

response.type('text/html');
response.status(500);
response.send(frontendPage({
body: html`
<h1>Error!</h1>
<div id="error-message"><pre>An unknown error occurred on the server.</pre></div>
<div><a href="/">Go home</a></div>
`,
}));
},
});


////////////////////////////////////////
// OPENROSA
Expand Down Expand Up @@ -355,6 +376,7 @@ const odataXmlEndpoint = endpointBase({

const builder = (container, preprocessors) => {
const result = defaultEndpoint(container, preprocessors);
result.html = htmlEndpoint(container, preprocessors);
result.openRosa = openRosaEndpoint(container, preprocessors);
result.odata = {
json: odataJsonEndpoint(container, preprocessors),
Expand Down
9 changes: 6 additions & 3 deletions lib/http/preprocessors.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,11 @@

const { isBlank, isPresent, noop, without } = require('../util/util');
const { isTrue } = require('../util/http');
const oidc = require('../util/oidc');
const Problem = require('../util/problem');
const { QueryOptions } = require('../util/db');
const { reject, getOrReject } = require('../util/promise');

const { SESSION_COOKIE } = require('../util/sessions');

// injects an empty/anonymous auth object into the request context.
const emptyAuthInjector = ({ Auth }, context) => context.with({ auth: Auth.by(null) });
Expand Down Expand Up @@ -68,6 +69,8 @@ const authHandler = ({ Sessions, Users, Auth, bcrypt }, context) => {

// Basic Auth, which is allowed over HTTPS only:
Copy link
Contributor

Choose a reason for hiding this comment

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

can you please update this comment as well

Copy link
Contributor Author

@alxndrsn alxndrsn Sep 6, 2023

Choose a reason for hiding this comment

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

I'm having trouble writing a comment which would be more readable than the code below. What would you recommend?

Copy link
Member

Choose a reason for hiding this comment

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

How about "Basic Auth, which is allowed only over HTTPS and only if SSO is not enabled"? Or "Basic Auth, which is allowed only if certain conditions are true"? I think right now, the comment seems a little outdated since it mentions HTTPS but not SSO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR at #982

} else if (isPresent(authHeader) && authHeader.startsWith('Basic ')) {
if (oidc.isEnabled()) return reject(Problem.user.basicAuthNotSupportedWhenOidcEnabled());

// fail the request unless we are under HTTPS.
// this logic does mean that if we are not under nginx it is possible to fool the server.
// but it is the user's prerogative to undertake this bypass, so their security is in their hands.
Expand Down Expand Up @@ -104,13 +107,13 @@ const authHandler = ({ Sessions, Users, Auth, bcrypt }, context) => {
return;

// otherwise get the cookie contents.
const token = /session=([^;]+)(?:;|$)/.exec(context.headers.cookie);
const token = context.cookies[SESSION_COOKIE];
if (token == null)
return;

// actually try to authenticate with it. no Problem on failure. short circuit
// out if we have a GET or HEAD request.
const maybeSession = authBySessionToken(decodeURIComponent(token[1]));
const maybeSession = authBySessionToken(decodeURIComponent(token));
if ((context.method === 'GET') || (context.method === 'HEAD')) return maybeSession;

// if non-GET run authentication as usual but we'll have to check CSRF afterwards.
Expand Down
Loading