Skip to content

refactor(credentials): drop the principal abstraction, keep the identity fixes #6207

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.

Sign up for GitHub

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

Closed
waleedlatif1 wants to merge 1 commit into from
Closed

refactor(credentials): drop the principal abstraction, keep the identity fixes #6207

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 8 additions & 30 deletions apps/sim/lib/credentials/client-credential-accounts/minters/box.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ describe('mintBoxServiceAccountToken', () => {
.mockResolvedValueOnce(jsonResponse(200, { access_token: 'box-access', expires_in: 3600 }))
.mockResolvedValueOnce(
jsonResponse(200, {
id: '33445566',
name: 'Sim Automation',
login: 'AutomationUser_123_abc@boxdevedition.com',
})
Expand All @@ -80,21 +79,22 @@ describe('mintBoxServiceAccountToken', () => {
expiresInSeconds: 3600,
identity: {
displayName: 'Sim Automation',
principal: {
kind: 'user',
id: '33445566',
label: 'AutomationUser_123_abc@boxdevedition.com',
auditMetadata: {
boxEnterpriseId: '1234567',
boxServiceAccountLogin: 'AutomationUser_123_abc@boxdevedition.com',
},
storedMetadata: {
enterpriseId: '1234567',
serviceAccountLogin: 'AutomationUser_123_abc@boxdevedition.com',
},
auditMetadata: { boxEnterpriseId: '1234567' },
storedMetadata: { enterpriseId: '1234567' },
},
})
expect(mockFetch).toHaveBeenCalledTimes(2)
expectMintCall()
expectIdentityCall()
})

it('marks the principal as lookup_failed when users/me fails', async () => {
it('still succeeds with a fallback identity when users/me fails', async () => {
mockFetch
.mockResolvedValueOnce(jsonResponse(200, { access_token: 'box-access', expires_in: 2400 }))
.mockResolvedValueOnce(jsonResponse(500, { message: 'boom' }))
Expand All @@ -105,26 +105,8 @@ describe('mintBoxServiceAccountToken', () => {
expect(result.expiresInSeconds).toBe(2400)
expect(result.identity).toEqual({
displayName: 'Box enterprise 1234567',
principal: { kind: 'lookup_failed', reason: 'HTTP 500' },
auditMetadata: { boxEnterpriseId: '1234567' },
storedMetadata: { enterpriseId: '1234567' },
})
})

it('marks the principal as lookup_failed when users/me omits the user id', async () => {
mockFetch
.mockResolvedValueOnce(jsonResponse(200, { access_token: 'box-access', expires_in: 3600 }))
.mockResolvedValueOnce(jsonResponse(200, { name: 'Sim Automation' }))

const result = await mintBoxServiceAccountToken(FIELDS)

expect(result.identity?.principal).toEqual({
kind: 'lookup_failed',
reason: 'response missing user id',
})
// Only the principal degrades — a name that did come back still beats the
// Enterprise-ID fallback, so the credential does not lose its label.
expect(result.identity?.displayName).toBe('Sim Automation')
})

it('still succeeds when the identity request itself throws', async () => {
Expand All @@ -136,10 +118,6 @@ describe('mintBoxServiceAccountToken', () => {

expect(result.accessToken).toBe('box-access')
expect(result.identity?.displayName).toBe('Box enterprise 1234567')
expect(result.identity?.principal).toEqual({
kind: 'lookup_failed',
reason: 'provider_unavailable (HTTP 502)',
})
})

it('throws invalid_credentials on 400 invalid_client', async () => {
Expand Down
65 changes: 20 additions & 45 deletions apps/sim/lib/credentials/client-credential-accounts/minters/box.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,12 @@ import {
fetchProvider,
isTransientProviderStatus,
parseProviderJson,
providerFailureReason,
readProviderErrorSnippet,
TokenServiceAccountValidationError,
} from '@/lib/credentials/token-service-accounts/errors'

const logger = createLogger('BoxServiceAccountMinter')

const IDENTITY_STEP = 'box_identity'

const BOX_TOKEN_URL = 'https://api.box.com/oauth2/token'
const BOX_CURRENT_USER_URL = 'https://api.box.com/2.0/users/me'

Expand All @@ -27,14 +24,7 @@ interface BoxTokenResponse {
expires_in?: number
}

/**
* `id`, `name`, and `login` are all in the standard field set `GET /2.0/users/me`
* returns without a `fields` parameter, so capturing the Service Account's user
* id costs no extra request.
* @see https://developer.box.com/reference/get-users-me/
*/
interface BoxCurrentUserResponse {
id?: string
name?: string
login?: string
}
Expand Down Expand Up @@ -67,67 +57,52 @@ function boxErrorHint(body: string): string | undefined {

/**
* Best-effort identity lookup for the app's Service Account user. A failure
* never fails the mint — the credential degrades to an Enterprise-ID-derived
* display name with a `lookup_failed` principal, so the audit record shows the
* identity was not captured rather than implying none exists.
* never fails the mint — the caller falls back to an Enterprise-ID-derived
* display name.
*/
async function fetchBoxServiceAccountIdentity(
accessToken: string,
orgId: string
): Promise<ClientCredentialAccountIdentity> {
/**
* `label` keeps whatever human name the lookup did return. A response can
* carry `name`/`login` but no `id` — the principal is then unusable, but the
* label still beats the Enterprise-ID fallback, so only the principal
* degrades and the credential does not silently lose its name.
*/
const degraded = (reason: string, label?: string): ClientCredentialAccountIdentity => ({
displayName: label ?? `Box enterprise ${orgId}`,
principal: { kind: 'lookup_failed', reason },
const fallback: ClientCredentialAccountIdentity = {
displayName: `Box enterprise ${orgId}`,
auditMetadata: { boxEnterpriseId: orgId },
storedMetadata: { enterpriseId: orgId },
})
}
try {
const res = await fetchProvider(
BOX_CURRENT_USER_URL,
{ headers: { Authorization: `Bearer ${accessToken}`, Accept: 'application/json' } },
IDENTITY_STEP
'box_identity'
)
if (!res.ok) {
logger.warn('Box service-account identity lookup failed', {
step: IDENTITY_STEP,
step: 'box_identity',
status: res.status,
enterpriseId: orgId,
})
return degraded(`HTTP ${res.status}`)
return fallback
}
const user = await parseProviderJson<BoxCurrentUserResponse>(res, IDENTITY_STEP)
const id = typeof user.id === 'string' && user.id ? user.id : undefined
const user = await parseProviderJson<BoxCurrentUserResponse>(res, 'box_identity')
const login = typeof user.login === 'string' && user.login ? user.login : undefined
const name = typeof user.name === 'string' && user.name ? user.name : undefined
if (!id) {
logger.warn('Box service-account identity response carried no user id', {
step: IDENTITY_STEP,
status: res.status,
enterpriseId: orgId,
})
return degraded('response missing user id', name ?? login)
}
return {
displayName: name ?? login ?? `Box enterprise ${orgId}`,
// The Service Account is a real Box user; `enterpriseId` is shared by
// every app in the enterprise and so is kept as separate context.
principal: { kind: 'user', id, ...(login ? { label: login } : {}) },
auditMetadata: { boxEnterpriseId: orgId },
storedMetadata: { enterpriseId: orgId },
displayName: name ?? login ?? fallback.displayName,
auditMetadata: {
boxEnterpriseId: orgId,
...(login ? { boxServiceAccountLogin: login } : {}),
},
storedMetadata: {
enterpriseId: orgId,
...(login ? { serviceAccountLogin: login } : {}),
},
}
} catch (error) {
logger.warn('Box service-account identity lookup threw', {
step: IDENTITY_STEP,
step: 'box_identity',
enterpriseId: orgId,
error: getErrorMessage(error),
})
return degraded(providerFailureReason(error))
return fallback
}
}

Expand Down
29 changes: 3 additions & 26 deletions apps/sim/lib/credentials/client-credential-accounts/minters/salesforce.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ describe('mintSalesforceServiceAccountToken', () => {
name: 'Integration User',
preferred_username: 'integration@yourorg.com',
organization_id: '00Dxx0000000001EAA',
user_id: '005xx000001Sv6DAAS',
})
)

Expand All @@ -91,19 +90,16 @@ describe('mintSalesforceServiceAccountToken', () => {
grantedScopes: ['api'],
identity: {
displayName: 'Integration User',
principal: {
kind: 'user',
id: '005xx000001Sv6DAAS',
label: 'integration@yourorg.com',
},
auditMetadata: {
salesforceMyDomainHost: HOST,
salesforceOrgId: '00Dxx0000000001EAA',
salesforceRunAsUsername: 'integration@yourorg.com',
},
storedMetadata: {
myDomainHost: HOST,
instanceUrl: INSTANCE_URL,
orgId: '00Dxx0000000001EAA',
runAsUsername: 'integration@yourorg.com',
grantedScopes: 'api',
},
},
Expand Down Expand Up @@ -263,7 +259,7 @@ describe('mintSalesforceServiceAccountToken', () => {
})
})

it('marks the principal as lookup_failed when the userinfo call throws', async () => {
it('falls back to a host-derived identity when the userinfo call fails', async () => {
mockFetch
.mockResolvedValueOnce(
jsonResponse(200, { access_token: 'sf-access', instance_url: INSTANCE_URL })
Expand All @@ -275,30 +271,11 @@ describe('mintSalesforceServiceAccountToken', () => {
expect(result.accessToken).toBe('sf-access')
expect(result.identity).toEqual({
displayName: `Salesforce ${HOST}`,
principal: { kind: 'lookup_failed', reason: 'provider_unavailable (HTTP 502)' },
auditMetadata: { salesforceMyDomainHost: HOST },
storedMetadata: { myDomainHost: HOST, instanceUrl: INSTANCE_URL },
})
})

it('marks the principal as lookup_failed when userinfo omits user_id', async () => {
mockFetch
.mockResolvedValueOnce(
jsonResponse(200, { access_token: 'sf-access', instance_url: INSTANCE_URL })
)
.mockResolvedValueOnce(jsonResponse(200, { name: 'Integration User' }))

const result = await mintSalesforceServiceAccountToken(FIELDS)

expect(result.identity?.principal).toEqual({
kind: 'lookup_failed',
reason: 'response missing user_id',
})
// Only the principal degrades — a name that did come back still beats the
// host fallback, so the credential does not lose its label.
expect(result.identity?.displayName).toBe('Integration User')
})

it('ignores a non-Salesforce instance_url and falls back to the validated host', async () => {
mockFetch
.mockResolvedValueOnce(
Expand Down
Loading
Loading