Skip to content

Entitlement verification: Not perform verification if request returns error#840

Merged
tonidero merged 1 commit into
entitlements-verificationfrom
entitlement-verification-not-verify-request-failure
Mar 8, 2023
Merged

Entitlement verification: Not perform verification if request returns error#840
tonidero merged 1 commit into
entitlements-verificationfrom
entitlement-verification-not-verify-request-failure

Conversation

@tonidero

@tonidero tonidero commented Mar 3, 2023

Copy link
Copy Markdown
Contributor

Description

We were performing signature verification when the request failed with 400/500 errors. We don't need to verify those requests since it will fail anyway. Network errors were already handled since they throw an exception that is catched later on.

@tonidero tonidero added the pr:fix A bug fix label Mar 3, 2023
val verificationResult = if (shouldSignResponse &&
nonce != null &&
RCHTTPStatusCodes.isSuccessful(responseCode)
) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Note that this means for 400/500s we will return NOT_VERIFIED as verification result. I believe this shouldn't matter since the code that handles errors later on will not let it be handled as a success, but I thought I would mention it.

@tonidero tonidero marked this pull request as ready for review March 3, 2023 13:09
@tonidero tonidero requested a review from a team March 3, 2023 13:09
@codecov

codecov Bot commented Mar 3, 2023

Copy link
Copy Markdown

Codecov Report

❗ No coverage uploaded for pull request base (entitlements-verification@746c8ce). Click here to learn what that means.
The diff coverage is n/a.

@@                     Coverage Diff                      @@
##             entitlements-verification     #840   +/-   ##
============================================================
  Coverage                             ?   82.62%           
============================================================
  Files                                ?      137           
  Lines                                ?     4500           
  Branches                             ?      586           
============================================================
  Hits                                 ?     3718           
  Misses                               ?      562           
  Partials                             ?      220           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

val verificationResult = if (shouldSignResponse && nonce != null) {
val verificationResult = if (shouldSignResponse &&
nonce != null &&
RCHTTPStatusCodes.isSuccessful(responseCode)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is skipping it for 304s too, is that intended?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm this is only filtering 4xx/5xx error codes actually. Lmk if I missed something though

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh ok, I was just looking at how isSuccessful is defined in iOS not in Android 👍🏻

@tonidero tonidero merged commit 31b3dea into entitlements-verification Mar 8, 2023
@tonidero tonidero deleted the entitlement-verification-not-verify-request-failure branch March 8, 2023 08:14
tonidero added a commit that referenced this pull request Mar 15, 2023
… error (#840)

### Description
We were performing signature verification when the request failed with
400/500 errors. We don't need to verify those requests since it will
fail anyway. Network errors were already handled since they throw an
exception that is catched later on.
@tonidero tonidero mentioned this pull request Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr:fix A bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants