Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Reintroduce TOTP support #890

Merged
merged 28 commits into from Jun 29, 2020
Merged

Reintroduce TOTP support #890

merged 28 commits into from Jun 29, 2020

Conversation

msfjarvis
Copy link
Member

@msfjarvis msfjarvis commented Jun 27, 2020 •
edited

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring
  • Dependency updates

📜 Description

Re-introduces TOTP support backed by extensive automated tests.

💡 Motivation and Context

Users have been very unhappy about losing support for OTP features and while HOTP is very ugly to add,
TOTP is a relatively easier effort so as a good compromise between maintainability and user experience,
this PR reintroduces support for TOTP secrets.

💚 How did you test it?

Users are testing this with their TOTP secrets and reporting back with success, and all our unit and instrumentation tests pass.

📝 Checklist

  • I formatted the code with the IDE's reformat action (Ctrl + Shift + L/Cmd + Shift + L)
  • I reviewed submitted code
  • I added a CHANGELOG entry if applicable

🔮 Next steps

Follow the patterns established here to break more things down to testable units

📸 Screenshots / GIFs

@msfjarvis
Reintroduce TOTP support
f7c34b5 
While HOTP is a pain to add support for, TOTP is relatively easy and also the most commonly
used variant. As per discussion in #806, it makes sense for us to add back TOTP at the very least
as a reasonable middle path between maintainability for us and a healthy feature set for our users

Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis
update changelog
020313f 
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis msfjarvis added this to the 1.10.0 milestone Jun 27, 2020
@msfjarvis msfjarvis mentioned this pull request Jun 27, 2020
Merged
6 tasks
@msfjarvis
Merge branch 'develop' into feature/totp
a6757b7 
* develop:
  Update Public Suffix List data (#888)
@msfjarvis
Remove dangling HOTP references
cd2bfcb 
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis
Simplify tests
9609731 
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis
Extract hardcoded string
18570ec 
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis msfjarvis marked this pull request as draft June 27, 2020 12:53
@msfjarvis
Copy link
Member Author

Well kill me now, the click listener refuses to fire :D

@msfjarvis
Add tests for TOTP
2472298 
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis
Copy link
Member Author

Good news: the tests have been verified with a separate app to be correct which means my Kotlin port of the Otp class works well
Bad news: I can't make this work in the app because my click listener never fires :|

@msfjarvis msfjarvis marked this pull request as ready for review June 27, 2020 13:56
@msfjarvis
Copy link
Member Author

Uhh this works now for some reason? @erayd @charvp @Helianthella here's a signed build to test the functionality and UX with: https://dl.msfjarvis.dev/share/aps_1.10.0-SNAPSHOT.apk

@chvp
Copy link

chvp commented Jun 27, 2020

Thanks! I've logged into a few sites, seems to work without a hitch.

@msfjarvis
PasswordEntry: cleanups
e3af34d 
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis
Rollback unrelated cleanups
af95522 
Will push this separately

Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis
KDoc fixups
f219a83 
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis
Add tests for UriTotpFinder
57740fe 
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis
Copy link
Member Author

I think this covers our bases well. If someone has more otpauth:// URI samples they wanna share to be added to these tests I'd be very grateful.

@erayd
Copy link

erayd commented Jun 28, 2020

@msfjarvis Initial testing with that APK seems to work well - I've tested a number of different entries, and the generated codes were all correct.

One comment - there is no option to view the generated OTP code; the only option is to copy it to the clipboard. For those who are logging in on a device that isn't their phone, that will cause problems. Displaying it in a similar manner to how the password and username are displayed would address that issue.

@msfjarvis
Copy link
Member Author

One comment - there is no option to view the generated OTP code; the only option is to copy it to the clipboard. For those who are logging in on a device that isn't their phone, that will cause problems. Displaying it in a similar manner to how the password and username are displayed would address that issue.

Yeah that makes sense, will get on it.

@erayd
Copy link

erayd commented Jun 28, 2020

Thanks @FabianHenneke 🙂.

  1. Assuming that lifecycleScope means "as long as everything else in the entry is viewable", that seems reasonable.

  2. Seems reasonable, provided it's still visible when editing. Wanting to copy the OTP seed rather than the generated code is a very rare use-case.

@msfjarvis
Fixup imports
02c0e1e 
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis
Drop OTP URLs from extra content
e231498 
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis
Show OTP copied text in Snackbar
e28e5c7 
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis
Copy link
Member Author

  1. Seems reasonable, provided it's still visible when editing. Wanting to copy the OTP seed rather than the generated code is a very rare use-case.

You can still manually copy it while editing so IMO our current setup is fine.

I've made the change to drop otpauth:// lines from the extra content, and changed the snackbar text to mention OTP codes since I added the string before anyway.

@erayd
Copy link

erayd commented Jun 28, 2020

I've made the change to drop otpauth:// lines

Does that commit also drop totp: lines? That wasn't obvious from looking at the commit, unless I missed something.

@msfjarvis
Copy link
Member Author

I've made the change to drop otpauth:// lines

Does that commit also drop totp: lines? That wasn't obvious from looking at the commit, unless I missed something.

No, just otpauth://. I'll add in totp://.

@erayd
Copy link

erayd commented Jun 28, 2020 •
edited

Not totp:// - just totp:

It's not a URL scheme, but rather a key / value line, similar to how the username is encoded.

@msfjarvis
Ignore both TOTP formats
f52bea9 
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis
Copy link
Member Author

Not totp:// - just totp:

It's not a URL scheme, but rather a key / value line, similar to how the username is encoded.

Ah yeah, accidentally typed totp:// here. Pushed the fix.

@msfjarvis
Merge branch 'develop' into feature/totp
18eb6b6 
* develop:
  Sync with release branch (#896)
  Rework GitHub Actions (#893)
@msfjarvis
Add test for padded vs unpadded invariant of the same secret
fd61374 
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis
Copy link
Member Author

If I understand Base32 padding correctly (don't let me down Wikipedia), this test covers that case as well which means this is good to go now.

@msfjarvis
Fix formatting fail
ccac08f 
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@TheLastProject
Copy link

I implemented parsing otpauth URLs in pyotp, so maybe the unit tests there may help too: https://github.com/pyauth/pyotp/blob/master/test.py#L342

@TheLastProject
Copy link

See also pyauth/pyotp#84 for the feature itself and the checks to ensure the URL meets the standards

fmeum
fmeum previously approved these changes Jun 29, 2020
View reviewed changes
@msfjarvis
Copy link
Member Author

I implemented parsing otpauth URLs in pyotp, so maybe the unit tests there may help too: pyauth/pyotp:test.py@master#L342

Thanks a lot, this is helpful! This gave me a couple ideas that I'll implement right now.

@msfjarvis
Otp: add check for invalid algorithm
908d426 
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis
Revert "Otp: add check for invalid algorithm"
b181d63 
This reverts commit 908d426.
@msfjarvis
Copy link
Member Author

@FabianHenneke reverted as per our discussion

@msfjarvis msfjarvis merged commit 063c1a1 into android-password-store:develop Jun 29, 2020
@msfjarvis msfjarvis deleted the feature/totp branch June 29, 2020 06:39
msfjarvis added a commit that referenced this pull request Jun 29, 2020
@msfjarvis
Merge branch 'develop' into feature/configure-pull-behavior
d9efd17 
* develop:
  Offer TOTP Autofill for OTP fields (#899)
  Merge SshKeyGenFragment into its activity (#897)
  Reintroduce TOTP support (#890)
  Sync with release branch (#896)
  Rework GitHub Actions (#893)
  Consolidate password list refresh (#887)
msfjarvis added a commit to fmeum/Android-Password-Store that referenced this pull request Jul 1, 2020
@msfjarvis
Merge branch 'develop' into fhenneke_multiple_ssh
41f92b1 
* develop: (62 commits)
  Scroll to files and enter folders when created (android-password-store#909)
  Run a treewide reformat (android-password-store#908)
  Improve how secrets and stored and used (android-password-store#907)
  Improve and refactor Autofill heuristics (android-password-store#905)
  Use PreferenceKeys file to manage SharedPreferences keys. (android-password-store#891)
  Revert "Support directly importing secrets" (android-password-store#904)
  Allow importing TOTP configuration through QR codes (android-password-store#903)
  Bump version
  Prepare release 1.9.2
  update changelog
  Workaround to prevent crash on first run (android-password-store#898)
  Workaround to prevent crash on first run (android-password-store#898)
  Offer TOTP Autofill for OTP fields (android-password-store#899)
  Merge SshKeyGenFragment into its activity (android-password-store#897)
  Reintroduce TOTP support (android-password-store#890)
  Sync with release branch (android-password-store#896)
  build: bump version
  Prepare release 1.9.1
  Backport Actions fixes (android-password-store#894)
  Rework GitHub Actions (android-password-store#893)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmeum fmeum fmeum approved these changes

@Skrilltrax Skrilltrax Awaiting requested review from Skrilltrax

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.10.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@msfjarvis @chvp @erayd @fmeum @TheLastProject