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

Issue #3325955 by navneet0693: Profile pictures missing on landing page content type. #3237

Merged
merged 1 commit into from
Jan 24, 2023

Conversation

navneet0693
Copy link
Collaborator

@navneet0693 navneet0693 commented Dec 8, 2022

Problem

The user images were not being displayed on the when profile entity is somehow added to landing_page content type, for example, having a featured section in a landing page having profile entity display.

Solution

  1. https://github.com/goalgorilla/open_social/blob/main/modules/social_features/social_profile/social_profile.module#L727 called for access check on File entity.
  2. This called https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/modules/file/src/FileAccessControlHandler.php#L21
  3. In the function mentioned above, the $entity_and_field_access = $referencing_entity->access('view', $account, TRUE)->andIf($referencing_entity->$field_name->access('view', $account, TRUE)); resulted in AccessResultAllowed
  4. This line checked for access on both File $referencing_entity->$field_name->access('view', $account, TRUE) and Profile $referencing_entity->access('view', $account, TRUE)
  5. Now, File access check returned false but Profile access check returned TRUE which shouldn’t be the access as our user is anonymous.
  6. On further digging, Profile entity access check was passed through https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php#L61
  7. This called for invoking of alter hooks in $this->moduleHandler()->invokeAll('entity_access', [$entity, $operation, $account]) and $this->moduleHandler()->invokeAll($entity->getEntityTypeId() . '_access', [$entity, $operation, $account])
  8. We have written social_profile_profile_access which was introduced in
    Issue #3197235 by SV: Display featured profile section for anonymous users #2224
  9. This checked for current node type and provided access for profiles to anonymous users.
  10. This made the statement mentioned in step 3 return “Allowed” instead of “Neutral” which then failed in the https://github.com/goalgorilla/open_social/blob/main/modules/social_features/social_profile/social_profile.module#L727
  11. On landing_page, where this code mentioned in social_profile_profile_access is called and due to which the reported behavior is seen.
  12. So, we added checks on basis of current user status and file system to avoid this conflict.

Issue tracker

https://www.drupal.org/project/social/issues/3325955

Theme issue tracker

N.A

How to test

  • Using the latest version of Open Social with the social_landing_page and social_private_file social_file_private modules enabled
  • Define $settings['file_private_path'] in settings.php
  • As a site manager, create a landing page, add "featured content" section and select any profile entities
  • When I open the created page as anonymous I expect to see added profiles with default profile images shown.

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

N.A

Release notes

When the website was using private file, then the profile images appear to be broken on landing pages for Anonymous users, if a featured content or a stream view was added. We resolved the access problem for anonymous users and now the default profile picture will be displayed.

Change Record

N.A

Translations

N.A

@navneet0693 navneet0693 added type: bug Fixes a bug in Open Social status: needs review This pull request is waiting for a requested review prio: high team: gravitiers labels Dec 8, 2022
@navneet0693 navneet0693 added this to the 11.4.11 milestone Dec 8, 2022
@mergeable
Copy link

mergeable bot commented Dec 8, 2022

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. 😊

@tbsiqueira tbsiqueira modified the milestones: 11.4.11, 11.4.12 Dec 13, 2022
@tbsiqueira tbsiqueira modified the milestones: 11.4.12, 11.5.5 Dec 20, 2022
@tbsiqueira tbsiqueira modified the milestones: 11.5.5, 11.5.6, 11.5.7 Jan 3, 2023
@tbsiqueira tbsiqueira modified the milestones: 11.5.7, 11.5.8 Jan 17, 2023
@zanvidmar zanvidmar self-assigned this Jan 20, 2023
Copy link
Contributor

@zanvidmar zanvidmar left a comment

Choose a reason for hiding this comment

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

I can approve that with a fix provided the described issue has been resolved. However some other test scenarios (logged in as authenticated user that is not verified user still fails)

@zanvidmar zanvidmar self-requested a review January 20, 2023 11:47
…ge content type.

The user images were not being displayed on the when profile entity is somehow added to landing_page content type, for example, having a featured section in a landing page having profile entity display.

Reason:

https://github.com/goalgorilla/open_social/blob/main/modules/social_features/social_profile/social_profile.module#L727 called for access check on File entity.

This called https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/modules/file/src/FileAccessControlHandler.php#L21

In the function mentioned above, the $entity_and_field_access = $referencing_entity->access('view', $account, TRUE)->andIf($referencing_entity->$field_name->access('view', $account, TRUE)); resulted in AccessResultAllowed

This line checked for access on both File $referencing_entity->$field_name->access('view', $account, TRUE) and Profile $referencing_entity->access('view', $account, TRUE)

Now, File access check returned false but Profile access check returned TRUE which shouldn’t be the access as our user is anonymous.

On further digging, Profile entity access check was passed through https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php#L61

This called for invoking of alter hooks in $this->moduleHandler()->invokeAll('entity_access', [$entity, $operation, $account]) and $this->moduleHandler()->invokeAll($entity->getEntityTypeId() . '_access', [$entity, $operation, $account])

We have written social_profile_profile_access which was introduced in #2224

This checked for current node type and provided access for profiles to anonymous users.

This made the statement mentioned in step 3 return “Allowed” instead of “Neutral” which then failed in the https://github.com/goalgorilla/open_social/blob/main/modules/social_features/social_profile/social_profile.module#L727

On landing_page, where this code mentioned in social_profile_profile_accessis called and due to which the reported behavior is seen.

Solution:

We added checks on basis of current user status and file system to avoid this conflict.
@tbsiqueira tbsiqueira merged commit d5f36e6 into main Jan 24, 2023
@tbsiqueira tbsiqueira deleted the issue/3325955-profile-images branch January 24, 2023 12:01
@tbsiqueira
Copy link
Contributor

Cherry-picked to 11.5.x 11.6.x and 11.7.x

@tbsiqueira tbsiqueira removed the status: needs review This pull request is waiting for a requested review label Jan 24, 2023
@tbsiqueira tbsiqueira added the backport: verified This pull request has been back ported to an older minor version label Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport: verified This pull request has been back ported to an older minor version prio: high type: bug Fixes a bug in Open Social
3 participants