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

Enhance param validation on refreshToken #28

Merged
merged 4 commits into from
Feb 7, 2022
Merged

Conversation

knazart
Copy link
Member

@knazart knazart commented Jan 28, 2022

Resolves #27

Problem this Pull Request solves

There was a fatal if the refreshToken action had missing parameters from an API refresh response. Work here prevents that from happening.

How has this been tested

Checking the logs. Refresh occurs every 2 days.

@knazart knazart self-assigned this Jan 28, 2022
Comment on lines 845 to 846
$data[ $key ] = is_array($value) ? EED_SquareOnsiteOAuth::cleanDataArray($value) : $value;
$data[ $key ] = in_array($key, $sensitive_data) ? '***' : $value;
Copy link
Member

Choose a reason for hiding this comment

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

this second line is overwriting the previous one and therefore negating what it has done

what about something like:

Suggested change
$data[ $key ] = is_array($value) ? EED_SquareOnsiteOAuth::cleanDataArray($value) : $value;
$data[ $key ] = in_array($key, $sensitive_data) ? '***' : $value;
$value = is_array($value) ? EED_SquareOnsiteOAuth::cleanDataArray($value) : $value;
$data[ $key ] = in_array($key, $sensitive_data) ? '***' : $value;

Copy link
Member

@tn3rb tn3rb left a comment

Choose a reason for hiding this comment

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

may have found an issue with some variable assignments
so please have a look and confirm

@knazart knazart requested a review from tn3rb January 28, 2022 21:37
Copy link
Member

@tn3rb tn3rb left a comment

Choose a reason for hiding this comment

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

cool 😎

@lorenzocaum
Copy link
Contributor

Hey Nazar, can we start on testing with this pull request?

@knazart
Copy link
Member Author

knazart commented Jan 31, 2022

Yes Lorenzo, please do. Although don't think you can reproduce without messing up the code on purpose.

@lorenzocaum
Copy link
Contributor

Testing sites with Event Espresso 4.10.26.p and the 27/param-validation branch.

I connected both through live mode.

I set a Slack reminder to check back on Wednesday afternoon.

@lorenzocaum
Copy link
Contributor

I checked both sites here (#28 (comment)) and Square is still activated.

I'm not exactly sure when the refreshtoken runs on either site so I'll check in the morning again. That way, I'm certain it has run since two days will have passed.

@knazart
Copy link
Member Author

knazart commented Feb 2, 2022

There should be a log Refresh the token in the Payment methods section. It logs every time the token is refreshed.

@lorenzocaum
Copy link
Contributor

Hey Nazar, thanks for the tip. I see a couple of entries on the first site from today and both are for domain status verified. The second site doesn't have anything for today.

@lorenzocaum
Copy link
Contributor

Hey Nazar, I rechecked both sites again today.

Both are still connected to Square and there isn't any fatal errors.

I checked the payment logs about the entry that should be made and I don't see any mentioning a refresh.

On this site (https://enzo12staging.wpengine.com), there are entries for today and yesterday. They all show this with the verified:

‹ Lorenzo Caum Site — WordPress 2022-02-03 at 3 21 57 PM

The other site doesn't have any entries for this month. Only some from end of January. (I think it may be because its an internal site and doesn't get traffic to trigger the cron.)

Do we have enough info to move forward with merging this in?

@lorenzocaum
Copy link
Contributor

I checked these sites again:

Both showed this:

‹ Lorenzo Caum Site — WordPress 2022-02-04 at 2 38 28 PM

I have been testing Square on MakeEventsNotWar so I'm not sure if that caused a problem since I'm using the same account.

@knazart
Copy link
Member Author

knazart commented Feb 7, 2022

Lorenzo Hi,

You need to check the logs. Or please give me access to those sites.
But yes, it can cause issues if you use the same test account at the same time.

@lorenzocaum
Copy link
Contributor

Hi Nazar, I invited you to the sites below through your email ([email protected]):

Let me know if you need anything else to look at the payment logs:

#28 (comment)

@knazart
Copy link
Member Author

knazart commented Feb 7, 2022

Well, the logs show that most are "This request could not be authorized."

But no info on Why... I do suspect the usage of the same account as the cause. Otherwise I don't know why those are authorized.

@lorenzocaum
Copy link
Contributor

Hi Nazar, I'm using the same Square account for testing on ES staging. I think that may have caused the not authorized.

Can we merge this in and bring it over to ES staging so I can test there?

@knazart
Copy link
Member Author

knazart commented Feb 7, 2022

On the other hand I don't see any logs about it even trying to refresh the token. And this could be because I see that on February 4 (4th) day there's already the "unauthorized" error while trying to do a health check. The token for sure didn't expire by then. So this brings me back to suspecting account usage.

@knazart
Copy link
Member Author

knazart commented Feb 7, 2022

/rebase

@knazart knazart merged commit d55aa3a into master Feb 7, 2022
@knazart knazart deleted the 27/param-validation branch February 7, 2022 18:57
eeteamcodebase pushed a commit that referenced this pull request Feb 7, 2022
…ram validation on refreshToken (#28)

* enhance param validation on refreshToken

* update the data cleaner

* remove debug code

* reuse the value"
@Pebblo Pebblo modified the milestones: 1.0.1.p, 1.0.2 Mar 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uncaught TypeError: Argument 1 passed to EED_SquareOnsiteOAuth::encryptString() must be of the type string
4 participants