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: secure attachment download upload URL's per context #453

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

Conversation

yassinedorbozgithub
Copy link
Contributor

Motivation

The main motivation of this PR is to make attachment accessible for the owner or the user having the admin role.

Fixes #422

Checklist:

  • I have performed a self-review of my own code
  • New and existing unit tests pass locally with my changes

@yassinedorbozgithub yassinedorbozgithub added the enhancement New feature or request label Dec 16, 2024
@yassinedorbozgithub yassinedorbozgithub self-assigned this Dec 16, 2024
@yassinedorbozgithub yassinedorbozgithub marked this pull request as ready for review December 16, 2024 15:36
api/src/attachment/attachment.module.ts Outdated Show resolved Hide resolved
api/src/attachment/schemas/attachment.schema.ts Outdated Show resolved Hide resolved
api/src/attachment/schemas/attachment.schema.ts Outdated Show resolved Hide resolved
@yassinedorbozgithub yassinedorbozgithub force-pushed the 422-spike-reseach-possible-design-about-how-to-deal-with-access-to-attachments branch from 6b400c9 to 5c1e506 Compare December 18, 2024 10:23
@yassinedorbozgithub yassinedorbozgithub force-pushed the 422-spike-reseach-possible-design-about-how-to-deal-with-access-to-attachments branch from 5c1e506 to 4086c0e Compare December 18, 2024 10:24
@yassinedorbozgithub yassinedorbozgithub changed the title 422 spike reseach possible design about how to deal with access to attachments feat: secure attachment download upload URL's per context Dec 18, 2024
Comment on lines +55 to +87
const paths = _parsedUrl.pathname.split('/');
const modelFromPathname = paths?.[1].toLowerCase() as TModel;
const isAttachmentUrl =
modelFromPathname === 'attachment' &&
['upload', 'download'].includes(paths?.[2]);

if (!user && !isAttachmentUrl) {
throw new UnauthorizedException();
}
if (!session?.cookie || session.cookie.expires < new Date()) {
throw new UnauthorizedException('Session expired');
}

if (isAttachmentUrl) {
// attachment
const attachmentUploadContext =
query?.context?.toString() as TContextType;
if (
method === 'POST' &&
paths?.[2] === 'upload' &&
attachmentUploadContext
)
return await this.hasRequiredUploadPermission(
user,
attachmentUploadContext,
);
else if (method === 'GET' && paths?.[2] === 'download')
return await this.hasRequiredDownloadPermission(user, paths?.[3]);
else {
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.

Let's move to the parent class in a protected method called "super.canActivate(...)" or a seperate service or class.

So that here you just call it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SPIKE] Reseach possible design about how to deal with access to attachments
2 participants