Skip to content

Conversation

carlobeltrame
Copy link
Member

@carlobeltrame carlobeltrame commented Sep 4, 2025

Extension of #8004, which fulfils the following requirement:

  • Prevent access to camp collaborators related data for non-role-users. (team, personal material lists, dayresponsibles, activityresponsibles, comments)

As per discussion, first merge #8004 and check the performance, then maybe merge this and check the performance again.

To be discussed / decided / done:

  • Are there big performance impacts due to this change with a production amount of data?
  • Is adding a second DB view okay, or should we go back to using joins for some of the DB queries?
  • Is the implementation of the filter logic in MaterialListRepository okay? Could / should it be done more efficiently than two nested OR statements?
  • Are there more relations / embedded entities which need to be filtered?
  • Is the relation security feature the right way to output different values based on user permissions? (It's the only way I found to make logic in an Entity depend on the current user permissions)
  • Personal material lists are filtered out in the API endpoint. Is it okay / correct that items in that material list are still accessible, with the material list link set to null? Is the security-conditional getter on MaterialItem the correct place to implement this, or could this somehow be done in the Doctrine query builder in the FiltersByUserExtension?
  • Check if filtering can be circumvented using nuxt print

@carlobeltrame carlobeltrame force-pushed the shared-camps-with-filters branch 5 times, most recently from 42c095b to e163435 Compare September 6, 2025 02:43
@carlobeltrame carlobeltrame force-pushed the shared-camps-with-filters branch 4 times, most recently from dca3ec0 to 13e558d Compare October 3, 2025 22:34
Follow-up for ecamp#8004

Querying by isPrototype OR isShared is slow, because databases don't
like OR conditions too much. We work around this by adding a combined
isPublic field which is enforced to always contain isShared || isPrototype
and have the DB queries filter by that.
@carlobeltrame carlobeltrame force-pushed the shared-camps-with-filters branch from 13e558d to f488958 Compare October 4, 2025 11:11
security: 'is_granted("CAMP_COLLABORATOR", camp) or
is_granted("CAMP_IS_SHARED", camp) or
is_granted("CAMP_IS_PROTOTYPE", camp)'
is_granted("CAMP_IS_PUBLIC", camp)'
Copy link
Member

Choose a reason for hiding this comment

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

When /camps/camp-id/camp_collaborations is accessed, and the camp has set 'isPublic', then this endpoint may be displayed, but as an empty array.
Am I correctly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly. This allows the frontend to "fail" gracefully. If someone visits the team page (even though it's not in the menu), they will see an empty list

$publicCampsQry = $queryBuilder->getEntityManager()->createQueryBuilder();
$publicCampsQry->select('c');
$publicCampsQry->from(Camp::class, 'c');
$publicCampsQry->where($queryBuilder->expr()->orX('c.isPrototype = true', 'c.isShared = true'));
Copy link
Member

Choose a reason for hiding this comment

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

Please use isPublic

Comment on lines +28 to +29
$queryBuilder->innerJoin("{$activity}.camp", 'camp');
$this->filterByCampCollaboration($queryBuilder, $user);
Copy link
Member

Choose a reason for hiding this comment

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

Why is this join necessary?

Comment on lines +38 to +47
$queryBuilder->andWhere(
$queryBuilder->expr()->orX(
$queryBuilder->expr()->andX(
"{$rootAlias}.campCollaboration IS NULL",
$queryBuilder->expr()->in("{$rootAlias}.camp", $publicCampsQry->getDQL())
),
$queryBuilder->expr()->in("{$rootAlias}.camp", $campsQry->getDQL())
)
);
$queryBuilder->setParameter('current_user', $user);
Copy link
Member

Choose a reason for hiding this comment

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

This could become a performance issue.
If that is the case, we will have to create our own view.

for each user ID, all visible materialist IDs:
[user] * [non-personal materiallists of public camps]
union
[user] -< [campcollaboration] -> [camp] -< [materiallist]

Copy link
Member

Choose a reason for hiding this comment

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

Actually, the discussion about the reliability of material_list.camp_collaboration is still open.

see:
#5537 | #5537 (comment)

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.

2 participants