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

Added Multi-Factor Authentication #70

Closed
wants to merge 26 commits into from

Conversation

chesspro13
Copy link

Added A tab in the options menu to enable Multi-Factor Authentication (MFA) in the form of Time-Based One Time Passwords (TOTP) for a layer of security.

White space corrected through the use of Prettier. Format standardization still needed issue #52

Addresses feature request #68

@chesspro13 chesspro13 requested a review from eliandoran May 7, 2024 19:14
@chesspro13 chesspro13 changed the title Mfa Added Multi-Factor Authentication May 7, 2024
@chesspro13 chesspro13 requested a review from a team May 8, 2024 00:23
@eliandoran
Copy link

@chesspro13 , since you're still working on the PR, I will put it in draft for now.
Feel free to put it into ready once you complete your development.
Keep up the good work!

@eliandoran eliandoran marked this pull request as draft May 11, 2024 06:37
@chesspro13
Copy link
Author

TOTP functionality is complete. I'm going through and rewriting everything to try to match the original formatting as much as possible. @eliandoran do you have a formatting config?

Copy link
Author

@chesspro13 chesspro13 left a comment

Choose a reason for hiding this comment

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

Changes required

})
.then((result) => {
if (result.success) {
toastService.showError('Password has been changed. Trilium will be reloaded after you press OK.');
Copy link
Author

Choose a reason for hiding this comment

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

Change this to reflect Secret, not password

Copy link
Author

Choose a reason for hiding this comment

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

Changed

@@ -0,0 +1,63 @@
import options = require('../../services/options');
Copy link
Author

Choose a reason for hiding this comment

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

This was another implementation worth docker and webui variables. It doesn't need to be here.

Copy link
Author

Choose a reason for hiding this comment

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

Removed unnecessary code

@@ -1,4 +1,4 @@
"use strict";
'use strict';
Copy link
Author

Choose a reason for hiding this comment

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

Remove stuff relating to secret keys being encrypted.

@chesspro13 chesspro13 marked this pull request as ready for review May 13, 2024 01:27
@chesspro13
Copy link
Author

@eliandoran, just to clarify, the policy is to request review when merging into the develop branch, correct? Am I correct in assuming I can merge this since it's a pull request from my fork to this MFA branch?

@eliandoran
Copy link

@chesspro13 , generally the process is to open a PR that targets the main development branch (develop, in our case). The reason is that a PR is basically a way of asking to merge code into the main branch, with support for reviews.

Generally, you create a branch either on your fork on this repo (I prefer to create branches directly on the TriliumNext repo) and then create the PR. No need to have a branch both on the fork and on TriliumNext.

In this case, my recommendation is to edit this PR (top-right) and change the base branch to develop.

@chesspro13 chesspro13 changed the base branch from mfa to develop May 13, 2024 05:44
@chesspro13 chesspro13 self-assigned this May 13, 2024
@chesspro13
Copy link
Author

Going to bundle this feature along with OAuth. Closing pull request.

@chesspro13 chesspro13 closed this May 23, 2024
@eliandoran eliandoran added this to the Alpha release milestone Jul 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants