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

[PB-2424] User can't details of an item shared by another member #445

Merged
merged 18 commits into from
Jan 30, 2025

Conversation

evillalba94
Copy link
Contributor

@evillalba94 evillalba94 commented Dec 24, 2024

READ

Ticket

Updates

  • A version of ancestors endpoint was created for Workspace.
  • Create token on endpoints to get shared files and folders. The token will be used to allow the user to access ancestors information in the Shared home folder in workspace.
  • Permissions Guard updated: Check is the token has isSharedItem as true to grant access user.
  • New decorator created: @IsSharedItem() <==> request.isSharedItem
  • Return the WorkspaceItemUser.creator as user
  • New SharingActionName.ViewDetails.

Important

  • Add the new value VIEW_DETAILS in the permissions table and associate it with the corresponding roles

@apsantiso
Copy link
Collaborator

apsantiso commented Dec 26, 2024

@evillalba94

Currently, Sharings Guard and Workspaces Guard are tied together, as sharings are also managed within workspaces.

To achieve your objective, you can:

  1. Add the following line to the controller.
  2. Create a migration to add this permission to the permissions table.

You can accomplish what you need by adding the next line to the controller and creating a new migration that adds this permission to the permissions table.

image

Note: This approach does not work for files/folders located in the root of the sharings section because these requests require a token (internxt-resources-token). However, this is a minor issue since the path is limited to shared/ in such cases.

REGARDING THIS WORKING ON INDIVIDUALS WITHOUT ANY TOKEN:

Requests to ancestors in individual sharings bypass permissions checks due to a bug in the query / request. You can confirm this behavior because parent folders are never returned in individual sharings, even if you are inside a children folder.

@sg-gs
Copy link
Member

sg-gs commented Jan 28, 2025

Please @evillalba94 test the migration changes most similarly to the production database state, that means: with existent rows, otherwise we face this:

Sequelize CLI [Node: 20.12.2, CLI: 6.6.2, ORM: 6.35.0]

Loaded configuration file "src/config/sequelize.js".
Using environment "production".
== 20250128142106-add-view-details-to-permissions: migrating =======

ERROR: null value in column "type" of relation "permissions" violates not-null constraint
ERROR DETAIL: Failing row contains (5e7e51fb-a19e-4771-a021-78d32e046619, 426be1ad-3ab4-40c4-a1bf-e1cdf2d768db, VIEW_DETAILS, 2025-01-28 17:48:10.176+00, 2025-01-28 17:48:10.176+00, null).

@evillalba94
Copy link
Contributor Author

Please @evillalba94 test the migration changes most similarly to the production database state, that means: with existent rows, otherwise we face this:

Sequelize CLI [Node: 20.12.2, CLI: 6.6.2, ORM: 6.35.0]

Loaded configuration file "src/config/sequelize.js".
Using environment "production".
== 20250128142106-add-view-details-to-permissions: migrating =======

ERROR: null value in column "type" of relation "permissions" violates not-null constraint
ERROR DETAIL: Failing row contains (5e7e51fb-a19e-4771-a021-78d32e046619, 426be1ad-3ab4-40c4-a1bf-e1cdf2d768db, VIEW_DETAILS, 2025-01-28 17:48:10.176+00, 2025-01-28 17:48:10.176+00, null).

The column type does not exist in the permissions table at the moment. It was replaced by the column name. I don't understand how this column (type) is requested.

There is a migration that was initially done by placing the type but then another migration drops the table and generates a replacement replacing type with name

image

@sg-gs

apsantiso
apsantiso previously approved these changes Jan 29, 2025
@@ -66,15 +66,26 @@ export class SharingPermissionsGuard implements CanActivate {
throw new ForbiddenException('Invalid token');
}

const extractData = this.getSharedDataFromRequest(request, context);
const isSharedWithMe = decoded.sharedWithUserUuid === requester.uuid;
Copy link
Collaborator

@apsantiso apsantiso Jan 29, 2025

Choose a reason for hiding this comment

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

I think you can just skip this isSharedWithMe thing as the functions isWorkspaceMemberAbleToPerfomAction and isUserAbleToPerfomAction already check this, but it is ok, just make sure it does not break anything.

@@ -87,12 +98,16 @@ export class SharingPermissionsGuard implements CanActivate {
}

if (!userIsAllowedToPerfomAction) {
return false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

just as note for the future, nestjs interpretes return false from guards as "forbidden exception". So you do not need to rewrite this kind of things

@sg-gs
Copy link
Member

sg-gs commented Jan 30, 2025

Dunno how that happened @evillalba94. This is the current situation in production, so you can adjust it (without doing any drop table, ofc)

Screenshot 2025-01-30 at 13 18 23

The name is just a label, the type is where you want to do the modification. However, both fields hold the same values. Adjust it and ping me again once the migration is again ready to be run.

@apsantiso
Copy link
Collaborator

@jvedrson @sg-gs Type is marked as "NOT NULL" in prod, so to prevent breaking our local environments, you can fetch any permission and check if it has defined the field "type" and if so, add it to the insert. We do not use to add this kind of logic inside migrations but is the best way.

…uction - check the column named type first
Copy link
Member

@sg-gs sg-gs left a comment

Choose a reason for hiding this comment

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

Sequelize CLI [Node: 20.12.2, CLI: 6.6.2, ORM: 6.35.0]

Loaded configuration file "src/config/sequelize.js".
Using environment "production".
== 20250128142106-add-view-details-to-permissions: migrating =======
== 20250128142106-add-view-details-to-permissions: migrated (0.332s)

✨  Done in 3.41s.

@evillalba94 evillalba94 merged commit 90b5d0d into master Jan 30, 2025
11 checks passed
@evillalba94 evillalba94 deleted the feat/shared-items-details branch January 30, 2025 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ready-for-preview
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants