Skip to content
This repository has been archived by the owner on Jun 23, 2023. It is now read-only.

Implement 2FA for Umbrel-Manager #111

Closed
wants to merge 2 commits into from

Conversation

dsbaars
Copy link

@dsbaars dsbaars commented Nov 18, 2021

Implementing 2FA using TOTP as requested in getumbrel/umbrel#985 and getumbrel/umbrel#1078.
Also needs modified version of umbrel-dashboard, related pull request getumbrel/umbrel-dashboard#395

Copy link
Contributor

@AaronDewes AaronDewes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few suggestions

Comment on lines +82 to +83
// update system password
await setSystemPassword(newPassword);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe reverting these changes would help getting this merged. I recommend to not lint any of your PRs, Umbrel hasn't used a configured linter in any project for over a year now, even though that could've prevented some bugs.

Comment on lines +160 to +163

req.logIn(user, function (err) {


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert this too to keep changes minimal.

middlewares/auth.js Outdated Show resolved Hide resolved
Co-authored-by: Aaron Dewes <aaron.dewes@protonmail.com>
@lukechilds
Copy link
Member

Hey thanks for working on this @dsbaars!

We decided to go for a slightly different approach:

The passport-totp module doesn't really fit well with our existing codebase since we don't use sessions for authentication. Also we wanted to build 2FA into the existing authentication endpoints as opposed to adding new ones, along with doing a custom UI for it.

Really appreciate you taking the time to put this together though!

@lukechilds lukechilds closed this Nov 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants