Skip to content
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

feat: files visiblity with file configuration #10438

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

SujithThirumalaisamy
Copy link
Contributor

Ref: #10404

  • Added FileFolderConfig with isPublic key.
  • Updated file-path-guard.ts to ignoreExpiration to validate the token if isPublic is true.
  • Token verification ignores expiration, assuming it's used to fetch file metadata with a required workspaceId as we cannot remove the token as we will loose the workspaceId.

@SujithThirumalaisamy
Copy link
Contributor Author

Sorry for the merges. Forgot to check my git global config :)

@SujithThirumalaisamy SujithThirumalaisamy marked this pull request as draft February 24, 2025 12:43
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR implements file visibility configuration to handle public assets like workspace logos, particularly addressing token expiration issues in emails.

  • Added FileFolderConfig type in file-folder.interface.ts with isPublic flag to control folder accessibility
  • Modified FilePathGuard to ignore token expiration for public folders while maintaining workspaceId validation
  • Added checkFileFolder utility function to validate folder paths, though with some redundant logic
  • Improved board column layout by repositioning the 'new record' button outside draggable components

💡 (2/5) Greptile learns from your feedback when you react with 👍/👎!

4 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines +52 to +65
export const checkFileFolder = (filePath: string): FileFolder => {
const allowedFolders = Object.values(FileFolder).map((value) =>
kebabCase(value),
);

const sanitizedFilePath = filePath.replace(/\0/g, '');
const [rootFolder] = sanitizedFilePath.split('/');

if (!allowedFolders.includes(rootFolder as AllowedFolders)) {
throw new BadRequestException(`Folder ${rootFolder} is not allowed`);
}

return rootFolder as FileFolder;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Duplicates folder validation logic from checkFilePath. Consider extracting common validation into a shared function.

);

const sanitizedFilePath = filePath.replace(/\0/g, '');
const [rootFolder] = sanitizedFilePath.split('/');
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: No validation for empty filePath or malformed paths that could cause rootFolder to be undefined

Suggested change
const [rootFolder] = sanitizedFilePath.split('/');
if (!sanitizedFilePath) {
throw new BadRequestException('File path cannot be empty');
}
const [rootFolder] = sanitizedFilePath.split('/');
if (!rootFolder) {
throw new BadRequestException('Invalid file path format');
}

Comment on lines 19 to 21
if (!query || !query['token']) {
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Still requiring token for public folders negates the purpose of making them public. Consider removing token requirement for public folders entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently the workspaceId which is required to fetch the images is in the token. So this is not feasible currently

@SujithThirumalaisamy SujithThirumalaisamy force-pushed the feat-Files-visiblity-with-file-configuration branch 2 times, most recently from 5a0d3f7 to 7382713 Compare February 24, 2025 13:02
@SujithThirumalaisamy SujithThirumalaisamy marked this pull request as ready for review February 24, 2025 13:03
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR refines the file visibility implementation by adding proper type definitions and folder configurations for public and private file access.

  • Introduced FileFolderConfig type with isPublic boolean flag in /packages/twenty-server/src/engine/core-modules/file/interfaces/file-folder.interface.ts
  • Configured public access for ProfilePicture, WorkspaceLogo, and PersonPicture folders while keeping Attachment and ServerlessFunction private
  • Potential security concern: FilePathGuard still requires tokens for public folders, which seems unnecessary and could be simplified

3 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

throw new BadRequestException(`Folder ${rootFolder} is not allowed`);
}

return rootFolder as FileFolder;
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Type casting to FileFolder may fail at runtime if rootFolder is kebab-cased. Need to reverse the kebab case transformation.

@SujithThirumalaisamy SujithThirumalaisamy force-pushed the feat-Files-visiblity-with-file-configuration branch from c3199a2 to c33b248 Compare February 24, 2025 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant