twofactor_gateway
twofactor_gateway copied to clipboard
nextcloud
Add Plivo as SMS Provider
@ChristophWurst @MorrisJobke @boppy @killerbees19
Added code to use Plivo as a SMS Provider.
Just minor thing (except for your switch of
callbackandsrc_numberinConfigure.php... Did this work for you?)... I was able to run the commandocc twofactorauth:gateway:configure smssucessfully. 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 withpackage.json? If I could do that, I could do an end-to-end test as I have a Plivo phone number.
I was able to run the command
occ twofactorauth:gateway:configure smssucessfully. 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 withpackage.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... ;)
@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).
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.
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)
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?
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
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):
@ChristophWurst @boppy I have the debug flag set to the IClient which produces additional debug info to the ajax call. Removing the flag work!!
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?
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:
https://github.com/nextcloud/twofactor_gateway/blob/93218ddb7040cf28f93fac5b381b1d59608a0270/appinfo/info.xml#L22
Are there other problems you know of?
@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 :)
@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?
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.
@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.
"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?
"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.
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.
@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.
