Skip to content

fix(core): explicitly persist tokens on OAuth2 re-auth #582

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

Merged
d-klotz merged 2 commits into from
May 5, 2026
Merged

fix(core): explicitly persist tokens on OAuth2 re-auth #582

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
db461cc
fix(core): explicitly persist tokens on OAuth2 re-auth
d-klotz May 5, 2026
388414b
fix(core): restore OAuth state forwarding lost during rebase
d-klotz May 5, 2026
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
33 changes: 26 additions & 7 deletions packages/core/integrations/integration-router.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,8 +507,7 @@ function setEntityRoutes(router, authenticateUser, useCases) {
const params = checkRequiredParams(req.query, ['entityType']);
const module = await getModuleInstanceFromType.execute(
userId,
params.entityType,
{ state: req.query.state }
params.entityType
);
Comment on lines 508 to 512
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot May 5, 2026

Choose a reason for hiding this comment

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

P2 Badge Forward OAuth state when building authorization requirements

The GET /api/authorize path no longer forwards req.query.state to getModuleInstanceFromType.execute, so modules that rely on the optional state round-trip cannot include the caller-provided value in their authorization URL. This regresses CSRF/callback correlation flows that depend on a custom state token being propagated into the OAuth requester.

Useful? React with 👍 / 👎.

const areRequirementsValid =
module.validateAuthorizationRequirements();
Expand All @@ -531,13 +530,33 @@ function setEntityRoutes(router, authenticateUser, useCases) {
'data',
]);

const entityDetails = await processAuthorizationCallback.execute(
userId,
params.entityType,
params.data
const dataKeys =
params.data && typeof params.data === 'object'
? Object.keys(params.data)
: [];
console.log(
`[Frigg] POST /api/authorize userId=${userId} entityType=${params.entityType} dataKeys=${JSON.stringify(dataKeys)}`
);

res.json(entityDetails);
try {
const entityDetails =
await processAuthorizationCallback.execute(
userId,
params.entityType,
params.data
);

console.log(
`[Frigg] POST /api/authorize success userId=${userId} entityType=${params.entityType} credentialId=${entityDetails?.credential_id} entityId=${entityDetails?.entity_id}`
);

res.json(entityDetails);
} catch (err) {
console.error(
`[Frigg] POST /api/authorize failed userId=${userId} entityType=${params.entityType} error=${err?.message || err}`
);
throw err;
}
})
);

Expand Down
43 changes: 43 additions & 0 deletions packages/core/modules/use-cases/__tests__/process-authorization-callback.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,15 @@ describe('ProcessAuthorizationCallback — re-auth status restoration', () => {
externalId: 'ext-1',
credential: 'cred-1',
}),
findEntitiesByUserIdAndModuleName: jest.fn().mockResolvedValue([
{
id: 'entity-1',
userId: 'user-1',
moduleName: 'testmodule',
externalId: 'ext-1',
credential: { id: 'cred-1', authIsValid: false },
},
]),
createEntity: jest.fn(),
};

Expand Down Expand Up @@ -216,4 +225,38 @@ describe('ProcessAuthorizationCallback — re-auth status restoration', () => {
success: true,
});
});

it('persists credentials explicitly on OAuth2 re-auth (belt-and-suspenders)', async () => {
// Force the OAuth2 path
const { Module } = require('../../module');
Module.mockImplementation(({ userId, definition, entity }) => ({
userId,
entity,
credential: undefined,
definition,
apiClass: { requesterType: 'oauth2' },
api: { delegate: null },
testAuth: jest.fn().mockResolvedValue(true),
apiParamsFromCredential: jest.fn().mockReturnValue({}),
apiParamsFromEntity: jest.fn().mockReturnValue({}),
getName: jest.fn().mockReturnValue('testmodule'),
}));

moduleDefinitions[0].requiredAuthMethods.getToken = jest
.fn()
.mockResolvedValue({
access_token: 'fresh-access',
refresh_token: 'fresh-refresh',
});

await useCase.execute('user-1', 'testmodule', { code: 'oauth-code' });

// upsertCredential must be called once even on the OAuth2 path —
// we no longer rely solely on the DLGT_TOKEN_UPDATE notification.
expect(credentialRepository.upsertCredential).toHaveBeenCalledTimes(1);
expect(
credentialRepository.upsertCredential.mock.calls[0][0].details
.authIsValid
).toBe(true);
});
});
54 changes: 49 additions & 5 deletions packages/core/modules/use-cases/process-authorization-callback.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@
}

async execute(userId, entityType, params) {
const hasCode = Boolean(params && params.code);

Check warning on line 31 in packages/core/modules/use-cases/process-authorization-callback.js

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Prefer using an optional chain expression instead, as it's more concise and easier to read. 


See more on https://sonarcloud.io/project/issues?id=friggframework_frigg&issues=AZ349-4LxFmG3FqHceHY&open=AZ349-4LxFmG3FqHceHY&pullRequest=582
console.log(
`[Frigg] processAuthorizationCallback start userId=${userId} entityType=${entityType} hasCode=${hasCode}`
);

const moduleDefinition = this.moduleDefinitions.find((def) => {
return entityType === def.moduleName;
});
Expand All @@ -38,12 +43,29 @@
);
}

// todo: check if we need to pass entity to Module, right now it's null
let entity = null;
// Bootstrap the Module with the existing entity (if any) so the API
// requester is preloaded with prior tokens. This enables a refresh
// fallback when callers lack a fresh OAuth code, and lets us match a
// re-auth back to the existing credential record by id.
const existingEntities =
await this.moduleRepository.findEntitiesByUserIdAndModuleName(
userId,
entityType
);
const existingEntity =
existingEntities && existingEntities.length > 0
? existingEntities[0]
: null;
Comment on lines +55 to +58
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot May 5, 2026

Choose a reason for hiding this comment

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

P1 Badge Select the re-auth target entity deterministically

Using existingEntities[0] makes OAuth2 re-auth pick an arbitrary prior entity when a user has multiple entities for the same module, because repository findMany calls do not enforce ordering. That arbitrary entity's credential is then preloaded into Module, and OAuth2Requester.setTokens preserves an existing refresh_token when the provider does not return one, so a re-auth for account B can persist account A's refresh token and break subsequent refreshes for the updated credential.

Useful? React with 👍 / 👎.


if (existingEntity) {
console.log(
`[Frigg] processAuthorizationCallback found existing entity id=${existingEntity.id} credentialId=${existingEntity.credential?.id}`
);
}

const module = new Module({
userId,
entity,
entity: existingEntity,
definition: moduleDefinition,
});

Expand All @@ -53,6 +75,16 @@
module.api,
params
);
console.log(
`[Frigg] processAuthorizationCallback OAuth getToken complete userId=${userId} entityType=${entityType}`
);
// Belt-and-suspenders: persist tokens explicitly here rather than
// relying solely on the DLGT_TOKEN_UPDATE notification chain
// inside setTokens. The notification path remains in place but
// has been observed to no-op silently in some prod paths,
// leaving newly-issued tokens unsaved while the user-visible
// OAuth flow appears to succeed.
await this.onTokenUpdate(module, moduleDefinition, userId);
} else {
tokenResponse =
await moduleDefinition.requiredAuthMethods.setAuthParams(
Expand All @@ -62,6 +94,10 @@
await this.onTokenUpdate(module, moduleDefinition, userId);
}

console.log(
`[Frigg] processAuthorizationCallback credential persisted credentialId=${module.credential?.id} authIsValid=${module.credential?.authIsValid}`
);

const authRes = await module.testAuth();
if (!authRes) {
throw new Error('Authorization failed');
Expand Down Expand Up @@ -90,7 +126,12 @@
// credential + entity are already persisted. Operators can recover
// stuck integrations manually.
try {
await this.restoreIntegrationsForEntity(persistedEntity.id);
const restoredCount = await this.restoreIntegrationsForEntity(
persistedEntity.id
);
console.log(
`[Frigg] processAuthorizationCallback restored ${restoredCount} integration(s) for entityId=${persistedEntity.id}`
);
} catch (err) {
console.error(
`[Frigg] Failed to restore integrations for entity ${persistedEntity.id} after successful re-auth — manual intervention may be needed`,
Expand All @@ -106,11 +147,12 @@
}

async restoreIntegrationsForEntity(entityId) {
if (!this.integrationRepository) return;
if (!this.integrationRepository) return 0;
const integrations =
await this.integrationRepository.findIntegrationsByEntityId(
entityId
);
let restored = 0;
for (const integration of integrations) {
if (STATUSES_RESET_ON_REAUTH.includes(integration.status)) {
console.log(
Expand All @@ -120,8 +162,10 @@
integration.id,
'ENABLED'
);
restored++;
}
}
return restored;
}

async onTokenUpdate(module, moduleDefinition, userId) {
Expand Down
Loading