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

Verification before enabling #22

Closed
rigrig opened this issue Jul 23, 2016 · 5 comments
Closed

Verification before enabling #22

rigrig opened this issue Jul 23, 2016 · 5 comments

Comments

@rigrig
Copy link

rigrig commented Jul 23, 2016

It might be a good idea to verify a code before actually enabling TOTP authentication, to ensure that the user has a working TOTP generator. That way you can't accidentally lock yourself out.

I imagine there could be an additional new text field and button below the QR code in the settings when enabling it, so it would be something like

[x] Enable TOTP
This is your new TOTP secret: [secret]
Scan this QR code with your TOTP app:
[image: QR code]
+And enter the generated TOTP code:
+[text field] [button: "Enable TOTP"]

Pressing the button checks the entered code:
valid code -> enable TOTP, display "TOTP Enabled"
invalid code -> don't enable TOTP, display "Invalid TOTP code, please try again"

@ChristophWurst ChristophWurst added this to the 9.2 milestone Jul 24, 2016
@ChristophWurst ChristophWurst removed this from the NC11/OC9.2 milestone Aug 19, 2016
@My1
Copy link
Contributor

My1 commented Sep 28, 2016

I fully agree and support this.

@My1
Copy link
Contributor

My1 commented Nov 26, 2016

This could be solved (actually I dealt with the issue on my own not too long ago) by upon enabling TOTP and showing the QR Code, storing the TOTP seed in the $_SESSION from PHP, and linking it to the nextcloud session (unless nextcloud already uses the $_SESSION already where this wouldn't be required) then when the user enters and submits the TOTP, it is challenged against the seed in the $_SESSION and if it is correct, we throw the seed in the user DB.

@ChristophWurst
Copy link
Member

@My1 thanks for your input! Would you like to help with the implementation?

FYI: we have a session abstraction, OCP\ISession, for working with the session.

@ChristophWurst
Copy link
Member

When I thought about a possible implementation I thought that we could also add a boolean flag to the secret in the database indicating whether it's enabled or not. So we would have the possibility to validate a TOTP token, while it's not yet enforced on the login screen. Once we know it's valid we update the entry.

@My1
Copy link
Contributor

My1 commented Nov 26, 2016

that would also be a way to do the check, although it obviously needs an extra column and introduces the problem that the secret may be shown again, which may or may not be a problem.

but if you have your session implementation the idea would be throwing the OTP seed into that session if that session supports custom fields that are stored on the server (we dont want manipulation here) and if not just use the PHP session and store a distinct identifier of the isession into it as well, to make sure that any half-baked session stealing wont help.

also I havent touched nc's PHP yet and I personally dont like OOP anyway (the only places where I use a class is a static context so it can isolate some variables and functions)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants