{"action":"created","issue":{"url":"https://api.github.com/repos/wmlele/devise-otp/issues/23","labels_url":"https://api.github.com/repos/wmlele/devise-otp/issues/23/labels{/name}","comments_url":"https://api.github.com/repos/wmlele/devise-otp/issues/23/comments","events_url":"https://api.github.com/repos/wmlele/devise-otp/issues/23/events","html_url":"https://github.com/wmlele/devise-otp/issues/23","id":97764038,"number":23,"title":"Problem with validating recovery token","user":{"login":"jonathansimmons","id":945202,"avatar_url":"https://avatars.githubusercontent.com/u/945202?v=3","gravatar_id":"","url":"https://api.github.com/users/jonathansimmons","html_url":"https://github.com/jonathansimmons","followers_url":"https://api.github.com/users/jonathansimmons/followers","following_url":"https://api.github.com/users/jonathansimmons/following{/other_user}","gists_url":"https://api.github.com/users/jonathansimmons/gists{/gist_id}","starred_url":"https://api.github.com/users/jonathansimmons/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jonathansimmons/subscriptions","organizations_url":"https://api.github.com/users/jonathansimmons/orgs","repos_url":"https://api.github.com/users/jonathansimmons/repos","events_url":"https://api.github.com/users/jonathansimmons/events{/privacy}","received_events_url":"https://api.github.com/users/jonathansimmons/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":1,"created_at":"2015-07-28T17:56:57Z","updated_at":"2015-07-28T22:09:09Z","closed_at":null,"body":"### Problem\r\nIt's very easy to invalidate recover tokens\r\n\r\n### Summary\r\nThe `otp_recover_counter` column is being incrementing on ever recover token attempt regardless of whether or not the token was accepted. The only token that will be accepted is the token that matches the current `otp_recovery_counter\r\n\r\n\r\n### Detail\r\n\r\nRight now in master the it `validate_otp_recovery_token` method looks like this:\r\n/lib/devise_otp_authenticatable/models/otp_authenticatable.rb\r\n```ruby\r\ndef validate_otp_recovery_token(token)\r\n  recovery_otp.verify(token, otp_recovery_counter).tap do\r\n    self.otp_recovery_counter += 1\r\n    save!\r\n  end\r\nend\r\n```\r\n\r\nThe problem with this is the block is being run even if the token is not valid. Which means the `otp_recover_counter` is being incremented even though a valid token was not provided. Which is an issue because it means the valid recover token becomes a moving target. Let me explain with scenarios.\r\n\r\n#### Scenarios\r\n\r\n**A perfect world**\r\n1.)  I successfully activate otp\r\n2.) I save my 10 tokens and sign out. \r\n3.) After losing my phone attempt to use token 0\r\n4.) I get signed in fine, so I remove that token from my token list.\r\n5.) The `otp_recovery_counter` is now on 1\r\n\r\nNotes: If I continue this process always using the first token in my list my `otp_token_counter` will always match the token I attempt to use and everything will be fine.\r\n\r\n**Real world**\r\n1.) I successfully activate otp\r\n2.) I save my 10 tokens and sign out. \r\n3.) After losing my phone attempt to use token, and randomly grab token 4\r\n4.) When I submit token 4 it valid to validate because my `otp_recover_counter` is 0. meaning only token 0 will work. \r\n5.) Even though the validation attempt failed the `otp_recovery_counter` increments from 0 to 1 *(it failed because I used token 4 but it would also fail if I typoed token 0)*\r\n6.) So I think weird that one didn't work, so I go try another. Catch is the only token that will work of my nine remaining options is token 1 because my `otp_recovery_counter` is currently 1. Problem is, I have a 1-9 change of picking the right token. If I chose anything but token 1 I'm right back at step 5\r\n\r\n\r\nI've resolved the counter issue by adjusting the code like so: \r\n```ruby\r\ndef validate_otp_recovery_token(token)\r\n  if recovery_otp.verify(token, otp_recovery_counter)\r\n    self.otp_recovery_counter += 1\r\n    save!\r\n  else\r\n    false\r\n  end\r\nend\r\n```\r\n\r\nWhile my fix above prevents the counter from being incorrectly incremented on an invalid token. I think we need to revise the way the recovery tokens are displayed altogether given the knowledge that only the recover code corresponding to the current counter will actually work. There just no point in ever displaying them more than one code.\r\n\r\n**Suggested Fix:**\r\n- [ ] Display only next most relevant token after enabling otp, instead of a list.\r\n- [ ] Consider what the workflow should be after using a recovery token. \r\n    - Should we disabled otp, forcing them re-setup and therefore get a new token? They have lost their phone afterall. \r\n\r\n@wmlele Let me know your thoughts. \r\n"},"comment":{"url":"https://api.github.com/repos/wmlele/devise-otp/issues/comments/125767479","html_url":"https://github.com/wmlele/devise-otp/issues/23#issuecomment-125767479","issue_url":"https://api.github.com/repos/wmlele/devise-otp/issues/23","id":125767479,"user":{"login":"jonathansimmons","id":945202,"avatar_url":"https://avatars.githubusercontent.com/u/945202?v=3","gravatar_id":"","url":"https://api.github.com/users/jonathansimmons","html_url":"https://github.com/jonathansimmons","followers_url":"https://api.github.com/users/jonathansimmons/followers","following_url":"https://api.github.com/users/jonathansimmons/following{/other_user}","gists_url":"https://api.github.com/users/jonathansimmons/gists{/gist_id}","starred_url":"https://api.github.com/users/jonathansimmons/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jonathansimmons/subscriptions","organizations_url":"https://api.github.com/users/jonathansimmons/orgs","repos_url":"https://api.github.com/users/jonathansimmons/repos","events_url":"https://api.github.com/users/jonathansimmons/events{/privacy}","received_events_url":"https://api.github.com/users/jonathansimmons/received_events","type":"User","site_admin":false},"created_at":"2015-07-28T22:09:09Z","updated_at":"2015-07-28T22:09:09Z","body":"You're clarification helps, I'm not sure what I think. At a minimum I think the gem needs to clarify in the readme how the recover tokens work, specifically that they must be used sequentially. Even I, having spend a lot of time in this gem, totally missed that the tokens needed to be used sequentially. \r\n\r\nEven still, I'm left wanting a better solution. I've not come across a TOTP service in the wild that required I pay attention to the sequence of the tokens. Again I'm modeling my implementations after some of the bigger services out there that use TOTP (Github, Dropbox, etc). \r\n \r\nI agree recovery tokens are not just for the lost my device scenario but man there has to be a better way than requiring the user pay attention to sequence, or potentially get placed in a denial-of-service situation.\r\n\r\nI'm gonna think on this one for a bit, and get back to you. "}}