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

Validate API credentials using the Parse.ly API validation endpoint #1897

Merged
merged 21 commits into from
Oct 11, 2023

Conversation

vaurdan
Copy link
Contributor

@vaurdan vaurdan commented Oct 3, 2023

Description

Validate the API credentials by doing a API request to the validation endpoint. This assures that only valid credentials can be saved on the Settings page.

Motivation and context

Fixes #1853

How has this been tested?

Tested on my local environment and with three new integration tests.

Screenshots (if appropriate)

image

@vaurdan vaurdan added Type: Enhancement Status: Needs Review Component: PHP Pull requests that update PHP code php Pull requests that update Php code labels Oct 3, 2023
@vaurdan vaurdan added this to the 3.11.0 milestone Oct 3, 2023
@vaurdan vaurdan requested a review from a team as a code owner October 3, 2023 12:28
@vaurdan
Copy link
Contributor Author

vaurdan commented Oct 3, 2023

Adding a "Do Not Merge" label as there is a problem with the e2e tests that need to be addressed. Looking into it.

@vaurdan
Copy link
Contributor Author

vaurdan commented Oct 3, 2023

Removing the Do Not Merge label as the E2E issues were addressed.

The issue was with testing with API credentials that aren't valid, since those are checked against the validate endpoint. I added a GET parameter called e2e_parsely_skip_api_validate that when is present in the settings page and set to y, it should skip the HTTP request to the validate endpoint and assume they are correct.

@acicovic
Copy link
Collaborator

acicovic commented Oct 4, 2023

Hey there! Thank you for your work!

I haven't looked into the code just yet, but using the UI I noticed that it seems the settings page won't allow saving an empty Site ID or API Secret, which should be possible (especially for the API Secret since not all customers may have API access). You can easily see this if you've got a filled API Secret and then try to empty it.

I'd suggest looking how develop behaves with saving empty credentials and replicate the same behavior in this branch, unless we've got a reason to change it. In this case we can discuss it! 🙂

@vaurdan
Copy link
Contributor Author

vaurdan commented Oct 4, 2023

@acicovic ah nice catch. I had it working like that previously but I think I accidentally reverted it when I amended a commit. Pushed the changes that should allow empty Site ID and Secret :)

Copy link
Collaborator

@acicovic acicovic left a comment

Choose a reason for hiding this comment

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

Excellent work! The effort that you've made to stick to project conventions is apparent and greatly appreciated!

Left some questions and a couple of minor adjustment proposals. This review is extremely super picky by the way and mostly focuses on details that are trivial.

Thank you so much!

src/class-validator.php Outdated Show resolved Hide resolved
src/RemoteAPI/class-validate-api.php Outdated Show resolved Hide resolved
src/RemoteAPI/class-validate-api.php Show resolved Hide resolved
src/RemoteAPI/class-validate-api.php Outdated Show resolved Hide resolved
tests/Integration/UI/SettingsPageTest.php Outdated Show resolved Hide resolved
views/parsely-settings.php Show resolved Hide resolved
src/RemoteAPI/class-remote-api-base.php Outdated Show resolved Hide resolved
@vaurdan vaurdan requested a review from acicovic October 4, 2023 15:14
@acicovic
Copy link
Collaborator

acicovic commented Oct 4, 2023

@vaurdan, I had a second look. Thanks for addressing the feedback and replying. If you agree to wipe out the non-API validations, please go ahead and I can take a final look.

Thanks! 🙂

@vaurdan
Copy link
Contributor Author

vaurdan commented Oct 6, 2023

@acicovic the older methods have been removed in favor of the new validate_api_credentials. Let me know your thoughts and feedback!

Copy link
Collaborator

@acicovic acicovic left a comment

Choose a reason for hiding this comment

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

Great work! Left some comments and questions, mostly details.

src/RemoteAPI/class-validate-api.php Outdated Show resolved Hide resolved
src/UI/class-settings-page.php Outdated Show resolved Hide resolved
src/class-validator.php Outdated Show resolved Hide resolved
src/class-validator.php Outdated Show resolved Hide resolved
src/class-validator.php Outdated Show resolved Hide resolved
tests/Integration/UI/SettingsPageTest.php Outdated Show resolved Hide resolved
tests/Integration/UI/SettingsPageTest.php Outdated Show resolved Hide resolved
tests/Integration/UI/SettingsPageTest.php Outdated Show resolved Hide resolved
tests/Integration/UI/SettingsPageTest.php Outdated Show resolved Hide resolved
tests/Integration/UI/SettingsPageTest.php Outdated Show resolved Hide resolved
Copy link
Collaborator

@acicovic acicovic left a comment

Choose a reason for hiding this comment

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

Thanks for working on this and bearing with me 🙂

@vaurdan vaurdan merged commit 7c95a67 into develop Oct 11, 2023
37 checks passed
@vaurdan vaurdan deleted the add/api-credentials-validation branch October 11, 2023 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Added Component: PHP Pull requests that update PHP code php Pull requests that update Php code Type: Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Settings page: Enhance credentials validation
2 participants