Fix `Verify2` bug that prevents verifications from being sent

ash-jc-allen commented 1 year ago

Description

I'm currently working on writing a blog post about how to add 2FA to Laravel Apps using the Verify 2 API. I'm building out a demo app to show how it could all work.

It seems that I get an exception is thrown because $container isn't set in the ClientFactory when I try to run the following code:

use App\Models\User;
use Vonage\Client;
use Vonage\Verify2\Request\SMSRequest;

final class TwoFactorAuthService
{
    public function sendVerification(User $user): string
    {
        $smsRequest = (new SMSRequest(
            to: 'PHONE NUMBER HERE',
            brand: 'Vonage Verify Tutorial',
        ))->setLength(6);

        $result = app(Client::class)
            ->verify2()
            ->startVerification($smsRequest);
    }
}

From looking at the other client factory classes, I've made the assumption that the argument needed adding to the __invoke method, so I added that in.

After I added this, it seemed to stop that exception from being thrown, but I then ran into another issue. It looks like we're trying to access $this->vonageClient in the setClient method but it doesn't look like that can be accessed because it's not a property anywhere (from what I can see).

I made a guess and removed that line (to match the other client factory classes) to see if it would fix the error.

I think it might have fixed the error but I'm not entirely sure. It looks like doing this allowed the request to be made to the API, but I then got the following error message:

Invalid params: The value of one or more parameters is invalid. See https://www.nexmo.com/messages/Errors#InvalidParams for more information

I'm not sure if this error message is a legitimate error message being returned from the API, or has been caused by me making any changes to the client factory.

I also noticed that when I went to the URL specified in that error message that I was redirected to https://www.vonage.com/communications-apis/?icid=nexmo_rd#InvalidParams so I wasn't able to figure out anything else about the error.

So this PR might solve an issue, but I'm not 100% sure because this is the first time I've tinkered with the Verify2 API before 🙂

Motivation and Context

I'm writing a blog post that makes use of the Verify 2 feature.

How Has This Been Tested?

I've only manually tested this, but I'm happy to add automated tests if needed.

Example Output or Screenshots (if appropriate):

N/A

Types of changes

[x] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

[x] My code follows the code style of this project.
[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[ ] I have read the CONTRIBUTING document.
[ ] I have added tests to cover my changes.
[x] All new and existing tests passed.

codecov-commenter commented 1 year ago

Codecov Report

Merging #397 (45534c6) into main (b9d3b25) will increase coverage by 0.01%. The diff coverage is 0.00%.

@@             Coverage Diff              @@
##               main     #397      +/-   ##
============================================
+ Coverage     77.50%   77.51%   +0.01%     
  Complexity     1929     1929              
============================================
  Files           190      190              
  Lines          5143     5142       -1     
============================================
  Hits           3986     3986              
+ Misses         1157     1156       -1

Impacted Files	Coverage Δ
src/Verify2/ClientFactory.php	`0.00% <0.00%> (ø)`

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

SecondeJK commented 1 year ago

Verify v2 has just only been released into the SDK, so I think these are the first kind of teething issues that we'll see. I've tested this with vanilla PHP, and live results have mirrored testing results. Symfony and Laravel might add some bugs into the service container like this that will need to be addressed.

This is ticket is scheduled in for work

SecondeJK commented 1 year ago

Oh.

Oh, wow.

Yeah the problem you're seeing is I left the testing code in the live code used in the factory. Whoops.

SecondeJK commented 1 year ago

One request @ash-jc-allen : can you swap the order in the setAuthHandler method? I got that wrong: we want to try the keypair first, then basic. Otherwise this is good to go

ash-jc-allen commented 1 year ago

Hey @SecondeJK! Sweet! I'll get these changes done now 😄

ash-jc-allen commented 1 year ago

Just leaving a note here...

I've just done a quick bit of testing and it looks like I was getting an API error because my "brand" field (which I set as "Vonage Verify Tutorial") exceeded the 16-character limit.

It seems that the error message is returned from the API, but isn't displayed in the exception message that's thrown 🙂

SecondeJK commented 1 year ago

Hmm, I'll think I'll put out a change to the specs on this because we should be telling the developer that this is the case. In the meantime: thanks for the PR, in it goes!

Vonage / vonage-php-sdk-core