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

feat: add keycloak authentication closes #1634 #1716

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 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
3 changes: 3 additions & 0 deletions backend/src/clients/cognito.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ export async function listUsers(query: string, exactMatch = false) {
let dnName: string
let userPoolId: string
try {
if (!config?.oauth?.cognito) {
throw ConfigurationError('OAuth Cognito configuration is missing')
}
dnName = config.oauth.cognito.userIdAttribute
userPoolId = config.oauth.cognito.userPoolId
} catch (e) {
Expand Down
84 changes: 84 additions & 0 deletions backend/src/clients/keycloak.ts
Copy link
Member

Choose a reason for hiding this comment

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

Please include unit tests

Copy link
Member

Choose a reason for hiding this comment

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

There are a few eslint warnings that need addressing. I'd recommend installing an ESLint plugin for your IDE, like this one for VS Code https://marketplace.visualstudio.com/items?itemName=dbaeumer.vscode-eslint

Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
import c from 'config'
import { UserInformation } from '../connectors/authentication/Base.js'
import config from '../utils/config.js'
import { ConfigurationError, InternalError } from '../utils/error.js'

export async function listUsers(query: string, exactMatch = false) {
let dnName: string
let realm: string
try {
if (!config?.oauth?.keycloak) {
Copy link
Member

Choose a reason for hiding this comment

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

Why the ? after config?

Copy link
Member

Choose a reason for hiding this comment

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

For readability, I would recommend moving this if statement to just before the try statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

both this an configurationerror have been fixed, just missed them on a proof read

throw ConfigurationError('OAuth Keycloak configuration is missing')
}
realm = config.oauth.keycloak.realm
} catch (e) {
throw ConfigurationError('Cannot find userIdAttribute in oauth configuration', { oauthConfiguration: config?.oauth })
}

const token = await getKeycloakToken()

const filter = exactMatch ? `${query}` : `${query}*`
const url = `${config.oauth.keycloak.serverUrl}/admin/realms/${realm}/users?search=${filter}`

let results
try {
results = await fetch(url, {
Copy link
Member

Choose a reason for hiding this comment

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

Please use node-fetch as we have done for similar clients.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

utilised node-fetch and added types of Response to results

method: 'GET',
headers: {
Authorization: `Bearer ${token}`
}
})
} catch (err) {
throw InternalError('Error when querying Keycloak for users.', { err })
}
const resultsData = await results.json() as any[]
Copy link
Member

Choose a reason for hiding this comment

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

I would recommend using a type guard here rather than using a type assertion with an array of type any

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added in new commit

console.log(resultsData)
if (!resultsData || resultsData.length === 0) {
return []
}

const initialValue: Array<UserInformation & { dn: string }> = []
const users = resultsData.reduce((acc, keycloakUser) => {
const dn = keycloakUser.id // Assuming 'id' is the dnName
if (!dn) {
return acc
}
const email = keycloakUser.email
const name = keycloakUser.firstName
const info: UserInformation = {
...(email && { email }),
...(name && { name }),
}
acc.push({ ...info, dn })
return acc
}, initialValue)
return users
}

async function getKeycloakToken() {
if (!config?.oauth?.keycloak) {
throw ConfigurationError('OAuth Keycloak configuration is missing')
}
const url = `${config.oauth.keycloak.serverUrl}/realms/${config.oauth.keycloak.realm}/protocol/openid-connect/token`
const params = new URLSearchParams()
params.append('client_id', config.oauth.keycloak.clientId)
params.append('client_secret', config.oauth.keycloak.clientSecret)
params.append('grant_type', 'client_credentials')

try {
const response = await fetch(url, {
method: 'POST',
headers: {
'Content-Type': 'application/x-www-form-urlencoded'
},
body: params
})
const data = await response.json() as { access_token: string }
Copy link
Member

Choose a reason for hiding this comment

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

See other comment about using type guards rather type assertions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added as above let me know if anything else needed

if (!data.access_token) {
throw InternalError('Access token is missing in the response', { response: data })
}
return data.access_token
} catch (err) {
throw InternalError('Error when obtaining Keycloak token.', { err })
}
}
14 changes: 13 additions & 1 deletion backend/src/connectors/authentication/oauth.ts
Copy link
Member

Choose a reason for hiding this comment

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

Did you consider having two separate connectors, one for oauthCognito and one for oauthKeyloak? It might be slightly clearer to keep them separate rather than have one with conditional logic. The conditional logic might just be one if statement currently but more will be needed when support for groups and roles are added. Having them separate will probably make adding changes to either the Cognito or Keycloak implementations easier. I don't think there is much difference though, let me know your thoughts.

Copy link
Member

Choose a reason for hiding this comment

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

I think this bullet point you've included in an earlier comment summarises the drawback in have a single connector like you have here "enabling a toggle system based on the config for the application to load the correct provider". The connector design itself is a toggle and so your implementation is adding a toggle within a toggle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the reasoning for going with this implementation is then each client type say you had a google oauth or azure entra is a future version, the flexibility provided comes from each client has a config and then the main oauth connector acts as a 'resolver'. So this implementation seemed to fit with the existing design to an extent with code seperated for the client and connector type

The challenge of splitting the connectors/clients completely apart would probably be duplication of code despite simplifying the configuration. I was aiming to take inspiration from next-auth where you have a general OAuth Connector and then configure your client to use for that OAuth connection. e.g?

https://github.com/nextauthjs/next-auth/blob/main/packages/core/src/index.ts
https://github.com/nextauthjs/next-auth/blob/main/packages/core/src/providers/cognito.ts

I think your lead on which way to implement is obviously best as I want to alter your codebase, so for the rest of comments for now I'll fix with the current implementation where possible but we can look at how this get structured. Either way is a trade of in flexibility at either a config or code level really.

Copy link
Member

Choose a reason for hiding this comment

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

Hi, apologies for the delayed response and thank you for your reply. For maintainability going forward I still think it would be best to have separate connectors for Keycloak and Cognito. I think this would simplify the configuration and simplify the implementation due to not needing to have conditionals for every provider that gets added going forward in a single class. I do agree though agree with your concern about duplication so I'd suggest having an abstract oauth base class that contains common oauth code. This would be the existing authenticationMiddleware related functions and would extend the existing base authentication connector. We could then have separate oauth connectors for each provider that extends the new oauth base class and implements all of the abstract methods in the current base class, the implementation of which will likely be provider specific.

Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,26 @@ import { NextFunction, Request, Response, Router } from 'express'
import session from 'express-session'
import grant from 'grant'

import { listUsers } from '../../clients/cognito.js'
import { listUsers as listUsersCognito } from '../../clients/cognito.js'
import { listUsers as listUsersKeycloak } from '../../clients/keycloak.js'

import { UserInterface } from '../../models/User.js'
import config from '../../utils/config.js'
import { getConnectionURI } from '../../utils/database.js'
import { fromEntity, toEntity } from '../../utils/entity.js'
import { InternalError, NotFound } from '../../utils/error.js'
import { BaseAuthenticationConnector, RoleKeys, UserInformation } from './Base.js'

function listUsers(query: string, exactMatch = false) {
if (config.oauth.cognito) {
return listUsersCognito(query, exactMatch)
} else if (config.oauth.keycloak) {
return listUsersKeycloak(query, exactMatch)
} else {
throw InternalError('No oauth configuration found', { oauthConfiguration: config.oauth })
}
}

const OauthEntityKind = {
User: 'user',
} as const
Expand Down
16 changes: 15 additions & 1 deletion backend/src/utils/config.ts
Copy link
Member

Choose a reason for hiding this comment

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

Please update the helm chart configuration with these changes here https://github.com/gchq/Bailo/blob/main/infrastructure/helm/bailo/templates/bailo/bailo.configmap.yaml and the values file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated chart with configmap and values changes.

Original file line number Diff line number Diff line change
Expand Up @@ -114,11 +114,17 @@ export interface Config {
oauth: {
provider: string
grant: grant.GrantConfig | grant.GrantOptions
cognito: {
cognito?: {
Copy link
Member

Choose a reason for hiding this comment

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

Having optional blocks of configuration isn't something we've decided to do for things like this previously as you can see from the oauth block which would only be used if the oauth connector is selected. We define everything anyway in the default configuration as a method of documenting example configuration so everything should always been defined there. This is another reason (in addition to https://github.com/gchq/Bailo/pull/1716/files#r190195685) why I'd have two separate connectors that can be chosen using the existing authentication connector config value rather than the presence of an optional config block. Most of the other existing optional config like the oauth config is very similar to this and is connector related and it only get used if the relevant connector is selected. As we don't explicitly define any of the configuration as optional though, it probably would a good idea to include this in documentation. I'll make a note to include this information in the connector documentation we're planning on adding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added my thoughts around this to the top comment, let me know what you would prefer and I'm happy to add around this as needed

identityProviderClient: { region: string; credentials: { accessKeyId: string; secretAccessKey: string } }
userPoolId: string
userIdAttribute: string
}
keycloak?: {
realm: string
clientId: string
clientSecret: string
serverUrl: string
}
}

defaultSchemas: {
Expand Down Expand Up @@ -172,4 +178,12 @@ export interface Config {
}

const config: Config = _config.util.toObject()

if (config.oauth &&
!config.oauth.keycloak &&
!config.oauth.cognito
) {
throw new Error('If OAuth is configured, either Keycloak or Cognito configuration must be provided.')
Copy link
Member

Choose a reason for hiding this comment

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

This error type would be well suited here

export function ConfigurationError(message: string, context?: BailoError['context'], logger?: Logger) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

altered

}

export default deepFreeze(config) as Config