From f56da27078210a41b14bf9a9fa6d394f681c4d73 Mon Sep 17 00:00:00 2001 From: shreddedbacon Date: Wed, 15 Feb 2023 19:07:38 +1100 Subject: [PATCH 01/11] chore: add mapper for projectid roles --- .../javascript/META-INF/keycloak-scripts.json | 5 ++ .../javascript/mappers/group-project-roles.js | 46 +++++++++++++++++++ .../startup-scripts/00-configure-lagoon.sh | 24 ++++++++++ 3 files changed, 75 insertions(+) create mode 100644 services/keycloak/javascript/mappers/group-project-roles.js diff --git a/services/keycloak/javascript/META-INF/keycloak-scripts.json b/services/keycloak/javascript/META-INF/keycloak-scripts.json index 94536c74d4..f769aa96e0 100644 --- a/services/keycloak/javascript/META-INF/keycloak-scripts.json +++ b/services/keycloak/javascript/META-INF/keycloak-scripts.json @@ -4,6 +4,11 @@ "name": "[Lagoon] Groups and Roles", "description": "Gets a users groups and roles", "fileName": "mappers/groups-and-roles.js" + }, + { + "name": "[Lagoon] Group and Project ID Roles", + "description": "Gets a users groups and project roles with IDs", + "fileName": "mappers/group-project-roles.js" } ], "policies": [ diff --git a/services/keycloak/javascript/mappers/group-project-roles.js b/services/keycloak/javascript/mappers/group-project-roles.js new file mode 100644 index 0000000000..143fcb0517 --- /dev/null +++ b/services/keycloak/javascript/mappers/group-project-roles.js @@ -0,0 +1,46 @@ +var ArrayList = Java.type("java.util.ArrayList"); +var HashMap = Java.type("java.util.HashMap"); +var HashSet = Java.type("java.util.HashSet"); +var groupProjectIds = new HashMap(); + +var groups = user.getGroups() + +var forEach = Array.prototype.forEach; +// add all groups the user is part of +forEach.call(groups.toArray(), function(group) { + var groupName = group.getName().replace(/-(owner|maintainer|developer|reporter|guest)$/,""); + var groupRoles = group.getRoleMappings(); + if(group.getFirstAttribute("type") == "role-subgroup") { + var parent = group.getParent(); + var projectIds = parent.getFirstAttribute("lagoon-projects"); + if(groupRoles.length == 1) { + groupRoles.forEach(function(roleModel) { + var rn = roleModel.getName(); + // array of project ids + var projectIdList = new ArrayList(); + if(projectIds !== null) { + var splitIds = new ArrayList(projectIds.split(",")) + forEach.call(splitIds, function(g) { + projectIdList.add(parseInt(g)) + }); + if (groupProjectIds[rn]) { + if (groupProjectIds[rn].length > 0) { + for(var a=0; a < groupProjectIds[rn].length; a++){ + if (!splitIds.contains(groupProjectIds[rn][a])) { + projectIdList.add(parseInt(groupProjectIds[rn][a])) + } + } + } + } + } + if (projectIdList.length > 0) { + groupProjectIds.put(rn, projectIdList) + } + }); + return; + } + } + return; +}); + +token.setOtherClaims("project_group_roles", groupProjectIds); \ No newline at end of file diff --git a/services/keycloak/startup-scripts/00-configure-lagoon.sh b/services/keycloak/startup-scripts/00-configure-lagoon.sh index 9db93075c0..ddabc9eb6a 100755 --- a/services/keycloak/startup-scripts/00-configure-lagoon.sh +++ b/services/keycloak/startup-scripts/00-configure-lagoon.sh @@ -1959,6 +1959,29 @@ EOF EOF } +function add_group_project_token_mapper { + CLIENT_ID=$(/opt/jboss/keycloak/bin/kcadm.sh get -r lagoon clients?clientId=api --config $CONFIG_PATH | jq -r '.[0]["id"]') + ui_client_id=$(/opt/jboss/keycloak/bin/kcadm.sh get -r lagoon clients?clientId=lagoon-ui --config $CONFIG_PATH | jq -r '.[0]["id"]') + echo "AA $ui_client_id" + group_project_token_mappers=$(/opt/jboss/keycloak/bin/kcadm.sh get -r lagoon clients/$ui_client_id/protocol-mappers/models --config $CONFIG_PATH) + echo "AA $group_project_token_mappers" + old_mapper_id=$(echo $group_project_token_mappers | jq -r '.[] | select(.name=="group_project_role_ids") | .id') + + echo "AA $old_mapper_id" + if [ "$old_mapper_id" != "" ]; then + echo "group_project_role_ids token mapper already configured" + return 0 + fi + auth_server_client_id=$(/opt/jboss/keycloak/bin/kcadm.sh get -r lagoon clients?clientId=auth-server --config $CONFIG_PATH | jq -r '.[0]["id"]') + api_client_id=$(/opt/jboss/keycloak/bin/kcadm.sh get -r lagoon clients?clientId=api --config $CONFIG_PATH | jq -r '.[0]["id"]') + + echo Configuring Group and Project Role Token Mapper + echo '{"name":"group_project_role_ids","protocolMapper":"script-mappers/group-project-roles.js","protocol":"openid-connect","config":{"id.token.claim":"true","access.token.claim":"true","userinfo.token.claim":"true","multivalued":"true","claim.name":"project_group_roles","jsonType.label":"String"}}' | /opt/jboss/keycloak/bin/kcadm.sh create -r ${KEYCLOAK_REALM:-master} clients/$ui_client_id/protocol-mappers/models --config $CONFIG_PATH -f - + echo '{"name":"group_project_role_ids","protocolMapper":"script-mappers/group-project-roles.js","protocol":"openid-connect","config":{"id.token.claim":"true","access.token.claim":"true","userinfo.token.claim":"true","multivalued":"true","claim.name":"project_group_roles","jsonType.label":"String"}}' | /opt/jboss/keycloak/bin/kcadm.sh create -r ${KEYCLOAK_REALM:-master} clients/$auth_server_client_id/protocol-mappers/models --config $CONFIG_PATH -f - + echo '{"name":"group_project_role_ids","protocolMapper":"script-mappers/group-project-roles.js","protocol":"openid-connect","config":{"id.token.claim":"true","access.token.claim":"true","userinfo.token.claim":"true","multivalued":"true","claim.name":"project_group_roles","jsonType.label":"String"}}' | /opt/jboss/keycloak/bin/kcadm.sh create -r ${KEYCLOAK_REALM:-master} clients/$api_client_id/protocol-mappers/models --config $CONFIG_PATH -f - + +} + ################## # Initialization # ################## @@ -1996,6 +2019,7 @@ function configure_keycloak { migrate_to_js_provider add_delete_env_var_permissions configure_lagoon_opensearch_sync_client + add_group_project_token_mapper # always run last sync_client_secrets From dbaa6ef9559374b7ea0f34a5218d49ac9f5df371 Mon Sep 17 00:00:00 2001 From: shreddedbacon Date: Wed, 15 Feb 2023 19:10:08 +1100 Subject: [PATCH 02/11] refactor: optimize keycloak queries, new resolver for group members to help reduce group only queries --- services/api/src/models/group.ts | 172 +++++++-------- services/api/src/models/user.ts | 39 ++-- services/api/src/resolvers.js | 4 +- .../api/src/resources/deployment/resolvers.ts | 22 +- services/api/src/resources/fact/resolvers.ts | 15 +- services/api/src/resources/group/resolvers.ts | 37 ++++ .../api/src/resources/project/resolvers.ts | 16 +- services/api/src/util/auth.ts | 199 +++++++++++------- 8 files changed, 293 insertions(+), 211 deletions(-) diff --git a/services/api/src/models/group.ts b/services/api/src/models/group.ts index edde368a7d..b6cb3bdfe2 100644 --- a/services/api/src/models/group.ts +++ b/services/api/src/models/group.ts @@ -121,7 +121,9 @@ export const Group = (clients: { groups: R.isEmpty(subGroups) ? [] : await transformKeycloakGroups(subGroups), - members: await getGroupMembership(group) + // retrieving members is a heavy operation + // this is now its own resolver + // members: await getGroupMembership(group) }); } @@ -130,7 +132,7 @@ export const Group = (clients: { const loadGroupById = async (id: string): Promise => { const keycloakGroup = await keycloakAdminClient.groups.findOne({ - id + id, }); if (R.isNil(keycloakGroup)) { @@ -187,14 +189,9 @@ export const Group = (clients: { }; const loadAllGroups = async (): Promise => { - const keycloakGroups = await keycloakAdminClient.groups.find(); + const keycloakGroups = await keycloakAdminClient.groups.find({briefRepresentation: false}); - let fullGroups: Group[] = []; - for (const group of keycloakGroups) { - const fullGroup = await loadGroupById(group.id); - - fullGroups = [...fullGroups, fullGroup]; - } + const fullGroups = transformKeycloakGroups(keycloakGroups); return fullGroups; }; @@ -230,18 +227,9 @@ export const Group = (clients: { const loadGroupsByAttribute = async ( filterFn: AttributeFilterFn ): Promise => { - const keycloakGroups = await keycloakAdminClient.groups.find(); - - let fullGroups: Group[] = []; - for (const group of keycloakGroups) { - const fullGroup = await keycloakAdminClient.groups.findOne({ - id: group.id - }); - - fullGroups = [...fullGroups, fullGroup]; - } + const keycloakGroups = await keycloakAdminClient.groups.find({briefRepresentation: false}); - const filteredGroups = filterGroupsByAttribute(fullGroups, filterFn); + const filteredGroups = filterGroupsByAttribute(keycloakGroups, filterFn); const groups = await transformKeycloakGroups(filteredGroups); @@ -260,39 +248,27 @@ export const Group = (clients: { return false; }; - let groupIds = []; - + // let groupIds = []; // This function is called often and is expensive to compute so prefer // performance over DRY - try { - groupIds = await redisClient.getProjectGroupsCache(projectId); - } catch (err) { - logger.warn(`Error loading project groups from cache: ${err.message}`); - groupIds = []; - } + // try { + // groupIds = await redisClient.getProjectGroupsCache(projectId); + // } catch (err) { + // logger.warn(`Error loading project groups from cache: ${err.message}`); + // groupIds = []; + // } - if (R.isEmpty(groupIds)) { - const keycloakGroups = await keycloakAdminClient.groups.find(); - // @ts-ignore - groupIds = R.pluck('id', keycloakGroups); - } - - let fullGroups = []; - for (const id of groupIds) { - const fullGroup = await keycloakAdminClient.groups.findOne({ - id - }); + // this request will be huge, but it is significantly faster than the alternative iteration that followed previously + let fullGroups = await keycloakAdminClient.groups.find({briefRepresentation: false}); - fullGroups = [...fullGroups, fullGroup]; - } const filteredGroups = filterGroupsByAttribute(fullGroups, filterFn); - try { - const filteredGroupIds = R.pluck('id', filteredGroups); - await redisClient.saveProjectGroupsCache(projectId, filteredGroupIds); - } catch (err) { - logger.warn(`Error saving project groups to cache: ${err.message}`); - } + // try { + // const filteredGroupIds = R.pluck('id', filteredGroups); + // await redisClient.saveProjectGroupsCache(projectId, filteredGroupIds); + // } catch (err) { + // logger.warn(`Error saving project groups to cache: ${err.message}`); + // } const groups = await transformKeycloakGroups(filteredGroups); @@ -491,9 +467,9 @@ export const Group = (clients: { }; const deleteGroup = async (id: string): Promise => { - const group = loadGroupById(id); + // const group = loadGroupById(id); // @ts-ignore - const projectIds = getProjectIdsFromGroup(group); + // const projectIds = getProjectIdsFromGroup(group); try { await keycloakAdminClient.groups.del({ id }); @@ -505,13 +481,13 @@ export const Group = (clients: { } } - for (const projectId of projectIds) { - try { - await redisClient.deleteProjectGroupsCache(projectId); - } catch (err) { - logger.warn(`Error deleting project groups cache: ${err.message}`); - } - } + // for (const projectId of projectIds) { + // try { + // await redisClient.deleteProjectGroupsCache(projectId); + // } catch (err) { + // logger.warn(`Error deleting project groups cache: ${err.message}`); + // } + // } }; const addUserToGroup = async ( @@ -553,11 +529,11 @@ export const Group = (clients: { throw new Error(`Could not add user to group: ${err.message}`); } - try { - await redisClient.deleteRedisUserCache(user.id); - } catch (err) { - logger.warn(`Error deleting user cache ${user.id}: ${err}`); - } + // try { + // await redisClient.deleteRedisUserCache(user.id); + // } catch (err) { + // logger.warn(`Error deleting user cache ${user.id}: ${err}`); + // } return await loadGroupById(group.id); }; @@ -582,11 +558,11 @@ export const Group = (clients: { throw new Error(`Could not remove user from group: ${err.message}`); } - try { - await redisClient.deleteRedisUserCache(user.id); - } catch (err) { - logger.warn(`Error deleting user cache ${user.id}: ${err}`); - } + // try { + // await redisClient.deleteRedisUserCache(user.id); + // } catch (err) { + // logger.warn(`Error deleting user cache ${user.id}: ${err}`); + // } } return await loadGroupById(group.id); @@ -626,21 +602,21 @@ export const Group = (clients: { } // Clear the cache for users that gained access to the project - const groupAndParentsMembers = await getMembersFromGroupAndParents(group); - const userIds = R.map(R.path(['user', 'id']), groupAndParentsMembers); - for (const userId of userIds) { - try { - await redisClient.deleteRedisUserCache(userId); - } catch (err) { - logger.warn(`Error deleting user cache ${userId}: ${err}`); - } - } - - try { - await redisClient.deleteProjectGroupsCache(projectId); - } catch (err) { - logger.warn(`Error deleting project groups cache: ${err.message}`); - } + // const groupAndParentsMembers = await getMembersFromGroupAndParents(group); + // const userIds = R.map(R.path(['user', 'id']), groupAndParentsMembers); + // for (const userId of userIds) { + // try { + // await redisClient.deleteRedisUserCache(userId); + // } catch (err) { + // logger.warn(`Error deleting user cache ${userId}: ${err}`); + // } + // } + + // try { + // await redisClient.deleteProjectGroupsCache(projectId); + // } catch (err) { + // logger.warn(`Error deleting project groups cache: ${err.message}`); + // } }; const removeProjectFromGroup = async ( @@ -676,21 +652,21 @@ export const Group = (clients: { } // Clear the cache for users that lost access to the project - const groupAndParentsMembers = await getMembersFromGroupAndParents(group); - const userIds = R.map(R.path(['user', 'id']), groupAndParentsMembers); - for (const userId of userIds) { - try { - await redisClient.deleteRedisUserCache(userId); - } catch (err) { - logger.warn(`Error deleting user cache ${userId}: ${err}`); - } - } - - try { - await redisClient.deleteProjectGroupsCache(projectId); - } catch (err) { - logger.warn(`Error deleting project groups cache: ${err.message}`); - } + // const groupAndParentsMembers = await getMembersFromGroupAndParents(group); + // const userIds = R.map(R.path(['user', 'id']), groupAndParentsMembers); + // for (const userId of userIds) { + // try { + // await redisClient.deleteRedisUserCache(userId); + // } catch (err) { + // logger.warn(`Error deleting user cache ${userId}: ${err}`); + // } + // } + + // try { + // await redisClient.deleteProjectGroupsCache(projectId); + // } catch (err) { + // logger.warn(`Error deleting project groups cache: ${err.message}`); + // } }; return { @@ -709,6 +685,8 @@ export const Group = (clients: { addUserToGroup, removeUserFromGroup, addProjectToGroup, - removeProjectFromGroup + removeProjectFromGroup, + transformKeycloakGroups, + getGroupMembership }; }; diff --git a/services/api/src/models/user.ts b/services/api/src/models/user.ts index c1ff34755b..527e46674f 100644 --- a/services/api/src/models/user.ts +++ b/services/api/src/models/user.ts @@ -3,6 +3,7 @@ import pickNonNil from '../util/pickNonNil'; import { logger } from '../loggers/logger'; import UserRepresentation from 'keycloak-admin/lib/defs/userRepresentation'; import { Group, isRoleSubgroup } from './group'; +import { GroupInterface } from '../resolvers'; export interface User { email: string; @@ -30,10 +31,11 @@ interface UserModel { loadUserByUsername: (username: string) => Promise; loadUserByIdOrUsername: (userInput: UserEdit) => Promise; getAllGroupsForUser: (userInput: User) => Promise; - getAllProjectsIdsForUser: (userInput: User) => Promise; + getAllProjectsIdsForUser: (userInput: User) => Promise<[number[], Group[]]>; getUserRolesForProject: ( userInput: User, - projectId: number + projectId: number, + userGroups: Group[] ) => Promise; addUser: (userInput: User) => Promise; updateUser: (userInput: UserEdit) => Promise; @@ -215,17 +217,15 @@ export const User = (clients: { let groups = []; const roleSubgroups = await keycloakAdminClient.users.listGroups({ - id: userInput.id + id: userInput.id, + briefRepresentation: false }); for (const roleSubgroup of roleSubgroups) { - const fullRoleSubgroup = await GroupModel.loadGroupById(roleSubgroup.id); - if (!isRoleSubgroup(fullRoleSubgroup)) { - continue; - } - - const roleSubgroupParent = await GroupModel.loadParentGroup( - fullRoleSubgroup + // just look up the group with what we know the parent name to be with the regex of the role stripped + let regexp = /-(owner|maintainer|developer|reporter|guest)$/g; + const roleSubgroupParent = await GroupModel.loadGroupByName( + roleSubgroup.name.replace(regexp, "") ); groups.push(roleSubgroupParent); @@ -236,7 +236,7 @@ export const User = (clients: { const getAllProjectsIdsForUser = async ( userInput: User - ): Promise => { + ): Promise<[number[], Group[]]> => { const GroupModel = Group(clients); let projects = []; @@ -249,17 +249,16 @@ export const User = (clients: { projects = [...projects, ...projectIds]; } - return R.uniq(projects); + return [R.uniq(projects), userGroups]; }; const getUserRolesForProject = async ( userInput: User, - projectId: number + projectId: number, + userGroups: Group[] ): Promise => { const GroupModel = Group(clients); - const userGroups = await getAllGroupsForUser(userInput); - let roles = []; for (const group of userGroups) { const projectIds = await GroupModel.getProjectsFromGroupAndSubgroups( @@ -365,11 +364,11 @@ export const User = (clients: { throw new Error(`Error deleting user ${id}: ${err}`); } } - try { - await redisClient.deleteRedisUserCache(id); - } catch (err) { - logger.error(`Error deleting user cache ${id}: ${err}`); - } + // try { + // await redisClient.deleteRedisUserCache(id); + // } catch (err) { + // logger.error(`Error deleting user cache ${id}: ${err}`); + // } }; return { diff --git a/services/api/src/resolvers.js b/services/api/src/resolvers.js index d1f6be2a89..ae85db15ad 100644 --- a/services/api/src/resolvers.js +++ b/services/api/src/resolvers.js @@ -226,6 +226,7 @@ const { removeUserFromGroup, addGroupsToProject, removeGroupsFromProject, + getMembersByGroupId, } = require('./resources/group/resolvers'); const { @@ -362,7 +363,8 @@ const resolvers = { }, }, Group: { - projects: getAllProjectsByGroupId + projects: getAllProjectsByGroupId, + members: getMembersByGroupId }, DeployTargetConfig: { project: getProjectById, diff --git a/services/api/src/resources/deployment/resolvers.ts b/services/api/src/resources/deployment/resolvers.ts index 1893323c9b..6df76a99b9 100644 --- a/services/api/src/resources/deployment/resolvers.ts +++ b/services/api/src/resources/deployment/resolvers.ts @@ -33,6 +33,7 @@ import sha1 from 'sha1'; import { generateBuildId } from '@lagoon/commons/dist/util/lagoon'; import { jsonMerge } from '@lagoon/commons/dist/util/func'; import { logger } from '../../loggers/logger'; +import { getUserProjectIdsFromToken } from '../../util/auth'; // @ts-ignore import uuid4 from 'uuid4'; @@ -135,9 +136,10 @@ export const getDeploymentsByBulkId: ResolverFn = async ( return []; } - userProjectIds = await models.UserModel.getAllProjectsIdsForUser({ - id: keycloakGrant.access_token.content.sub - }); + // pull the project ids from the token + // this can have a negative effect if the token is not refreshed after performing an operation like adding a project + // the user will probably have to refresh their token + userProjectIds = getUserProjectIdsFromToken(keycloakGrant); } let queryBuilder = knex('deployment') @@ -184,9 +186,10 @@ export const getDeploymentsByFilter: ResolverFn = async ( return []; } - userProjectIds = await models.UserModel.getAllProjectsIdsForUser({ - id: keycloakGrant.access_token.content.sub - }); + // pull the project ids from the token + // this can have a negative effect if the token is not refreshed after performing an operation like adding a project + // the user will probably have to refresh their token + userProjectIds = getUserProjectIdsFromToken(keycloakGrant); } let queryBuilder = knex.select("deployment.*").from('deployment'). @@ -1210,9 +1213,10 @@ export const bulkDeployEnvironmentLatest: ResolverFn = async ( return []; } - userProjectIds = await models.UserModel.getAllProjectsIdsForUser({ - id: keycloakGrant.access_token.content.sub - }); + // pull the project ids from the token + // this can have a negative effect if the token is not refreshed after performing an operation like adding a project + // the user will probably have to refresh their token + userProjectIds = getUserProjectIdsFromToken(keycloakGrant); } let bulkId = uuid4(); diff --git a/services/api/src/resources/fact/resolvers.ts b/services/api/src/resources/fact/resolvers.ts index c8304c3ad9..01fc6f234b 100644 --- a/services/api/src/resources/fact/resolvers.ts +++ b/services/api/src/resources/fact/resolvers.ts @@ -9,6 +9,7 @@ import crypto from 'crypto'; import { Service } from 'aws-sdk'; import * as api from '@lagoon/commons/dist/api'; import { getEnvironmentsByProjectId } from '../environment/resolvers'; +import { getUserProjectIdsFromToken } from '../../util/auth'; export const getFactsByEnvironmentId: ResolverFn = async ( { id: environmentId, environmentAuthz }, @@ -136,9 +137,10 @@ export const getProjectsByFactSearch: ResolverFn = async ( return []; } - userProjectIds = await models.UserModel.getAllProjectsIdsForUser({ - id: keycloakGrant.access_token.content.sub - }); + // pull the project ids from the token + // this can have a negative effect if the token is not refreshed after performing an operation like adding a project + // the user will probably have to refresh their token + userProjectIds = getUserProjectIdsFromToken(keycloakGrant); } const count = await getFactFilteredProjectsCount(input, userProjectIds, sqlClientPool, isAdmin); @@ -168,9 +170,10 @@ export const getEnvironmentsByFactSearch: ResolverFn = async ( return []; } - userProjectIds = await models.UserModel.getAllProjectsIdsForUser({ - id: keycloakGrant.access_token.content.sub - }); + // pull the project ids from the token + // this can have a negative effect if the token is not refreshed after performing an operation like adding a project + // the user will probably have to refresh their token + userProjectIds = getUserProjectIdsFromToken(keycloakGrant); } const count = await getFactFilteredEnvironmentsCount(input, userProjectIds, sqlClientPool, isAdmin); diff --git a/services/api/src/resources/group/resolvers.ts b/services/api/src/resources/group/resolvers.ts index ba0eca6fbc..509eb6f806 100644 --- a/services/api/src/resources/group/resolvers.ts +++ b/services/api/src/resources/group/resolvers.ts @@ -54,6 +54,43 @@ export const getAllGroups: ResolverFn = async ( } }; +export const getMembersByGroupId: ResolverFn = async ( + { id }, + _input, + { hasPermission, models, keycloakGrant } +) => { + try { + await hasPermission('group', 'viewAll'); + const group = await models.GroupModel.loadGroupById(id); + const members = await models.GroupModel.getGroupMembership(group); + return members; + } catch (err) { + if (err instanceof KeycloakUnauthorizedError) { + if (!keycloakGrant) { + logger.debug('No grant available for getGroupByName'); + throw new GroupNotFoundError(`Group not found: ${id}`); + } else { + const user = await models.UserModel.loadUserById( + keycloakGrant.access_token.content.sub + ); + const userGroups = await models.UserModel.getAllGroupsForUser(user); + + const group = R.head(R.filter(R.propEq('id', id), userGroups)); + + if (R.isEmpty(group)) { + throw new GroupNotFoundError(`Group not found: ${id}`); + } + const members = await models.GroupModel.getGroupMembership(group); + + return members; + } + } + + logger.warn(`getGroupByName failed unexpectedly: ${err.message}`); + throw err; + } +} + export const getGroupsByProjectId: ResolverFn = async ( { id: pid }, _input, diff --git a/services/api/src/resources/project/resolvers.ts b/services/api/src/resources/project/resolvers.ts index 69d0c79ddd..90a82274fd 100644 --- a/services/api/src/resources/project/resolvers.ts +++ b/services/api/src/resources/project/resolvers.ts @@ -12,6 +12,7 @@ import * as OS from '../openshift/sql'; import { generatePrivateKey, getSshKeyFingerprint } from '../sshKey'; import { Sql as sshKeySql } from '../sshKey/sql'; import { createHarborOperations } from './harborSetup'; +import { getUserProjectIdsFromToken } from '../../util/auth'; import sql from '../user/sql'; const DISABLE_CORE_HARBOR = process.env.DISABLE_CORE_HARBOR || "false" @@ -62,9 +63,10 @@ export const getAllProjects: ResolverFn = async ( return []; } - userProjectIds = await models.UserModel.getAllProjectsIdsForUser({ - id: keycloakGrant.access_token.content.sub - }); + // pull the project ids from the token + // this can have a negative effect if the token is not refreshed after performing an operation like adding a project + // the user will probably have to refresh their token + userProjectIds = getUserProjectIdsFromToken(keycloakGrant); } let queryBuilder = knex('project'); @@ -211,10 +213,10 @@ export const getProjectsByMetadata: ResolverFn = async ( logger.debug('No grant available for getProjectsByMetadata'); return []; } - - userProjectIds = await models.UserModel.getAllProjectsIdsForUser({ - id: keycloakGrant.access_token.content.sub - }); + // pull the project ids from the token + // this can have a negative effect if the token is not refreshed after performing an operation like adding a project + // the user will probably have to refresh their token + userProjectIds = getUserProjectIdsFromToken(keycloakGrant); } let queryBuilder = knex('project'); diff --git a/services/api/src/util/auth.ts b/services/api/src/util/auth.ts index 3bd3633b29..8981e38218 100644 --- a/services/api/src/util/auth.ts +++ b/services/api/src/util/auth.ts @@ -41,6 +41,49 @@ const sortRolesByWeight = (a, b) => { return 0; }; +export const getUserProjectIdsFromToken = ( + token +): number[] => { + const groupRoleIds = token.access_token.content.project_group_roles + let upids = []; + for (const r in groupRoleIds) { + for (const pid in groupRoleIds[r]) { + upids.indexOf(groupRoleIds[r][pid]) === -1 ? upids.push(groupRoleIds[r][pid]) : "" + } + } + return R.uniq(upids); +}; + +export const getUserRoleForProjectFromToken = ( + token, projectId +): [string, number[]] => { + const groupRoleIds = token.access_token.content.project_group_roles + let upids = []; + let roles = []; + for (const r in groupRoleIds) { + for (const pid in groupRoleIds[r]) { + upids.indexOf(groupRoleIds[r][pid]) === -1 ? upids.push(groupRoleIds[r][pid]) : "" + if (projectId == groupRoleIds[r][pid]) { + roles.push(r) + } + } + } + + const highestRoleForProject = getHighestRole(roles); + + return [highestRoleForProject, R.uniq(upids)]; +}; + +const getHighestRole = (roles) => { + return R.pipe( + R.uniq, + R.reject(R.isEmpty), + R.reject(R.isNil), + R.sort(sortRolesByWeight), + R.last + )(roles); +}; + export const isLegacyToken = R.pathSatisfies(isNotNil, ['payload', 'role']); export const isKeycloakToken = R.pathSatisfies(isNotNil, ['payload', 'typ']); @@ -108,55 +151,52 @@ export const keycloakHasPermission = (grant, requestCache, modelClients) => { return async (resource, scope, attributes: IKeycloakAuthAttributes = {}) => { const currentUserId: string = grant.access_token.content.sub; - // Check if the same set of permissions has been granted already for this - // api query. - // TODO Is it possible to ask keycloak for ALL permissions (given a project - // or group context) and cache a single query instead? - const cacheKey = `${currentUserId}:${resource}:${scope}:${JSON.stringify( - attributes - )}`; - const cachedPermissions = requestCache.get(cacheKey); - if (cachedPermissions === true) { - return true; - } else if (!cachedPermissions === false) { - userActivityLogger.user_info( - `User does not have permission to '${scope}' on '${resource}'`, - { - user: grant ? grant.access_token.content : null - } - ); - throw new KeycloakUnauthorizedError( - `Unauthorized: You don't have permission to "${scope}" on "${resource}": ${JSON.stringify( - attributes - )}` - ); - } + // const cacheKey = `${currentUserId}:${resource}:${scope}:${JSON.stringify( + // attributes + // )}`; + // const resourceScope = { resource, scope, currentUserId, ...attributes }; + + // const cachedPermissions = requestCache.get(cacheKey); + // if (cachedPermissions === true) { + // return true; + // } else if (!cachedPermissions === false) { + // userActivityLogger.user_info( + // `User does not have permission to '${scope}' on '${resource}'`, + // { + // user: grant ? grant.access_token.content : null + // } + // ); + // throw new KeycloakUnauthorizedError( + // `Unauthorized: You don't have permission to "${scope}" on "${resource}": ${JSON.stringify( + // attributes + // )}` + // ); + // } - // Check the redis cache before doing a full keycloak lookup. - const resourceScope = { resource, scope, currentUserId, ...attributes }; - let redisCacheResult: number; - try { - const data = await getRedisCache(resourceScope); - redisCacheResult = parseInt(data, 10); - } catch (err) { - logger.warn(`Couldn't check redis authz cache: ${err.message}`); - } + // // Check the redis cache before doing a full keycloak lookup. + // let redisCacheResult: number; + // try { + // const data = await getRedisCache(resourceScope); + // redisCacheResult = parseInt(data, 10); + // } catch (err) { + // logger.warn(`Couldn't check redis authz cache: ${err.message}`); + // } - if (redisCacheResult === 1) { - return true; - } else if (redisCacheResult === 0) { - userActivityLogger.user_info( - `User does not have permission to '${scope}' on '${resource}'`, - { - user: grant.access_token.content - } - ); - throw new KeycloakUnauthorizedError( - `Unauthorized: You don't have permission to "${scope}" on "${resource}": ${JSON.stringify( - attributes - )}` - ); - } + // if (redisCacheResult === 1) { + // return true; + // } else if (redisCacheResult === 0) { + // userActivityLogger.user_info( + // `User does not have permission to '${scope}' on '${resource}'`, + // { + // user: grant.access_token.content + // } + // ); + // throw new KeycloakUnauthorizedError( + // `Unauthorized: You don't have permission to "${scope}" on "${resource}": ${JSON.stringify( + // attributes + // )}` + // ); + // } const currentUser = await UserModel.loadUserById(currentUserId); const serviceAccount = await keycloakGrantManager.obtainFromClientCredentials(); @@ -172,6 +212,9 @@ export const keycloakHasPermission = (grant, requestCache, modelClients) => { currentUser: [currentUserId] }; + + logger.info(`A ${JSON.stringify(grant.access_token.token)}`) + const usersAttribute = R.prop('users', attributes); if (usersAttribute && usersAttribute.length) { claims = { @@ -197,9 +240,7 @@ export const keycloakHasPermission = (grant, requestCache, modelClients) => { projectQuery: [`${projectId}`] }; - const userProjects = await UserModel.getAllProjectsIdsForUser( - currentUser - ); + const [highestRoleForProject, userProjects] = await getUserRoleForProjectFromToken(grant, R.prop('project', attributes)) if (userProjects.length) { claims = { @@ -208,24 +249,40 @@ export const keycloakHasPermission = (grant, requestCache, modelClients) => { }; } - const roles = await UserModel.getUserRolesForProject( - currentUser, - projectId - ); - - const highestRoleForProject = R.pipe( - R.uniq, - R.reject(R.isEmpty), - R.reject(R.isNil), - R.sort(sortRolesByWeight), - R.last - )(roles); - if (highestRoleForProject) { claims = { ...claims, userProjectRole: [highestRoleForProject] }; + } else { + // no project or role detected in the token, fall back to checking keycloak + // this is a heavier operation for users that are in a lot of groups, use the token as often as possible + // but with the way the api works, sometimes it isn't possible to use the token if it lacks something like a newly created project id + + // logger.debug(`There was no project or role determined for the requested project resource, falling back to checking keycloak the slow way`) + + const [userProjects, userGroups] = await UserModel.getAllProjectsIdsForUser(currentUser); + + if (userProjects.length) { + claims = { + ...claims, + userProjects: [userProjects.join('-')] + }; + } + const roles = await UserModel.getUserRolesForProject( + currentUser, + projectId, + userGroups + ); + + const highestRoleForProject = getHighestRole(roles); + + if (highestRoleForProject) { + claims = { + ...claims, + userProjectRole: [highestRoleForProject] + }; + } } } catch (err) { logger.error( @@ -308,12 +365,12 @@ export const keycloakHasPermission = (grant, requestCache, modelClients) => { ); if (newGrant.access_token.hasPermission(resource, scope)) { - requestCache.set(cacheKey, true); - try { - await saveRedisCache(resourceScope, 1); - } catch (err) { - logger.warn(`Couldn't save redis authz cache: ${err.message}`); - } + // requestCache.set(cacheKey, true); + // try { + // await saveRedisCache(resourceScope, 1); + // } catch (err) { + // logger.warn(`Couldn't save redis authz cache: ${err.message}`); + // } return; } @@ -323,12 +380,12 @@ export const keycloakHasPermission = (grant, requestCache, modelClients) => { userActivityLogger.user_info( `User does not have permission to '${scope}' on '${resource}'`, { - user: currentUser + user: currentUserId } ); } - requestCache.set(cacheKey, false); + // requestCache.set(cacheKey, false); // TODO: Re-enable when we can distinguish between error and access denied // try { // await saveRedisCache(resourceScope, 0); From ce657b5ae2afe92f4872359a220d44bb97f2ff96 Mon Sep 17 00:00:00 2001 From: shreddedbacon Date: Thu, 16 Feb 2023 09:12:04 +1100 Subject: [PATCH 03/11] chore: disable redis more --- services/api/src/apolloServer.js | 6 +++--- services/api/src/util/auth.ts | 2 -- services/keycloak/startup-scripts/00-configure-lagoon.sh | 3 --- 3 files changed, 3 insertions(+), 8 deletions(-) diff --git a/services/api/src/apolloServer.js b/services/api/src/apolloServer.js index b912fb7b16..0ac3aa24be 100644 --- a/services/api/src/apolloServer.js +++ b/services/api/src/apolloServer.js @@ -19,7 +19,7 @@ const { } = require('./util/auth'); const { sqlClientPool } = require('./clients/sqlClient'); const esClient = require('./clients/esClient'); -const redisClient = require('./clients/redisClient'); +// const redisClient = require('./clients/redisClient'); const { getKeycloakAdminClient } = require('./clients/keycloak-admin'); const { logger } = require('./loggers/logger'); const { userActivityLogger } = require('./loggers/userActivityLogger'); @@ -117,7 +117,7 @@ const apolloServer = new ApolloServer({ sqlClientPool, keycloakAdminClient, esClient, - redisClient + // redisClient }; return { @@ -163,7 +163,7 @@ const apolloServer = new ApolloServer({ sqlClientPool, keycloakAdminClient, esClient, - redisClient + // redisClient }; return { diff --git a/services/api/src/util/auth.ts b/services/api/src/util/auth.ts index 8981e38218..71bf668438 100644 --- a/services/api/src/util/auth.ts +++ b/services/api/src/util/auth.ts @@ -213,8 +213,6 @@ export const keycloakHasPermission = (grant, requestCache, modelClients) => { }; - logger.info(`A ${JSON.stringify(grant.access_token.token)}`) - const usersAttribute = R.prop('users', attributes); if (usersAttribute && usersAttribute.length) { claims = { diff --git a/services/keycloak/startup-scripts/00-configure-lagoon.sh b/services/keycloak/startup-scripts/00-configure-lagoon.sh index ddabc9eb6a..c904769783 100755 --- a/services/keycloak/startup-scripts/00-configure-lagoon.sh +++ b/services/keycloak/startup-scripts/00-configure-lagoon.sh @@ -1962,12 +1962,9 @@ EOF function add_group_project_token_mapper { CLIENT_ID=$(/opt/jboss/keycloak/bin/kcadm.sh get -r lagoon clients?clientId=api --config $CONFIG_PATH | jq -r '.[0]["id"]') ui_client_id=$(/opt/jboss/keycloak/bin/kcadm.sh get -r lagoon clients?clientId=lagoon-ui --config $CONFIG_PATH | jq -r '.[0]["id"]') - echo "AA $ui_client_id" group_project_token_mappers=$(/opt/jboss/keycloak/bin/kcadm.sh get -r lagoon clients/$ui_client_id/protocol-mappers/models --config $CONFIG_PATH) - echo "AA $group_project_token_mappers" old_mapper_id=$(echo $group_project_token_mappers | jq -r '.[] | select(.name=="group_project_role_ids") | .id') - echo "AA $old_mapper_id" if [ "$old_mapper_id" != "" ]; then echo "group_project_role_ids token mapper already configured" return 0 From 846657bb7d01d375e01e4a370e7a81e59a568dd2 Mon Sep 17 00:00:00 2001 From: shreddedbacon Date: Fri, 17 Feb 2023 15:30:29 +1100 Subject: [PATCH 04/11] chore: add some additional comments around changes --- services/api/src/apolloServer.js | 6 +++++ services/api/src/models/group.ts | 25 +++++++++++++++++-- services/api/src/models/user.ts | 6 ++++- .../api/src/resources/deployment/resolvers.ts | 6 ++--- services/api/src/resources/fact/resolvers.ts | 4 +-- .../api/src/resources/project/resolvers.ts | 4 +-- services/api/src/util/auth.ts | 22 ++++++++++++++-- .../javascript/META-INF/keycloak-scripts.json | 2 +- .../javascript/mappers/group-project-roles.js | 10 ++++++-- 9 files changed, 70 insertions(+), 15 deletions(-) diff --git a/services/api/src/apolloServer.js b/services/api/src/apolloServer.js index 0ac3aa24be..81aad02370 100644 --- a/services/api/src/apolloServer.js +++ b/services/api/src/apolloServer.js @@ -19,7 +19,9 @@ const { } = require('./util/auth'); const { sqlClientPool } = require('./clients/sqlClient'); const esClient = require('./clients/esClient'); +/* REDIS // const redisClient = require('./clients/redisClient'); +REDIS */ const { getKeycloakAdminClient } = require('./clients/keycloak-admin'); const { logger } = require('./loggers/logger'); const { userActivityLogger } = require('./loggers/userActivityLogger'); @@ -117,7 +119,9 @@ const apolloServer = new ApolloServer({ sqlClientPool, keycloakAdminClient, esClient, + /* REDIS // redisClient + REDIS */ }; return { @@ -163,7 +167,9 @@ const apolloServer = new ApolloServer({ sqlClientPool, keycloakAdminClient, esClient, + /* REDIS // redisClient + REDIS */ }; return { diff --git a/services/api/src/models/group.ts b/services/api/src/models/group.ts index b6cb3bdfe2..f7d3923334 100644 --- a/services/api/src/models/group.ts +++ b/services/api/src/models/group.ts @@ -132,7 +132,7 @@ export const Group = (clients: { const loadGroupById = async (id: string): Promise => { const keycloakGroup = await keycloakAdminClient.groups.findOne({ - id, + id }); if (R.isNil(keycloakGroup)) { @@ -189,6 +189,8 @@ export const Group = (clients: { }; const loadAllGroups = async (): Promise => { + // briefRepresentation pulls all the group information from keycloak including the attributes + // this means we don't need to iterate over all the groups one by one anymore to get the full group information const keycloakGroups = await keycloakAdminClient.groups.find({briefRepresentation: false}); const fullGroups = transformKeycloakGroups(keycloakGroups); @@ -227,6 +229,8 @@ export const Group = (clients: { const loadGroupsByAttribute = async ( filterFn: AttributeFilterFn ): Promise => { + // briefRepresentation pulls all the group information from keycloak including the attributes + // this means we don't need to iterate over all the groups one by one anymore to get the full group information const keycloakGroups = await keycloakAdminClient.groups.find({briefRepresentation: false}); const filteredGroups = filterGroupsByAttribute(keycloakGroups, filterFn); @@ -248,6 +252,7 @@ export const Group = (clients: { return false; }; + /* REDIS // let groupIds = []; // This function is called often and is expensive to compute so prefer // performance over DRY @@ -257,18 +262,22 @@ export const Group = (clients: { // logger.warn(`Error loading project groups from cache: ${err.message}`); // groupIds = []; // } + REDIS */ // this request will be huge, but it is significantly faster than the alternative iteration that followed previously + // briefRepresentation pulls all the group information from keycloak including the attributes + // this means we don't need to iterate over all the groups one by one anymore to get the full group information let fullGroups = await keycloakAdminClient.groups.find({briefRepresentation: false}); - const filteredGroups = filterGroupsByAttribute(fullGroups, filterFn); + /* REDIS // try { // const filteredGroupIds = R.pluck('id', filteredGroups); // await redisClient.saveProjectGroupsCache(projectId, filteredGroupIds); // } catch (err) { // logger.warn(`Error saving project groups to cache: ${err.message}`); // } + REDIS */ const groups = await transformKeycloakGroups(filteredGroups); @@ -467,9 +476,11 @@ export const Group = (clients: { }; const deleteGroup = async (id: string): Promise => { + /* REDIS // const group = loadGroupById(id); // @ts-ignore // const projectIds = getProjectIdsFromGroup(group); + REDIS */ try { await keycloakAdminClient.groups.del({ id }); @@ -481,6 +492,7 @@ export const Group = (clients: { } } + /* REDIS // for (const projectId of projectIds) { // try { // await redisClient.deleteProjectGroupsCache(projectId); @@ -488,6 +500,7 @@ export const Group = (clients: { // logger.warn(`Error deleting project groups cache: ${err.message}`); // } // } + REDIS */ }; const addUserToGroup = async ( @@ -529,11 +542,13 @@ export const Group = (clients: { throw new Error(`Could not add user to group: ${err.message}`); } + /* REDIS // try { // await redisClient.deleteRedisUserCache(user.id); // } catch (err) { // logger.warn(`Error deleting user cache ${user.id}: ${err}`); // } + REDIS */ return await loadGroupById(group.id); }; @@ -558,11 +573,13 @@ export const Group = (clients: { throw new Error(`Could not remove user from group: ${err.message}`); } + /* REDIS // try { // await redisClient.deleteRedisUserCache(user.id); // } catch (err) { // logger.warn(`Error deleting user cache ${user.id}: ${err}`); // } + REDIS */ } return await loadGroupById(group.id); @@ -601,6 +618,7 @@ export const Group = (clients: { ); } + /* REDIS // Clear the cache for users that gained access to the project // const groupAndParentsMembers = await getMembersFromGroupAndParents(group); // const userIds = R.map(R.path(['user', 'id']), groupAndParentsMembers); @@ -617,6 +635,7 @@ export const Group = (clients: { // } catch (err) { // logger.warn(`Error deleting project groups cache: ${err.message}`); // } + REDIS */ }; const removeProjectFromGroup = async ( @@ -651,6 +670,7 @@ export const Group = (clients: { ); } + /* REDIS // Clear the cache for users that lost access to the project // const groupAndParentsMembers = await getMembersFromGroupAndParents(group); // const userIds = R.map(R.path(['user', 'id']), groupAndParentsMembers); @@ -667,6 +687,7 @@ export const Group = (clients: { // } catch (err) { // logger.warn(`Error deleting project groups cache: ${err.message}`); // } + REDIS */ }; return { diff --git a/services/api/src/models/user.ts b/services/api/src/models/user.ts index 527e46674f..1462dbd749 100644 --- a/services/api/src/models/user.ts +++ b/services/api/src/models/user.ts @@ -3,7 +3,6 @@ import pickNonNil from '../util/pickNonNil'; import { logger } from '../loggers/logger'; import UserRepresentation from 'keycloak-admin/lib/defs/userRepresentation'; import { Group, isRoleSubgroup } from './group'; -import { GroupInterface } from '../resolvers'; export interface User { email: string; @@ -216,6 +215,8 @@ export const User = (clients: { const GroupModel = Group(clients); let groups = []; + // briefRepresentation pulls all the group information from keycloak including the attributes + // this means we don't need to iterate over all the groups one by one anymore to get the full group information const roleSubgroups = await keycloakAdminClient.users.listGroups({ id: userInput.id, briefRepresentation: false @@ -223,6 +224,7 @@ export const User = (clients: { for (const roleSubgroup of roleSubgroups) { // just look up the group with what we know the parent name to be with the regex of the role stripped + // eventually this logic will have to change when we support custom roles, but this applies to *everything* if that happens let regexp = /-(owner|maintainer|developer|reporter|guest)$/g; const roleSubgroupParent = await GroupModel.loadGroupByName( roleSubgroup.name.replace(regexp, "") @@ -364,11 +366,13 @@ export const User = (clients: { throw new Error(`Error deleting user ${id}: ${err}`); } } + /* REDIS // try { // await redisClient.deleteRedisUserCache(id); // } catch (err) { // logger.error(`Error deleting user cache ${id}: ${err}`); // } + REDIS */ }; return { diff --git a/services/api/src/resources/deployment/resolvers.ts b/services/api/src/resources/deployment/resolvers.ts index 6df76a99b9..1cdb4f015a 100644 --- a/services/api/src/resources/deployment/resolvers.ts +++ b/services/api/src/resources/deployment/resolvers.ts @@ -138,7 +138,7 @@ export const getDeploymentsByBulkId: ResolverFn = async ( // pull the project ids from the token // this can have a negative effect if the token is not refreshed after performing an operation like adding a project - // the user will probably have to refresh their token + // the user will probably have to refresh their token, with short token lifespans this should not be much of an issue userProjectIds = getUserProjectIdsFromToken(keycloakGrant); } @@ -188,7 +188,7 @@ export const getDeploymentsByFilter: ResolverFn = async ( // pull the project ids from the token // this can have a negative effect if the token is not refreshed after performing an operation like adding a project - // the user will probably have to refresh their token + // the user will probably have to refresh their token, with short token lifespans this should not be much of an issue userProjectIds = getUserProjectIdsFromToken(keycloakGrant); } @@ -1215,7 +1215,7 @@ export const bulkDeployEnvironmentLatest: ResolverFn = async ( // pull the project ids from the token // this can have a negative effect if the token is not refreshed after performing an operation like adding a project - // the user will probably have to refresh their token + // the user will probably have to refresh their token, with short token lifespans this should not be much of an issue userProjectIds = getUserProjectIdsFromToken(keycloakGrant); } diff --git a/services/api/src/resources/fact/resolvers.ts b/services/api/src/resources/fact/resolvers.ts index 01fc6f234b..1b5e21abb6 100644 --- a/services/api/src/resources/fact/resolvers.ts +++ b/services/api/src/resources/fact/resolvers.ts @@ -139,7 +139,7 @@ export const getProjectsByFactSearch: ResolverFn = async ( // pull the project ids from the token // this can have a negative effect if the token is not refreshed after performing an operation like adding a project - // the user will probably have to refresh their token + // the user will probably have to refresh their token, with short token lifespans this should not be much of an issue userProjectIds = getUserProjectIdsFromToken(keycloakGrant); } @@ -172,7 +172,7 @@ export const getEnvironmentsByFactSearch: ResolverFn = async ( // pull the project ids from the token // this can have a negative effect if the token is not refreshed after performing an operation like adding a project - // the user will probably have to refresh their token + // the user will probably have to refresh their token, with short token lifespans this should not be much of an issue userProjectIds = getUserProjectIdsFromToken(keycloakGrant); } diff --git a/services/api/src/resources/project/resolvers.ts b/services/api/src/resources/project/resolvers.ts index 90a82274fd..746c440201 100644 --- a/services/api/src/resources/project/resolvers.ts +++ b/services/api/src/resources/project/resolvers.ts @@ -65,7 +65,7 @@ export const getAllProjects: ResolverFn = async ( // pull the project ids from the token // this can have a negative effect if the token is not refreshed after performing an operation like adding a project - // the user will probably have to refresh their token + // the user will probably have to refresh their token, with short token lifespans this should not be much of an issue userProjectIds = getUserProjectIdsFromToken(keycloakGrant); } @@ -215,7 +215,7 @@ export const getProjectsByMetadata: ResolverFn = async ( } // pull the project ids from the token // this can have a negative effect if the token is not refreshed after performing an operation like adding a project - // the user will probably have to refresh their token + // the user will probably have to refresh their token, with short token lifespans this should not be much of an issue userProjectIds = getUserProjectIdsFromToken(keycloakGrant); } diff --git a/services/api/src/util/auth.ts b/services/api/src/util/auth.ts index 71bf668438..9d7c7fab93 100644 --- a/services/api/src/util/auth.ts +++ b/services/api/src/util/auth.ts @@ -44,6 +44,11 @@ const sortRolesByWeight = (a, b) => { export const getUserProjectIdsFromToken = ( token ): number[] => { + // https://github.com/uselagoon/lagoon/pull/3358 references potential issue with the lagoon-projects attribute where there could be empty values + // the structure of this token payload is: + /* + {"guest":[13],"devloper":[20,14],"maintainer":[13,18,19,12]} + */ const groupRoleIds = token.access_token.content.project_group_roles let upids = []; for (const r in groupRoleIds) { @@ -151,6 +156,10 @@ export const keycloakHasPermission = (grant, requestCache, modelClients) => { return async (resource, scope, attributes: IKeycloakAuthAttributes = {}) => { const currentUserId: string = grant.access_token.content.sub; + + logger.info(JSON.stringify(grant.access_token.content.project_group_roles)) + + /* REDIS // const cacheKey = `${currentUserId}:${resource}:${scope}:${JSON.stringify( // attributes // )}`; @@ -197,6 +206,7 @@ export const keycloakHasPermission = (grant, requestCache, modelClients) => { // )}` // ); // } + REDIS */ const currentUser = await UserModel.loadUserById(currentUserId); const serviceAccount = await keycloakGrantManager.obtainFromClientCredentials(); @@ -238,7 +248,9 @@ export const keycloakHasPermission = (grant, requestCache, modelClients) => { projectQuery: [`${projectId}`] }; - const [highestRoleForProject, userProjects] = await getUserRoleForProjectFromToken(grant, R.prop('project', attributes)) + // we pull the users role and project ids from the token here for the requested project, and we run the same functions that previously ran to pick out the highest role + // that the user has on the project + const [highestRoleForProject, userProjects] = await getUserRoleForProjectFromToken(grant, projectId) if (userProjects.length) { claims = { @@ -256,9 +268,10 @@ export const keycloakHasPermission = (grant, requestCache, modelClients) => { // no project or role detected in the token, fall back to checking keycloak // this is a heavier operation for users that are in a lot of groups, use the token as often as possible // but with the way the api works, sometimes it isn't possible to use the token if it lacks something like a newly created project id - // logger.debug(`There was no project or role determined for the requested project resource, falling back to checking keycloak the slow way`) + // this is the previous run function almost entirely unchanged, and will only be called if the user has requested a project id that is not within their token + // this would be hit if a user requests a project they've recently been added to with a token that hasn't refreshed yet const [userProjects, userGroups] = await UserModel.getAllProjectsIdsForUser(currentUser); if (userProjects.length) { @@ -273,6 +286,7 @@ export const keycloakHasPermission = (grant, requestCache, modelClients) => { userGroups ); + // getHighestRole just uses the previous highest role check now in a function for re-use elsewhere const highestRoleForProject = getHighestRole(roles); if (highestRoleForProject) { @@ -363,12 +377,14 @@ export const keycloakHasPermission = (grant, requestCache, modelClients) => { ); if (newGrant.access_token.hasPermission(resource, scope)) { + /* REDIS // requestCache.set(cacheKey, true); // try { // await saveRedisCache(resourceScope, 1); // } catch (err) { // logger.warn(`Couldn't save redis authz cache: ${err.message}`); // } + REDIS */ return; } @@ -383,6 +399,7 @@ export const keycloakHasPermission = (grant, requestCache, modelClients) => { ); } + /* REDIS // requestCache.set(cacheKey, false); // TODO: Re-enable when we can distinguish between error and access denied // try { @@ -390,6 +407,7 @@ export const keycloakHasPermission = (grant, requestCache, modelClients) => { // } catch (err) { // logger.warn(`Couldn't save redis authz cache: ${err.message}`); // } + REDIS */ userActivityLogger.user_info( `User does not have permission to '${scope}' on '${resource}'`, { diff --git a/services/keycloak/javascript/META-INF/keycloak-scripts.json b/services/keycloak/javascript/META-INF/keycloak-scripts.json index f769aa96e0..828c39081e 100644 --- a/services/keycloak/javascript/META-INF/keycloak-scripts.json +++ b/services/keycloak/javascript/META-INF/keycloak-scripts.json @@ -7,7 +7,7 @@ }, { "name": "[Lagoon] Group and Project ID Roles", - "description": "Gets a users groups and project roles with IDs", + "description": "Gets a users roles for associted project IDs", "fileName": "mappers/group-project-roles.js" } ], diff --git a/services/keycloak/javascript/mappers/group-project-roles.js b/services/keycloak/javascript/mappers/group-project-roles.js index 143fcb0517..28bf1e19b8 100644 --- a/services/keycloak/javascript/mappers/group-project-roles.js +++ b/services/keycloak/javascript/mappers/group-project-roles.js @@ -21,13 +21,19 @@ forEach.call(groups.toArray(), function(group) { if(projectIds !== null) { var splitIds = new ArrayList(projectIds.split(",")) forEach.call(splitIds, function(g) { - projectIdList.add(parseInt(g)) + // https://github.com/uselagoon/lagoon/pull/3358 references potential issue with the lagoon-projects attribute where there could be empty values + // check the parsed value is not nan here before adding it to the token payload + if (!isNaN(parseInt(g))) { + projectIdList.add(parseInt(g)) + } }); if (groupProjectIds[rn]) { if (groupProjectIds[rn].length > 0) { for(var a=0; a < groupProjectIds[rn].length; a++){ if (!splitIds.contains(groupProjectIds[rn][a])) { - projectIdList.add(parseInt(groupProjectIds[rn][a])) + if (!isNaN(parseInt(groupProjectIds[rn][a]))) { + projectIdList.add(parseInt(groupProjectIds[rn][a])) + } } } } From 13a17eb7e43fc5ca3c996d96f2169dbfec0df505 Mon Sep 17 00:00:00 2001 From: shreddedbacon Date: Thu, 23 Feb 2023 19:52:08 +1100 Subject: [PATCH 05/11] chore: remove keycloak token for groups/role ids --- .../javascript/META-INF/keycloak-scripts.json | 5 -- .../javascript/mappers/group-project-roles.js | 52 ------------------- .../startup-scripts/00-configure-lagoon.sh | 21 -------- 3 files changed, 78 deletions(-) delete mode 100644 services/keycloak/javascript/mappers/group-project-roles.js diff --git a/services/keycloak/javascript/META-INF/keycloak-scripts.json b/services/keycloak/javascript/META-INF/keycloak-scripts.json index 828c39081e..94536c74d4 100644 --- a/services/keycloak/javascript/META-INF/keycloak-scripts.json +++ b/services/keycloak/javascript/META-INF/keycloak-scripts.json @@ -4,11 +4,6 @@ "name": "[Lagoon] Groups and Roles", "description": "Gets a users groups and roles", "fileName": "mappers/groups-and-roles.js" - }, - { - "name": "[Lagoon] Group and Project ID Roles", - "description": "Gets a users roles for associted project IDs", - "fileName": "mappers/group-project-roles.js" } ], "policies": [ diff --git a/services/keycloak/javascript/mappers/group-project-roles.js b/services/keycloak/javascript/mappers/group-project-roles.js deleted file mode 100644 index 28bf1e19b8..0000000000 --- a/services/keycloak/javascript/mappers/group-project-roles.js +++ /dev/null @@ -1,52 +0,0 @@ -var ArrayList = Java.type("java.util.ArrayList"); -var HashMap = Java.type("java.util.HashMap"); -var HashSet = Java.type("java.util.HashSet"); -var groupProjectIds = new HashMap(); - -var groups = user.getGroups() - -var forEach = Array.prototype.forEach; -// add all groups the user is part of -forEach.call(groups.toArray(), function(group) { - var groupName = group.getName().replace(/-(owner|maintainer|developer|reporter|guest)$/,""); - var groupRoles = group.getRoleMappings(); - if(group.getFirstAttribute("type") == "role-subgroup") { - var parent = group.getParent(); - var projectIds = parent.getFirstAttribute("lagoon-projects"); - if(groupRoles.length == 1) { - groupRoles.forEach(function(roleModel) { - var rn = roleModel.getName(); - // array of project ids - var projectIdList = new ArrayList(); - if(projectIds !== null) { - var splitIds = new ArrayList(projectIds.split(",")) - forEach.call(splitIds, function(g) { - // https://github.com/uselagoon/lagoon/pull/3358 references potential issue with the lagoon-projects attribute where there could be empty values - // check the parsed value is not nan here before adding it to the token payload - if (!isNaN(parseInt(g))) { - projectIdList.add(parseInt(g)) - } - }); - if (groupProjectIds[rn]) { - if (groupProjectIds[rn].length > 0) { - for(var a=0; a < groupProjectIds[rn].length; a++){ - if (!splitIds.contains(groupProjectIds[rn][a])) { - if (!isNaN(parseInt(groupProjectIds[rn][a]))) { - projectIdList.add(parseInt(groupProjectIds[rn][a])) - } - } - } - } - } - } - if (projectIdList.length > 0) { - groupProjectIds.put(rn, projectIdList) - } - }); - return; - } - } - return; -}); - -token.setOtherClaims("project_group_roles", groupProjectIds); \ No newline at end of file diff --git a/services/keycloak/startup-scripts/00-configure-lagoon.sh b/services/keycloak/startup-scripts/00-configure-lagoon.sh index c904769783..9db93075c0 100755 --- a/services/keycloak/startup-scripts/00-configure-lagoon.sh +++ b/services/keycloak/startup-scripts/00-configure-lagoon.sh @@ -1959,26 +1959,6 @@ EOF EOF } -function add_group_project_token_mapper { - CLIENT_ID=$(/opt/jboss/keycloak/bin/kcadm.sh get -r lagoon clients?clientId=api --config $CONFIG_PATH | jq -r '.[0]["id"]') - ui_client_id=$(/opt/jboss/keycloak/bin/kcadm.sh get -r lagoon clients?clientId=lagoon-ui --config $CONFIG_PATH | jq -r '.[0]["id"]') - group_project_token_mappers=$(/opt/jboss/keycloak/bin/kcadm.sh get -r lagoon clients/$ui_client_id/protocol-mappers/models --config $CONFIG_PATH) - old_mapper_id=$(echo $group_project_token_mappers | jq -r '.[] | select(.name=="group_project_role_ids") | .id') - - if [ "$old_mapper_id" != "" ]; then - echo "group_project_role_ids token mapper already configured" - return 0 - fi - auth_server_client_id=$(/opt/jboss/keycloak/bin/kcadm.sh get -r lagoon clients?clientId=auth-server --config $CONFIG_PATH | jq -r '.[0]["id"]') - api_client_id=$(/opt/jboss/keycloak/bin/kcadm.sh get -r lagoon clients?clientId=api --config $CONFIG_PATH | jq -r '.[0]["id"]') - - echo Configuring Group and Project Role Token Mapper - echo '{"name":"group_project_role_ids","protocolMapper":"script-mappers/group-project-roles.js","protocol":"openid-connect","config":{"id.token.claim":"true","access.token.claim":"true","userinfo.token.claim":"true","multivalued":"true","claim.name":"project_group_roles","jsonType.label":"String"}}' | /opt/jboss/keycloak/bin/kcadm.sh create -r ${KEYCLOAK_REALM:-master} clients/$ui_client_id/protocol-mappers/models --config $CONFIG_PATH -f - - echo '{"name":"group_project_role_ids","protocolMapper":"script-mappers/group-project-roles.js","protocol":"openid-connect","config":{"id.token.claim":"true","access.token.claim":"true","userinfo.token.claim":"true","multivalued":"true","claim.name":"project_group_roles","jsonType.label":"String"}}' | /opt/jboss/keycloak/bin/kcadm.sh create -r ${KEYCLOAK_REALM:-master} clients/$auth_server_client_id/protocol-mappers/models --config $CONFIG_PATH -f - - echo '{"name":"group_project_role_ids","protocolMapper":"script-mappers/group-project-roles.js","protocol":"openid-connect","config":{"id.token.claim":"true","access.token.claim":"true","userinfo.token.claim":"true","multivalued":"true","claim.name":"project_group_roles","jsonType.label":"String"}}' | /opt/jboss/keycloak/bin/kcadm.sh create -r ${KEYCLOAK_REALM:-master} clients/$api_client_id/protocol-mappers/models --config $CONFIG_PATH -f - - -} - ################## # Initialization # ################## @@ -2016,7 +1996,6 @@ function configure_keycloak { migrate_to_js_provider add_delete_env_var_permissions configure_lagoon_opensearch_sync_client - add_group_project_token_mapper # always run last sync_client_secrets From f11e6297c1c96e684a7dffdc9f62ba089cbb6861 Mon Sep 17 00:00:00 2001 From: shreddedbacon Date: Thu, 23 Feb 2023 20:04:52 +1100 Subject: [PATCH 06/11] chore: add some initial keycloak queries to apollo context to reduce repeat calls --- services/api/src/apolloServer.js | 80 ++++++++++++++++++++++++----- services/api/src/resources/index.ts | 3 ++ 2 files changed, 69 insertions(+), 14 deletions(-) diff --git a/services/api/src/apolloServer.js b/services/api/src/apolloServer.js index 81aad02370..e2a88a3916 100644 --- a/services/api/src/apolloServer.js +++ b/services/api/src/apolloServer.js @@ -19,9 +19,6 @@ const { } = require('./util/auth'); const { sqlClientPool } = require('./clients/sqlClient'); const esClient = require('./clients/esClient'); -/* REDIS -// const redisClient = require('./clients/redisClient'); -REDIS */ const { getKeycloakAdminClient } = require('./clients/keycloak-admin'); const { logger } = require('./loggers/logger'); const { userActivityLogger } = require('./loggers/userActivityLogger'); @@ -119,11 +116,11 @@ const apolloServer = new ApolloServer({ sqlClientPool, keycloakAdminClient, esClient, - /* REDIS - // redisClient - REDIS */ }; + let keycloakGroups = {} + let keycloakUsersGroups = [] + return { keycloakAdminClient, sqlClientPool, @@ -137,7 +134,9 @@ const apolloServer = new ApolloServer({ GroupModel: Group.Group(modelClients), ProjectModel: ProjectModel.ProjectModel(modelClients), EnvironmentModel: EnvironmentModel.EnvironmentModel(modelClients) - } + }, + keycloakGroups, + keycloakUsersGroups, }; }, onDisconnect: (websocket, context) => { @@ -167,17 +166,62 @@ const apolloServer = new ApolloServer({ sqlClientPool, keycloakAdminClient, esClient, - /* REDIS - // redisClient - REDIS */ }; + // get all keycloak groups, do this early to reduce the number of times this is called otherwise + // would be good if this could be wrapped in a way so that only resolvers that need groups are called + // but doing this early and once is pretty cheap + let allGroups = await Group.Group(modelClients).loadAllGroups(); + let keycloakGroups = await Group.Group(modelClients).transformKeycloakGroups(allGroups); + + // if this is a user request, get the users keycloak groups too, do this one to reduce the number of times it is called elsewhere + let keycloakUsersGroups = [] + let kcg = req.kauth ? req.kauth.grant : null + if (kcg) { + keycloakUsersGroups = await User.User(modelClients).getAllGroupsForUser(kcg.access_token.content.sub); + } + + // do a permission check to see if the user is platform admin/owner, or has permission for `viewAll` on certain resources + // this reduces the number of `viewAll` permission look ups that could potentially occur during subfield resolvers for non admin users + // every `hasPermission` check adds a delay, and if you're a member of a group that has a lot of projects and environments, hasPermissions is costly when we perform + // the viewAll permission check, to then error out and follow through with the standard user permission check, effectively costing 2 hasPermission calls for every request + // this eliminates a huge number of these by making it available in the apollo context + let hasPermission = req.kauth + ? keycloakHasPermission(req.kauth.grant, requestCache, modelClients) + : legacyHasPermission(req.legacyCredentials) + let projectViewAll = false + let groupViewAll = false + let environmentViewAll = false + let deploytargetViewAll = false + try { + await hasPermission("project","viewAll") + projectViewAll = true + } catch(err) { + // do nothing + } + try { + await hasPermission("group","viewAll") + groupViewAll = true + } catch(err) { + // do nothing + } + try { + await hasPermission("environment","viewAll") + environmentViewAll = true + } catch(err) { + // do nothing + } + try { + await hasPermission("openshift","viewAll") + deploytargetViewAll = true + } catch(err) { + // do nothing + } + return { keycloakAdminClient, sqlClientPool, - hasPermission: req.kauth - ? keycloakHasPermission(req.kauth.grant, requestCache, modelClients) - : legacyHasPermission(req.legacyCredentials), + hasPermission: hasPermission, keycloakGrant: req.kauth ? req.kauth.grant : null, requestCache, userActivityLogger: (message, meta) => { @@ -199,7 +243,15 @@ const apolloServer = new ApolloServer({ GroupModel: Group.Group(modelClients), ProjectModel: ProjectModel.ProjectModel(modelClients), EnvironmentModel: EnvironmentModel.EnvironmentModel(modelClients) - } + }, + keycloakGroups, + keycloakUsersGroups, + adminScopes: { + projectViewAll: projectViewAll, + groupViewAll: groupViewAll, + environmentViewAll: environmentViewAll, + deploytargetViewAll: deploytargetViewAll, + }, }; } }, diff --git a/services/api/src/resources/index.ts b/services/api/src/resources/index.ts index 1341a13b82..f1450e4d53 100644 --- a/services/api/src/resources/index.ts +++ b/services/api/src/resources/index.ts @@ -19,6 +19,9 @@ export interface ResolverFn { ProjectModel, EnvironmentModel, }, + keycloakGroups?: any | null, + keycloakUsersGroups?: any | null, + adminScopes?: any | null, }, info? ): any; From 0dd3ae34a2312e686d13aaa4e9feeef6fea9b83e Mon Sep 17 00:00:00 2001 From: shreddedbacon Date: Thu, 23 Feb 2023 20:05:55 +1100 Subject: [PATCH 07/11] chore: remove redis checks and remove token queries back to previous collection from groups using new faster keycloak requests --- services/api/src/util/auth.ts | 140 +++++----------------------------- 1 file changed, 17 insertions(+), 123 deletions(-) diff --git a/services/api/src/util/auth.ts b/services/api/src/util/auth.ts index 9d7c7fab93..ff7779288b 100644 --- a/services/api/src/util/auth.ts +++ b/services/api/src/util/auth.ts @@ -1,5 +1,4 @@ import * as R from 'ramda'; -import { getRedisCache, saveRedisCache } from '../clients/redisClient'; import { verify } from 'jsonwebtoken'; import { logger } from '../loggers/logger'; import { getConfigFromEnv } from '../util/config'; @@ -41,34 +40,32 @@ const sortRolesByWeight = (a, b) => { return 0; }; -export const getUserProjectIdsFromToken = ( - token +export const getUserProjectIdsFromRoleProjectIds = ( + roleProjectIds ): number[] => { // https://github.com/uselagoon/lagoon/pull/3358 references potential issue with the lagoon-projects attribute where there could be empty values - // the structure of this token payload is: + // the structure of this payload is: /* {"guest":[13],"devloper":[20,14],"maintainer":[13,18,19,12]} */ - const groupRoleIds = token.access_token.content.project_group_roles let upids = []; - for (const r in groupRoleIds) { - for (const pid in groupRoleIds[r]) { - upids.indexOf(groupRoleIds[r][pid]) === -1 ? upids.push(groupRoleIds[r][pid]) : "" + for (const r in roleProjectIds) { + for (const pid in roleProjectIds[r]) { + upids.indexOf(roleProjectIds[r][pid]) === -1 ? upids.push(roleProjectIds[r][pid]) : "" } } return R.uniq(upids); }; -export const getUserRoleForProjectFromToken = ( - token, projectId +export const getUserRoleForProjectFromRoleProjectIds = ( + roleProjectIds, projectId ): [string, number[]] => { - const groupRoleIds = token.access_token.content.project_group_roles let upids = []; let roles = []; - for (const r in groupRoleIds) { - for (const pid in groupRoleIds[r]) { - upids.indexOf(groupRoleIds[r][pid]) === -1 ? upids.push(groupRoleIds[r][pid]) : "" - if (projectId == groupRoleIds[r][pid]) { + for (const r in roleProjectIds) { + for (const pid in roleProjectIds[r]) { + upids.indexOf(roleProjectIds[r][pid]) === -1 ? upids.push(roleProjectIds[r][pid]) : "" + if (projectId == roleProjectIds[r][pid]) { roles.push(r) } } @@ -149,65 +146,13 @@ export class KeycloakUnauthorizedError extends Error { } } -export const keycloakHasPermission = (grant, requestCache, modelClients) => { +export const keycloakHasPermission = (grant, requestCache, modelClients, keycloakUsersGroups) => { const UserModel = User(modelClients); const GroupModel = Group(modelClients); return async (resource, scope, attributes: IKeycloakAuthAttributes = {}) => { const currentUserId: string = grant.access_token.content.sub; - - logger.info(JSON.stringify(grant.access_token.content.project_group_roles)) - - /* REDIS - // const cacheKey = `${currentUserId}:${resource}:${scope}:${JSON.stringify( - // attributes - // )}`; - // const resourceScope = { resource, scope, currentUserId, ...attributes }; - - // const cachedPermissions = requestCache.get(cacheKey); - // if (cachedPermissions === true) { - // return true; - // } else if (!cachedPermissions === false) { - // userActivityLogger.user_info( - // `User does not have permission to '${scope}' on '${resource}'`, - // { - // user: grant ? grant.access_token.content : null - // } - // ); - // throw new KeycloakUnauthorizedError( - // `Unauthorized: You don't have permission to "${scope}" on "${resource}": ${JSON.stringify( - // attributes - // )}` - // ); - // } - - // // Check the redis cache before doing a full keycloak lookup. - // let redisCacheResult: number; - // try { - // const data = await getRedisCache(resourceScope); - // redisCacheResult = parseInt(data, 10); - // } catch (err) { - // logger.warn(`Couldn't check redis authz cache: ${err.message}`); - // } - - // if (redisCacheResult === 1) { - // return true; - // } else if (redisCacheResult === 0) { - // userActivityLogger.user_info( - // `User does not have permission to '${scope}' on '${resource}'`, - // { - // user: grant.access_token.content - // } - // ); - // throw new KeycloakUnauthorizedError( - // `Unauthorized: You don't have permission to "${scope}" on "${resource}": ${JSON.stringify( - // attributes - // )}` - // ); - // } - REDIS */ - const currentUser = await UserModel.loadUserById(currentUserId); const serviceAccount = await keycloakGrantManager.obtainFromClientCredentials(); @@ -248,14 +193,12 @@ export const keycloakHasPermission = (grant, requestCache, modelClients) => { projectQuery: [`${projectId}`] }; - // we pull the users role and project ids from the token here for the requested project, and we run the same functions that previously ran to pick out the highest role - // that the user has on the project - const [highestRoleForProject, userProjects] = await getUserRoleForProjectFromToken(grant, projectId) - - if (userProjects.length) { + const groupRoleIds = await UserModel.getAllProjectsIdsForUser(currentUser, keycloakUsersGroups); + const [highestRoleForProject, upids] = getUserRoleForProjectFromRoleProjectIds(groupRoleIds, projectId) + if (upids.length) { claims = { ...claims, - userProjects: [userProjects.join('-')] + userProjects: [upids.join('-')] }; } @@ -264,37 +207,6 @@ export const keycloakHasPermission = (grant, requestCache, modelClients) => { ...claims, userProjectRole: [highestRoleForProject] }; - } else { - // no project or role detected in the token, fall back to checking keycloak - // this is a heavier operation for users that are in a lot of groups, use the token as often as possible - // but with the way the api works, sometimes it isn't possible to use the token if it lacks something like a newly created project id - // logger.debug(`There was no project or role determined for the requested project resource, falling back to checking keycloak the slow way`) - - // this is the previous run function almost entirely unchanged, and will only be called if the user has requested a project id that is not within their token - // this would be hit if a user requests a project they've recently been added to with a token that hasn't refreshed yet - const [userProjects, userGroups] = await UserModel.getAllProjectsIdsForUser(currentUser); - - if (userProjects.length) { - claims = { - ...claims, - userProjects: [userProjects.join('-')] - }; - } - const roles = await UserModel.getUserRolesForProject( - currentUser, - projectId, - userGroups - ); - - // getHighestRole just uses the previous highest role check now in a function for re-use elsewhere - const highestRoleForProject = getHighestRole(roles); - - if (highestRoleForProject) { - claims = { - ...claims, - userProjectRole: [highestRoleForProject] - }; - } } } catch (err) { logger.error( @@ -377,15 +289,6 @@ export const keycloakHasPermission = (grant, requestCache, modelClients) => { ); if (newGrant.access_token.hasPermission(resource, scope)) { - /* REDIS - // requestCache.set(cacheKey, true); - // try { - // await saveRedisCache(resourceScope, 1); - // } catch (err) { - // logger.warn(`Couldn't save redis authz cache: ${err.message}`); - // } - REDIS */ - return; } } catch (err) { @@ -399,15 +302,6 @@ export const keycloakHasPermission = (grant, requestCache, modelClients) => { ); } - /* REDIS - // requestCache.set(cacheKey, false); - // TODO: Re-enable when we can distinguish between error and access denied - // try { - // await saveRedisCache(resourceScope, 0); - // } catch (err) { - // logger.warn(`Couldn't save redis authz cache: ${err.message}`); - // } - REDIS */ userActivityLogger.user_info( `User does not have permission to '${scope}' on '${resource}'`, { From 865bbbcca832a628168b33b64f16a6b0ecd02cc9 Mon Sep 17 00:00:00 2001 From: shreddedbacon Date: Thu, 23 Feb 2023 20:07:03 +1100 Subject: [PATCH 08/11] chore: initial round of improvements to collection of information from keycloak and processing to cut down on repeated calls within resolvers --- services/api/src/models/group.ts | 167 ++++------ services/api/src/models/user.ts | 133 +++++--- .../api/src/resources/deployment/resolvers.ts | 32 +- .../src/resources/environment/resolvers.ts | 4 + services/api/src/resources/fact/resolvers.ts | 22 +- services/api/src/resources/group/resolvers.ts | 289 +++++++++--------- services/api/src/resources/project/helpers.ts | 12 +- .../api/src/resources/project/resolvers.ts | 24 +- 8 files changed, 350 insertions(+), 333 deletions(-) diff --git a/services/api/src/models/group.ts b/services/api/src/models/group.ts index f7d3923334..d0eaae07ea 100644 --- a/services/api/src/models/group.ts +++ b/services/api/src/models/group.ts @@ -131,17 +131,20 @@ export const Group = (clients: { }; const loadGroupById = async (id: string): Promise => { - const keycloakGroup = await keycloakAdminClient.groups.findOne({ - id - }); + let keycloakGroup: Group + keycloakGroup = await keycloakAdminClient.groups.findOne({ + id, + briefRepresentation: false, + }); if (R.isNil(keycloakGroup)) { throw new GroupNotFoundError(`Group not found: ${id}`); } const groups = await transformKeycloakGroups([keycloakGroup]); + keycloakGroup = groups[0] - return groups[0]; + return keycloakGroup; }; const loadGroupByName = async (name: string): Promise => { @@ -191,11 +194,10 @@ export const Group = (clients: { const loadAllGroups = async (): Promise => { // briefRepresentation pulls all the group information from keycloak including the attributes // this means we don't need to iterate over all the groups one by one anymore to get the full group information - const keycloakGroups = await keycloakAdminClient.groups.find({briefRepresentation: false}); - - const fullGroups = transformKeycloakGroups(keycloakGroups); + const fullGroups = await keycloakAdminClient.groups.find({briefRepresentation: false}); + const keycloakGroups = await transformKeycloakGroups(fullGroups); - return fullGroups; + return keycloakGroups; }; const loadParentGroup = async (groupInput: Group): Promise => @@ -229,15 +231,11 @@ export const Group = (clients: { const loadGroupsByAttribute = async ( filterFn: AttributeFilterFn ): Promise => { - // briefRepresentation pulls all the group information from keycloak including the attributes - // this means we don't need to iterate over all the groups one by one anymore to get the full group information const keycloakGroups = await keycloakAdminClient.groups.find({briefRepresentation: false}); const filteredGroups = filterGroupsByAttribute(keycloakGroups, filterFn); - const groups = await transformKeycloakGroups(filteredGroups); - - return groups; + return await transformKeycloakGroups(filteredGroups); }; const loadGroupsByProjectId = async (projectId: number): Promise => { @@ -252,36 +250,37 @@ export const Group = (clients: { return false; }; - /* REDIS - // let groupIds = []; - // This function is called often and is expensive to compute so prefer - // performance over DRY - // try { - // groupIds = await redisClient.getProjectGroupsCache(projectId); - // } catch (err) { - // logger.warn(`Error loading project groups from cache: ${err.message}`); - // groupIds = []; - // } - REDIS */ - // this request will be huge, but it is significantly faster than the alternative iteration that followed previously // briefRepresentation pulls all the group information from keycloak including the attributes // this means we don't need to iterate over all the groups one by one anymore to get the full group information - let fullGroups = await keycloakAdminClient.groups.find({briefRepresentation: false}); + const groups = await keycloakAdminClient.groups.find({briefRepresentation: false}); - const filteredGroups = filterGroupsByAttribute(fullGroups, filterFn); - /* REDIS - // try { - // const filteredGroupIds = R.pluck('id', filteredGroups); - // await redisClient.saveProjectGroupsCache(projectId, filteredGroupIds); - // } catch (err) { - // logger.warn(`Error saving project groups to cache: ${err.message}`); - // } - REDIS */ + const filteredGroups = filterGroupsByAttribute(groups, filterFn); - const groups = await transformKeycloakGroups(filteredGroups); + const fullGroups = await transformKeycloakGroups(filteredGroups); - return groups; + return fullGroups; + }; + + + // loadGroupsByProjectIdFromGroups does the same thing as loadGroupsByProjectId, except takes a groups input in the arguments + // from another source that has already calculated the required groups + const loadGroupsByProjectIdFromGroups = async (projectId: number, groups: Group[]): Promise => { + const filterFn = attribute => { + if (attribute.name === 'lagoon-projects') { + const value = R.is(Array, attribute.value) + ? R.path(['value', 0], attribute) + : attribute.value; + return R.test(new RegExp(`\\b${projectId}\\b`), value); + } + + return false; + }; + const filteredGroups = filterGroupsByAttribute(groups, filterFn); + + const fullGroups = await transformKeycloakGroups(filteredGroups); + + return fullGroups; }; // Recursive function to load membership "up" the group chain @@ -320,19 +319,26 @@ export const Group = (clients: { const getProjectsFromGroupAndSubgroups = async ( group: Group ): Promise => { - const groupProjectIds = getProjectIdsFromGroup(group); + try { + const groupProjectIds = getProjectIdsFromGroup(group); + + let subGroupProjectIds = []; + // @TODO: check is `groups.groups` ever used? it always appears to be empty + if (group.groups) { + for (const subGroup of group.groups) { + const projectIds = await getProjectsFromGroupAndSubgroups(subGroup); + subGroupProjectIds = [...subGroupProjectIds, ...projectIds]; + } + } - let subGroupProjectIds = []; - for (const subGroup of group.groups) { - const projectIds = await getProjectsFromGroupAndSubgroups(subGroup); - subGroupProjectIds = [...subGroupProjectIds, ...projectIds]; + return [ + // @ts-ignore + ...groupProjectIds, + ...subGroupProjectIds + ]; + } catch (err) { + return []; } - - return [ - // @ts-ignore - ...groupProjectIds, - ...subGroupProjectIds - ]; }; const getGroupMembership = async ( @@ -471,7 +477,6 @@ export const Group = (clients: { }); } } - return newGroup; }; @@ -491,16 +496,6 @@ export const Group = (clients: { throw new Error(`Error deleting group ${id}: ${err}`); } } - - /* REDIS - // for (const projectId of projectIds) { - // try { - // await redisClient.deleteProjectGroupsCache(projectId); - // } catch (err) { - // logger.warn(`Error deleting project groups cache: ${err.message}`); - // } - // } - REDIS */ }; const addUserToGroup = async ( @@ -542,14 +537,6 @@ export const Group = (clients: { throw new Error(`Could not add user to group: ${err.message}`); } - /* REDIS - // try { - // await redisClient.deleteRedisUserCache(user.id); - // } catch (err) { - // logger.warn(`Error deleting user cache ${user.id}: ${err}`); - // } - REDIS */ - return await loadGroupById(group.id); }; @@ -573,13 +560,6 @@ export const Group = (clients: { throw new Error(`Could not remove user from group: ${err.message}`); } - /* REDIS - // try { - // await redisClient.deleteRedisUserCache(user.id); - // } catch (err) { - // logger.warn(`Error deleting user cache ${user.id}: ${err}`); - // } - REDIS */ } return await loadGroupById(group.id); @@ -617,25 +597,6 @@ export const Group = (clients: { `Error setting projects for group ${group.name}: ${err.message}` ); } - - /* REDIS - // Clear the cache for users that gained access to the project - // const groupAndParentsMembers = await getMembersFromGroupAndParents(group); - // const userIds = R.map(R.path(['user', 'id']), groupAndParentsMembers); - // for (const userId of userIds) { - // try { - // await redisClient.deleteRedisUserCache(userId); - // } catch (err) { - // logger.warn(`Error deleting user cache ${userId}: ${err}`); - // } - // } - - // try { - // await redisClient.deleteProjectGroupsCache(projectId); - // } catch (err) { - // logger.warn(`Error deleting project groups cache: ${err.message}`); - // } - REDIS */ }; const removeProjectFromGroup = async ( @@ -669,25 +630,6 @@ export const Group = (clients: { `Error setting projects for group ${group.name}: ${err.message}` ); } - - /* REDIS - // Clear the cache for users that lost access to the project - // const groupAndParentsMembers = await getMembersFromGroupAndParents(group); - // const userIds = R.map(R.path(['user', 'id']), groupAndParentsMembers); - // for (const userId of userIds) { - // try { - // await redisClient.deleteRedisUserCache(userId); - // } catch (err) { - // logger.warn(`Error deleting user cache ${userId}: ${err}`); - // } - // } - - // try { - // await redisClient.deleteProjectGroupsCache(projectId); - // } catch (err) { - // logger.warn(`Error deleting project groups cache: ${err.message}`); - // } - REDIS */ }; return { @@ -698,6 +640,7 @@ export const Group = (clients: { loadParentGroup, loadGroupsByAttribute, loadGroupsByProjectId, + loadGroupsByProjectIdFromGroups, getProjectsFromGroupAndParents, getProjectsFromGroupAndSubgroups, addGroup, diff --git a/services/api/src/models/user.ts b/services/api/src/models/user.ts index 1462dbd749..c92212ecb8 100644 --- a/services/api/src/models/user.ts +++ b/services/api/src/models/user.ts @@ -29,8 +29,8 @@ interface UserModel { loadUserById: (id: string) => Promise; loadUserByUsername: (username: string) => Promise; loadUserByIdOrUsername: (userInput: UserEdit) => Promise; - getAllGroupsForUser: (userInput: User) => Promise; - getAllProjectsIdsForUser: (userInput: User) => Promise<[number[], Group[]]>; + getAllGroupsForUser: (userId: string) => Promise; + getAllProjectsIdsForUser: (userInput: User, groups?: Group[]) => Promise<{}>; getUserRolesForProject: ( userInput: User, projectId: number, @@ -154,19 +154,20 @@ export const User = (clients: { }; const loadUserById = async (id: string): Promise => { - const keycloakUser = await keycloakAdminClient.users.findOne({ + let keycloakUser: User + keycloakUser = await keycloakAdminClient.users.findOne({ id }); + const users = await transformKeycloakUsers([keycloakUser]); + keycloakUser = users[0] if (R.isNil(keycloakUser)) { - throw new UserNotFoundError(`User not found: ${id}`); + throw new UserNotFoundError(`User not found a: ${id}`); } - - const users = await transformKeycloakUsers([keycloakUser]); - - return users[0]; + return keycloakUser; }; + // used by project resolver only, so leave this one out of redis for now const loadUserByUsername = async (username: string): Promise => { const keycloakUsers = await keycloakAdminClient.users.find({ username @@ -211,47 +212,108 @@ export const User = (clients: { return users; }; - const getAllGroupsForUser = async (userInput: User): Promise => { + const getAllGroupsForUser = async (userId: string): Promise => { const GroupModel = Group(clients); let groups = []; - // briefRepresentation pulls all the group information from keycloak including the attributes - // this means we don't need to iterate over all the groups one by one anymore to get the full group information const roleSubgroups = await keycloakAdminClient.users.listGroups({ - id: userInput.id, + id: userId, briefRepresentation: false }); + const fullGroups = await keycloakAdminClient.groups.find({briefRepresentation: false}); - for (const roleSubgroup of roleSubgroups) { - // just look up the group with what we know the parent name to be with the regex of the role stripped - // eventually this logic will have to change when we support custom roles, but this applies to *everything* if that happens - let regexp = /-(owner|maintainer|developer|reporter|guest)$/g; - const roleSubgroupParent = await GroupModel.loadGroupByName( - roleSubgroup.name.replace(regexp, "") - ); + const regexp = /-(owner|maintainer|developer|reporter|guest)$/g; - groups.push(roleSubgroupParent); + for (const fullGroup of fullGroups) { + for (const roleSubgroup of roleSubgroups) { + for (const fullSubgroup of fullGroup.subGroups) { + if (roleSubgroup.name.replace(regexp, "") == fullSubgroup.name) { + groups.push(fullSubgroup) + } + } + if (roleSubgroup.name.replace(regexp, "") == fullGroup.name) { + groups.push(fullGroup) + } + } } - return groups; + const retGroups = await GroupModel.transformKeycloakGroups(groups); + return retGroups; }; const getAllProjectsIdsForUser = async ( - userInput: User - ): Promise<[number[], Group[]]> => { + userInput: User, + groups?: Group[] + ): Promise<{}> => { const GroupModel = Group(clients); - let projects = []; + let roleProjectIds = {}; + + if (groups) { + // if groups are provided (eg, the groups have previously been calculated in a prior step), then process those groups here and extract the project ids from them + for (const roleSubgroup of groups) { + for (const fullSubgroup of roleSubgroup.subGroups) { + // https://github.com/uselagoon/lagoon/pull/3358 references potential issue with the lagoon-projects attribute where there could be empty values + // getProjectsFromGroupAndSubgroups already covers this fix + const projectIds = await GroupModel.getProjectsFromGroupAndSubgroups( + roleSubgroup + ); + if (!roleProjectIds[fullSubgroup.realmRoles[0]]) { + roleProjectIds[fullSubgroup.realmRoles[0]] = [] + } + projectIds.forEach(pid => { + roleProjectIds[fullSubgroup.realmRoles[0]].indexOf(pid) === -1 ? roleProjectIds[fullSubgroup.realmRoles[0]].push(pid) : "" + }) + } + } - const userGroups = await getAllGroupsForUser(userInput); + return roleProjectIds; + } else { + // otherwise fall back to the previous method of getting groups and project ids which is an expensive call to keycloak if repeated often + const roleSubgroups = await keycloakAdminClient.users.listGroups({ + id: userInput.id, + briefRepresentation: false + }); - for (const group of userGroups) { - const projectIds = await GroupModel.getProjectsFromGroupAndSubgroups( - group - ); - projects = [...projects, ...projectIds]; - } + const fullGroups = await keycloakAdminClient.groups.find({briefRepresentation: false}); + + // currently in lagoon groups with a role will have the role as a prefix, this regix can be used to identify and remove it to get the parent group name + const regexp = /-(owner|maintainer|developer|reporter|guest)$/g; + + for (const fullGroup of fullGroups) { + for (const roleSubgroup of roleSubgroups) { + for (const fullSubgroup of fullGroup.subGroups) { + if (roleSubgroup.name.replace(regexp, "") == fullSubgroup.name) { + // https://github.com/uselagoon/lagoon/pull/3358 references potential issue with the lagoon-projects attribute where there could be empty values + // getProjectsFromGroupAndSubgroups already covers this fix + const projectIds = await GroupModel.getProjectsFromGroupAndSubgroups( + fullSubgroup + ); + if (!roleProjectIds[roleSubgroup.realmRoles[0]]) { + roleProjectIds[roleSubgroup.realmRoles[0]] = [] + } + projectIds.forEach(pid => { + roleProjectIds[roleSubgroup.realmRoles[0]].indexOf(pid) === -1 ? roleProjectIds[roleSubgroup.realmRoles[0]].push(pid) : "" + }) + } + } + if (roleSubgroup.name.replace(regexp, "") == fullGroup.name) { + // https://github.com/uselagoon/lagoon/pull/3358 references potential issue with the lagoon-projects attribute where there could be empty values + // getProjectsFromGroupAndSubgroups already covers this fix + const projectIds = await GroupModel.getProjectsFromGroupAndSubgroups( + fullGroup + ); + if (!roleProjectIds[roleSubgroup.realmRoles[0]]) { + roleProjectIds[roleSubgroup.realmRoles[0]] = [] + } + projectIds.forEach(pid => { + roleProjectIds[roleSubgroup.realmRoles[0]].indexOf(pid) === -1 ? roleProjectIds[roleSubgroup.realmRoles[0]].push(pid) : "" + }) + } + } + } - return [R.uniq(projects), userGroups]; + return roleProjectIds; + } }; const getUserRolesForProject = async ( @@ -366,13 +428,6 @@ export const User = (clients: { throw new Error(`Error deleting user ${id}: ${err}`); } } - /* REDIS - // try { - // await redisClient.deleteRedisUserCache(id); - // } catch (err) { - // logger.error(`Error deleting user cache ${id}: ${err}`); - // } - REDIS */ }; return { diff --git a/services/api/src/resources/deployment/resolvers.ts b/services/api/src/resources/deployment/resolvers.ts index 1cdb4f015a..c356c519bd 100644 --- a/services/api/src/resources/deployment/resolvers.ts +++ b/services/api/src/resources/deployment/resolvers.ts @@ -33,7 +33,7 @@ import sha1 from 'sha1'; import { generateBuildId } from '@lagoon/commons/dist/util/lagoon'; import { jsonMerge } from '@lagoon/commons/dist/util/func'; import { logger } from '../../loggers/logger'; -import { getUserProjectIdsFromToken } from '../../util/auth'; +import { getUserProjectIdsFromRoleProjectIds } from '../../util/auth'; // @ts-ignore import uuid4 from 'uuid4'; @@ -116,7 +116,7 @@ export const getBuildLog: ResolverFn = async ( export const getDeploymentsByBulkId: ResolverFn = async ( root, { bulkId }, - { sqlClientPool, hasPermission, models, keycloakGrant } + { sqlClientPool, hasPermission, models, keycloakGrant, keycloakUsersGroups } ) => { /* @@ -136,10 +136,10 @@ export const getDeploymentsByBulkId: ResolverFn = async ( return []; } - // pull the project ids from the token - // this can have a negative effect if the token is not refreshed after performing an operation like adding a project - // the user will probably have to refresh their token, with short token lifespans this should not be much of an issue - userProjectIds = getUserProjectIdsFromToken(keycloakGrant); + const userProjectRoles = await models.UserModel.getAllProjectsIdsForUser({ + id: keycloakGrant.access_token.content.sub + }, keycloakUsersGroups); + userProjectIds = getUserProjectIdsFromRoleProjectIds(userProjectRoles); } let queryBuilder = knex('deployment') @@ -164,7 +164,7 @@ export const getDeploymentsByBulkId: ResolverFn = async ( export const getDeploymentsByFilter: ResolverFn = async ( root, input, - { sqlClientPool, hasPermission, models, keycloakGrant } + { sqlClientPool, hasPermission, models, keycloakGrant, keycloakUsersGroups } ) => { const { openshifts, deploymentStatus = ["NEW", "PENDING", "RUNNING", "QUEUED"] } = input; @@ -186,10 +186,10 @@ export const getDeploymentsByFilter: ResolverFn = async ( return []; } - // pull the project ids from the token - // this can have a negative effect if the token is not refreshed after performing an operation like adding a project - // the user will probably have to refresh their token, with short token lifespans this should not be much of an issue - userProjectIds = getUserProjectIdsFromToken(keycloakGrant); + const userProjectRoles = await models.UserModel.getAllProjectsIdsForUser({ + id: keycloakGrant.access_token.content.sub + }, keycloakUsersGroups); + userProjectIds = getUserProjectIdsFromRoleProjectIds(userProjectRoles); } let queryBuilder = knex.select("deployment.*").from('deployment'). @@ -1193,7 +1193,7 @@ export const switchActiveStandby: ResolverFn = async ( export const bulkDeployEnvironmentLatest: ResolverFn = async ( _root, { input: { environments: environmentsInput, buildVariables, name: bulkName } }, - { keycloakGrant, models, sqlClientPool, hasPermission, userActivityLogger } + { keycloakGrant, models, sqlClientPool, hasPermission, userActivityLogger, keycloakUsersGroups } ) => { /* @@ -1213,10 +1213,10 @@ export const bulkDeployEnvironmentLatest: ResolverFn = async ( return []; } - // pull the project ids from the token - // this can have a negative effect if the token is not refreshed after performing an operation like adding a project - // the user will probably have to refresh their token, with short token lifespans this should not be much of an issue - userProjectIds = getUserProjectIdsFromToken(keycloakGrant); + const userProjectRoles = await models.UserModel.getAllProjectsIdsForUser({ + id: keycloakGrant.access_token.content.sub + }, keycloakUsersGroups); + userProjectIds = getUserProjectIdsFromRoleProjectIds(userProjectRoles); } let bulkId = uuid4(); diff --git a/services/api/src/resources/environment/resolvers.ts b/services/api/src/resources/environment/resolvers.ts index f42f833ebf..5a659e9961 100644 --- a/services/api/src/resources/environment/resolvers.ts +++ b/services/api/src/resources/environment/resolvers.ts @@ -11,6 +11,7 @@ import { Sql } from './sql'; import { Sql as projectSql } from '../project/sql'; import { Helpers as projectHelpers } from '../project/helpers'; import { getFactFilteredEnvironmentIds } from '../fact/resolvers'; +import { logger } from '../../loggers/logger'; export const getEnvironmentByName: ResolverFn = async ( root, @@ -75,6 +76,9 @@ export const getEnvironmentsByProjectId: ResolverFn = async ( // @TODO: When this performance issue is fixed for real, remove this hack as // it hardcodes a "everyone can view environments" authz rule. if (!R.prop('environmentAuthz', project)) { + // when requesting a large number of projects outside of the `allProjects` resolver + // eg, allGroups, or any other resolver that can sub resolve projects and environments + // this can cause significant response slowness as this is a check against keycloak directly for every environment await hasPermission('environment', 'view', { project: pid }); diff --git a/services/api/src/resources/fact/resolvers.ts b/services/api/src/resources/fact/resolvers.ts index 1b5e21abb6..3aa9fb44c8 100644 --- a/services/api/src/resources/fact/resolvers.ts +++ b/services/api/src/resources/fact/resolvers.ts @@ -9,7 +9,7 @@ import crypto from 'crypto'; import { Service } from 'aws-sdk'; import * as api from '@lagoon/commons/dist/api'; import { getEnvironmentsByProjectId } from '../environment/resolvers'; -import { getUserProjectIdsFromToken } from '../../util/auth'; +import { getUserProjectIdsFromRoleProjectIds } from '../../util/auth'; export const getFactsByEnvironmentId: ResolverFn = async ( { id: environmentId, environmentAuthz }, @@ -123,7 +123,7 @@ const getSqlPredicate = (predicate) => { export const getProjectsByFactSearch: ResolverFn = async ( root, { input }, - { sqlClientPool, hasPermission, keycloakGrant, models } + { sqlClientPool, hasPermission, keycloakGrant, models, keycloakUsersGroups } ) => { let isAdmin = false; @@ -137,10 +137,10 @@ export const getProjectsByFactSearch: ResolverFn = async ( return []; } - // pull the project ids from the token - // this can have a negative effect if the token is not refreshed after performing an operation like adding a project - // the user will probably have to refresh their token, with short token lifespans this should not be much of an issue - userProjectIds = getUserProjectIdsFromToken(keycloakGrant); + const userProjectRoles = await models.UserModel.getAllProjectsIdsForUser({ + id: keycloakGrant.access_token.content.sub + }, keycloakUsersGroups); + userProjectIds = getUserProjectIdsFromRoleProjectIds(userProjectRoles); } const count = await getFactFilteredProjectsCount(input, userProjectIds, sqlClientPool, isAdmin); @@ -156,7 +156,7 @@ export const getProjectsByFactSearch: ResolverFn = async ( export const getEnvironmentsByFactSearch: ResolverFn = async ( root, { input }, - { sqlClientPool, hasPermission, keycloakGrant, models } + { sqlClientPool, hasPermission, keycloakGrant, models, keycloakUsersGroups } ) => { let isAdmin = false; @@ -170,10 +170,10 @@ export const getEnvironmentsByFactSearch: ResolverFn = async ( return []; } - // pull the project ids from the token - // this can have a negative effect if the token is not refreshed after performing an operation like adding a project - // the user will probably have to refresh their token, with short token lifespans this should not be much of an issue - userProjectIds = getUserProjectIdsFromToken(keycloakGrant); + const userProjectRoles = await models.UserModel.getAllProjectsIdsForUser({ + id: keycloakGrant.access_token.content.sub + }, keycloakUsersGroups); + userProjectIds = getUserProjectIdsFromRoleProjectIds(userProjectRoles); } const count = await getFactFilteredEnvironmentsCount(input, userProjectIds, sqlClientPool, isAdmin); diff --git a/services/api/src/resources/group/resolvers.ts b/services/api/src/resources/group/resolvers.ts index 509eb6f806..6ca99d6a6c 100644 --- a/services/api/src/resources/group/resolvers.ts +++ b/services/api/src/resources/group/resolvers.ts @@ -11,57 +11,75 @@ import { KeycloakUnauthorizedError } from '../../util/auth'; export const getAllGroups: ResolverFn = async ( root, { name, type }, - { hasPermission, models, keycloakGrant } + { hasPermission, models, keycloakGrant, keycloakGroups, keycloakUsersGroups, adminScopes } ) => { - try { - await hasPermission('group', 'viewAll'); - - if (name) { - const group = await models.GroupModel.loadGroupByName(name); - return [group]; - } else { - const groups = await models.GroupModel.loadAllGroups(); - const filterFn = (key, val) => group => group[key].includes(val); - const filteredByName = groups.filter(filterFn('name', name)); - const filteredByType = groups.filter(filterFn('type', type)); - return name || type ? R.union(filteredByName, filteredByType) : groups; - } - } catch (err) { - if (name && err instanceof GroupNotFoundError) { - throw err; - } + // use the admin scope check instead of `hasPermission` for speed + if (adminScopes.groupViewAll) { + try { - if (err instanceof KeycloakUnauthorizedError) { - if (!keycloakGrant) { - logger.debug('No grant available for getAllGroups'); - return []; + if (name) { + const group = await models.GroupModel.loadGroupByName(name); + return [group]; } else { - const user = await models.UserModel.loadUserById( - keycloakGrant.access_token.content.sub - ); - const userGroups = await models.UserModel.getAllGroupsForUser(user); - - if (name) { - return R.filter(R.propEq('name', name), userGroups); - } else { - return userGroups; + const groups = keycloakGroups; + const filterFn = (key, val) => group => group[key].includes(val); + const filteredByName = groups.filter(filterFn('name', name)); + const filteredByType = groups.filter(filterFn('type', type)); + return name || type ? R.union(filteredByName, filteredByType) : groups; + } + } catch (err) { + if (name && err instanceof GroupNotFoundError) { + throw err; + } + + if (err instanceof KeycloakUnauthorizedError) { + if (!keycloakGrant) { + logger.debug('No grant available for getAllGroups'); + return []; } } + + logger.warn(`getAllGroups failed unexpectedly: ${err.message}`); + throw err; } + } - logger.warn(`getAllGroups failed unexpectedly: ${err.message}`); - throw err; + const userGroups = await keycloakUsersGroups; + + if (name) { + return R.filter(R.propEq('name', name), userGroups); + } else { + return userGroups; } + }; +// TODO: recursive lookups for groups in groups? +export const getGroupFromGroups = async (id, groups) => { + const d = R.filter(R.propEq('id', id), groups); + if (d.length) { + return d[0]; + } + for (const group in groups) { + if (groups[group].groups.length) { + const d = R.filter(R.propEq('id', id), groups[group].groups) + if (d.length) { + return d[0]; + } + } + } + return {}; +} + export const getMembersByGroupId: ResolverFn = async ( { id }, _input, - { hasPermission, models, keycloakGrant } + { hasPermission, models, keycloakGrant, keycloakGroups } ) => { try { - await hasPermission('group', 'viewAll'); - const group = await models.GroupModel.loadGroupById(id); + // members resolver is only called by group, no need to check the permissions on the group + // as the group resolver will have already checked permission + const group = await getGroupFromGroups(id, keycloakGroups); const members = await models.GroupModel.getGroupMembership(group); return members; } catch (err) { @@ -69,24 +87,10 @@ export const getMembersByGroupId: ResolverFn = async ( if (!keycloakGrant) { logger.debug('No grant available for getGroupByName'); throw new GroupNotFoundError(`Group not found: ${id}`); - } else { - const user = await models.UserModel.loadUserById( - keycloakGrant.access_token.content.sub - ); - const userGroups = await models.UserModel.getAllGroupsForUser(user); - - const group = R.head(R.filter(R.propEq('id', id), userGroups)); - - if (R.isEmpty(group)) { - throw new GroupNotFoundError(`Group not found: ${id}`); - } - const members = await models.GroupModel.getGroupMembership(group); - - return members; } } - logger.warn(`getGroupByName failed unexpectedly: ${err.message}`); + logger.warn(`getGroupByName failed unexpectedly: ${err.message} ${id}`); throw err; } } @@ -94,98 +98,87 @@ export const getMembersByGroupId: ResolverFn = async ( export const getGroupsByProjectId: ResolverFn = async ( { id: pid }, _input, - { hasPermission, models, keycloakGrant } + { hasPermission, models, keycloakGrant, keycloakGroups, keycloakUsersGroups, adminScopes } ) => { - const projectGroups = await models.GroupModel.loadGroupsByProjectId(pid); - - try { - await hasPermission('group', 'viewAll'); - - return projectGroups; - } catch (err) { - if (!keycloakGrant) { - logger.debug('No grant available for getGroupsByProjectId'); - return []; + // use the admin scope check instead of `hasPermission` for speed + if (adminScopes.groupViewAll) { + try { + const projectGroups = await models.GroupModel.loadGroupsByProjectIdFromGroups(pid, keycloakGroups); + return projectGroups; + } catch (err) { + if (!keycloakGrant) { + logger.debug('No grant available for getGroupsByProjectId'); + return []; + } } + } - const user = await models.UserModel.loadUserById( - keycloakGrant.access_token.content.sub - ); - const userGroups = await models.UserModel.getAllGroupsForUser(user); - const userProjectGroups = R.intersection(projectGroups, userGroups); + const projectGroups = await models.GroupModel.loadGroupsByProjectIdFromGroups(pid, keycloakGroups); + const userGroups = keycloakUsersGroups; + const userProjectGroups = R.intersection(projectGroups, userGroups); - return userProjectGroups; - } + return userProjectGroups; }; export const getGroupsByUserId: ResolverFn = async ( { id: uid }, _input, - { hasPermission, models, keycloakGrant } + { hasPermission, models, keycloakGrant, keycloakUsersGroups, adminScopes } ) => { - const queryUser = await models.UserModel.loadUserById(uid); - const queryUserGroups = await models.UserModel.getAllGroupsForUser(queryUser); - - try { - await hasPermission('group', 'viewAll'); + // use the admin scope check instead of `hasPermission` for speed + if (adminScopes.groupViewAll) { + try { + const queryUserGroups = await models.UserModel.getAllGroupsForUser(uid); - return queryUserGroups; - } catch (err) { - if (!keycloakGrant) { - logger.debug('No grant available for getGroupsByUserId'); - return []; + return queryUserGroups; + } catch (err) { + if (!keycloakGrant) { + logger.debug('No grant available for getGroupsByUserId'); + return []; + } } - - const currentUser = await models.UserModel.loadUserById( - keycloakGrant.access_token.content.sub - ); - const currentUserGroups = await models.UserModel.getAllGroupsForUser( - currentUser - ); - const bothUserGroups = R.intersection(queryUserGroups, currentUserGroups); - - return bothUserGroups; } + const currentUserGroups = keycloakUsersGroups; + // const bothUserGroups = R.intersection(queryUserGroups, currentUserGroups); + + return currentUserGroups; }; export const getGroupByName: ResolverFn = async ( root, { name }, - { models, hasPermission, keycloakGrant } + { models, hasPermission, keycloakGrant, keycloakUsersGroups, adminScopes } ) => { - try { - await hasPermission('group', 'viewAll'); - - const group = await models.GroupModel.loadGroupByName(name); - return group; - } catch (err) { - if (err instanceof GroupNotFoundError) { - throw err; - } - - if (err instanceof KeycloakUnauthorizedError) { - if (!keycloakGrant) { - logger.debug('No grant available for getGroupByName'); - throw new GroupNotFoundError(`Group not found: ${name}`); - } else { - const user = await models.UserModel.loadUserById( - keycloakGrant.access_token.content.sub - ); - const userGroups = await models.UserModel.getAllGroupsForUser(user); - - const group = R.head(R.filter(R.propEq('name', name), userGroups)); + // use the admin scope check instead of `hasPermission` for speed + if (adminScopes.groupViewAll) { + try { + const group = await models.GroupModel.loadGroupByName(name); + return group; + } catch (err) { + if (err instanceof GroupNotFoundError) { + throw err; + } - if (R.isEmpty(group)) { + if (err instanceof KeycloakUnauthorizedError) { + if (!keycloakGrant) { + logger.debug('No grant available for getGroupByName'); throw new GroupNotFoundError(`Group not found: ${name}`); } - - return group; } + + logger.warn(`getGroupByName failed unexpectedly: ${err.message}`); + throw err; } + } - logger.warn(`getGroupByName failed unexpectedly: ${err.message}`); - throw err; + const userGroups = keycloakUsersGroups; + const group = R.head(R.filter(R.propEq('name', name), userGroups)); + + if (R.isEmpty(group)) { + throw new GroupNotFoundError(`Group not found: ${name}`); } + + return group; }; export const addGroup: ResolverFn = async ( @@ -312,11 +305,11 @@ export const deleteGroup: ResolverFn = async ( export const deleteAllGroups: ResolverFn = async ( _root, _args, - { models, hasPermission } + { models, hasPermission, keycloakGroups } ) => { await hasPermission('group', 'deleteAll'); - const groups = await models.GroupModel.loadAllGroups(); + const groups = keycloakGroups; let deleteErrors: String[] = []; for (const group of groups) { @@ -489,28 +482,40 @@ export const getAllProjectsByGroupId: ResolverFn = async ( export const getAllProjectsInGroup: ResolverFn = async ( _root, { input: groupInput }, - { models, sqlClientPool, hasPermission, keycloakGrant } + { models, sqlClientPool, hasPermission, keycloakGrant, keycloakGroups, keycloakUsersGroups, adminScopes } ) => { const { GroupModel: { loadGroupByIdOrName, getProjectsFromGroupAndSubgroups } } = models; - try { - await hasPermission('group', 'viewAll'); - - const group = await loadGroupByIdOrName(groupInput); - const projectIdsArray = await getProjectsFromGroupAndSubgroups(group); - return projectIdsArray.map(async id => - projectHelpers(sqlClientPool).getProjectByProjectInput({ id }) - ); - } catch (err) { - if (err instanceof GroupNotFoundError) { - throw err; - } + // Since we've + // already authorized the user has access to the group, and thus the associated projects we are + // returning, AND all user roles are allowed to view all environments of a project, we can + // short-circuit the slow keycloak check in the getEnvironmentsByProjectId + // resolver. + // + // @TODO: When this performance issue is fixed for real, remove this hack as + // it hardcodes a "everyone can view environments" authz rule. + let environmentAuthz = true + + // use the admin scope check instead of `hasPermission` for speed + if (adminScopes.groupViewAll) { + try { + // get group from all keycloak groups apollo context + const group = await getGroupFromGroups(groupInput.id, keycloakGroups) + const projectIdsArray = await getProjectsFromGroupAndSubgroups(group); + return projectIdsArray.map(async id => + projectHelpers(sqlClientPool).getProjectByProjectInput({ id }, environmentAuthz) + ); + } catch (err) { + if (err instanceof GroupNotFoundError) { + throw err; + } - if (!(err instanceof KeycloakUnauthorizedError)) { - logger.warn(`getAllGroups failed unexpectedly: ${err.message}`); - throw err; + if (!(err instanceof KeycloakUnauthorizedError)) { + logger.warn(`getAllGroups failed unexpectedly: ${err.message}`); + throw err; + } } } @@ -518,22 +523,20 @@ export const getAllProjectsInGroup: ResolverFn = async ( logger.debug('No grant available for getAllProjectsInGroup'); return []; } else { - const group = await loadGroupByIdOrName(groupInput); - - const user = await models.UserModel.loadUserById( - keycloakGrant.access_token.content.sub - ); - const userGroups = await models.UserModel.getAllGroupsForUser(user); + // get group from all keycloak groups apollo context + const group = await getGroupFromGroups(groupInput.id, keycloakGroups) + // get users groups from users keycloak groups apollo context + const userGroups = keycloakUsersGroups; // @ts-ignore if (!R.contains(group.name, R.pluck('name', userGroups))) { logger.debug('No grant available for getAllProjectsInGroup'); return []; } - const projectIdsArray = await getProjectsFromGroupAndSubgroups(group); + return projectIdsArray.map(async id => - projectHelpers(sqlClientPool).getProjectByProjectInput({ id }) + projectHelpers(sqlClientPool).getProjectByProjectInput({ id }, environmentAuthz) ); } }; diff --git a/services/api/src/resources/project/helpers.ts b/services/api/src/resources/project/helpers.ts index e5274ecd51..a5f011b07f 100644 --- a/services/api/src/resources/project/helpers.ts +++ b/services/api/src/resources/project/helpers.ts @@ -67,7 +67,7 @@ export const Helpers = (sqlClientPool: Pool) => { return parseInt(pid, 10); }, - getProjectByProjectInput: async projectInput => { + getProjectByProjectInput: async (projectInput, environmentAuthz?) => { const notEmpty = R.complement(R.anyPass([R.isNil, R.isEmpty])); const hasId = R.both(R.has('id'), R.propSatisfies(notEmpty, 'id')); const hasName = R.both(R.has('name'), R.propSatisfies(notEmpty, 'name')); @@ -76,6 +76,11 @@ export const Helpers = (sqlClientPool: Pool) => { if (!project) { throw new Error('Unauthorized'); } + if (environmentAuthz) { + // @TODO: When this performance issue is fixed for real, remove this hack as + // it hardcodes a "everyone can view environments" authz rule. + project.environmentAuthz = true; + } return project; }); @@ -87,6 +92,11 @@ export const Helpers = (sqlClientPool: Pool) => { if (!project) { throw new Error('Unauthorized'); } + if (environmentAuthz) { + // @TODO: When this performance issue is fixed for real, remove this hack as + // it hardcodes a "everyone can view environments" authz rule. + project.environmentAuthz = true; + } return project; }); diff --git a/services/api/src/resources/project/resolvers.ts b/services/api/src/resources/project/resolvers.ts index 746c440201..79cfd751e8 100644 --- a/services/api/src/resources/project/resolvers.ts +++ b/services/api/src/resources/project/resolvers.ts @@ -12,7 +12,7 @@ import * as OS from '../openshift/sql'; import { generatePrivateKey, getSshKeyFingerprint } from '../sshKey'; import { Sql as sshKeySql } from '../sshKey/sql'; import { createHarborOperations } from './harborSetup'; -import { getUserProjectIdsFromToken } from '../../util/auth'; +import { getUserProjectIdsFromRoleProjectIds } from '../../util/auth'; import sql from '../user/sql'; const DISABLE_CORE_HARBOR = process.env.DISABLE_CORE_HARBOR || "false" @@ -51,7 +51,7 @@ export const getPrivateKey: ResolverFn = async ( export const getAllProjects: ResolverFn = async ( root, { order, createdAfter, gitUrl }, - { sqlClientPool, hasPermission, models, keycloakGrant } + { sqlClientPool, hasPermission, models, keycloakGrant, keycloakGroups, keycloakUsersGroups } ) => { let userProjectIds: number[]; @@ -63,10 +63,11 @@ export const getAllProjects: ResolverFn = async ( return []; } - // pull the project ids from the token - // this can have a negative effect if the token is not refreshed after performing an operation like adding a project - // the user will probably have to refresh their token, with short token lifespans this should not be much of an issue - userProjectIds = getUserProjectIdsFromToken(keycloakGrant); + const userProjectRoles = await models.UserModel.getAllProjectsIdsForUser({ + id: keycloakGrant.access_token.content.sub, + + }, keycloakUsersGroups); + userProjectIds = getUserProjectIdsFromRoleProjectIds(userProjectRoles); } let queryBuilder = knex('project'); @@ -203,7 +204,7 @@ export const getProjectByName: ResolverFn = async ( export const getProjectsByMetadata: ResolverFn = async ( root, { metadata }, - { sqlClientPool, hasPermission, keycloakGrant, models } + { sqlClientPool, hasPermission, keycloakGrant, models, keycloakUsersGroups } ) => { let userProjectIds: number[]; try { @@ -213,10 +214,11 @@ export const getProjectsByMetadata: ResolverFn = async ( logger.debug('No grant available for getProjectsByMetadata'); return []; } - // pull the project ids from the token - // this can have a negative effect if the token is not refreshed after performing an operation like adding a project - // the user will probably have to refresh their token, with short token lifespans this should not be much of an issue - userProjectIds = getUserProjectIdsFromToken(keycloakGrant); + + const userProjectRoles = await models.UserModel.getAllProjectsIdsForUser({ + id: keycloakGrant.access_token.content.sub + }, keycloakUsersGroups); + userProjectIds = getUserProjectIdsFromRoleProjectIds(userProjectRoles); } let queryBuilder = knex('project'); From 7840fa20ec171a47c56a7ec712ef98dc4970739e Mon Sep 17 00:00:00 2001 From: shreddedbacon Date: Mon, 6 Mar 2023 11:02:51 +1100 Subject: [PATCH 09/11] chore: do more in the request --- services/api/src/apolloServer.js | 23 +++++++++++++++-------- services/api/src/util/auth.ts | 19 ++++++------------- 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/services/api/src/apolloServer.js b/services/api/src/apolloServer.js index e2a88a3916..7b451f6195 100644 --- a/services/api/src/apolloServer.js +++ b/services/api/src/apolloServer.js @@ -24,6 +24,7 @@ const { logger } = require('./loggers/logger'); const { userActivityLogger } = require('./loggers/userActivityLogger'); const typeDefs = require('./typeDefs'); const resolvers = require('./resolvers'); +const { keycloakGrantManager } = require('./clients/keycloakClient'); const User = require('./models/user'); const Group = require('./models/group'); @@ -169,16 +170,22 @@ const apolloServer = new ApolloServer({ }; // get all keycloak groups, do this early to reduce the number of times this is called otherwise - // would be good if this could be wrapped in a way so that only resolvers that need groups are called // but doing this early and once is pretty cheap let allGroups = await Group.Group(modelClients).loadAllGroups(); let keycloakGroups = await Group.Group(modelClients).transformKeycloakGroups(allGroups); + let currentUser = {}; + let serviceAccount = {}; // if this is a user request, get the users keycloak groups too, do this one to reduce the number of times it is called elsewhere let keycloakUsersGroups = [] - let kcg = req.kauth ? req.kauth.grant : null - if (kcg) { - keycloakUsersGroups = await User.User(modelClients).getAllGroupsForUser(kcg.access_token.content.sub); + let groupRoleProjectIds = [] + const keycloakGrant = req.kauth ? req.kauth.grant : null + if (keycloakGrant) { + keycloakUsersGroups = await User.User(modelClients).getAllGroupsForUser(keycloakGrant.access_token.content.sub); + serviceAccount = await keycloakGrantManager.obtainFromClientCredentials(); + currentUser = await User.User(modelClients).loadUserById(keycloakGrant.access_token.content.sub); + // grab the users project ids and roles in the first request + groupRoleProjectIds = await User.User(modelClients).getAllProjectsIdsForUser(currentUser, keycloakUsersGroups); } // do a permission check to see if the user is platform admin/owner, or has permission for `viewAll` on certain resources @@ -186,8 +193,8 @@ const apolloServer = new ApolloServer({ // every `hasPermission` check adds a delay, and if you're a member of a group that has a lot of projects and environments, hasPermissions is costly when we perform // the viewAll permission check, to then error out and follow through with the standard user permission check, effectively costing 2 hasPermission calls for every request // this eliminates a huge number of these by making it available in the apollo context - let hasPermission = req.kauth - ? keycloakHasPermission(req.kauth.grant, requestCache, modelClients) + const hasPermission = req.kauth + ? keycloakHasPermission(req.kauth.grant, requestCache, modelClients, serviceAccount, currentUser, groupRoleProjectIds) : legacyHasPermission(req.legacyCredentials) let projectViewAll = false let groupViewAll = false @@ -221,8 +228,8 @@ const apolloServer = new ApolloServer({ return { keycloakAdminClient, sqlClientPool, - hasPermission: hasPermission, - keycloakGrant: req.kauth ? req.kauth.grant : null, + hasPermission, + keycloakGrant, requestCache, userActivityLogger: (message, meta) => { let defaultMeta = { diff --git a/services/api/src/util/auth.ts b/services/api/src/util/auth.ts index ff7779288b..9c8da4b109 100644 --- a/services/api/src/util/auth.ts +++ b/services/api/src/util/auth.ts @@ -5,7 +5,6 @@ import { getConfigFromEnv } from '../util/config'; import { isNotNil } from './func'; import { keycloakGrantManager } from '../clients/keycloakClient'; const { userActivityLogger } = require('../loggers/userActivityLogger'); -import { User } from '../models/user'; import { Group } from '../models/group'; interface ILegacyToken { @@ -146,15 +145,10 @@ export class KeycloakUnauthorizedError extends Error { } } -export const keycloakHasPermission = (grant, requestCache, modelClients, keycloakUsersGroups) => { - const UserModel = User(modelClients); +export const keycloakHasPermission = (grant, requestCache, modelClients, serviceAccount, currentUser, groupRoleProjectIds) => { const GroupModel = Group(modelClients); return async (resource, scope, attributes: IKeycloakAuthAttributes = {}) => { - const currentUserId: string = grant.access_token.content.sub; - - const currentUser = await UserModel.loadUserById(currentUserId); - const serviceAccount = await keycloakGrantManager.obtainFromClientCredentials(); let claims: { currentUser: [string]; @@ -164,7 +158,7 @@ export const keycloakHasPermission = (grant, requestCache, modelClients, keycloa userProjectRole?: [string]; userGroupRole?: [string]; } = { - currentUser: [currentUserId] + currentUser: [currentUser.id] }; @@ -178,7 +172,7 @@ export const keycloakHasPermission = (grant, requestCache, modelClients, keycloa R.prop('users') )(attributes) ], - currentUser: [currentUserId] + currentUser: [currentUser.id] }; } @@ -193,8 +187,7 @@ export const keycloakHasPermission = (grant, requestCache, modelClients, keycloa projectQuery: [`${projectId}`] }; - const groupRoleIds = await UserModel.getAllProjectsIdsForUser(currentUser, keycloakUsersGroups); - const [highestRoleForProject, upids] = getUserRoleForProjectFromRoleProjectIds(groupRoleIds, projectId) + const [highestRoleForProject, upids] = getUserRoleForProjectFromRoleProjectIds(groupRoleProjectIds, projectId) if (upids.length) { claims = { ...claims, @@ -223,7 +216,7 @@ export const keycloakHasPermission = (grant, requestCache, modelClients, keycloa const groupRoles = R.pipe( R.filter(membership => - R.pathEq(['user', 'id'], currentUserId, membership) + R.pathEq(['user', 'id'], currentUser.id, membership) ), R.pluck('role') )(group.members); @@ -297,7 +290,7 @@ export const keycloakHasPermission = (grant, requestCache, modelClients, keycloa userActivityLogger.user_info( `User does not have permission to '${scope}' on '${resource}'`, { - user: currentUserId + user: currentUser.id } ); } From 0bab73a1e55f63fb6cc182ebbb9ec297d4937a21 Mon Sep 17 00:00:00 2001 From: shreddedbacon Date: Mon, 6 Mar 2023 11:03:29 +1100 Subject: [PATCH 10/11] chore: remove environmentauthz since haspermission is a lot faster now, and add adminscope checks for some permissions --- .../api/src/resources/backup/resolvers.ts | 11 ++-- .../api/src/resources/deployment/resolvers.ts | 6 +- .../src/resources/env-variables/resolvers.ts | 54 ++++++++++-------- .../src/resources/environment/resolvers.ts | 14 +---- services/api/src/resources/fact/resolvers.ts | 23 +++----- services/api/src/resources/group/resolvers.ts | 14 +---- .../api/src/resources/insight/resolvers.ts | 6 +- .../api/src/resources/openshift/resolvers.ts | 21 ++++--- .../api/src/resources/problem/resolvers.ts | 10 ++-- services/api/src/resources/project/helpers.ts | 12 +--- .../api/src/resources/project/resolvers.ts | 57 +++++++------------ services/api/src/resources/task/resolvers.ts | 11 ++-- 12 files changed, 105 insertions(+), 134 deletions(-) diff --git a/services/api/src/resources/backup/resolvers.ts b/services/api/src/resources/backup/resolvers.ts index 31e799779f..503bd6bfd0 100644 --- a/services/api/src/resources/backup/resolvers.ts +++ b/services/api/src/resources/backup/resolvers.ts @@ -138,14 +138,17 @@ export const getRestoreLocation: ResolverFn = async ( export const getBackupsByEnvironmentId: ResolverFn = async ( { id: environmentId }, { includeDeleted, limit }, - { sqlClientPool, hasPermission } + { sqlClientPool, hasPermission, adminScopes } ) => { const environment = await environmentHelpers( sqlClientPool ).getEnvironmentById(environmentId); - await hasPermission('backup', 'view', { - project: environment.project - }); + + if (!adminScopes.projectViewAll) { + await hasPermission('backup', 'view', { + project: environment.project + }); + } let queryBuilder = knex('environment_backup') .where('environment', environmentId) diff --git a/services/api/src/resources/deployment/resolvers.ts b/services/api/src/resources/deployment/resolvers.ts index 063421a216..4092874304 100644 --- a/services/api/src/resources/deployment/resolvers.ts +++ b/services/api/src/resources/deployment/resolvers.ts @@ -217,15 +217,15 @@ export const getDeploymentsByFilter: ResolverFn = async ( }; export const getDeploymentsByEnvironmentId: ResolverFn = async ( - { id: eid, environmentAuthz }, + { id: eid }, { name, limit }, - { sqlClientPool, hasPermission } + { sqlClientPool, hasPermission, adminScopes } ) => { const environment = await environmentHelpers( sqlClientPool ).getEnvironmentById(eid); - if (!environmentAuthz) { + if (!adminScopes.projectViewAll) { await hasPermission('deployment', 'view', { project: environment.project }); diff --git a/services/api/src/resources/env-variables/resolvers.ts b/services/api/src/resources/env-variables/resolvers.ts index 1c36105d82..b4105bf178 100644 --- a/services/api/src/resources/env-variables/resolvers.ts +++ b/services/api/src/resources/env-variables/resolvers.ts @@ -12,11 +12,13 @@ import { logger } from '../../loggers/logger'; export const getEnvVarsByProjectId: ResolverFn = async ( { id: pid }, args, - { sqlClientPool, hasPermission } + { sqlClientPool, hasPermission, adminScopes } ) => { - await hasPermission('env_var', 'project:view', { - project: pid - }); + if (!adminScopes.projectViewAll) { + await hasPermission('env_var', 'project:view', { + project: pid + }); + } const rows = await query( sqlClientPool, @@ -29,19 +31,21 @@ export const getEnvVarsByProjectId: ResolverFn = async ( export const getEnvVarsByEnvironmentId: ResolverFn = async ( { id: eid }, args, - { sqlClientPool, hasPermission } + { sqlClientPool, hasPermission, adminScopes } ) => { const environment = await environmentHelpers( sqlClientPool ).getEnvironmentById(eid); - await hasPermission( - 'env_var', - `environment:view:${environment.environmentType}`, - { - project: environment.project - } - ); + if (!adminScopes.projectViewAll) { + await hasPermission( + 'env_var', + `environment:view:${environment.environmentType}`, + { + project: environment.project + } + ); + } const rows = await query( sqlClientPool, @@ -335,7 +339,7 @@ export const addOrUpdateEnvVariableByName: ResolverFn = async ( export const getEnvVariablesByProjectEnvironmentName: ResolverFn = async ( root, { input: { project: projectName, environment: environmentName } }, - { sqlClientPool, hasPermission, userActivityLogger } + { sqlClientPool, hasPermission, userActivityLogger, adminScopes } ) => { const projectId = await projectHelpers(sqlClientPool).getProjectIdByName( projectName @@ -349,13 +353,15 @@ export const getEnvVariablesByProjectEnvironmentName: ResolverFn = async ( ); const environment = environmentRows[0]; - await hasPermission( - 'env_var', - `environment:view:${environment.environmentType}`, - { - project: projectId - } - ); + if (!adminScopes.projectViewAll) { + await hasPermission( + 'env_var', + `environment:view:${environment.environmentType}`, + { + project: projectId + } + ); + } const environmentVariables = await query( sqlClientPool, @@ -364,9 +370,11 @@ export const getEnvVariablesByProjectEnvironmentName: ResolverFn = async ( return environmentVariables } else { - await hasPermission('env_var', 'project:view', { - project: projectId - }); + if (!adminScopes.projectViewAll) { + await hasPermission('env_var', 'project:view', { + project: projectId + }); + } // is project const projectVariables = await query( sqlClientPool, diff --git a/services/api/src/resources/environment/resolvers.ts b/services/api/src/resources/environment/resolvers.ts index 5a659e9961..d01b5f9c5d 100644 --- a/services/api/src/resources/environment/resolvers.ts +++ b/services/api/src/resources/environment/resolvers.ts @@ -66,19 +66,11 @@ export const getEnvironmentById = async ( export const getEnvironmentsByProjectId: ResolverFn = async ( project, args, - { sqlClientPool, hasPermission, keycloakGrant, models } + { sqlClientPool, hasPermission, keycloakGrant, models, adminScopes } ) => { const { id: pid } = project; - // The getAllProjects resolver will authorize environment access already, - // so we can skip the request to keycloak. - // - // @TODO: When this performance issue is fixed for real, remove this hack as - // it hardcodes a "everyone can view environments" authz rule. - if (!R.prop('environmentAuthz', project)) { - // when requesting a large number of projects outside of the `allProjects` resolver - // eg, allGroups, or any other resolver that can sub resolve projects and environments - // this can cause significant response slowness as this is a check against keycloak directly for every environment + if (!adminScopes.projectViewAll) { await hasPermission('environment', 'view', { project: pid }); @@ -104,7 +96,7 @@ export const getEnvironmentsByProjectId: ResolverFn = async ( ); const withK8s = Helpers(sqlClientPool).aliasOpenshiftToK8s(rows); - return withK8s.map(row => ({ ...row, environmentAuthz: true })); + return withK8s; }; export const getEnvironmentByDeploymentId: ResolverFn = async ( diff --git a/services/api/src/resources/fact/resolvers.ts b/services/api/src/resources/fact/resolvers.ts index 3aa9fb44c8..a88027f991 100644 --- a/services/api/src/resources/fact/resolvers.ts +++ b/services/api/src/resources/fact/resolvers.ts @@ -12,15 +12,15 @@ import { getEnvironmentsByProjectId } from '../environment/resolvers'; import { getUserProjectIdsFromRoleProjectIds } from '../../util/auth'; export const getFactsByEnvironmentId: ResolverFn = async ( - { id: environmentId, environmentAuthz }, + { id: environmentId }, { keyFacts, limit, summary }, - { sqlClientPool, hasPermission } + { sqlClientPool, hasPermission, adminScopes } ) => { const environment = await environmentHelpers( sqlClientPool ).getEnvironmentById(environmentId); - if (!environmentAuthz) { + if (!adminScopes.projectViewAll) { await hasPermission('fact', 'view', { project: environment.project }); @@ -123,12 +123,15 @@ const getSqlPredicate = (predicate) => { export const getProjectsByFactSearch: ResolverFn = async ( root, { input }, - { sqlClientPool, hasPermission, keycloakGrant, models, keycloakUsersGroups } + { sqlClientPool, hasPermission, keycloakGrant, models, keycloakUsersGroups }, + info ) => { let isAdmin = false; let userProjectIds: number[]; + try { + // admin check, if passed then pre-set authz await hasPermission('project', 'viewAll'); isAdmin = true; } catch (err) { @@ -146,11 +149,7 @@ export const getProjectsByFactSearch: ResolverFn = async ( const count = await getFactFilteredProjectsCount(input, userProjectIds, sqlClientPool, isAdmin); const rows = await getFactFilteredProjects(input, userProjectIds, sqlClientPool, isAdmin); - // Just like the getAllProjects resolver, we can pass a 'environmentAuthz' prop to bypass extra - // keycloak checks at the environments level. - const rowsWithEnvironmentAuthz = rows && rows.map(row => ({ ...row, environmentAuthz: true })); - - return { projects: rowsWithEnvironmentAuthz, count }; + return { projects: rows, count }; } export const getEnvironmentsByFactSearch: ResolverFn = async ( @@ -179,11 +178,7 @@ export const getEnvironmentsByFactSearch: ResolverFn = async ( const count = await getFactFilteredEnvironmentsCount(input, userProjectIds, sqlClientPool, isAdmin); const rows = await getFactFilteredEnvironments(input, userProjectIds, sqlClientPool, isAdmin); - // Just like the getAllProjects resolver, we can pass a 'environmentAuthz' prop to bypass extra - // keycloak checks at the environments level. - const rowsWithEnvironmentAuthz = rows && rows.map(row => ({ ...row, environmentAuthz: true })); - - return { environments: rowsWithEnvironmentAuthz, count }; + return { environments: rows, count }; } export const processAddFacts = async (facts, sqlClientPool, hasPermission) => { diff --git a/services/api/src/resources/group/resolvers.ts b/services/api/src/resources/group/resolvers.ts index 6ca99d6a6c..a2ed181657 100644 --- a/services/api/src/resources/group/resolvers.ts +++ b/services/api/src/resources/group/resolvers.ts @@ -488,16 +488,6 @@ export const getAllProjectsInGroup: ResolverFn = async ( GroupModel: { loadGroupByIdOrName, getProjectsFromGroupAndSubgroups } } = models; - // Since we've - // already authorized the user has access to the group, and thus the associated projects we are - // returning, AND all user roles are allowed to view all environments of a project, we can - // short-circuit the slow keycloak check in the getEnvironmentsByProjectId - // resolver. - // - // @TODO: When this performance issue is fixed for real, remove this hack as - // it hardcodes a "everyone can view environments" authz rule. - let environmentAuthz = true - // use the admin scope check instead of `hasPermission` for speed if (adminScopes.groupViewAll) { try { @@ -505,7 +495,7 @@ export const getAllProjectsInGroup: ResolverFn = async ( const group = await getGroupFromGroups(groupInput.id, keycloakGroups) const projectIdsArray = await getProjectsFromGroupAndSubgroups(group); return projectIdsArray.map(async id => - projectHelpers(sqlClientPool).getProjectByProjectInput({ id }, environmentAuthz) + projectHelpers(sqlClientPool).getProjectByProjectInput({ id }) ); } catch (err) { if (err instanceof GroupNotFoundError) { @@ -536,7 +526,7 @@ export const getAllProjectsInGroup: ResolverFn = async ( const projectIdsArray = await getProjectsFromGroupAndSubgroups(group); return projectIdsArray.map(async id => - projectHelpers(sqlClientPool).getProjectByProjectInput({ id }, environmentAuthz) + projectHelpers(sqlClientPool).getProjectByProjectInput({ id }) ); } }; diff --git a/services/api/src/resources/insight/resolvers.ts b/services/api/src/resources/insight/resolvers.ts index 7dbef50156..a7d4c2463d 100644 --- a/services/api/src/resources/insight/resolvers.ts +++ b/services/api/src/resources/insight/resolvers.ts @@ -119,9 +119,9 @@ export const getInsightsFileData: ResolverFn = async ( }; export const getInsightsFilesByEnvironmentId: ResolverFn = async ( - { id: eid, environmentAuthz }, + { id: eid }, { name, limit }, - { sqlClientPool, hasPermission } + { sqlClientPool, hasPermission, adminScopes } ) => { if (!eid) { @@ -132,7 +132,7 @@ export const getInsightsFilesByEnvironmentId: ResolverFn = async ( sqlClientPool ).getEnvironmentById(parseInt(eid)); - if (!environmentAuthz) { + if (!adminScopes.projectViewAll) { await hasPermission('environment', 'view', { project: environmentData.project }); diff --git a/services/api/src/resources/openshift/resolvers.ts b/services/api/src/resources/openshift/resolvers.ts index 1ae1edeb48..c746a11994 100644 --- a/services/api/src/resources/openshift/resolvers.ts +++ b/services/api/src/resources/openshift/resolvers.ts @@ -96,11 +96,14 @@ export const getAllOpenshifts: ResolverFn = async ( export const getOpenshiftByProjectId: ResolverFn = async ( { id: pid }, args, - { sqlClientPool, hasPermission } + { sqlClientPool, hasPermission, adminScopes } ) => { - await hasPermission('openshift', 'view', { - project: pid - }); + + if (!adminScopes.openshiftViewAll) { + await hasPermission('openshift', 'view', { + project: pid + }); + } const rows = await query( sqlClientPool, @@ -157,7 +160,7 @@ export const getOpenshiftByDeployTargetId: ResolverFn = async ( export const getOpenshiftByEnvironmentId: ResolverFn = async ( { id: eid }, args, - { sqlClientPool, hasPermission } + { sqlClientPool, hasPermission, adminScopes } ) => { // get the project id for the environment const project = await projectHelpers( @@ -165,9 +168,11 @@ export const getOpenshiftByEnvironmentId: ResolverFn = async ( ).getProjectByEnvironmentId(eid); // check permissions on the project - await hasPermission('openshift', 'view', { - project: project.project - }); + if (!adminScopes.openshiftViewAll) { + await hasPermission('openshift', 'view', { + project: project.project + }); + } const rows = await query( sqlClientPool, diff --git a/services/api/src/resources/problem/resolvers.ts b/services/api/src/resources/problem/resolvers.ts index 8c64c970e8..779e1fd131 100644 --- a/services/api/src/resources/problem/resolvers.ts +++ b/services/api/src/resources/problem/resolvers.ts @@ -98,15 +98,17 @@ export const getProblemSources: ResolverFn = async ( export const getProblemsByEnvironmentId: ResolverFn = async ( { id: environmentId }, { severity, source }, - { sqlClientPool, hasPermission } + { sqlClientPool, hasPermission, adminScopes } ) => { const environment = await environmentHelpers( sqlClientPool ).getEnvironmentById(environmentId); - await hasPermission('problem', 'view', { - project: environment.project - }); + if (!adminScopes.projectViewAll) { + await hasPermission('problem', 'view', { + project: environment.project + }); + } const rows = await query( sqlClientPool, diff --git a/services/api/src/resources/project/helpers.ts b/services/api/src/resources/project/helpers.ts index a5f011b07f..a47bac3ed2 100644 --- a/services/api/src/resources/project/helpers.ts +++ b/services/api/src/resources/project/helpers.ts @@ -67,7 +67,7 @@ export const Helpers = (sqlClientPool: Pool) => { return parseInt(pid, 10); }, - getProjectByProjectInput: async (projectInput, environmentAuthz?) => { + getProjectByProjectInput: async (projectInput) => { const notEmpty = R.complement(R.anyPass([R.isNil, R.isEmpty])); const hasId = R.both(R.has('id'), R.propSatisfies(notEmpty, 'id')); const hasName = R.both(R.has('name'), R.propSatisfies(notEmpty, 'name')); @@ -76,11 +76,6 @@ export const Helpers = (sqlClientPool: Pool) => { if (!project) { throw new Error('Unauthorized'); } - if (environmentAuthz) { - // @TODO: When this performance issue is fixed for real, remove this hack as - // it hardcodes a "everyone can view environments" authz rule. - project.environmentAuthz = true; - } return project; }); @@ -92,11 +87,6 @@ export const Helpers = (sqlClientPool: Pool) => { if (!project) { throw new Error('Unauthorized'); } - if (environmentAuthz) { - // @TODO: When this performance issue is fixed for real, remove this hack as - // it hardcodes a "everyone can view environments" authz rule. - project.environmentAuthz = true; - } return project; }); diff --git a/services/api/src/resources/project/resolvers.ts b/services/api/src/resources/project/resolvers.ts index 79cfd751e8..d10a908957 100644 --- a/services/api/src/resources/project/resolvers.ts +++ b/services/api/src/resources/project/resolvers.ts @@ -17,16 +17,6 @@ import sql from '../user/sql'; const DISABLE_CORE_HARBOR = process.env.DISABLE_CORE_HARBOR || "false" -const isAdminCheck = async (hasPermission) => { - try { - // check user is admin - await hasPermission('project', 'viewAll'); - return true; - } catch (err) { - return false; - } -}; - const isValidGitUrl = value => /(?:git|ssh|https?|git@[-\w.]+):(\/\/)?(.*?)(\.git)(\/?|\#[-\d\w._]+?)$/.test( value @@ -51,18 +41,20 @@ export const getPrivateKey: ResolverFn = async ( export const getAllProjects: ResolverFn = async ( root, { order, createdAfter, gitUrl }, - { sqlClientPool, hasPermission, models, keycloakGrant, keycloakGroups, keycloakUsersGroups } + { sqlClientPool, hasPermission, models, keycloakGrant, keycloakUsersGroups } ) => { let userProjectIds: number[]; try { + // admin check, if passed then pre-set authz await hasPermission('project', 'viewAll'); } catch (err) { + // else user if (!keycloakGrant) { logger.debug('No grant available for getAllProjects'); return []; } - + // get the project ids from the users groups const userProjectRoles = await models.UserModel.getAllProjectsIdsForUser({ id: keycloakGrant.access_token.content.sub, @@ -91,15 +83,7 @@ export const getAllProjects: ResolverFn = async ( const rows = await query(sqlClientPool, queryBuilder.toString()); const withK8s = Helpers(sqlClientPool).aliasOpenshiftToK8s(rows); - // This resolver is used for the main UI page and is quite slow. Since we've - // already authorized the user has access to all the projects we are - // returning, AND all user roles are allowed to view all environments, we can - // short-circuit the slow keycloak check in the getEnvironmentsByProjectId - // resolver. - // - // @TODO: When this performance issue is fixed for real, remove this hack as - // it hardcodes a "everyone can view environments" authz rule. - return withK8s.map(row => ({ ...row, environmentAuthz: true })); + return withK8s; }; export const getProjectByEnvironmentId: ResolverFn = async ( @@ -204,10 +188,13 @@ export const getProjectByName: ResolverFn = async ( export const getProjectsByMetadata: ResolverFn = async ( root, { metadata }, - { sqlClientPool, hasPermission, keycloakGrant, models, keycloakUsersGroups } + { sqlClientPool, hasPermission, keycloakGrant, models, keycloakUsersGroups }, + info ) => { let userProjectIds: number[]; + try { + // admin check, if passed then pre-set authz await hasPermission('project', 'viewAll'); } catch (err) { if (!keycloakGrant) { @@ -243,13 +230,15 @@ export const getProjectsByMetadata: ResolverFn = async ( } const rows = await query(sqlClientPool, queryBuilder.toString(), queryArgs); - return Helpers(sqlClientPool).aliasOpenshiftToK8s(rows); + const withK8s = Helpers(sqlClientPool).aliasOpenshiftToK8s(rows); + + return withK8s; }; export const addProject = async ( root, { input }, - { hasPermission, sqlClientPool, models, keycloakGrant, userActivityLogger } + { hasPermission, sqlClientPool, models, keycloakGrant, userActivityLogger, adminScopes } ) => { await hasPermission('project', 'add'); @@ -294,8 +283,7 @@ export const addProject = async ( // check if a user has permission to disable deployments of a project or not let deploymentsDisabled = 0; if (input.deploymentsDisabled) { - const canDisableProject = await isAdminCheck(hasPermission); - if (canDisableProject) { + if (adminScopes.projectViewAll) { deploymentsDisabled = input.deploymentsDisabled } } @@ -400,12 +388,9 @@ export const addProject = async ( } // Add the user who submitted this request to the project - let userAlreadyHasAccess; - try { - await hasPermission('project', 'viewAll'); - userAlreadyHasAccess = true; - } catch (e) { - userAlreadyHasAccess = false; + let userAlreadyHasAccess = false; + if (adminScopes.projectViewAll) { + userAlreadyHasAccess = true } if (!userAlreadyHasAccess && keycloakGrant) { @@ -549,7 +534,7 @@ export const updateProject: ResolverFn = async ( } } }, - { sqlClientPool, hasPermission, userActivityLogger, models } + { sqlClientPool, hasPermission, userActivityLogger, models, adminScopes } ) => { await hasPermission('project', 'update', { project: id @@ -557,8 +542,7 @@ export const updateProject: ResolverFn = async ( // check if a user has permission to disable deployments of a project or not if (deploymentsDisabled) { - const canDisableProject = await isAdminCheck(hasPermission); - if (canDisableProject == false) { + if (!adminScopes.projectViewAll) { deploymentsDisabled = 0; } } @@ -579,8 +563,7 @@ export const updateProject: ResolverFn = async ( // renaming projects is prohibited because lagoon uses the project name for quite a few things // which if changed can have unintended consequences for any existing environments if (patch.name) { - const canUpdateName = await isAdminCheck(hasPermission); - if (!canUpdateName) { + if (!adminScopes.projectViewAll) { throw new Error('Project renaming is only available to administrators.'); } } diff --git a/services/api/src/resources/task/resolvers.ts b/services/api/src/resources/task/resolvers.ts index 4686748b37..86b787d6af 100644 --- a/services/api/src/resources/task/resolvers.ts +++ b/services/api/src/resources/task/resolvers.ts @@ -99,14 +99,17 @@ export const getTaskLog: ResolverFn = async ( export const getTasksByEnvironmentId: ResolverFn = async ( { id: eid }, { id: filterId, taskName: taskName, limit }, - { sqlClientPool, hasPermission } + { sqlClientPool, hasPermission, adminScopes } ) => { const environment = await environmentHelpers( sqlClientPool ).getEnvironmentById(eid); - await hasPermission('task', 'view', { - project: environment.project - }); + + if (!adminScopes.projectViewAll) { + await hasPermission('task', 'view', { + project: environment.project + }); + } let queryBuilder = knex('task') .where('environment', eid) From 5c1524271edc365eaeb09f9233864c9e8b670f7f Mon Sep 17 00:00:00 2001 From: shreddedbacon Date: Mon, 6 Mar 2023 11:17:57 +1100 Subject: [PATCH 11/11] chore: more redis cleanup --- services/api/src/models/group.ts | 8 +------- services/api/src/models/user.ts | 2 +- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/services/api/src/models/group.ts b/services/api/src/models/group.ts index d0eaae07ea..018c83b9c5 100644 --- a/services/api/src/models/group.ts +++ b/services/api/src/models/group.ts @@ -95,7 +95,7 @@ export const Group = (clients: { sqlClientPool: Pool; esClient: any; }) => { - const { keycloakAdminClient, redisClient } = clients; + const { keycloakAdminClient } = clients; const transformKeycloakGroups = async ( keycloakGroups: GroupRepresentation[] @@ -481,12 +481,6 @@ export const Group = (clients: { }; const deleteGroup = async (id: string): Promise => { - /* REDIS - // const group = loadGroupById(id); - // @ts-ignore - // const projectIds = getProjectIdsFromGroup(group); - REDIS */ - try { await keycloakAdminClient.groups.del({ id }); } catch (err) { diff --git a/services/api/src/models/user.ts b/services/api/src/models/user.ts index c92212ecb8..276b78990b 100644 --- a/services/api/src/models/user.ts +++ b/services/api/src/models/user.ts @@ -71,7 +71,7 @@ export const User = (clients: { sqlClientPool: any; esClient: any; }): UserModel => { - const { keycloakAdminClient, redisClient } = clients; + const { keycloakAdminClient } = clients; const fetchGitlabId = async (user: User): Promise => { const identities = await keycloakAdminClient.users.listFederatedIdentities({