Follow-up to #22827, addressing the review comments left around merge time and applying the endpoint convergence discussed afterwards. ## Review comments from #22827 - **Swallowed error in dev sync asset read** ([comment](https://github.com/twentyhq/twenty/pull/22827#discussion_r3571513265)): the swallow is intentional (a missing public asset must not fail the whole dev sync) but it now logs a warning with the asset path and error, and the registration keeps its previously stored file for that path instead of losing it. - **`isAbsoluteUrl` location** ([comment](https://github.com/twentyhq/twenty/pull/22827#discussion_r3571524234)): moved to `twenty-shared/utils/url`. The server, and now also `twenty-sdk`'s `normalize-application-assets`, use the shared util. - **Soft delete vs file cleanup** ([comment](https://github.com/twentyhq/twenty/pull/22827#discussion_r3571589558)): per review, deleting a registration is now a hard delete. Stored assets (bytes + rows) are deleted with it, dependent rows are removed by their existing FK cascades, and installed applications keep working with their registration link nulled. No soft-delete/cron mechanism. - **Asset cap too generous** ([comment](https://github.com/twentyhq/twenty/pull/22827#discussion_r3571595745)): lowered to 10MB per review and documented in the publishing and public-assets docs pages. - **One missing image retriggers a full asset sync** ([comment](https://github.com/twentyhq/twenty/pull/22827#discussion_r3571646243)): `storeRegistrationAssets` now takes `skipAlreadyStoredPaths`; the catalog sync passes it when the package version is unchanged, so only assets missing a stored file are fetched instead of re-downloading everything. - **`existing.logo` already contains the new logo** ([comment](https://github.com/twentyhq/twenty/pull/22827#discussion_r3571667847)): correct, `updateFromManifest` runs first, so the previous "keep fileId when the path did not change" guard compared the new logo against itself. The fileId preservation is now keyed on the stored server file for the exact path (files are unique per `(applicationRegistrationId, path)`): a changed logo path no longer inherits the old file's id, and a transient download failure on an unchanged path still keeps the working file. This also removed the fileId-preservation bookkeeping from `storeRegistrationAssets`. ## Endpoint convergence - **Path-addressed public route for registration assets**: `GET /file/server/application-registration/:fileId` is replaced by `GET /files/application-registrations/:registrationId/*path`, mirroring the manifest's public-folder paths and leaving room for a future `:version` segment. Assets stay addressable by stable ids server-side; the fileId now only marks a path as stored. No URL is ever persisted (all are built at query time), and the old route never shipped in a release, so there is nothing to migrate. - **`Application.logoUrl` resolved server-side**: new `ResolveField` on the `Application` type builds the `/public-assets/...` display URL (or passes absolute URLs through). `useApplicationChipData` now reads it from `currentWorkspace.installedApplications`, and the frontend `buildApplicationLogoUrl` util is deleted, so clients no longer construct file URLs themselves. ## Validation - Unit: `file.controller.spec` (route renamed, traversal case added), `server-file-storage.service.spec` (`findServerFile`, `deleteByApplicationRegistrationId`), `application-registration-asset-url.service.spec` (new URL shape, url-encoding), new `isAbsoluteUrl` test; all application/file suites pass. - Live against a local server: new route serves tarball and rehosted npm assets with `public, max-age=3600` (nested paths included), 404s on missing files, unknown registrations, traversal attempts, and the removed old route; `findManyApplicationRegistrations` returns path-addressed URLs for stored assets, CDN fallback for npm, absolute passthrough; `installedApplications.logoUrl` resolves the public-assets URL and stays null for logo-less apps. Registration hard delete verified against the DB: file rows cascade, application rows keep a nulled registration link. - Typecheck + lint on twenty-server, twenty-front, twenty-shared, twenty-sdk; metadata codegen and client-sdk regenerated.
This commit is contained in:
+10
-2
@@ -1,4 +1,4 @@
|
||||
import { Injectable } from '@nestjs/common';
|
||||
import { Injectable, Logger } from '@nestjs/common';
|
||||
|
||||
import { FileFolder } from 'twenty-shared/types';
|
||||
import { isDefined } from 'twenty-shared/utils';
|
||||
@@ -41,6 +41,8 @@ const ALLOWED_APPLICATION_FILE_FOLDERS: FileFolder[] = [
|
||||
|
||||
@Injectable()
|
||||
export class ApplicationDevelopmentService {
|
||||
private readonly logger = new Logger(ApplicationDevelopmentService.name);
|
||||
|
||||
constructor(
|
||||
private readonly applicationService: ApplicationService,
|
||||
private readonly applicationSyncService: ApplicationSyncService,
|
||||
@@ -361,7 +363,13 @@ export class ApplicationDevelopmentService {
|
||||
});
|
||||
|
||||
return await streamToBuffer(stream);
|
||||
} catch {
|
||||
} catch (error) {
|
||||
// A missing or unreadable asset must not fail the whole dev sync; the
|
||||
// registration keeps its previously stored file for that path, if any.
|
||||
this.logger.warn(
|
||||
`Could not read public asset "${path}" for application ${applicationUniversalIdentifier}: ${error.message}`,
|
||||
);
|
||||
|
||||
return null;
|
||||
}
|
||||
}
|
||||
|
||||
+3
-1
@@ -73,7 +73,8 @@ export class MarketplaceCatalogSyncService {
|
||||
// Rehost the logo and gallery images from the registry CDN so display
|
||||
// urls are served from fileIds like every other source. Skipped when
|
||||
// the version is unchanged and the files are already stored; the
|
||||
// query-time url builder falls back to CDN urls until they are.
|
||||
// query-time url builder falls back to CDN urls until they are. On an
|
||||
// unchanged version, only assets missing a stored file are fetched.
|
||||
if (
|
||||
previousVersion !== pkg.version ||
|
||||
!areRegistrationAssetsStored(
|
||||
@@ -91,6 +92,7 @@ export class MarketplaceCatalogSyncService {
|
||||
pkg.version,
|
||||
path,
|
||||
),
|
||||
skipAlreadyStoredPaths: previousVersion === pkg.version,
|
||||
},
|
||||
);
|
||||
}
|
||||
|
||||
+1
-1
@@ -7,7 +7,7 @@ import { z } from 'zod';
|
||||
import { buildRegistryCdnUrl } from 'src/engine/core-modules/application/application-marketplace/utils/build-registry-cdn-url.util';
|
||||
import { TwentyConfigService } from 'src/engine/core-modules/twenty-config/twenty-config.service';
|
||||
|
||||
const MAX_REGISTRY_ASSET_SIZE_BYTES = 100 * 1024 * 1024; // 100Mb
|
||||
const MAX_REGISTRY_ASSET_SIZE_BYTES = 10 * 1024 * 1024; // 10Mb
|
||||
|
||||
export type RegistryPackageInfo = {
|
||||
name: string;
|
||||
|
||||
+15
-2
@@ -12,6 +12,7 @@ const CONFIG_VALUES: Record<string, string> = {
|
||||
};
|
||||
|
||||
const baseRegistration = {
|
||||
id: 'registration-id',
|
||||
sourceType: ApplicationRegistrationSourceType.TARBALL,
|
||||
sourcePackage: null,
|
||||
latestAvailableVersion: '1.0.0',
|
||||
@@ -50,7 +51,19 @@ describe('ApplicationRegistrationAssetUrlService', () => {
|
||||
});
|
||||
|
||||
expect(logoUrl).toBe(
|
||||
'https://api.twenty.com/file/server/application-registration/file-id',
|
||||
'https://api.twenty.com/files/application-registrations/registration-id/public/logo.png',
|
||||
);
|
||||
});
|
||||
|
||||
it('should url-encode stored asset path segments', () => {
|
||||
const logoUrl = service.buildLogoUrl({
|
||||
...baseRegistration,
|
||||
logo: 'public/logo #1.png',
|
||||
logoFileId: 'file-id',
|
||||
});
|
||||
|
||||
expect(logoUrl).toBe(
|
||||
'https://api.twenty.com/files/application-registrations/registration-id/public/logo%20%231.png',
|
||||
);
|
||||
});
|
||||
|
||||
@@ -105,7 +118,7 @@ describe('ApplicationRegistrationAssetUrlService', () => {
|
||||
});
|
||||
|
||||
expect(urls).toEqual([
|
||||
'https://api.twenty.com/file/server/application-registration/file-1',
|
||||
'https://api.twenty.com/files/application-registrations/registration-id/public/one.png',
|
||||
'https://example.com/two.png',
|
||||
]);
|
||||
});
|
||||
|
||||
+17
-11
@@ -1,24 +1,22 @@
|
||||
import { Injectable } from '@nestjs/common';
|
||||
|
||||
import { ServerFileFolder } from 'twenty-shared/types';
|
||||
import { isDefined, isNonEmptyArray } from 'twenty-shared/utils';
|
||||
import { isAbsoluteUrl, isDefined, isNonEmptyArray } from 'twenty-shared/utils';
|
||||
|
||||
import { buildRegistryCdnUrl } from 'src/engine/core-modules/application/application-marketplace/utils/build-registry-cdn-url.util';
|
||||
import { SERVER_FILE_STORAGE_PREFIX } from 'src/engine/core-modules/file-storage/constants/server-file-storage-prefix.constant';
|
||||
import { type ApplicationRegistrationEntity } from 'src/engine/core-modules/application/application-registration/application-registration.entity';
|
||||
import { ApplicationRegistrationSourceType } from 'src/engine/core-modules/application/application-registration/enums/application-registration-source-type.enum';
|
||||
import { toGalleryImagePaths } from 'src/engine/core-modules/application/application-registration/utils/to-gallery-image-paths.util';
|
||||
import { isAbsoluteUrl } from 'src/engine/core-modules/application/application-registration/utils/is-absolute-url.util';
|
||||
import { TwentyConfigService } from 'src/engine/core-modules/twenty-config/twenty-config.service';
|
||||
|
||||
type AssetSourceFields = Pick<
|
||||
ApplicationRegistrationEntity,
|
||||
'sourceType' | 'sourcePackage' | 'latestAvailableVersion'
|
||||
'id' | 'sourceType' | 'sourcePackage' | 'latestAvailableVersion'
|
||||
>;
|
||||
|
||||
// Builds display URLs for application registration assets at query time.
|
||||
// Assets stored as instance-global server files (TARBALL, LOCAL) are served by
|
||||
// fileId; NPM assets are served straight from the registry CDN; absolute URLs
|
||||
// Assets stored as instance-global server files (TARBALL, LOCAL, rehosted NPM)
|
||||
// are served path-addressed under /files/application-registrations; NPM assets
|
||||
// not yet rehosted are served straight from the registry CDN; absolute URLs
|
||||
// pass through untouched.
|
||||
@Injectable()
|
||||
export class ApplicationRegistrationAssetUrlService {
|
||||
@@ -77,14 +75,22 @@ export class ApplicationRegistrationAssetUrlService {
|
||||
path: string | null;
|
||||
registration: AssetSourceFields;
|
||||
}): string | null {
|
||||
if (!isDefined(path) || path.length === 0) {
|
||||
return null;
|
||||
}
|
||||
|
||||
// A fileId marks the path as stored in server file storage.
|
||||
if (isDefined(fileId)) {
|
||||
const serverUrl = this.twentyConfigService.get('SERVER_URL');
|
||||
|
||||
return `${serverUrl}/file/${SERVER_FILE_STORAGE_PREFIX}/${ServerFileFolder.ApplicationRegistration}/${fileId}`;
|
||||
}
|
||||
// Encode segments so URL-reserved characters (#, ?, spaces) in file
|
||||
// names survive; directory separators are kept as route path segments.
|
||||
const encodedPath = path
|
||||
.split('/')
|
||||
.map((segment) => encodeURIComponent(segment))
|
||||
.join('/');
|
||||
|
||||
if (!isDefined(path) || path.length === 0) {
|
||||
return null;
|
||||
return `${serverUrl}/files/application-registrations/${registration.id}/${encodedPath}`;
|
||||
}
|
||||
|
||||
if (isAbsoluteUrl(path)) {
|
||||
|
||||
+93
-78
@@ -10,6 +10,7 @@ import { ApplicationRegistrationEntity } from 'src/engine/core-modules/applicati
|
||||
import { type ApplicationRegistrationGalleryImage } from 'src/engine/core-modules/application/application-registration/types/application-registration-gallery-image.type';
|
||||
import { isStorableAssetPath } from 'src/engine/core-modules/application/application-registration/utils/is-storable-asset-path.util';
|
||||
import { toGalleryImagePaths } from 'src/engine/core-modules/application/application-registration/utils/to-gallery-image-paths.util';
|
||||
import { FileStorageException } from 'src/engine/core-modules/file-storage/interfaces/file-storage-exception';
|
||||
import { ServerFileStorageService } from 'src/engine/core-modules/file-storage/services/server-file-storage.service';
|
||||
import { prepareFileForStorageOrThrow } from 'src/engine/core-modules/file-storage/utils/prepare-file-for-storage-or-throw.util';
|
||||
import type { ApplicationManifest } from 'twenty-shared/application';
|
||||
@@ -35,89 +36,27 @@ export class ApplicationRegistrationAssetService {
|
||||
applicationRegistrationId,
|
||||
manifestApplication,
|
||||
readAsset,
|
||||
skipAlreadyStoredPaths = false,
|
||||
}: {
|
||||
applicationRegistrationId: string;
|
||||
manifestApplication: ApplicationManifest | undefined;
|
||||
readAsset: ReadRegistrationAsset;
|
||||
// Set when asset contents are known to be unchanged (same package
|
||||
// version): assets that already have a stored file for the same path are
|
||||
// not re-downloaded, only missing ones are.
|
||||
skipAlreadyStoredPaths?: boolean;
|
||||
}): Promise<void> {
|
||||
const existing = await this.applicationRegistrationRepository.findOneOrFail(
|
||||
{
|
||||
select: ['id', 'logo', 'logoFileId', 'galleryImages'],
|
||||
where: { id: applicationRegistrationId },
|
||||
},
|
||||
);
|
||||
|
||||
const logoFileId = await this.storeLogoFile({
|
||||
applicationRegistrationId,
|
||||
manifestApplication,
|
||||
readAsset,
|
||||
});
|
||||
|
||||
const galleryImages = await this.storeGalleryImageFiles({
|
||||
applicationRegistrationId,
|
||||
manifestApplication,
|
||||
readAsset,
|
||||
});
|
||||
|
||||
// A transient read/download failure must not clobber a working asset:
|
||||
// keep the previously stored fileId when the path did not change.
|
||||
const logoPath = manifestApplication?.logo ?? manifestApplication?.logoUrl;
|
||||
const existingFileIdByPath = new Map(
|
||||
(existing.galleryImages ?? []).map(({ path, fileId }) => [path, fileId]),
|
||||
);
|
||||
|
||||
await this.applicationRegistrationRepository.update(
|
||||
applicationRegistrationId,
|
||||
{
|
||||
logoFileId:
|
||||
logoFileId ??
|
||||
(isDefined(logoPath) &&
|
||||
isStorableAssetPath(logoPath) &&
|
||||
existing.logo === logoPath
|
||||
? existing.logoFileId
|
||||
: null),
|
||||
galleryImages: galleryImages.map((galleryImage) => ({
|
||||
...galleryImage,
|
||||
fileId:
|
||||
galleryImage.fileId ??
|
||||
existingFileIdByPath.get(galleryImage.path) ??
|
||||
null,
|
||||
})),
|
||||
} as QueryDeepPartialEntity<ApplicationRegistrationEntity>,
|
||||
);
|
||||
}
|
||||
|
||||
private async storeLogoFile({
|
||||
applicationRegistrationId,
|
||||
manifestApplication,
|
||||
readAsset,
|
||||
}: {
|
||||
applicationRegistrationId: string;
|
||||
manifestApplication: ApplicationManifest | undefined;
|
||||
readAsset: ReadRegistrationAsset;
|
||||
}): Promise<string | null> {
|
||||
const logoPath = manifestApplication?.logo ?? manifestApplication?.logoUrl;
|
||||
|
||||
if (!isDefined(logoPath)) {
|
||||
return null;
|
||||
}
|
||||
const logoFileId = isDefined(logoPath)
|
||||
? await this.storeAssetFile({
|
||||
applicationRegistrationId,
|
||||
path: logoPath,
|
||||
readAsset,
|
||||
skipAlreadyStoredPaths,
|
||||
})
|
||||
: null;
|
||||
|
||||
return this.storeAssetFile({
|
||||
applicationRegistrationId,
|
||||
path: logoPath,
|
||||
readAsset,
|
||||
});
|
||||
}
|
||||
|
||||
private async storeGalleryImageFiles({
|
||||
applicationRegistrationId,
|
||||
manifestApplication,
|
||||
readAsset,
|
||||
}: {
|
||||
applicationRegistrationId: string;
|
||||
manifestApplication: ApplicationManifest | undefined;
|
||||
readAsset: ReadRegistrationAsset;
|
||||
}): Promise<ApplicationRegistrationGalleryImage[]> {
|
||||
const galleryImages: ApplicationRegistrationGalleryImage[] = [];
|
||||
|
||||
for (const path of toGalleryImagePaths(manifestApplication)) {
|
||||
@@ -125,6 +64,7 @@ export class ApplicationRegistrationAssetService {
|
||||
applicationRegistrationId,
|
||||
path,
|
||||
readAsset,
|
||||
skipAlreadyStoredPaths,
|
||||
});
|
||||
|
||||
// Entries without a fileId (absolute URLs, missing files) are kept so
|
||||
@@ -132,27 +72,49 @@ export class ApplicationRegistrationAssetService {
|
||||
galleryImages.push({ path, fileId });
|
||||
}
|
||||
|
||||
return galleryImages;
|
||||
await this.applicationRegistrationRepository.update(
|
||||
applicationRegistrationId,
|
||||
{
|
||||
logoFileId,
|
||||
galleryImages,
|
||||
} as QueryDeepPartialEntity<ApplicationRegistrationEntity>,
|
||||
);
|
||||
}
|
||||
|
||||
private async storeAssetFile({
|
||||
applicationRegistrationId,
|
||||
path,
|
||||
readAsset,
|
||||
skipAlreadyStoredPaths,
|
||||
}: {
|
||||
applicationRegistrationId: string;
|
||||
path: string;
|
||||
readAsset: ReadRegistrationAsset;
|
||||
skipAlreadyStoredPaths: boolean;
|
||||
}): Promise<string | null> {
|
||||
if (!isStorableAssetPath(path)) {
|
||||
return null;
|
||||
}
|
||||
|
||||
if (skipAlreadyStoredPaths) {
|
||||
const alreadyStoredFileId = await this.findStoredAssetFileId({
|
||||
applicationRegistrationId,
|
||||
path,
|
||||
});
|
||||
|
||||
if (isDefined(alreadyStoredFileId)) {
|
||||
return alreadyStoredFileId;
|
||||
}
|
||||
}
|
||||
|
||||
try {
|
||||
const contents = await readAsset(path);
|
||||
|
||||
if (!isDefined(contents)) {
|
||||
return null;
|
||||
return this.keepPreviouslyStoredAssetFileId({
|
||||
applicationRegistrationId,
|
||||
path,
|
||||
});
|
||||
}
|
||||
|
||||
const { sourceFile, mimeType } = await prepareFileForStorageOrThrow({
|
||||
@@ -176,7 +138,60 @@ export class ApplicationRegistrationAssetService {
|
||||
`Failed to store asset "${path}" for registration ${applicationRegistrationId}: ${error.message}`,
|
||||
);
|
||||
|
||||
return null;
|
||||
return this.keepPreviouslyStoredAssetFileId({
|
||||
applicationRegistrationId,
|
||||
path,
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
// A transient read/download failure must not clobber a working asset: the
|
||||
// file previously stored for the same path is kept. When the path changed,
|
||||
// no file exists for it and the asset correctly resolves to null.
|
||||
private async keepPreviouslyStoredAssetFileId({
|
||||
applicationRegistrationId,
|
||||
path,
|
||||
}: {
|
||||
applicationRegistrationId: string;
|
||||
path: string;
|
||||
}): Promise<string | null> {
|
||||
const previouslyStoredFileId = await this.findStoredAssetFileId({
|
||||
applicationRegistrationId,
|
||||
path,
|
||||
});
|
||||
|
||||
if (isDefined(previouslyStoredFileId)) {
|
||||
this.logger.warn(
|
||||
`Keeping previously stored file for asset "${path}" of registration ${applicationRegistrationId}`,
|
||||
);
|
||||
}
|
||||
|
||||
return previouslyStoredFileId;
|
||||
}
|
||||
|
||||
private async findStoredAssetFileId({
|
||||
applicationRegistrationId,
|
||||
path,
|
||||
}: {
|
||||
applicationRegistrationId: string;
|
||||
path: string;
|
||||
}): Promise<string | null> {
|
||||
try {
|
||||
const storedFile = await this.serverFileStorageService.findServerFile({
|
||||
fileFolder: ServerFileFolder.ApplicationRegistration,
|
||||
applicationRegistrationId,
|
||||
resourcePath: path,
|
||||
});
|
||||
|
||||
return storedFile?.id ?? null;
|
||||
} catch (error) {
|
||||
// Only an invalid path means "nothing stored for this path"; transient
|
||||
// lookup failures must propagate so a working fileId is never cleared.
|
||||
if (error instanceof FileStorageException) {
|
||||
return null;
|
||||
}
|
||||
|
||||
throw error;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
+4
-3
@@ -439,10 +439,9 @@ export class ApplicationRegistrationService {
|
||||
|
||||
async delete(id: string, ownerWorkspaceId: string): Promise<boolean> {
|
||||
await this.findOneById(id, ownerWorkspaceId);
|
||||
await this.applicationRegistrationRepository.softDelete(id);
|
||||
|
||||
// Stored assets (logo, gallery images) are gone for good; deleting the
|
||||
// file rows also nulls logoFileId through its ON DELETE SET NULL fk.
|
||||
// Stored assets (logo, gallery images) go with the registration; deleting
|
||||
// them first also removes the bytes, which the row FK cascade cannot do.
|
||||
try {
|
||||
await this.serverFileStorageService.deleteByApplicationRegistrationId(id);
|
||||
} catch (error) {
|
||||
@@ -452,6 +451,8 @@ export class ApplicationRegistrationService {
|
||||
);
|
||||
}
|
||||
|
||||
await this.applicationRegistrationRepository.delete(id);
|
||||
|
||||
await this.invalidateMarketplaceAppsCache();
|
||||
|
||||
return true;
|
||||
|
||||
-2
@@ -1,2 +0,0 @@
|
||||
export const isAbsoluteUrl = (url: string): boolean =>
|
||||
/^https?:\/\//i.test(url);
|
||||
+2
-1
@@ -1,4 +1,5 @@
|
||||
import { isAbsoluteUrl } from 'src/engine/core-modules/application/application-registration/utils/is-absolute-url.util';
|
||||
import { isAbsoluteUrl } from 'twenty-shared/utils';
|
||||
|
||||
import { isImageFilePath } from 'src/engine/core-modules/application/application-registration/utils/is-image-file-path.util';
|
||||
|
||||
// Only relative image paths are copied into server file storage; absolute
|
||||
|
||||
@@ -3,6 +3,7 @@ import { TypeOrmModule } from '@nestjs/typeorm';
|
||||
|
||||
import { ApplicationRegistrationEntity } from 'src/engine/core-modules/application/application-registration/application-registration.entity';
|
||||
import { ApplicationEntity } from 'src/engine/core-modules/application/application.entity';
|
||||
import { ApplicationResolver } from 'src/engine/core-modules/application/application.resolver';
|
||||
import { ApplicationService } from 'src/engine/core-modules/application/application.service';
|
||||
import { WorkspaceFlatApplicationMapCacheService } from 'src/engine/core-modules/application/workspace-flat-application-map-cache.service';
|
||||
import { ApplicationVariableEntity } from 'src/engine/core-modules/application/application-variable/application-variable.entity';
|
||||
@@ -38,6 +39,7 @@ import { WorkspaceCacheModule } from 'src/engine/workspace-cache/workspace-cache
|
||||
],
|
||||
exports: [ApplicationService, WorkspaceFlatApplicationMapCacheService],
|
||||
providers: [
|
||||
ApplicationResolver,
|
||||
ApplicationService,
|
||||
WorkspaceFlatApplicationMapCacheService,
|
||||
provideWorkspaceScopedRepository(AgentEntity),
|
||||
|
||||
@@ -0,0 +1,40 @@
|
||||
import { UseGuards } from '@nestjs/common';
|
||||
import { Parent, ResolveField } from '@nestjs/graphql';
|
||||
|
||||
import { isAbsoluteUrl, isDefined } from 'twenty-shared/utils';
|
||||
|
||||
import { MetadataResolver } from 'src/engine/api/graphql/graphql-config/decorators/metadata-resolver.decorator';
|
||||
import { ApplicationDTO } from 'src/engine/core-modules/application/dtos/application.dto';
|
||||
import { TwentyConfigService } from 'src/engine/core-modules/twenty-config/twenty-config.service';
|
||||
import { WorkspaceEntity } from 'src/engine/core-modules/workspace/workspace.entity';
|
||||
import { AuthWorkspace } from 'src/engine/decorators/auth/auth-workspace.decorator';
|
||||
import { NoPermissionGuard } from 'src/engine/guards/no-permission.guard';
|
||||
import { WorkspaceAuthGuard } from 'src/engine/guards/workspace-auth.guard';
|
||||
|
||||
@UseGuards(WorkspaceAuthGuard, NoPermissionGuard)
|
||||
@MetadataResolver(() => ApplicationDTO)
|
||||
export class ApplicationResolver {
|
||||
constructor(private readonly twentyConfigService: TwentyConfigService) {}
|
||||
|
||||
// Resolves the display url of the logo bundled in the installed
|
||||
// application's public assets, so clients never build file urls themselves.
|
||||
@ResolveField(() => String, { nullable: true })
|
||||
logoUrl(
|
||||
@Parent() application: Pick<ApplicationDTO, 'id' | 'logo'>,
|
||||
@AuthWorkspace() workspace: WorkspaceEntity,
|
||||
): string | null {
|
||||
const logo = application.logo;
|
||||
|
||||
if (!isDefined(logo) || logo.length === 0) {
|
||||
return null;
|
||||
}
|
||||
|
||||
if (isAbsoluteUrl(logo)) {
|
||||
return logo;
|
||||
}
|
||||
|
||||
const serverUrl = this.twentyConfigService.get('SERVER_URL');
|
||||
|
||||
return `${serverUrl}/public-assets/${workspace.id}/${application.id}/${logo}`;
|
||||
}
|
||||
}
|
||||
+24
-63
@@ -193,6 +193,7 @@ describe('ServerFileStorageService', () => {
|
||||
mockServerFileRepository.findOneBy.mockResolvedValue({
|
||||
id: 'server-file-id',
|
||||
path: 'application-registration/registration-id/manifests/manifest.json',
|
||||
mimeType: 'application/json',
|
||||
} as FileEntity);
|
||||
mockDriver.readFile.mockResolvedValue(stream);
|
||||
|
||||
@@ -203,6 +204,7 @@ describe('ServerFileStorageService', () => {
|
||||
});
|
||||
|
||||
expect(mockServerFileRepository.findOneBy).toHaveBeenCalledWith({
|
||||
applicationRegistrationId: 'registration-id',
|
||||
path: 'application-registration/registration-id/manifests/manifest.json',
|
||||
workspaceId: IsNull(),
|
||||
});
|
||||
@@ -210,7 +212,7 @@ describe('ServerFileStorageService', () => {
|
||||
filePath:
|
||||
'server/application-registration/registration-id/manifests/manifest.json',
|
||||
});
|
||||
expect(result).toBe(stream);
|
||||
expect(result).toEqual({ stream, mimeType: 'application/json' });
|
||||
});
|
||||
|
||||
it('should propagate the missing-file exception from the driver', async () => {
|
||||
@@ -257,48 +259,39 @@ describe('ServerFileStorageService', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('readServerFileById', () => {
|
||||
it('should read the bytes of the row storage path', async () => {
|
||||
const stream = Readable.from(['{}']);
|
||||
|
||||
mockServerFileRepository.findOneBy.mockResolvedValue({
|
||||
describe('findServerFile', () => {
|
||||
it('should return the row matching the registration and path', async () => {
|
||||
const serverFile = {
|
||||
id: 'server-file-id',
|
||||
path: 'application-registration/registration-id/manifest.json',
|
||||
mimeType: 'application/json',
|
||||
} as FileEntity);
|
||||
mockDriver.readFile.mockResolvedValue(stream);
|
||||
} as FileEntity;
|
||||
|
||||
const result = await service.readServerFileById(
|
||||
'server-file-id',
|
||||
ServerFileFolder.ApplicationRegistration,
|
||||
);
|
||||
mockServerFileRepository.findOneBy.mockResolvedValue(serverFile);
|
||||
|
||||
const result = await service.findServerFile({
|
||||
fileFolder: ServerFileFolder.ApplicationRegistration,
|
||||
applicationRegistrationId: 'registration-id',
|
||||
resourcePath: 'manifest.json',
|
||||
});
|
||||
|
||||
expect(mockServerFileRepository.findOneBy).toHaveBeenCalledWith({
|
||||
id: 'server-file-id',
|
||||
applicationRegistrationId: 'registration-id',
|
||||
path: 'application-registration/registration-id/manifest.json',
|
||||
workspaceId: IsNull(),
|
||||
});
|
||||
expect(mockDriver.readFile).toHaveBeenCalledWith({
|
||||
filePath:
|
||||
'server/application-registration/registration-id/manifest.json',
|
||||
});
|
||||
expect(result).toEqual({ stream, mimeType: 'application/json' });
|
||||
expect(result).toBe(serverFile);
|
||||
});
|
||||
|
||||
it('should throw a missing-file exception when the row does not exist', async () => {
|
||||
it('should return null when no row matches', async () => {
|
||||
mockServerFileRepository.findOneBy.mockResolvedValue(null);
|
||||
|
||||
await expect(
|
||||
service.readServerFileById(
|
||||
'unknown-id',
|
||||
ServerFileFolder.ApplicationRegistration,
|
||||
),
|
||||
).rejects.toThrow(
|
||||
expect.objectContaining({
|
||||
code: FileStorageExceptionCode.FILE_NOT_FOUND,
|
||||
}),
|
||||
);
|
||||
const result = await service.findServerFile({
|
||||
fileFolder: ServerFileFolder.ApplicationRegistration,
|
||||
applicationRegistrationId: 'registration-id',
|
||||
resourcePath: 'missing.json',
|
||||
});
|
||||
|
||||
expect(mockDriver.readFile).not.toHaveBeenCalled();
|
||||
expect(result).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -368,38 +361,6 @@ describe('ServerFileStorageService', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('deleteByServerFileId', () => {
|
||||
it('should delete the bytes and the row of the given id', async () => {
|
||||
mockServerFileRepository.findOneBy.mockResolvedValue({
|
||||
id: 'server-file-id',
|
||||
path: 'application-registration/registration-id/manifest.json',
|
||||
} as FileEntity);
|
||||
|
||||
await service.deleteByServerFileId('server-file-id');
|
||||
|
||||
expect(mockDriver.delete).toHaveBeenCalledWith({
|
||||
folderPath: 'server/application-registration/registration-id',
|
||||
filename: 'manifest.json',
|
||||
});
|
||||
expect(mockServerFileRepository.delete).toHaveBeenCalledWith({
|
||||
id: 'server-file-id',
|
||||
workspaceId: IsNull(),
|
||||
});
|
||||
});
|
||||
|
||||
it('should throw a missing-file exception when the row does not exist', async () => {
|
||||
mockServerFileRepository.findOneBy.mockResolvedValue(null);
|
||||
|
||||
await expect(service.deleteByServerFileId('unknown-id')).rejects.toThrow(
|
||||
expect.objectContaining({
|
||||
code: FileStorageExceptionCode.FILE_NOT_FOUND,
|
||||
}),
|
||||
);
|
||||
|
||||
expect(mockServerFileRepository.delete).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe('deleteByApplicationRegistrationId', () => {
|
||||
it('should delete the bytes of every file then the rows', async () => {
|
||||
mockServerFileRepository.findBy.mockResolvedValue([
|
||||
|
||||
+29
-56
@@ -123,53 +123,55 @@ export class ServerFileStorageService {
|
||||
fileFolder,
|
||||
applicationRegistrationId,
|
||||
resourcePath,
|
||||
}: ServerResourceIdentifier): Promise<Readable> {
|
||||
}: ServerResourceIdentifier): Promise<{
|
||||
stream: Readable;
|
||||
mimeType: string;
|
||||
}> {
|
||||
const driver = this.fileStorageDriverFactory.getCurrentDriver();
|
||||
|
||||
const { onStorageFilePath, filePath } =
|
||||
const { onStorageFilePath } =
|
||||
this.validateAndBuildServerFileStoragePathOrThrow({
|
||||
fileFolder,
|
||||
applicationRegistrationId,
|
||||
resourcePath,
|
||||
});
|
||||
|
||||
const serverFile = await this.serverFileRepository.findOneBy({
|
||||
path: filePath,
|
||||
workspaceId: IsNull(),
|
||||
const serverFile = await this.findServerFile({
|
||||
fileFolder,
|
||||
applicationRegistrationId,
|
||||
resourcePath,
|
||||
});
|
||||
|
||||
if (!isDefined(serverFile)) {
|
||||
throw new FileStorageException(
|
||||
`Server file ${filePath} not found`,
|
||||
`Server file ${fileFolder}/${applicationRegistrationId}/${resourcePath} not found`,
|
||||
FileStorageExceptionCode.FILE_NOT_FOUND,
|
||||
);
|
||||
}
|
||||
|
||||
return driver.readFile({ filePath: onStorageFilePath });
|
||||
}
|
||||
|
||||
async readServerFileById(
|
||||
id: string,
|
||||
fileFolder: ServerFileFolder,
|
||||
): Promise<{ stream: Readable; mimeType: string }> {
|
||||
const serverFile = await this.findServerFileByIdOrThrow(id);
|
||||
|
||||
if (!serverFile.path.startsWith(`${fileFolder}/`)) {
|
||||
throw new FileStorageException(
|
||||
`Server file ${id} not found`,
|
||||
FileStorageExceptionCode.FILE_NOT_FOUND,
|
||||
);
|
||||
}
|
||||
|
||||
const driver = this.fileStorageDriverFactory.getCurrentDriver();
|
||||
|
||||
const stream = await driver.readFile({
|
||||
filePath: this.buildServerOnStorageFilePath(serverFile),
|
||||
});
|
||||
const stream = await driver.readFile({ filePath: onStorageFilePath });
|
||||
|
||||
return { stream, mimeType: serverFile.mimeType };
|
||||
}
|
||||
|
||||
async findServerFile({
|
||||
fileFolder,
|
||||
applicationRegistrationId,
|
||||
resourcePath,
|
||||
}: ServerResourceIdentifier): Promise<FileEntity | null> {
|
||||
const { filePath } = this.validateAndBuildServerFileStoragePathOrThrow({
|
||||
fileFolder,
|
||||
applicationRegistrationId,
|
||||
resourcePath,
|
||||
});
|
||||
|
||||
return this.serverFileRepository.findOneBy({
|
||||
applicationRegistrationId,
|
||||
path: filePath,
|
||||
workspaceId: IsNull(),
|
||||
});
|
||||
}
|
||||
|
||||
checkServerFileExists({
|
||||
fileFolder,
|
||||
applicationRegistrationId,
|
||||
@@ -207,19 +209,6 @@ export class ServerFileStorageService {
|
||||
});
|
||||
}
|
||||
|
||||
async deleteByServerFileId(id: string): Promise<void> {
|
||||
const serverFile = await this.findServerFileByIdOrThrow(id);
|
||||
|
||||
await this.deleteServerFileBytesBestEffort(
|
||||
this.buildServerOnStorageFilePath(serverFile),
|
||||
);
|
||||
|
||||
await this.serverFileRepository.delete({
|
||||
id,
|
||||
workspaceId: IsNull(),
|
||||
});
|
||||
}
|
||||
|
||||
async deleteByApplicationRegistrationId(
|
||||
applicationRegistrationId: string,
|
||||
): Promise<void> {
|
||||
@@ -240,22 +229,6 @@ export class ServerFileStorageService {
|
||||
});
|
||||
}
|
||||
|
||||
private async findServerFileByIdOrThrow(id: string): Promise<FileEntity> {
|
||||
const serverFile = await this.serverFileRepository.findOneBy({
|
||||
id,
|
||||
workspaceId: IsNull(),
|
||||
});
|
||||
|
||||
if (!isDefined(serverFile)) {
|
||||
throw new FileStorageException(
|
||||
`Server file ${id} not found`,
|
||||
FileStorageExceptionCode.FILE_NOT_FOUND,
|
||||
);
|
||||
}
|
||||
|
||||
return serverFile;
|
||||
}
|
||||
|
||||
private buildServerOnStorageFilePath(serverFile: FileEntity): string {
|
||||
return join(SERVER_FILE_STORAGE_PREFIX, serverFile.path);
|
||||
}
|
||||
|
||||
+63
-33
@@ -74,7 +74,7 @@ describe('FileController', () => {
|
||||
{
|
||||
provide: ServerFileStorageService,
|
||||
useValue: {
|
||||
readServerFileById: jest.fn(),
|
||||
readServerFile: jest.fn(),
|
||||
},
|
||||
},
|
||||
],
|
||||
@@ -322,28 +322,31 @@ describe('FileController', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('getApplicationRegistrationFileById', () => {
|
||||
describe('getApplicationRegistrationAsset', () => {
|
||||
const createAssetRequest = (path: string[] = ['images', 'logo.png']) =>
|
||||
({ params: { path } }) as any;
|
||||
|
||||
it('should stream the file with public cache headers', async () => {
|
||||
const mockStream = createMockStream();
|
||||
|
||||
jest
|
||||
.spyOn(serverFileStorageService, 'readServerFileById')
|
||||
.mockResolvedValue({
|
||||
stream: mockStream,
|
||||
mimeType: 'image/png',
|
||||
});
|
||||
jest.spyOn(serverFileStorageService, 'readServerFile').mockResolvedValue({
|
||||
stream: mockStream,
|
||||
mimeType: 'image/png',
|
||||
});
|
||||
|
||||
const mockResponse = createMockResponse() as any;
|
||||
|
||||
await controller.getApplicationRegistrationFileById(
|
||||
await controller.getApplicationRegistrationAsset(
|
||||
mockResponse,
|
||||
'file-123',
|
||||
createAssetRequest(),
|
||||
'registration-id',
|
||||
);
|
||||
|
||||
expect(serverFileStorageService.readServerFileById).toHaveBeenCalledWith(
|
||||
'file-123',
|
||||
ServerFileFolder.ApplicationRegistration,
|
||||
);
|
||||
expect(serverFileStorageService.readServerFile).toHaveBeenCalledWith({
|
||||
fileFolder: ServerFileFolder.ApplicationRegistration,
|
||||
applicationRegistrationId: 'registration-id',
|
||||
resourcePath: 'images/logo.png',
|
||||
});
|
||||
expect(mockResponse.setHeader).toHaveBeenCalledWith(
|
||||
'Content-Type',
|
||||
'image/png',
|
||||
@@ -357,10 +360,10 @@ describe('FileController', () => {
|
||||
|
||||
it('should throw FILE_NOT_FOUND when the file does not exist', async () => {
|
||||
jest
|
||||
.spyOn(serverFileStorageService, 'readServerFileById')
|
||||
.spyOn(serverFileStorageService, 'readServerFile')
|
||||
.mockRejectedValue(
|
||||
new FileStorageException(
|
||||
'Server file unknown-id not found',
|
||||
'Server file not found',
|
||||
FileStorageExceptionCode.FILE_NOT_FOUND,
|
||||
),
|
||||
);
|
||||
@@ -368,9 +371,35 @@ describe('FileController', () => {
|
||||
const mockResponse = createMockResponse() as any;
|
||||
|
||||
await expect(
|
||||
controller.getApplicationRegistrationFileById(
|
||||
controller.getApplicationRegistrationAsset(
|
||||
mockResponse,
|
||||
'unknown-id',
|
||||
createAssetRequest(['missing.png']),
|
||||
'registration-id',
|
||||
),
|
||||
).rejects.toThrow(
|
||||
new FileException('File not found', FileExceptionCode.FILE_NOT_FOUND),
|
||||
);
|
||||
|
||||
expect(mockPipeline).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('should throw FILE_NOT_FOUND when the path is rejected by validation', async () => {
|
||||
jest
|
||||
.spyOn(serverFileStorageService, 'readServerFile')
|
||||
.mockRejectedValue(
|
||||
new FileStorageException(
|
||||
'Invalid file path',
|
||||
FileStorageExceptionCode.ACCESS_DENIED,
|
||||
),
|
||||
);
|
||||
|
||||
const mockResponse = createMockResponse() as any;
|
||||
|
||||
await expect(
|
||||
controller.getApplicationRegistrationAsset(
|
||||
mockResponse,
|
||||
createAssetRequest(['..', 'escape.png']),
|
||||
'registration-id',
|
||||
),
|
||||
).rejects.toThrow(
|
||||
new FileException('File not found', FileExceptionCode.FILE_NOT_FOUND),
|
||||
@@ -380,19 +409,21 @@ describe('FileController', () => {
|
||||
});
|
||||
|
||||
it('should throw INTERNAL_SERVER_ERROR when the stream errors before headers are sent', async () => {
|
||||
jest
|
||||
.spyOn(serverFileStorageService, 'readServerFileById')
|
||||
.mockResolvedValue({
|
||||
stream: createMockStream(),
|
||||
mimeType: 'image/png',
|
||||
});
|
||||
jest.spyOn(serverFileStorageService, 'readServerFile').mockResolvedValue({
|
||||
stream: createMockStream(),
|
||||
mimeType: 'image/png',
|
||||
});
|
||||
|
||||
mockPipeline.mockRejectedValue(new Error('source backend exploded'));
|
||||
|
||||
const mockResponse = createMockResponse({ headersSent: false }) as any;
|
||||
|
||||
await expect(
|
||||
controller.getApplicationRegistrationFileById(mockResponse, 'file-123'),
|
||||
controller.getApplicationRegistrationAsset(
|
||||
mockResponse,
|
||||
createAssetRequest(),
|
||||
'registration-id',
|
||||
),
|
||||
).rejects.toThrow(
|
||||
new FileException(
|
||||
'Error streaming file from storage',
|
||||
@@ -404,20 +435,19 @@ describe('FileController', () => {
|
||||
});
|
||||
|
||||
it('should destroy the response without throwing when the stream errors after headers are sent', async () => {
|
||||
jest
|
||||
.spyOn(serverFileStorageService, 'readServerFileById')
|
||||
.mockResolvedValue({
|
||||
stream: createMockStream(),
|
||||
mimeType: 'image/png',
|
||||
});
|
||||
jest.spyOn(serverFileStorageService, 'readServerFile').mockResolvedValue({
|
||||
stream: createMockStream(),
|
||||
mimeType: 'image/png',
|
||||
});
|
||||
|
||||
mockPipeline.mockRejectedValue(new Error('socket reset mid-flight'));
|
||||
|
||||
const mockResponse = createMockResponse({ headersSent: true }) as any;
|
||||
|
||||
await controller.getApplicationRegistrationFileById(
|
||||
await controller.getApplicationRegistrationAsset(
|
||||
mockResponse,
|
||||
'file-123',
|
||||
createAssetRequest(),
|
||||
'registration-id',
|
||||
);
|
||||
|
||||
expect(mockResponse.destroy).toHaveBeenCalledTimes(1);
|
||||
|
||||
+18
-14
@@ -16,7 +16,6 @@ import { type Readable } from 'stream';
|
||||
import { Request, Response } from 'express';
|
||||
import { FileFolder, ServerFileFolder } from 'twenty-shared/types';
|
||||
|
||||
import { SERVER_FILE_STORAGE_PREFIX } from 'src/engine/core-modules/file-storage/constants/server-file-storage-prefix.constant';
|
||||
import {
|
||||
FileStorageException,
|
||||
FileStorageExceptionCode,
|
||||
@@ -48,27 +47,32 @@ export class FileController {
|
||||
private readonly serverFileStorageService: ServerFileStorageService,
|
||||
) {}
|
||||
|
||||
// Serves application registration assets (logo, gallery images). These are
|
||||
// instance-global marketplace resources, also displayed on the public OAuth
|
||||
// authorize page, hence no auth token. The /server/ segment separates
|
||||
// instance-global server files from the workspace-scoped /file/:folder/:id.
|
||||
@Get(`file/${SERVER_FILE_STORAGE_PREFIX}/application-registration/:id`)
|
||||
// Serves application registration assets (logo, gallery images) by their
|
||||
// public folder path. These are instance-global marketplace resources, also
|
||||
// displayed on the public OAuth authorize page, hence no auth token, unlike
|
||||
// the workspace-scoped /file/:folder/:id.
|
||||
@Get('files/application-registrations/:applicationRegistrationId/*path')
|
||||
@UseGuards(PublicEndpointGuard, NoPermissionGuard)
|
||||
async getApplicationRegistrationFileById(
|
||||
async getApplicationRegistrationAsset(
|
||||
@Res() res: Response,
|
||||
@Param('id') fileId: string,
|
||||
@Req() req: Request,
|
||||
@Param('applicationRegistrationId') applicationRegistrationId: string,
|
||||
) {
|
||||
const filepath = join(...req.params.path);
|
||||
|
||||
let fileResponse: { stream: Readable; mimeType: string };
|
||||
|
||||
try {
|
||||
fileResponse = await this.serverFileStorageService.readServerFileById(
|
||||
fileId,
|
||||
ServerFileFolder.ApplicationRegistration,
|
||||
);
|
||||
fileResponse = await this.serverFileStorageService.readServerFile({
|
||||
fileFolder: ServerFileFolder.ApplicationRegistration,
|
||||
applicationRegistrationId,
|
||||
resourcePath: filepath,
|
||||
});
|
||||
} catch (error) {
|
||||
if (
|
||||
error instanceof FileStorageException &&
|
||||
error.code === FileStorageExceptionCode.FILE_NOT_FOUND
|
||||
(error.code === FileStorageExceptionCode.FILE_NOT_FOUND ||
|
||||
error.code === FileStorageExceptionCode.ACCESS_DENIED)
|
||||
) {
|
||||
throw new FileException(
|
||||
'File not found',
|
||||
@@ -76,7 +80,7 @@ export class FileController {
|
||||
);
|
||||
}
|
||||
|
||||
this.logger.error('readServerFileById failed unexpectedly', { error });
|
||||
this.logger.error('readServerFile failed unexpectedly', { error });
|
||||
|
||||
throw new FileException(
|
||||
'Error retrieving file',
|
||||
|
||||
Reference in New Issue
Block a user