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

On separate admin domain, reviews are not saved with correct store ID #17510

Closed
CNanninga opened this issue Aug 10, 2018 · 14 comments · Fixed by #32259
Closed

On separate admin domain, reviews are not saved with correct store ID #17510

CNanninga opened this issue Aug 10, 2018 · 14 comments · Fixed by #32259
Assignees
Labels
Component: Review Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Priority: P1 Once P0 defects have been fixed, a defect having this priority is the next candidate for fixing. Progress: done Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release Reproduced on 2.4.x The issue has been reproduced on latest 2.4-develop branch Severity: S2 Major restrictions or short-term circumventions are required until a fix is available.

Comments

@CNanninga
Copy link

CNanninga commented Aug 10, 2018

Preconditions

  1. Magento 2.2.5
  2. Have only a single store configured on the install
  3. VERY IMPORTANT: Admin is on a different domain than the front-end store. e.g., front-end is www.example.com, but admin.example.com is a domain that's been configured to load the admin view.
  4. Set the value of MAGERUN_TYPE to "store" and the value of MAGE_RUNCODE to "admin" via server configuration

Steps to reproduce

  1. Place a review for a product on the front-end, which will initially go into Pending status.
  2. The review is associated appropriately with store ID 1 (and 0)
  3. In the admin, view the review, mark as Approved, and save.

Expected result

  1. The review remains associated with store ID 1 (i.e., its "visibility", based on having a record in the table review_store)

Actual result

  1. The record in review_store associating the review with store ID 1 is deleted, leaving only a record for store 0, and the store no longer has visibility in any front-end store.

Additional information

Under the hood, this is a result of the user's "store" cookie being used (incorrectly) to derive the appropriate store context in many areas of the admin.

In \Magento\Review\Block\Adminhtml\Edit\Form::prepareForm, a check is made for whether there is only a single store on the Magento install. If not, a visible stores multi-select is added to the form, and this issue does not occur. If it _is a single store install, only a hidden input with the store ID is added to the form.

The value given to this hidden input is not derived from the store actually associated with the review. It is derived from \Magento\Store\Model\StoreManager::getStore (with no store ID passed in), which in turn gets the store ID from \Magento\Store\Model\StoreResolver::getCurrentStoreId. This method gets the store ID from a request param if it exists (it doesn't in this case), or else gets it from the "store" cookie.

If the admin is on the same domain as the front-end (e.g., www.example.com/backend), this ends up working. If the admin is on a different domain and therefore the Magento run code has been directly set to the admin store code, this cookie has the ID 0.

@magento-engcom-team
Copy link
Contributor

Hi @CNanninga. Thank you for your report.
To help us process this issue please make sure that you provided the following information:

  • Summary of the issue
  • Information on your environment
  • Steps to reproduce
  • Expected and actual results

Please make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, please, add a comment to the issue:

@magento-engcom-team give me {$VERSION} instance

where {$VERSION} is version tags (starting from 2.2.0+) or develop branches (2.2-develop +).
For more details, please, review the Magento Contributor Assistant documentation.

@CNanninga do you confirm that you was able to reproduce the issue on vanilla Magento instance following steps to reproduce?

  • yes
  • no

@magento-engcom-team magento-engcom-team added the Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed label Aug 10, 2018
@engcom-backlog-nickolas

Hello @CNanninga, thank you for your report. I cannot reproduce this issue.
peek 2018-08-13 13-54
Could you please correct my steps, or add more details?

@engcom-backlog-nickolas

@CNanninga, we are closing this issue due to inactivity. If you'd like to update it, please reopen the issue.

@hostep
Copy link
Contributor

hostep commented Aug 27, 2018

@engcom-backlog-nickolas: can you please test this again, by not using a subdomain for the admin domain, but an actually different domain? Subdomains inherit the cookies from the main domain, so in this case the bug probably won't trigger.

(I'm not saying the bug exists, I'm just saying you didn't follow the exact steps to reproduce, because in the example of @CNanninga he didn't use a subdomain)

@CNanninga
Copy link
Author

@engcom-backlog-nickolas - I'd also point out that it would be helpful if your demonstration could also include explicitly checking the value of the "store" cookie. If it's "admin" when you are editing the review in the admin, I believe the bug will occur. (We actually did use a sub-domain for admin, but our Nginx configuration is set to explicitly set a Magento run code for the admin store on that sub-domain, so we do end up with a different cookie value, specifically associated with the admin.site.com domain.)

Haven't yet had a chance to do all the local setup required (including the aforementioned Nginx configuration) to verify this on a vanilla install. And I don't think getting an auto-deployed instance via the Git comment would help, as I assume there wouldn't be a way to set it up with a separate admin domain.

@CNanninga CNanninga reopened this Aug 27, 2018
@ghost ghost self-assigned this Aug 29, 2018
@ghost
Copy link

ghost commented Aug 29, 2018

@CNanninga, we cannot reproduce the issue by the steps you provided.
1

@CNanninga
Copy link
Author

@engcom-backlog-pb - Thanks for the demo. Clearly in your reproduction steps, the value of your store cookie is still "default" even though you're browsing the separate admin domain. How do you have your server configuration configured to accomplish this? We have Nginx configuration for the admin-specific domain that sets the environment variable MAGE_RUN_TYPE to "store" and MAGE_RUN_CODE to "admin." This results in the cookie value being "admin." (And I'd add that it doesn't seem like having a store cookie with the value "admin" when browsing the admin should be considered an invalid state.)

@ghost
Copy link

ghost commented Aug 29, 2018

@CNanninga, I'm using Apache, not nginx, and I cannot help you with nginx configuration. Anyway, the problem with nginx is not a Magento 2 core issue, so I believe this ticket should be closed.

@CNanninga
Copy link
Author

@engcom-backlog-pb - You don't need to give me Nginx-specific instructions. I would still very much appreciate you elaborating on how you are setting up the separate admin domain to resolve to the admin. Are you setting MAGE_RUN_TYPE and MAGE_RUN_CODE environment variables to the admin store, or using some other mechanism?

@ghost
Copy link

ghost commented Aug 30, 2018

Are you setting MAGE_RUN_TYPE and MAGE_RUN_CODE environment variables to the admin store, or using some other mechanism?

@CNanninga, I have made no specific settings for admin store. Only changed ServerName for VirtualHost.

@CNanninga
Copy link
Author

@engcom-backlog-pb - Then there are some additional reproduction steps. We are not just pointing two different domains to the same install and just browsing to /backend on one but not the other. We are locking down the admin domain so that it will always (and only) load the admin.

For full reproduction steps, please make sure you have configuration like the following in the array in env.php:

'system' => [ 'websites' => [ 'admin' => [ 'web' => [ 'unsecure' => [ 'base_url' => 'https://admin.example.com/', 'base_link_url' => 'https://admin.example.com/', 'base_static_url' => 'https://admin.example.com/static/', 'base_media_url' => 'https://admin.example.com/media/' ], 'secure' => [ 'base_url' => 'https://admin.example.com/', 'base_link_url' => 'https://admin.example.com/', 'base_static_url' => 'https://admin.example.com/static/', 'base_media_url' => 'https://admin.example.com/media/' ] ] ] ]

… and set the value of MAGE_RUN_TYPE to "store" and the value of MAGE_RUN_CODE to "admin" via server configuration (it doesn't matter whether it's Nginx or Apache).

@ghost ghost removed the Progress: needs update label Aug 31, 2018
@ghost
Copy link

ghost commented Aug 31, 2018

@CNanninga, thank you for update.
We've acknowledged the issue and added to our backlog.

@ghost ghost added Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release Component: Review Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development labels Aug 31, 2018
@ghost ghost removed their assignment Aug 31, 2018
@magento-engcom-team magento-engcom-team added Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Severity: S2 Major restrictions or short-term circumventions are required until a fix is available. labels Nov 30, 2020
@gabrieldagama gabrieldagama added Priority: P1 Once P0 defects have been fixed, a defect having this priority is the next candidate for fixing. and removed Priority: P2 A defect with this priority could have functionality issues which are not to expectations. labels Feb 11, 2021
@engcom-Kilo engcom-Kilo self-assigned this Feb 23, 2021
@m2-assistant
Copy link

m2-assistant bot commented Feb 23, 2021

Hi @engcom-Kilo. Thank you for working on this issue.
Looks like this issue is already verified and confirmed. But if you want to validate it one more time, please, go though the following instruction:

    1. Add/Edit Component: XXXXX label(s) to the ticket, indicating the components it may be related to.
    1. Verify that the issue is reproducible on 2.4-develop branch
      Details- Add the comment @magento give me 2.4-develop instance to deploy test instance on Magento infrastructure.
      - If the issue is reproducible on 2.4-develop branch, please, add the label Reproduced on 2.4.x.
      - If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!
    1. If the issue is not relevant or is not reproducible any more, feel free to close it.

@engcom-Bravo engcom-Bravo added Reproduced on 2.4.x The issue has been reproduced on latest 2.4-develop branch Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed and removed Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed labels Feb 23, 2021
@magento-engcom-team
Copy link
Contributor

✅ Confirmed by @engcom-Bravo
Thank you for verifying the issue. Based on the provided information internal tickets MC-41003 were created

Issue Available: @engcom-Bravo, You will be automatically unassigned. Contributors/Maintainers can claim this issue to continue. To reclaim and continue work, reassign the ticket to yourself.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Review Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Priority: P1 Once P0 defects have been fixed, a defect having this priority is the next candidate for fixing. Progress: done Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release Reproduced on 2.4.x The issue has been reproduced on latest 2.4-develop branch Severity: S2 Major restrictions or short-term circumventions are required until a fix is available.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

9 participants