Add Plivo as SMS Provider #352

cdjenkins · 2020-05-31T18:50:09Z

@ChristophWurst @MorrisJobke @boppy @killerbees19

Added code to use Plivo as a SMS Provider.

boppy

Just minor thing (except for your switch of callback and src_number in Configure.php... Did this work for you?)...

doc/Admin Documentation.md

lib/Command/Configure.php

lib/Service/Gateway/SMS/Provider/PlivoConfig.php

cdjenkins · 2020-05-31T21:16:03Z

Just minor thing (except for your switch of callback and src_number in Configure.php... Did this work for you?)...
I was able to run the command occ twofactorauth:gateway:configure sms sucessfully. What I'm not able to do is build the javascript files so I can enable it in personal settings of the user to test. Do you know how to build the javascript files with package.json? If I could do that, I could do an end-to-end test as I have a Plivo phone number.

boppy · 2020-05-31T21:26:04Z

I was able to run the command occ twofactorauth:gateway:configure sms sucessfully. What I'm not able to do is build the javascript files so I can enable it in personal settings of the user to test. Do you know how to build the javascript files with package.json? If I could do that, I could do an end-to-end test as I have a Plivo phone number.

I'm the one with the dirty tricks, so my approach was simply to copy the changed files to my test-nextcloud's directory and test it there. No npm, etc... ;)

cdjenkins · 2020-05-31T21:27:17Z

Yes, that is dirty...lolol but does work.

boppy

This is only to prove that I have this petty German abilities to find something everywhere. Just give me a chance. :-D

Looks good to me!

Regarding Coding Style: I just remembered me running in the same "problem" - I'm far away from really knowing the guideline. But please don't tell anyone! ^^

lib/Command/Configure.php

cdjenkins

Updated files as per comments

cdjenkins · 2020-06-03T00:27:46Z

@boppy @ChristophWurst I just setup a development server on my VPS. Once I figure out how to generate the build folder inside the js folder, I'll test the Plivo sms verification. From what I can tell, package.json requires me to install npm. On my prod server, I can enable 2FA. On my dev server, I can't. The difference is the build folder (from what I can tell).

cdjenkins · 2020-06-03T00:44:32Z

So I decided to copy the build directory over to my dev server. I have the option now to enable!!!

One problem: I run the configure command and select plivo, enter the params, and the web interface no longer allows me to enable 2FA (i.e. no enable button). I runt the configure command again, select the defaults, and the web interface shows me the enable button.

cdjenkins · 2020-06-03T00:57:24Z

I found a small bug in my code, which once fixes, the enable button appeared on the web interface. Now I get this: Could not verify your code. Please try again. Enter your identification (e.g. phone number to start the verification)

cdjenkins · 2020-06-03T01:41:25Z

I fixed another issue. The code sent, but I never received it on my phone. @ChristophWurst @boppy Is there a way to log the URL call that is made?

cdjenkins · 2020-06-03T02:39:31Z

I'm finally at the point where the code $this->client->post gives me and error ,but $this->client->get works. However, the plivo api needs for post to work. The error I get is OCA\TwoFactorGateway\Exception\VerificationTransmissionException: could not send verification code

cdjenkins · 2020-06-03T03:13:16Z

Figured out the issue. Had an invalid param being passed to Plivo. I get the 2fa codes to my cell phone!! However, the web UI still says Could not verify your code. Please try again. Enter your identification (e.g. phone number to start the verification):

cdjenkins · 2020-06-03T09:51:55Z

@ChristophWurst @boppy I have the debug flag set to the IClient which produces additional debug info to the ajax call. Removing the flag work!!

cdjenkins · 2020-06-10T23:17:30Z

@ChristophWurst What else do I need to do so this PR can be merged?

cdjenkins · 2020-06-14T18:04:35Z

Thanks for the approval! @boppy @ChristophWurst Should we be concerned that Travis CI doesn't pass all the tests? I took a look at the error codes, and it should to be unrelated to the source code.

Also, do I need to sign-off on my code?

boppy · 2020-06-15T07:16:42Z

Should we be concerned that Travis CI doesn't pass all the tests? I took a look at the error codes, and it should to be unrelated to the source code.

While adding #346 I've also had those messages without a cause in my code. Since Christoph approved those, I think it's okay...

Also, do I need to sign-off on my code?

If you have the the tool chain at hand, I would totally recommend to sign any code you publish. And it's a 1-time-task to get this set up. You only have to touch it again if your signing key expires.

You might want to squash and sign...

@ChristophWurst I think those messages originate from the fact that nc/core#master is the 20 alpha already while 2fa has:

twofactor_gateway/appinfo/info.xml

Line 22 in 93218dd

Are there other problems you know of?

ChristophWurst

Some nitpicks

When you're done please squash the commit into one and add the sign-off :)

lib/Command/Configure.php

ChristophWurst · 2020-06-15T13:17:02Z

lib/Service/Gateway/SMS/Provider/Plivo.php

+			$this->logger->debug("api call: https://api.plivo.com/v1/Account/$authID/Message/" .print_r($apiParams,true));
+			$this->client->post("https://api.plivo.com/v1/Account/$authID/Message/", $apiParams);
+		} catch (Exception $ex) {
+			$this->logger->error($ex->getMessage());


Suggested change

$this->logger->error($ex->getMessage());

$this->logger->logException($ex, [

'message' => 'Could not send Plivo message: ' . $ex->getMessage(),

]);

then the logged line will also contain a trace

copy that. Given the 2 choices, when do you use error and when do you use logException?
@ChristophWurst @boppy

ChristophWurst · 2020-06-16T12:48:03Z

@ChristophWurst I think those messages originate from the fact that nc/core#master is the 20 alpha already while 2fa has

Right. Please bump this to 20 and add a job to the Travis matrix that tests against stable19 in a separate PR :)

cdjenkins · 2020-06-16T22:53:32Z

@ChristophWurst I think those messages originate from the fact that nc/core#master is the 20 alpha already while 2fa has

Right. Please bump this to 20 and add a job to the Travis matrix that tests against stable19 in a separate PR :)

Just to confirm, you are expecting @boppy to update the XML and Travis or myself?

…rray

cdjenkins

I've reviewed, updated, and posted a new commit

cdjenkins · 2020-06-16T23:18:53Z

I merged in the most recent changes from nextcloud/twofactor_gateway/master. I'll do another test on my dev server to be sure I can:

Configure
Receive SMS code
Login correctly

If there are tests which handle this, please let me know.

@boppy Thanks for the links. Since I pushed my changes to my repo, can I still squash (i.e. rebase) the commits?

A word of caution: Only do this on commits that haven’t been pushed an external repository.

cdjenkins

fixed requested changes

ChristophWurst · 2020-06-18T09:54:21Z

@boppy Thanks for the links. Since I pushed my changes to my repo, can I still squash (i.e. rebase) the commits?

Yes! That should be fine.

Once this is merged you can reset your fork's master to ours or simply delete the fork repo because you're now allowed to push here directly :) Ideally features are developed on branches, so you can have more than one at a time and you master stays in sync with the upstream repo.

cdjenkins · 2020-06-19T01:36:24Z

"and you master stays in sync with the upstream repo." Does github automatically handle this? master to master sync? A better question is, how to reset my fork?

ChristophWurst · 2020-06-19T09:32:09Z

"and you master stays in sync with the upstream repo." Does github automatically handle this? master to master sync? A better question is, how to reset my fork?

No, it does not.

https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/syncing-a-fork I think that describes it well :)

Don't update the fork until this is merged, though, or you'll risk losing the commits.

cdjenkins · 2020-06-19T17:01:43Z

https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/syncing-a-fork I think that describes it well :)

Don't update the fork until this is merged, though, or you'll risk losing the commits.

Thanks for the warning.

vitormattos · 2022-06-28T16:54:04Z

@cdjenkins can you fix the conflicts and DCO issues?
I think that when all tests pass, will be good to squash all commits in only one.

vitormattos · 2025-09-20T19:16:17Z

Thank you for your contribution and the time you put into this PR.

Since then, I’ve taken over as maintainer of this app, and it has gone through a major refactor that changed much of the internal architecture and file structure. Because of that, this PR can’t be merged in its current form.

If you’re still interested in this contribution, please feel free to rebase or open a new PR against the current main branch.

Your effort is very valuable, even if we can’t merge this directly, it helps us move the app forward. Thanks again!

cdjenkins added 4 commits May 30, 2020 01:19

1st draft impl of Plivo SMS provider

dab96f0

finish Plivo implementation

6545e7e

fix compilation issues

2d44d85

add plivo to documentation

8cd0c96

boppy requested changes May 31, 2020

View reviewed changes

fix indentation. sort providers alphabetically.

d22c44d

boppy requested changes May 31, 2020

View reviewed changes

lib/Command/Configure.php Outdated Show resolved Hide resolved

improve use of variable inside string

0e99191

cdjenkins commented Jun 2, 2020

View reviewed changes

cdjenkins requested a review from boppy June 2, 2020 18:38

cdjenkins added 2 commits June 3, 2020 00:57

fix bug. referenced const array incorrectly

83cb8b4

fix bug. calling wrong functions

16c9729

add logger. post doesn't work, but get does on IClient

209cb8a

fix bug. don't pass 'url' param to plivo unless you have a valid url

f126375

remove debug flag. successfully receive 2fa code over sms from plivo

a614378

remove callback url to reduce complexity of app

0e89632

boppy approved these changes Jun 14, 2020

View reviewed changes

ChristophWurst requested changes Jun 15, 2020

View reviewed changes

use class constants with clas name. improve logging. use multi-line a…

72c1e25

…rray

cdjenkins commented Jun 16, 2020

View reviewed changes

cdjenkins added 2 commits June 16, 2020 23:11

fix php formatting

8d4e305

Merge remote-tracking branch 'origin/master'

44fd17e

cdjenkins commented Jun 17, 2020

View reviewed changes

-			$this->logger->error($ex->getMessage());
+			$this->logger->logException($ex, [
+			  'message' => 'Could not send Plivo message: ' . $ex->getMessage(),
+			]);

Add Plivo as SMS Provider #352

Are you sure you want to change the base?

Add Plivo as SMS Provider #352

Uh oh!

Conversation

cdjenkins commented May 31, 2020

Uh oh!

boppy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cdjenkins commented May 31, 2020

Uh oh!

boppy commented May 31, 2020

Uh oh!

cdjenkins commented May 31, 2020

Uh oh!

boppy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cdjenkins left a comment

Choose a reason for hiding this comment

Uh oh!

cdjenkins commented Jun 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cdjenkins commented Jun 3, 2020

Uh oh!

cdjenkins commented Jun 3, 2020

Uh oh!

cdjenkins commented Jun 3, 2020

Uh oh!

cdjenkins commented Jun 3, 2020

Uh oh!

cdjenkins commented Jun 3, 2020

Uh oh!

cdjenkins commented Jun 3, 2020

Uh oh!

cdjenkins commented Jun 10, 2020

Uh oh!

cdjenkins commented Jun 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

boppy commented Jun 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChristophWurst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ChristophWurst Jun 15, 2020

Choose a reason for hiding this comment

Uh oh!

cdjenkins Jun 16, 2020

Choose a reason for hiding this comment

Uh oh!

ChristophWurst commented Jun 16, 2020

Uh oh!

cdjenkins commented Jun 16, 2020

Uh oh!

cdjenkins left a comment

Choose a reason for hiding this comment

Uh oh!

cdjenkins commented Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cdjenkins left a comment

Choose a reason for hiding this comment

Uh oh!

ChristophWurst commented Jun 18, 2020

Uh oh!

cdjenkins commented Jun 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

cdjenkins commented Jun 3, 2020 •

edited

Loading

cdjenkins commented Jun 14, 2020 •

edited

Loading

boppy commented Jun 15, 2020 •

edited

Loading

cdjenkins commented Jun 16, 2020 •

edited

Loading

cdjenkins commented Jun 19, 2020 •

edited

Loading