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

PLAT 638 Adjust login wih session and cookies #1180

Merged
merged 47 commits into from
Mar 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
661cfc0
PLAT 638 Adjust login wih session and cookies
nour-borgi Jan 30, 2023
1331d74
Merge branch 'master' into PLAT-638-adjust-login-cookies
nour-borgi Jan 30, 2023
375df90
Remove keycloak from config
nour-borgi Jan 30, 2023
0d81797
Update koaApi.js
nour-borgi Jan 30, 2023
1a8ff9f
Address feedback
nour-borgi Feb 2, 2023
8af79eb
Fixes
nour-borgi Feb 2, 2023
f302cc5
Remove unused import
nour-borgi Feb 3, 2023
0b7d5e6
Run and Fix eslint for all the files
nour-borgi Feb 3, 2023
d00df3b
Fix all the tests with new way of authentication
nour-borgi Feb 8, 2023
b576181
Fix status codes and updateUser and other fixes
nour-borgi Feb 8, 2023
2c8687d
Merge branch 'PLAT-638-adjust-login-cookies' of https://github.com/je…
nour-borgi Feb 8, 2023
6c9ad38
Fix setup of tests
nour-borgi Feb 8, 2023
7937958
Adjust defaults of config
nour-borgi Feb 8, 2023
307bb2c
Adjust default config and set 1s for cookies expiry in tests
nour-borgi Feb 8, 2023
b65f099
Add tests and fix others
nour-borgi Feb 8, 2023
3f4d658
Fix name of var + handle error and remove useless errors handling
nour-borgi Feb 8, 2023
b6e6c3a
Merge branch 'PLAT-638-adjust-login-cookies' of https://github.com/je…
nour-borgi Feb 8, 2023
9809ba0
Fix package-lock.json with node v 14
nour-borgi Feb 9, 2023
2d2d8c0
Revert tlsAuthentication.js
nour-borgi Feb 9, 2023
3cdaf2c
Add config description
nour-borgi Feb 9, 2023
25e38de
Remove unused passport module
nour-borgi Feb 16, 2023
7b4493d
Fixes and code improvements
nour-borgi Feb 20, 2023
9ea8a43
Fix userData update and basic auth
nour-borgi Feb 20, 2023
ec1a782
Merge branch 'PLAT-638-adjust-login-cookies' of https://github.com/je…
nour-borgi Feb 20, 2023
12538de
Bring back token strategy with deprecated warning
nour-borgi Feb 21, 2023
ad55d3d
Merge branch 'PLAT-638-adjust-login-cookies' of https://github.com/je…
nour-borgi Feb 22, 2023
a72c0d7
Fix authenticate middleware, fix user update and creation
nour-borgi Feb 22, 2023
c15b5cf
Merge branch 'PLAT-638-adjust-login-cookies' of https://github.com/je…
nour-borgi Feb 22, 2023
a65c99c
Add token tests
nour-borgi Feb 23, 2023
3bd1f53
Fix an auth type is not enabled + some other fixes
nour-borgi Feb 23, 2023
bd50f7b
Merge branch 'PLAT-638-adjust-login-cookies' of https://github.com/je…
nour-borgi Feb 23, 2023
fcb3017
Apply eslint
nour-borgi Feb 23, 2023
431ad69
Apply eslint
nour-borgi Feb 23, 2023
d19c4d6
Remove key from koaApi.js
nour-borgi Mar 2, 2023
1372b80
Update test/integration/auditAPITests.js
nour-borgi Mar 16, 2023
1364746
Update test/unit/passportTest.js
nour-borgi Mar 16, 2023
ddb95ad
Update test/unit/passportTest.js
nour-borgi Mar 16, 2023
8358ead
Update test/unit/passportTest.js
nour-borgi Mar 16, 2023
825e799
Update test/unit/passportTest.js
nour-borgi Mar 16, 2023
b497230
Update test/unit/usersTest.js
nour-borgi Mar 16, 2023
d7f4abc
Update test/unit/usersTest.js
nour-borgi Mar 16, 2023
ef2f2dd
Update test/unit/usersTest.js
nour-borgi Mar 16, 2023
0100068
Update test/unit/utilsTest.js
nour-borgi Mar 16, 2023
7f6d37e
Update test/integration/generalAPITests.js
nour-borgi Mar 16, 2023
a64eb6f
Update src/api/authentication.js
nour-borgi Mar 17, 2023
4c14812
Update authentication.js
nour-borgi Mar 24, 2023
ab76407
Merge pull request #1182 from jembi/PLAT-638-adjust-login-cookies-tes…
nour-borgi Mar 24, 2023
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
1 change: 1 addition & 0 deletions .eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"es6": true,
"node": true
},
"parser": "@babel/eslint-parser",
"parserOptions": {
"ecmaVersion": 2017,
"sourceType": "module"
Expand Down
13 changes: 12 additions & 1 deletion config/config.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,13 @@ The following config option are provided by the OpenHIM. All of these options ha
"timeout": 60000
},
"api": {
// The session secret key used for the hashing of signed cookie (used to detect if the client modified the cookie)
// Signed cookie is another cookie of the same name with the .sig suffix appended
"sessionKey": "r8q,+&1LM3)CD*zAGpx1xm{NeQhc;#",
// The session max age is the session cookie expiration time (in milliseconds)
"maxAge": 7200000,
// The number of characters that will be used to generate a random salt for the encryption of passwords
"salt": 10,
// The port that the OpenHIM API uses
"port": 8080,
// The protocol that the OpenHIM API uses
Expand All @@ -60,7 +67,11 @@ The following config option are provided by the OpenHIM. All of these options ha
// A message to append to detail strings that have been truncated
"truncateAppend": "\n[truncated ...]",
// The types of authentication to use for the API
// Supported types are "token" and "basic"
// Supported types are "token" and "basic" and "local"
// * "local" means through the UI with hitting "/authentication/local" endpoint with username and password,
// this will create a session for the user and set cookies in the browser.
// * "basic" means with basic auth either through browser or postman by giving also username and password.
// * [Deprecated] "token" means that a request should provide in the header an 'auth-token', 'auth-salt' and 'auth-ts' to be authenticated.
"authenicationTypes": ["token"]
},
"rerun": {
Expand Down
5 changes: 4 additions & 1 deletion config/default.json
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@
"pollPeriodMins": 60
},
"api": {
"sessionKey": "r8q,+&1LM3)CD*zAGpx1xm{NeQhc;#",
"maxAge": 7200000,
"salt": 10,
"enabled": true,
"protocol": "https",
"port": 8080,
Expand All @@ -39,7 +42,7 @@
"maxPayloadSizeMB": 50,
"truncateSize": 15000,
"truncateAppend": "\n[truncated ...]",
"authenticationTypes": ["basic", "token"]
nour-borgi marked this conversation as resolved.
Show resolved Hide resolved
"authenticationTypes": ["basic", "local", "token"]
},
"rerun": {
"httpPort": 7786,
Expand Down
2 changes: 1 addition & 1 deletion config/test.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
"maxPayloadSizeMB": 50,
"truncateSize": 10,
"truncateAppend": "\n[truncated ...]",
"authenticationTypes": ["token", "basic"]
"authenticationTypes": ["token", "basic", "local"]
},
"caching": {
"enabled": false
Expand Down
5,585 changes: 2,769 additions & 2,816 deletions package-lock.json

Large diffs are not rendered by default.

9 changes: 9 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
"test:seed": "node performance/seed.js",
"test:seed:ci": "npm run test:seed -- --quiet",
"start": "node lib/server.js",
"start:dev": "nodemon lib/server.js",
"stop": "pkill -SIGINT Core",
"spec": "speculate"
},
Expand All @@ -56,8 +57,11 @@
"kcors": "2.2.2",
"koa": "^2.13.0",
"koa-bodyparser": "^4.3.0",
"koa-compose": "^4.1.0",
"koa-compress": "^5.1.0",
"koa-passport": "^4.0.0",
"koa-route": "3.2.0",
"koa-session": "^6.3.1",
"lodash": "^4.17.20",
"moment": "^2.29.1",
"moment-timezone": "^0.5.31",
Expand All @@ -67,6 +71,9 @@
"mongoose-patch-history": "^2.0.0",
"nconf": "0.10.0",
"nodemailer": "^6.6.3",
"passport-custom": "^1.1.1",
"passport-http": "^0.3.0",
"passport-local": "^1.0.0",
"pem": "^1.14.4",
"raw-body": "^2.4.1",
"semver": "^7.3.2",
Expand All @@ -81,6 +88,7 @@
"devDependencies": {
"@babel/cli": "^7.15.4",
"@babel/core": "^7.15.5",
"@babel/eslint-parser": "^7.19.1",
"@babel/preset-env": "^7.15.6",
"@babel/register": "^7.15.3",
"codecov": "^3.8.3",
Expand All @@ -94,6 +102,7 @@
"faker": "^5.5.3",
"finalhandler": "^1.1.2",
"mocha": "^8.4.0",
"nodemon": "^2.0.20",
"nyc": "^15.1.0",
"prettier": "^2.4.0",
"progress": "2.0.3",
Expand Down
218 changes: 73 additions & 145 deletions src/api/authentication.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,13 @@
'use strict'

import atna from 'atna-audit'
import basicAuth from 'basic-auth'
import crypto from 'crypto'
import logger from 'winston'
import os from 'os'

import * as auditing from '../auditing'
import * as authorisation from './authorisation'
import {UserModelAPI} from '../model/users'
import {caseInsensitiveRegex, logAndSetResponse} from '../utils'
import passport from '../passport'
import {logAndSetResponse} from '../utils'
import {config} from '../config'
import {
BASIC_AUTH_TYPE,
Expand All @@ -36,108 +34,26 @@ const auditingExemptPaths = [
/\/logs/
]

const isUndefOrEmpty = string => string == null || string === ''

async function authenticateBasic(ctx) {
const credentials = basicAuth(ctx)
if (credentials == null) {
// No basic auth details found
return null
}
const {name: email, pass: password} = credentials
const user = await UserModelAPI.findOne({
email: caseInsensitiveRegex(email)
})
if (user == null) {
// not authenticated - user not found
ctx.throw(
401,
`No user exists for ${email}, denying access to API, request originated from ${ctx.request.host}`,
{email}
)
}
// Basic auth using middleware
await passport.authenticate('basic', function (err, user) {
nour-borgi marked this conversation as resolved.
Show resolved Hide resolved
if (user) {
ctx.req.user = user
ctx.body = 'User Authenticated Successfully'
ctx.status = 200
}
})(ctx, () => {})

const hash = crypto.createHash(user.passwordAlgorithm)
hash.update(user.passwordSalt)
hash.update(password)
if (user.passwordHash !== hash.digest('hex')) {
// not authenticated - password mismatch
ctx.throw(
401,
`Password did not match expected value, denying access to API, the request was made by ${email} from ${ctx.request.host}`,
{email}
)
}
return user
return ctx.req.user || null
}

/**
* @deprecated
*/
async function authenticateToken(ctx) {
nour-borgi marked this conversation as resolved.
Show resolved Hide resolved
const {header} = ctx.request
const email = header['auth-username']
const authTS = header['auth-ts']
const authSalt = header['auth-salt']
const authToken = header['auth-token']
await passport.authenticate('token')(ctx, () => {})

// if any of the required headers aren't present
if (
isUndefOrEmpty(email) ||
isUndefOrEmpty(authTS) ||
isUndefOrEmpty(authSalt) ||
isUndefOrEmpty(authToken)
) {
ctx.throw(
401,
`API request made by ${email} from ${ctx.request.host} is missing required API authentication headers, denying access`,
{email}
)
}

// check if request is recent
const requestDate = new Date(Date.parse(authTS))

const authWindowSeconds =
config.api.authWindowSeconds != null ? config.api.authWindowSeconds : 10
const to = new Date()
to.setSeconds(to.getSeconds() + authWindowSeconds)
const from = new Date()
from.setSeconds(from.getSeconds() - authWindowSeconds)

if (requestDate < from || requestDate > to) {
// request expired
ctx.throw(
401,
`API request made by ${email} from ${ctx.request.host} has expired, denying access`,
{email}
)
}

const user = await UserModelAPI.findOne({
email: caseInsensitiveRegex(email)
})
if (user == null) {
// not authenticated - user not found
ctx.throw(
401,
`No user exists for ${email}, denying access to API, request originated from ${ctx.request.host}`,
{email}
)
}

const hash = crypto.createHash('sha512')
hash.update(user.passwordHash)
hash.update(authSalt)
hash.update(authTS)

if (authToken !== hash.digest('hex')) {
// not authenticated - token mismatch
ctx.throw(
401,
`API token did not match expected value, denying access to API, the request was made by ${email} from ${ctx.request.host}`,
{email}
)
}

return user
return ctx.req.user || null
nour-borgi marked this conversation as resolved.
Show resolved Hide resolved
}

function getEnabledAuthenticationTypesFromConfig(config) {
Expand All @@ -160,20 +76,26 @@ function getEnabledAuthenticationTypesFromConfig(config) {
return []
}

function isAuthenticationTypeEnabled(type) {
export function isAuthenticationTypeEnabled(type) {
return getEnabledAuthenticationTypesFromConfig(config).includes(type)
}

async function authenticateRequest(ctx) {
let user
// First attempt basic authentication if enabled
if (user == null && isAuthenticationTypeEnabled('basic')) {
user = await authenticateBasic(ctx)
let user = null

// First attempt local authentication if enabled
if (ctx.req.user) {
user = ctx.req.user
}
// Otherwise try token based authentication if enabled
if (user == null && isAuthenticationTypeEnabled('token')) {
// Otherwise try token based authentication if enabled (@deprecated)
if (user == null) {
michaelloosen marked this conversation as resolved.
Show resolved Hide resolved
user = await authenticateToken(ctx)
}
// Otherwise try basic based authentication if enabled
if (user == null) {
// Basic auth using middleware
user = await authenticateBasic(ctx)
}
// User could not be authenticated
if (user == null) {
const enabledTypes =
Expand All @@ -195,9 +117,50 @@ function handleAuditResponse(err) {
}

export async function authenticate(ctx, next) {
let user
try {
user = await authenticateRequest(ctx)
// Authenticate Request either by basic or local or token
const user = await authenticateRequest(ctx)

if (ctx.isAuthenticated()) {
// Set the user on the context for consumption by other middleware
ctx.authenticated = user

// Deal with paths exempt from audit
if (ctx.path === '/transactions') {
if (
!ctx.query.filterRepresentation ||
ctx.query.filterRepresentation !== 'full'
) {
// exempt from auditing success
return next()
}
} else {
for (const pathTest of auditingExemptPaths) {
if (pathTest.test(ctx.path)) {
// exempt from auditing success
return next()
}
}
}
// Send an auth success audit event
let audit = atna.construct.userLoginAudit(
atna.constants.OUTCOME_SUCCESS,
himSourceID,
os.hostname(),
ctx.authenticated.email,
ctx.authenticated.groups.join(','),
ctx.authenticated.groups.join(',')
)
audit = atna.construct.wrapInSyslog(audit)
auditing.sendAuditEvent(audit, handleAuditResponse)

return next()
} else {
ctx.throw(
401,
`Denying access for an API request from ${ctx.request.host}`
)
}
} catch (err) {
// Handle authentication errors
if (err.status === 401) {
Expand All @@ -210,7 +173,7 @@ export async function authenticate(ctx, next) {
atna.constants.OUTCOME_SERIOUS_FAILURE,
himSourceID,
os.hostname(),
err.email
`Unknown with ip ${ctx.request.ip}`
)
audit = atna.construct.wrapInSyslog(audit)
auditing.sendAuditEvent(audit, handleAuditResponse)
Expand All @@ -219,41 +182,6 @@ export async function authenticate(ctx, next) {
// Rethrow other errors
throw err
}

// Set the user on the context for consumption by other middleware
ctx.authenticated = user

// Deal with paths exempt from audit
if (ctx.path === '/transactions') {
if (
!ctx.query.filterRepresentation ||
ctx.query.filterRepresentation !== 'full'
) {
// exempt from auditing success
return next()
}
} else {
for (const pathTest of auditingExemptPaths) {
if (pathTest.test(ctx.path)) {
// exempt from auditing success
return next()
}
}
}

// Send an auth success audit event
let audit = atna.construct.userLoginAudit(
atna.constants.OUTCOME_SUCCESS,
himSourceID,
os.hostname(),
user.email,
user.groups.join(','),
user.groups.join(',')
)
audit = atna.construct.wrapInSyslog(audit)
auditing.sendAuditEvent(audit, handleAuditResponse)

return next()
}

export async function getEnabledAuthenticationTypes(ctx, next) {
Expand Down
Loading