diff --git a/api/src/limits/service.ts b/api/src/limits/service.ts index 636fd6e0..87233f7d 100644 --- a/api/src/limits/service.ts +++ b/api/src/limits/service.ts @@ -17,9 +17,10 @@ export const getOrgLimits = async (org: Organization) => { return limit } -// NHIs are not members: findMembers excludes them by default, this query has to do the same +// NHIs count as members here: they hold an org membership like humans do and consume the same quota, +// even though findMembers hides them by default const getNbMembers = async (orgId: string) => { - return mongo.users.countDocuments({ 'organizations.id': orgId, plannedDeletion: { $exists: false }, nhi: { $exists: false } }) + return mongo.users.countDocuments({ 'organizations.id': orgId, plannedDeletion: { $exists: false } }) } export const setNbMembersLimit = async (orgId: string) => { diff --git a/api/src/nhis/router.ts b/api/src/nhis/router.ts index 83920cb3..e67f399d 100644 --- a/api/src/nhis/router.ts +++ b/api/src/nhis/router.ts @@ -4,7 +4,7 @@ import eventsLog from '@data-fair/lib-express/events-log.js' import config from '#config' import storages from '#storages' import { reqI18n } from '#i18n' -import { postUserIdentityWebhook, deleteIdentityWebhook } from '#services' +import { postUserIdentityWebhook, deleteIdentityWebhook, getOrgLimits, setNbMembersLimit } from '#services' import { isOrgAdmin } from '../organizations/service.ts' import { checkProvider } from './keys.ts' import { checkAllowedIps } from './ips.ts' @@ -43,9 +43,15 @@ router.post('', async (req: Request, res) => { if (!roles.includes(body.role)) throw httpError(400, 'unknown role') await checkProvider(body.provider) if (body.allowedIps) checkAllowedIps(body.allowedIps) + logContext.account = { type: 'organization', id: org.id, name: org.name } + const limits = await getOrgLimits(org) + if (limits.store_nb_members.limit > 0 && limits.store_nb_members.consumption >= limits.store_nb_members.limit) { + eventsLog.info('sd.nhi.limit', `limit error for NHI creation in org ${org.id}`, logContext) + throw httpError(429, reqI18n(req).messages.errors.maxNbMembers) + } const sessionUser = reqSessionAuthenticated(req).user const user = await createNhi(org, body, { id: sessionUser.id, name: sessionUser.name }) - logContext.account = { type: 'organization', id: org.id, name: org.name } + await setNbMembersLimit(org.id) eventsLog.info('sd.nhi.create', `an NHI was created ${user.id} in org ${org.id}`, logContext) postUserIdentityWebhook(user) res.status(201).send(user) @@ -111,6 +117,7 @@ router.delete('/:nhiId', async (req: Request, res) => { const user = await getNhi(req.params.organizationId, req.params.nhiId) logContext.account = { type: 'organization', id: req.params.organizationId, name: user.organizations[0]?.name } await storages.globalStorage.deleteUser(user.id) + await setNbMembersLimit(req.params.organizationId) eventsLog.info('sd.nhi.delete', `an NHI was deleted ${user.id}`, logContext) deleteIdentityWebhook('user', user.id) res.status(204).send() diff --git a/docs/architecture/non-human-identities.md b/docs/architecture/non-human-identities.md index 6db82128..988616fa 100644 --- a/docs/architecture/non-human-identities.md +++ b/docs/architecture/non-human-identities.md @@ -369,16 +369,19 @@ $exists: false }`). An NHI id passed as `:userId` 404s before the `assertNotNhiSession` guard is even relevant — the guard covers the case of an NHI *session* acting on someone else, the 404 gate covers an NHI as *target*. `GET /api/organizations/:id/members` uses the same default, so -existing consumers (quota counts, invitation UIs, other services) see no +existing consumers (invitation UIs, other services) see no behavior change unless they opt in with `?types=nhi` or `?types=user,nhi`. -**`findMembers` is not the only place that has to exclude NHIs.** +**NHIs do count in the members quota.** Unlike `findMembers`, `getNbMembers` (`api/src/limits/service.ts`), which feeds the -`store_nb_members` quota, counts the `users` collection directly instead of -going through `findMembers`, so it repeats the `nhi: { $exists: false }` -filter itself. Any new consumer that counts memberships with its own query -has to do the same. +`store_nb_members` quota, counts every membership in the `users` collection, +NHIs included: a service account holds an org membership and acts with its +role, so it consumes a member slot like a human does. NHI creation +(`POST /api/organizations/:organizationId/nhis`) is refused with a 429 when the +org is full, and both creation and deletion recompute the counter. The +consequence is that `store_nb_members.consumption` can exceed the `count` of +the default (human only) member listing. **Cleanup jobs exclude NHIs.** `storage.findInactiveUsers` / `findUsersToDelete` (`api/src/storages/mongo.ts`) filter NHIs out, so diff --git a/tests/features/nhis.api.spec.ts b/tests/features/nhis.api.spec.ts index 7ddee8bd..70319f9e 100644 --- a/tests/features/nhis.api.spec.ts +++ b/tests/features/nhis.api.spec.ts @@ -356,27 +356,29 @@ test('nhi provider is validated at create and patch time', async () => { await ax.patch(`/api/organizations/${org.id}/nhis/${nhi.id}`, { name: 'Renamed validated agent' }) }) -test('NHIs do not consume a member slot in the organization limits', async () => { - const { ax, user } = await createUser('nhi-limits@test.com') +test('NHIs consume a member slot in the organization limits', async () => { + const { ax } = await createUser('nhi-limits@test.com') const org = (await ax.post('/api/organizations', { name: 'NHI limits org' })).data ax.setOrg(org.id) + const { ax: adminAx } = await createUser('admin@test.com', true) + const getConsumption = async () => (await ax.get(`/api/limits/organization/${org.id}`)).data.store_nb_members.consumption // reading the limits is what creates the denormalized counter - assert.equal((await ax.get(`/api/limits/organization/${org.id}`)).data.store_nb_members.consumption, 1) - - await ax.post(`/api/organizations/${org.id}/nhis`, nhiBody()) + assert.equal(await getConsumption(), 1) - // deleting the last human recomputes the counter: the NHI must not hold the org at 1 member, - // consistently with the member listing that excludes NHIs by default - const { ax: adminAx } = await createUser('admin@test.com', true) - await adminAx.delete(`/api/users/${user.id}`) + const nhi = (await ax.post(`/api/organizations/${org.id}/nhis`, nhiBody())).data + assert.equal(await getConsumption(), 2) - const orgLimits = (await adminAx.get('/api/limits', { params: { type: 'organization', id: org.id } })).data.results[0] - assert.equal(orgLimits.store_nb_members.consumption, 0) + // a full org refuses another NHI, like it refuses another invitation + await adminAx.post(`/api/limits/organization/${org.id}`, { store_nb_members: { limit: 2, consumption: 2 }, lastUpdate: new Date().toISOString() }) + await assert.rejects(ax.post(`/api/organizations/${org.id}/nhis`, nhiBody({ name: 'Second agent' })), { status: 429 }) + assert.equal((await ax.get(`/api/organizations/${org.id}/nhis`)).data.count, 1) - // the creator is deleted above, so DELETE /api/test-env cannot scope this org by created.id - // any more -- drop it here, or it accumulates across runs and pollutes name-based org searches - await adminAx.delete(`/api/organizations/${org.id}`) + // deleting the NHI frees its slot + await ax.delete(`/api/organizations/${org.id}/nhis/${nhi.id}`) + assert.equal(await getConsumption(), 1) + await ax.post(`/api/organizations/${org.id}/nhis`, nhiBody({ name: 'Second agent' })) + assert.equal(await getConsumption(), 2) }) test('nhi token exchange is restricted to the declared allowedIps', async () => { diff --git a/ui/src/components/add-nhi-menu.vue b/ui/src/components/add-nhi-menu.vue index d220be81..d07a8150 100644 --- a/ui/src/components/add-nhi-menu.vue +++ b/ui/src/components/add-nhi-menu.vue @@ -22,7 +22,15 @@ {{ $t('pages.organization.addNhi') }} -