Repository navigation
feat: team-owned API keys #3435
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
jordan-simonovski
wants to merge
7
commits into
main
Choose a base branch
from
jordansimonovski/org-api-keys
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
826626b
feat: team-owned API keys managed by admins
jordan-simonovski 851818b
Merge branch 'main' into jordansimonovski/org-api-keys
jordan-simonovski 256d5e9
fix: address review findings on org API keys
jordan-simonovski c28468e
refactor: org API keys authenticate as service-account users
jordan-simonovski fec5151
fix: make last-member protection safer for org API keys
jordan-simonovski acf2767
refactor: drop key scopes from org API keys
jordan-simonovski 12fa9fd
Merge branch 'main' into jordansimonovski/org-api-keys
jordan-simonovski File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| '@hyperdx/api': minor | ||
| '@hyperdx/app': minor | ||
| '@hyperdx/common-utils': minor | ||
| --- | ||
|
|
||
| feat: Team members can create team-owned API keys with a description and expiry that keep working after their creator leaves. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
237 changes: 237 additions & 0 deletions
237
packages/api/src/controllers/__tests__/orgApiKey.int.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,237 @@ | ||
| import mongoose from 'mongoose'; | ||
|
|
||
| import { | ||
| createOrgApiKey, | ||
| deleteOrgApiKey, | ||
| findActiveOrgApiKey, | ||
| hashOrgApiKey, | ||
| listOrgApiKeys, | ||
| touchOrgApiKeyLastUsed, | ||
| updateOrgApiKeyDescription, | ||
| } from '@/controllers/orgApiKey'; | ||
| import { createTeam } from '@/controllers/team'; | ||
| import { | ||
| deleteTeamMemberKeepingOne, | ||
| findUserByAccessKey, | ||
| findUsersByTeam, | ||
| } from '@/controllers/user'; | ||
| import { clearDBCollections, closeDB, connectDB } from '@/fixtures'; | ||
| import OrgApiKey from '@/models/orgApiKey'; | ||
| import User from '@/models/user'; | ||
|
|
||
| const DAY_MS = 24 * 60 * 60 * 1000; | ||
|
|
||
| describe('orgApiKey controller', () => { | ||
| const creator = new mongoose.Types.ObjectId(); | ||
|
|
||
| beforeAll(async () => { | ||
| await connectDB(); | ||
| }); | ||
|
|
||
| afterEach(async () => { | ||
| await clearDBCollections(); | ||
| }); | ||
|
|
||
| afterAll(async () => { | ||
| await closeDB(); | ||
| }); | ||
|
|
||
| const create = async ( | ||
| teamId: mongoose.Types.ObjectId, | ||
| expiry: '30d' | '1y' | 'never' = '30d', | ||
| ) => | ||
| createOrgApiKey(teamId, creator, { | ||
| description: 'terraform', | ||
| expiry, | ||
| }); | ||
|
|
||
| it('returns a prefixed key once and stores only its hash', async () => { | ||
| const team = await createTeam({ name: 'A' }); | ||
| const { doc, key } = await create(team._id); | ||
|
|
||
| expect(key).toMatch(/^hdx_org_[A-Za-z0-9_-]{43}$/); | ||
| const stored = await OrgApiKey.findById(doc._id).lean(); | ||
| expect(stored?.keyHash).toBe(hashOrgApiKey(key)); | ||
| expect(Object.values(stored ?? {})).not.toContain(key); | ||
| }); | ||
|
|
||
| it.each([ | ||
| ['30d', 30 * DAY_MS], | ||
| ['1y', 365 * DAY_MS], | ||
| ] as const)('sets expiresAt for %s', async (expiry, offset) => { | ||
| const team = await createTeam({ name: 'A' }); | ||
| const before = Date.now(); | ||
| const { doc } = await create(team._id, expiry); | ||
| const expiresAt = doc.expiresAt?.getTime() ?? 0; | ||
| expect(Math.abs(expiresAt - (before + offset))).toBeLessThan(1000); | ||
| }); | ||
|
|
||
| it('stores null expiresAt for never', async () => { | ||
| const team = await createTeam({ name: 'A' }); | ||
| const { doc } = await create(team._id, 'never'); | ||
| expect(doc.expiresAt).toBeNull(); | ||
| }); | ||
|
|
||
| it('finds active keys and rejects expired, unknown, and non-org keys', async () => { | ||
| const team = await createTeam({ name: 'A' }); | ||
| const { doc, key } = await create(team._id); | ||
|
|
||
| expect((await findActiveOrgApiKey(key))?.orgApiKey._id).toEqual(doc._id); | ||
| expect( | ||
| await findActiveOrgApiKey(key, doc.expiresAt ?? new Date(0)), | ||
| ).toBeNull(); | ||
| expect(await findActiveOrgApiKey(`hdx_org_${'x'.repeat(43)}`)).toBeNull(); | ||
| expect( | ||
| await findActiveOrgApiKey('4b9e3c1a-7f2d-4e8b-9a6c-1d2e3f4a5b6c'), | ||
| ).toBeNull(); | ||
| }); | ||
|
|
||
| it('scopes list and delete to the team', async () => { | ||
| const teamA = await createTeam({ name: 'A' }); | ||
| // OSS allows a single team, so team B only needs to be a distinct id. | ||
| const teamB = new mongoose.Types.ObjectId(); | ||
| const { doc } = await create(teamA._id); | ||
|
|
||
| expect(await listOrgApiKeys(teamB)).toHaveLength(0); | ||
| expect(await deleteOrgApiKey(teamB, doc._id.toString())).toBeNull(); | ||
| expect(await OrgApiKey.findById(doc._id)).not.toBeNull(); | ||
| }); | ||
|
|
||
| it('writes lastUsedAt at most once per 5 minutes', async () => { | ||
| const team = await createTeam({ name: 'A' }); | ||
| const { doc } = await create(team._id); | ||
| const t0 = new Date(); | ||
|
|
||
| const lastUsed = async () => { | ||
| // Writes are fire-and-forget; give them a moment to land. | ||
| await new Promise(r => setTimeout(r, 100)); | ||
| return (await OrgApiKey.findById(doc._id))?.lastUsedAt?.getTime(); | ||
| }; | ||
|
|
||
| touchOrgApiKeyLastUsed(doc, t0); | ||
| expect(await lastUsed()).toBe(t0.getTime()); | ||
|
|
||
| const fresh = (await OrgApiKey.findById(doc._id))!; | ||
| touchOrgApiKeyLastUsed(fresh, new Date(t0.getTime() + 60_000)); | ||
| expect(await lastUsed()).toBe(t0.getTime()); | ||
|
|
||
| const t6 = new Date(t0.getTime() + 6 * 60_000); | ||
| touchOrgApiKeyLastUsed(fresh, t6); | ||
| expect(await lastUsed()).toBe(t6.getTime()); | ||
| }); | ||
|
|
||
| describe('service accounts', () => { | ||
| it('creates a service account user for each key', async () => { | ||
| const team = await createTeam({ name: 'A' }); | ||
| const { doc } = await create(team._id); | ||
| const account = await User.findById(doc.serviceAccount); | ||
|
|
||
| expect(account).toMatchObject({ | ||
| isServiceAccount: true, | ||
| name: 'terraform', | ||
| }); | ||
| expect(account?.team).toEqual(team._id); | ||
| expect(account?.email).toMatch( | ||
| /^sa-[0-9a-f]{24}@service-accounts\.hyperdx\.invalid$/, | ||
| ); | ||
| }); | ||
|
|
||
| it('resolves the key to its service account', async () => { | ||
| const team = await createTeam({ name: 'A' }); | ||
| const { doc, key } = await create(team._id); | ||
| const found = await findActiveOrgApiKey(key); | ||
| expect(found?.serviceAccount._id).toEqual(doc.serviceAccount); | ||
| }); | ||
|
|
||
| it('keeps service accounts out of members and personal-key auth', async () => { | ||
| const team = await createTeam({ name: 'A' }); | ||
| const { doc } = await create(team._id); | ||
| const account = await User.findById(doc.serviceAccount); | ||
|
|
||
| expect(await findUsersByTeam(team._id)).toHaveLength(0); | ||
| expect(await findUserByAccessKey(account!.accessKey)).toBeNull(); | ||
| }); | ||
|
|
||
| it('renames the service account with the key', async () => { | ||
| const team = await createTeam({ name: 'A' }); | ||
| const { doc } = await create(team._id); | ||
| await updateOrgApiKeyDescription(team._id, doc._id.toString(), 'ci'); | ||
| expect((await User.findById(doc.serviceAccount))?.name).toBe('ci'); | ||
| }); | ||
|
|
||
| it('keeps the service account when the key is revoked', async () => { | ||
| const team = await createTeam({ name: 'A' }); | ||
| const { doc, key } = await create(team._id); | ||
| await deleteOrgApiKey(team._id, doc._id.toString()); | ||
| expect(await findActiveOrgApiKey(key)).toBeNull(); | ||
| expect(await User.findById(doc.serviceAccount)).not.toBeNull(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('member removal by a service account', () => { | ||
| const setupTeam = async (people: number) => { | ||
| const team = await createTeam({ name: 'A' }); | ||
| const { doc } = await create(team._id); | ||
| const users = await Promise.all( | ||
| Array.from({ length: people }, (_, i) => | ||
| User.create({ email: `p${i}@example.com`, team: team._id }), | ||
| ), | ||
| ); | ||
| return { team, serviceAccount: doc.serviceAccount, users }; | ||
| }; | ||
|
|
||
| afterEach(() => jest.restoreAllMocks()); | ||
|
|
||
| it('refuses the last person without deleting them', async () => { | ||
| const { team, serviceAccount, users } = await setupTeam(1); | ||
| const deleteSpy = jest.spyOn(User, 'findOneAndDelete'); | ||
|
|
||
| const result = await deleteTeamMemberKeepingOne( | ||
| team._id, | ||
| users[0]._id.toString(), | ||
| serviceAccount, | ||
| ); | ||
| expect(result).toBe('last_member'); | ||
| expect(deleteSpy).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('retries putting a member back when the first insert fails', async () => { | ||
| const { team, serviceAccount, users } = await setupTeam(2); | ||
| // Simulate a concurrent removal: two people before the delete, none after. | ||
| // Widened to SpyInstance: the Mongoose overloads type these mocks as never. | ||
| const count: jest.SpyInstance = jest.spyOn(User, 'countDocuments'); | ||
| count.mockResolvedValueOnce(2).mockResolvedValueOnce(0); | ||
| const insert: jest.SpyInstance = jest.spyOn(User.collection, 'insertOne'); | ||
| insert.mockRejectedValueOnce(new Error('transient')); | ||
|
|
||
| const result = await deleteTeamMemberKeepingOne( | ||
| team._id, | ||
| users[0]._id.toString(), | ||
| serviceAccount, | ||
| ); | ||
| expect(result).toBe('last_member'); | ||
| expect(await User.findById(users[0]._id)).not.toBeNull(); | ||
| }); | ||
|
|
||
| it('treats a duplicate _id on restore as already restored', async () => { | ||
| const { team, serviceAccount, users } = await setupTeam(2); | ||
| const count: jest.SpyInstance = jest.spyOn(User, 'countDocuments'); | ||
| count.mockResolvedValueOnce(2).mockResolvedValueOnce(0); | ||
| const insert: jest.SpyInstance = jest.spyOn(User.collection, 'insertOne'); | ||
| insert.mockRejectedValueOnce( | ||
| Object.assign(new Error('E11000'), { | ||
| code: 11000, | ||
| keyPattern: { _id: 1 }, | ||
| }), | ||
| ); | ||
|
|
||
| const result = await deleteTeamMemberKeepingOne( | ||
| team._id, | ||
| users[0]._id.toString(), | ||
| serviceAccount, | ||
| ); | ||
| expect(result).toBe('last_member'); | ||
| expect(insert).toHaveBeenCalledTimes(1); | ||
| }); | ||
| }); | ||
| }); | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔵 minor — The lastUsedAt throttle test never exercises the DB-side conditional filter that is meant to guard concurrent writes
The second call passes
freshwithlastUsedAt = t0, sotouchOrgApiKeyLastUsedreturns early in JS and never callsupdateOne. The test would still pass if the$or: [{lastUsedAt: null}, {lastUsedAt: {$lt: staleBefore}}]filter were removed. Fix: add a case that passes a stale in-memory doc (e.g. the originaldocwithlastUsedAt: null) after a recent write, and assert that the stored value doesn't change.Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond