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

When downloading a virtual good, check if the order was made by the current user. #12023

Merged
merged 8 commits into from Jan 8, 2019

Conversation

Projects
None yet
5 participants
@garnele007
Copy link
Contributor

garnele007 commented Jan 4, 2019

Questions Answers
Branch? develop
Description? See below.
Type? bug fix
Category? FO
BC breaks? no
Deprecations? no
How to test? Download a virtual good without being logged in / without specifying the order_id or secure_key url parameter.

Previously anyone with the correct link (correct &key=... URL parameter) could download a virtual good. Specifically:

  • If no user was logged in, and either no order ID or no secure key was specified.
  • If a user was logged in, but they used a download link from another user.

This change is Reviewable

garnele007 added some commits Jan 4, 2019

Fix invalid URL encoding
Due to the wrong URL encoding the key parameter was lost during the redirect after authentication.
Check if the order was made by the current user
Previously anyone with the correct link could download a virtual good. Specifically:
- If no user was logged in, and either no order ID or no secure key was specified.
- If a user was logged in, but they used a download link from another user.
@prestonBot

This comment has been minimized.

Copy link
Collaborator

prestonBot commented Jan 4, 2019

Hello @garnele007!

This is your first pull request on the PrestaShop project. Thank you, and welcome to this Open Source community!

@garnele007

This comment has been minimized.

Copy link
Contributor Author

garnele007 commented Jan 4, 2019

Can someone please help me with resolving the CI linter errors? I can't find any problem... Even running the PrestaShop/.github/contrib/pre-commit script doesn't report an error.

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 4, 2019

Hi @garnele007 and thanks for the contribution 😉

In order to solve CI lint errors, you can run php-cs-fixer (you need to install composer dev dependencies before): php ./vendor/bin/php-cs-fixer fix

See https://devdocs.prestashop.com/1.7/development/coding-standards/ for more details 😉

@garnele007

This comment has been minimized.

Copy link
Contributor Author

garnele007 commented Jan 4, 2019

Thanks. Now it's passing the linter. Using php-cs-fixer didn't fix the problem. I had to manually remove some trailing whitespace.

@Quetzacoalt91

This comment has been minimized.

Copy link
Member

Quetzacoalt91 commented Jan 7, 2019

Hi @garnele007,

By looking at the code you deleted, I wonder if this wasn't expected actually. What happens if the order was made by a guest account?

garnele007 added some commits Jan 7, 2019

Handle guest customers
Fixes a bug where guests weren't able to download virtual products.
Display error message
If only either the order id or the secure key are set, display an error message.
@garnele007

This comment has been minimized.

Copy link
Contributor Author

garnele007 commented Jan 7, 2019

Hi @Quetzacoalt91,

True. I didn't think about guests. I just made a commit that fixes this. Now my previously committed code only runs if the order was made by a customer with an account.

@marionf marionf added QA ✔️ and removed waiting for QA labels Jan 8, 2019

@Quetzacoalt91 Quetzacoalt91 added this to the 1.7.6.0 milestone Jan 8, 2019

@Quetzacoalt91 Quetzacoalt91 merged commit 543db29 into PrestaShop:develop Jan 8, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Quetzacoalt91

This comment has been minimized.

Copy link
Member

Quetzacoalt91 commented Jan 8, 2019

Thank you @garnele007

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