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.

Sign up for GitHub

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

Jump to bottom

Issue #3339566 by vnech: Entity access is ignoring in "Custom content list block" block type #3305

Merged
merged 1 commit into from
Mar 8, 2023
Merged

Issue #3339566 by vnech: Entity access is ignoring in "Custom content list block" block type #3305

merged 1 commit into from
Mar 8, 2023

Conversation

nechai
Copy link
Contributor

@nechai nechai commented Feb 6, 2023 •
edited by ribel
Loading

Problem

Anonymous users can see community secret flexible group teasers in "Custom content list block" block:
alt

Should be:
alt

Solution

Add access check before entities view build.

Issue tracker

Theme issue tracker

n/a

How to test

  • Login as admin
  • Create flexible groups with different visibilities: public, community, secret
  • Create a "Custom content list block" on /block/add/custom_content_list to display created groups
  • Place block to any visible region on "Block layout" (for example, "Page title")
  • As anonymous user visit the page where this block appears

Definition of done

Before merge

  • Code/peer review is completed
  • All commit messages are clear and clean. If applicable a rebase was performed
  • All automated tests are green
  • Functional/manual tests of the acceptance criteria are approved
  • All acceptance criteria were met
  • New features or changes to existing features are covered by tests, either unit (preferably) or behat
  • Update path is tested. New hook_updates should respect update order, right naming convention and consider hook_post_update code
  • Module can be safely uninstalled. Update/implement hook_uninstall and make sure that removed configuration or dependencies are removed/uninstalled
  • This pull request has all required labels (team/type/priority)
  • This pull request has a milestone
  • This pull request has an assignee (if applicable)
  • Any front end changes are tested on all major browsers
  • New UI elements, or changes on UI elements are approved by the design team
  • New features, or feature changes are approved by the product owner

After merge

  • Code is tested on all branches that it has been cherry-picked
  • Update hook number might need adjustment, make sure they have the correct order
  • The Drupal.org ticket(s) are updated according to this pull request status

Screenshots

Before changes:
alt

After changes:
alt

Release notes

Now "Custom content list block" block displays only content (and groups) that user has access to.

Change Record

n/a

Translations

n/a

@nechai nechai added type: bug Fixes a bug in Open Social team: enterprise This PR originates from the ECI team status: needs review This pull request is waiting for a requested review prio: medium labels Feb 6, 2023
@nechai nechai added this to the 11.7.1 milestone Feb 6, 2023
@nechai nechai self-assigned this Feb 6, 2023
@mergeable
Copy link

mergeable bot commented Feb 6, 2023

Thanks for contributing towards Open Social! A maintainer from the @goalgorilla/maintainers group might not review all changes from all teams/contributors. Please don't be discouraged if it takes a while. In the meantime, we have some automated checks running and it might be that you will see our comments with some tips or requests to speed up the review process. 😊

@nechai nechai force-pushed the issue/3339566-add-entity-access-check-to-custom-content-list-block branch from 86844f6 to 8a30c02 Compare February 6, 2023 11:49
@nechai nechai force-pushed the issue/3339566-add-entity-access-check-to-custom-content-list-block branch from 8a30c02 to 5daf76a Compare February 6, 2023 17:20
@tbsiqueira tbsiqueira modified the milestones: 11.7.1, 11.7.2 Feb 7, 2023
ribel
ribel reviewed Feb 8, 2023
View reviewed changes
modules/social_features/social_content_block/src/ContentBuilder.php
@@ -162,6 +172,13 @@ static function ($field_name) use ($block_content) {
->loadMultiple($entities);

foreach ($entities as $key => $entity) {
// @todo Better to move to entity query check but
Copy link
Contributor

@ribel ribel Feb 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@nechai could you explain more (here in GitHub comment) how did you try to make it better and why it didn't work?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, this needs a bit more explaining on why this change is needed, also release notes are a bit vague. Ideally we would write tests for this, so we don't break it in the future again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I mean here that group visibility doesn't implemented on query level... To achieve this we should rewrite default group query access handler. This work require deeper investigation and refinement as can have critical impact on the whole distro.

ribel
ribel approved these changes Feb 8, 2023
View reviewed changes
Copy link
Contributor

@ribel ribel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Changes looks good to me.
Also tested it on Tugboat and all works as expected.

LU:
image

AN:
image

@ribel ribel modified the milestones: 11.7.2, 11.6.6 Feb 8, 2023
@tbsiqueira tbsiqueira modified the milestones: 11.6.6, 11.6.7 Feb 15, 2023
nkoporec
nkoporec requested changes Feb 21, 2023
View reviewed changes
modules/social_features/social_content_block/src/ContentBuilder.php
@@ -162,6 +172,13 @@ static function ($field_name) use ($block_content) {
->loadMultiple($entities);

foreach ($entities as $key => $entity) {
// @todo Better to move to entity query check but
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, this needs a bit more explaining on why this change is needed, also release notes are a bit vague. Ideally we would write tests for this, so we don't break it in the future again.

@open-social-tugboat
Copy link

Tugboat has finished building the preview for this pull request!

Link:

Dashboard:

5 similar comments
@open-social-tugboat
Copy link

Tugboat has finished building the preview for this pull request!

Link:

Dashboard:

@open-social-tugboat
Copy link

Tugboat has finished building the preview for this pull request!

Link:

Dashboard:

@open-social-tugboat
Copy link

Tugboat has finished building the preview for this pull request!

Link:

Dashboard:

@open-social-tugboat
Copy link

Tugboat has finished building the preview for this pull request!

Link:

Dashboard:

@open-social-tugboat
Copy link

Tugboat has finished building the preview for this pull request!

Link:

Dashboard:

@tbsiqueira tbsiqueira modified the milestones: 11.6.7, 11.6.8 Mar 1, 2023
@nechai
Copy link
Contributor Author

nechai commented Mar 7, 2023

@nkoporec I agree that would be nice to have behat tests for that feature but as there are a lot of cases that need to be covered (custom content list block can display any content entity in OS) I suggest to create a follow up for tests (currently I don't have a time to work on it).
Does this work for you?

@nkoporec
Copy link
Contributor

nkoporec commented Mar 8, 2023

@nechai yeah I think creating a follow up is a good plan indeed. Can you address the comments from @ribel ? Once we clarify that, we can then merge it if everything looks good.

@nechai
Copy link
Contributor Author

nechai commented Mar 8, 2023

@ribel What do you think regarding this?

@ribel
Copy link
Contributor

ribel commented Mar 8, 2023

Hi @nechai and @nkoporec
I've created follow up ticket for adding behat tests and add it to our backlog: https://getopensocial.atlassian.net/browse/PROD-24500
Explanations and release notes looks clear to me now.

@nkoporec nkoporec merged commit bfefd5d into main Mar 8, 2023
@nkoporec nkoporec deleted the issue/3339566-add-entity-access-check-to-custom-content-list-block branch March 8, 2023 17:20
@nkoporec
Copy link
Contributor

nkoporec commented Mar 8, 2023

🍒 picked to 11.6.x, 11.7.x, 11.8.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ribel ribel ribel approved these changes

@nkoporec nkoporec nkoporec approved these changes

@tbsiqueira tbsiqueira Awaiting requested review from tbsiqueira

Assignees

@nechai nechai

Labels
prio: medium status: needs review This pull request is waiting for a requested review team: enterprise This PR originates from the ECI team type: bug Fixes a bug in Open Social
Milestone
11.6.8
5 participants
@nechai @open-social-tugboat @nkoporec @ribel @tbsiqueira