add validate api for totp #38

karakayasemi · 2018-01-11T13:48:21Z

add OCS API for TOTP validation. Implementation of #29. @Touchwoody please test.

Open tasks before release

Needs documentation

Usage

URL
/ocs/v1.php/apps/twofactor_totp/api/v1/validate/:user_id/:key?format=json
**Example test query for curl **
curl -u admin:pass 'http://base_url/ocs/v1.php/apps/twofactor_totp/api/v1/validate/{user_id}/{key}?format=json'
Method:
GET
URL Params
Required:
user_id=[string]
key=[string]


* **Success Response:**
{"ocs":{"meta":{"status":"ok","statuscode":100,"message":"OK","totalitems":"","itemsperpage":""},"data":{"result":true}}}
 
* **Fail Response:**
{"ocs":{"meta":{"status":"ok","statuscode":100,"message":"OK","totalitems":"","itemsperpage":""},"data":{"result":false}}}

* **User or Secret Not Found Response:**
{"ocs":{"meta":{"status":"failure","statuscode":404,"message":"OK","totalitems":"","itemsperpage":""},"data":{"result":false}}}

Touchwoody · 2018-02-16T10:30:35Z

Hey @karakayasemi,
sorry for the delay, lately I lacked an owncloud test installation, so I asked my colleagues to help me out and I 'hijacked' one of our staging servers.

We patched this commit to a 10.0.4EE ("version":"10.0.4.4","versionstring":"10.0.4","edition":"Enterprise"). I was able to query your API succesfully then:

$ curl -X GET https://xxx-branding.sciebo.de/index.php/apps/twofactor_totp/api/schild@sciebotest.de_9823/123123
false
$ curl -X GET https://xxx-branding.sciebo.de/index.php/apps/twofactor_totp/api/schild@sciebotest.de_9823/585973
true
$ curl -X GET https://xxx-branding.sciebo.de/index.php/apps/twofactor_totp/api/schild@sciebotest.de_9823/wrongnumber
false

That's exactly the functionality I was hoping to get, thanks.

karakayasemi · 2018-02-16T13:11:59Z

@Touchwoody I am glad that the expectations are met. @DeepDiver1975 I do not have much experience in API development. Your code review would be nice in here. Thanks.

cdamken · 2018-03-29T19:45:38Z

Hi Guys, Is there a plan when is this going to be this part released?

karakayasemi · 2018-04-24T16:42:00Z

@cdamken it is waiting for a review, I have not any idea about the release schedule

cdamken · 2018-04-25T12:46:45Z

@DeepDiver1975 Can you please review it?

@patrickjahns Once is reviewed, could @owncloud/qa check if we can have some test?

phil-davis · 2018-04-25T14:54:44Z

Tests should be easy enough, we can access whatever API the web UI does to enable TOTP for a user, and remember their secret key. Then there seem to be plenty of implementations of the TOTP key generator out there. So we can generate a current key and use it to make expected good queries. Excepted bad queries will be easy!

jvillafanez · 2018-04-25T15:13:02Z

🚫

"secret" in the url is a no go for me because "secret" in the url is no longer secret. HTTPS won't even hide it.
It's not clear for me what is the purpose of the call. If it's to verify, it should just verify, not update things.
If the plan is to update things, use a POST or PUT call. Use POST to create things and PUT to update them. I'd expect POST to fail if the resource is already created.
Using "index.php/apps/twofactor_totp/api/{user_id}/{secret}" as endpoint doesn't look good. It doesn't have any meaning, and you don't know what to expect. Without a clear understanding I can't recommend anything, but to update secrets I'd prefer "index.php/apps/twofactor_totp/api/secret"; send a PUT there to update the secret of the logged in user. I expect only one secret per user, otherwise something like ""index.php/apps/twofactor_totp/api/secrets/{secret_id}" is more appropiate. Note that the "secret_id" is just an identifier, not the secret itself.
Use OCSResponse for the responses. According to @DeepDiver1975 external APIs should be using this. (I'm the first one that doesn't like it, but that's how it is)
For the responses, you can use just the HTTP codes. In fact, you should be using a 400+ code for the failure regardless of any content you want to send back to the client (recheck with @DeepDiver1975 because I think the OCSResponse always returns 200 by default, but I don't think we want this behaviour). Returning just "true" or "false" feels bad when you have the HTTP codes for the same purpose.

phil-davis · 2018-04-25T16:06:01Z

I don't think the "secret" here is the "full secret private key" that was generated initially for the user. It is the 6-digit code that changes every 30 seconds or so. That "secret" is old and unusable 30 seconds later, so it is not so critical to keep it secret.

The code here is not updating anything, it is just getting a confirmation if the 6-digit code is a currently-valid code.

I am just clarifying a bit of what I see the code doing. @jvillafanez points above are still valid.

jvillafanez · 2018-04-25T17:08:59Z

I don't think the "secret" here is the "full secret private key" that was generated initially for the user. It is the 6-digit code that changes every 30 seconds or so. That "secret" is old and unusable 30 seconds later, so it is not so critical to keep it secret.

Fair enough. However, I'd use "one-time-code" or "timed-code" or something similar instead of "secret" to prevent confusions. Just this rename feels a bit "safer" to send such code over the URL, but still I'd prefer to avoid it.

The code here is not updating anything, it is just getting a confirmation if the 6-digit code is a currently-valid code.

If only one line of the function is dedicated to validation and 3 lines to update the dbSecret.... you can rename the function to updateDbSecret and it still makes sense. This is a problem because the purpose of the function isn't clear anymore.
I'm not saying to change anything, but comments would help the reader (me in this case) to pull him back on track: "we have to update the dbSecret because...." would imply that updating isn't the purpose of the function but something you have to do there.

Touchwoody · 2018-04-26T06:51:28Z

To clarify, what I requested for was an API call that tests, if an OneTimePassword is correct or not. No updates, just verify. For me, the call does exactly just that.

This is a similar behavior as in the common LinOTP Service, see e.g.:
https://wwuotp.uni-muenster.de/validate/simplecheck?user=schild&pass=123456
(or https://wwuotp.uni-muenster.de/validate/check?user=schild&pass=123456)

I agree on putting more explanatory strings in the URL like "validate" or similar, will help to denote what the API call does.

Come to think of it: I don't know if this is inherent in the API code base, but you need some brute force protection. Else I could send thousands of queries per second, might get a lucky hit and then know the current OTP for a few seconds.

karakayasemi · 2018-04-26T07:05:54Z

Fair enough. However, I'd use "one-time-code" or "timed-code" or something similar instead of "secret" to prevent confusions. Just this rename feels a bit "safer" to send such code over the URL, but still I'd prefer to avoid it.

I just track old convention in the code by using "secret" keyword. Also, in the RFC of the TOTP protocol "shared secret key" is being used, because of that, I can not say wrong to use "secret" keyword in here.

I'm not saying to change anything, but comments would help the reader (me in this case) to pull him back on track: "we have to update the dbSecret because...." would imply that updating isn't the purpose of the function but something you have to do there.

I refactored verifySecret function of the Totp class by adding a try-catch block to catch an unhandled exception. But nothing changed in logic. As far as I understand, that was also confusing in the PR. We can move this to a separate commit.

patrickjahns · 2018-04-30T06:04:43Z

@cdamken

@patrickjahns Once is reviewed, could @owncloud/qa check if we can have some test?

The app has unit tests - anything else can be added to the backlog and will need to be prioritized + scheduled accordingly

codecov · 2019-02-15T16:35:01Z

Codecov Report

Merging #38 into master will increase coverage by 0.33%.
The diff coverage is 66.66%.

@@             Coverage Diff              @@
##             master      #38      +/-   ##
============================================
+ Coverage      62.6%   62.94%   +0.33%     
- Complexity       57       60       +3     
============================================
  Files            12       13       +1     
  Lines           230      251      +21     
============================================
+ Hits            144      158      +14     
- Misses           86       93       +7

Impacted Files	Coverage Δ	Complexity Δ
appinfo/routes.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
lib/Controller/TotpApiController.php	`100% <100%> (ø)`	`3 <3> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3c8d9ed...fbf11d2. Read the comment docs.

codecov · 2019-02-15T16:35:02Z

Codecov Report

Merging #38 into master will increase coverage by 2.07%.
The diff coverage is 64%.

@@             Coverage Diff              @@
##             master      #38      +/-   ##
============================================
+ Coverage      62.6%   64.68%   +2.07%     
- Complexity       57       61       +4     
============================================
  Files            12       13       +1     
  Lines           230      252      +22     
============================================
+ Hits            144      163      +19     
- Misses           86       89       +3

Impacted Files	Coverage Δ	Complexity Δ
appinfo/routes.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
lib/Provider/TotpProvider.php	`31.25% <100%> (+31.25%)`	`8 <0> (ø)`	⬇️
lib/Controller/TotpApiController.php	`100% <100%> (ø)`	`4 <4> (?)`
lib/Service/Totp.php	`33.33% <50%> (ø)`	`14 <4> (ø)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ca4c26...7abb5d7. Read the comment docs.

karakayasemi · 2019-02-18T20:03:30Z

I converted API to OCS API and removed refactoring part of the commit.
Now, this PR is only allowing to validate time-based-one-time-password via an OCS_API call.

@jvillafanez I am not so experienced on this topic. Please help me with your code review. @Touchwoody also you can help me by testing the functionality. Thanks.

jvillafanez · 2019-02-21T11:21:24Z

lib/Controller/TotpApiController.php

+	 * @param string $secret
+	 * @return Result
+	 */
+	public function validate($id, $secret) {


Try to use more meaningful names. Is the "id" as user id? a token id? a security id?
In addition, as said, "secret" is a bad name here since this is "linked" to the route.

Adjust the PHPDoc accordingly.

jvillafanez · 2019-02-21T11:24:08Z

lib/Controller/TotpApiController.php

+	 */
+	public function validate($id, $secret) {
+		$user = $this->userManager->get($id);
+		if (!empty($user)) {


The userManager returns either a IUser object or null. Check for null or include a comment in the code explaining why empty is a better choice.

jvillafanez · 2019-02-21T11:25:30Z

lib/Controller/TotpApiController.php

+			} catch (NoTotpSecretFoundException $e) {
+			}
+		}
+		return new Result(['result' => false]);


Try to use DataResponse since this class is public. Also verify the HTTP codes make sense

jvillafanez · 2019-02-21T11:49:17Z

lib/Service/Totp.php

@@ -104,6 +104,8 @@ public function verifySecret(IUser $user, $key) {
 	/**
 	 * @param IUser $user
 	 * @param string $key
+	 * @return boolean


Docs on the interface must be updated too. If there is an interface available I only care about what the interface says, if the implementation does any other thing outside interface, the implementation is buggy. If the interface doesn't say it can throw an exception, the implementation is the one to be blamed about bugs caused by it throwing exceptions. So, as said, update the interface too.

In addition, specially in the interface, explain the parameters. I don't want to check in every implementation what the "key" does (is it the secret to be validated?, a known id to fetch the secret from another place?, can I use any random string and expect it to work?) or why do I need to check for a "NoToptSecretFoundException" to be thrown. It must be known to whoever implements the interface under what conditions that exception is expected to be thrown.

I'm not saying to change anything in the code, I'm just suggesting to document the code better.

jvillafanez · 2019-02-21T12:04:16Z

appinfo/routes.php

+			'url' => '/api/v1/validate/{id}/{secret}',
+			'verb' => 'GET',
+			'requirements' => [
+				'id' => '.+',


I'll need an explanation for these requirements.
For an url such as /api/v1/validate/userId/verySecret I expect "id => userId" and "secret => verySecret". I expect that "/" is used as separator. The only way I think this could go wrong is if either the id or the secret contains a "/".
If the secret contains a "/" such as in /api/v1/validate/userId/very/top/Secret I expect "id => userId" and "secret => very/top/Secret", which might be fair (I don't know what chars are used as secret, but having "/" as part of the set doesn't seem a good idea). However, the system could also assume "id => userId/very/top" and "secret => Secret" for the same url.

You are right about the secret keyword, I will update code to use key keyword instead of secret and use uid instead of id.
However, Totp key is 6 digit number, cannot contain "/", also only the following characters are allowed in a username: "a-z", "A-Z", "0-9", and "_.@-' . So, it can not contain "/" too. IMHO, there is no risk in url.

jvillafanez · 2019-02-21T12:06:18Z

lib/Controller/TotpApiController.php

+		if (!empty($user)) {
+			try {
+				return new Result(['result' => $this->totp->validateSecret($user, $secret)]);
+			} catch (NoTotpSecretFoundException $e) {


I expect this to be a rare occurrence, so at least we should log the exception.

Actually, not every user have a totp secret. We are not enforcing 2-Factor Totp usage. So, the frequency changes depending on the usage of the API. But, we can log the exception.

karakayasemi · 2019-02-24T18:15:07Z

@jvillafanez I believe, I addressed all your requests. Please review one more time. Thanks.

jvillafanez

Just double-check that inserting a "/" in either the userid or the key is safe.

karakayasemi · 2019-02-25T11:19:13Z

Just double-check that inserting a "/" in either the userid or the key is safe.

@jvillafanez I tried to insert "/" to userid with both occ command and user management interface. It is not possible. Totp key is also can consist of only numbers. I believe it will not be a problem.

@PVince81 It is ready to merge. I guess we should document this with the release.

PVince81 · 2019-03-01T09:17:07Z

@karakayasemi please raise documentation tickets as needed and provide some content to explain what to document

karakayasemi added the enhancement label Jan 11, 2018

karakayasemi self-assigned this Jan 11, 2018

karakayasemi requested a review from DeepDiver1975 January 11, 2018 13:48

patrickjahns added this to the development milestone Jan 15, 2018

karakayasemi force-pushed the addapi branch 2 times, most recently from f40c08c to fa71cd8 Compare January 19, 2018 11:13

karakayasemi changed the title ~~add api for totp~~ add verify api for totp Apr 25, 2018

karakayasemi force-pushed the addapi branch 2 times, most recently from aaef3f2 to fbf11d2 Compare February 15, 2019 16:34

karakayasemi force-pushed the addapi branch from fbf11d2 to 0d03efe Compare February 18, 2019 19:25

karakayasemi changed the title ~~add verify api for totp~~ [WIP] add validate api for totp Feb 18, 2019

karakayasemi force-pushed the addapi branch from 0d03efe to cbfe4f1 Compare February 18, 2019 19:49

jvillafanez reviewed Feb 21, 2019

View reviewed changes

karakayasemi force-pushed the addapi branch 2 times, most recently from 3b8bf94 to f65d624 Compare February 23, 2019 16:34

karakayasemi force-pushed the addapi branch 2 times, most recently from 07b37bb to c312afc Compare February 24, 2019 17:49

karakayasemi changed the title ~~[WIP] add validate api for totp~~ add validate api for totp Feb 24, 2019

jvillafanez approved these changes Feb 25, 2019

View reviewed changes

add validate api for totp

7abb5d7

karakayasemi force-pushed the addapi branch from c312afc to 7abb5d7 Compare February 25, 2019 11:05

PVince81 merged commit 8cd2517 into master Mar 1, 2019

PVince81 deleted the addapi branch March 1, 2019 09:16

PVince81 mentioned this pull request Mar 1, 2019

TOTP 0.6.0 #103

Closed

27 tasks

This was referenced Mar 1, 2019

REST API #29

Closed

Document twofactor_totp key validation OCS API owncloud/docs#674

Closed

davitol modified the milestones: development, qa Jul 9, 2019

davitol mentioned this pull request Jul 10, 2019

Tracking testing results for twofactor_totp 0.6.0 #128

Closed

bhawanaprasain mentioned this pull request Jul 15, 2019

Test for two factor authentication #131

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add validate api for totp #38

add validate api for totp #38

karakayasemi commented Jan 11, 2018 •

edited

Loading

Touchwoody commented Feb 16, 2018 •

edited

Loading

karakayasemi commented Feb 16, 2018

cdamken commented Mar 29, 2018

karakayasemi commented Apr 24, 2018

cdamken commented Apr 25, 2018

phil-davis commented Apr 25, 2018

jvillafanez commented Apr 25, 2018

phil-davis commented Apr 25, 2018

jvillafanez commented Apr 25, 2018

Touchwoody commented Apr 26, 2018

karakayasemi commented Apr 26, 2018 •

edited

Loading

patrickjahns commented Apr 30, 2018

codecov bot commented Feb 15, 2019

codecov bot commented Feb 15, 2019 •

edited

Loading

karakayasemi commented Feb 18, 2019

jvillafanez Feb 21, 2019

jvillafanez Feb 21, 2019

jvillafanez Feb 21, 2019

jvillafanez Feb 21, 2019

jvillafanez Feb 21, 2019

karakayasemi Feb 23, 2019 •

edited

Loading

jvillafanez Feb 21, 2019

karakayasemi Feb 23, 2019

karakayasemi commented Feb 24, 2019

jvillafanez left a comment

karakayasemi commented Feb 25, 2019 •

edited

Loading

PVince81 commented Mar 1, 2019

add validate api for totp #38

add validate api for totp #38

Conversation

karakayasemi commented Jan 11, 2018 • edited Loading

Open tasks before release

Usage

Touchwoody commented Feb 16, 2018 • edited Loading

karakayasemi commented Feb 16, 2018

cdamken commented Mar 29, 2018

karakayasemi commented Apr 24, 2018

cdamken commented Apr 25, 2018

phil-davis commented Apr 25, 2018

jvillafanez commented Apr 25, 2018

phil-davis commented Apr 25, 2018

jvillafanez commented Apr 25, 2018

Touchwoody commented Apr 26, 2018

karakayasemi commented Apr 26, 2018 • edited Loading

patrickjahns commented Apr 30, 2018

codecov bot commented Feb 15, 2019

Codecov Report

codecov bot commented Feb 15, 2019 • edited Loading

Codecov Report

karakayasemi commented Feb 18, 2019

jvillafanez Feb 21, 2019

Choose a reason for hiding this comment

jvillafanez Feb 21, 2019

Choose a reason for hiding this comment

jvillafanez Feb 21, 2019

Choose a reason for hiding this comment

jvillafanez Feb 21, 2019

Choose a reason for hiding this comment

jvillafanez Feb 21, 2019

Choose a reason for hiding this comment

karakayasemi Feb 23, 2019 • edited Loading

Choose a reason for hiding this comment

jvillafanez Feb 21, 2019

Choose a reason for hiding this comment

karakayasemi Feb 23, 2019

Choose a reason for hiding this comment

karakayasemi commented Feb 24, 2019

jvillafanez left a comment

Choose a reason for hiding this comment

karakayasemi commented Feb 25, 2019 • edited Loading

PVince81 commented Mar 1, 2019

karakayasemi commented Jan 11, 2018 •

edited

Loading

Touchwoody commented Feb 16, 2018 •

edited

Loading

karakayasemi commented Apr 26, 2018 •

edited

Loading

codecov bot commented Feb 15, 2019 •

edited

Loading

karakayasemi Feb 23, 2019 •

edited

Loading

karakayasemi commented Feb 25, 2019 •

edited

Loading