2FA Two Authentication Factor #164 by asbiin · Pull Request #724 · monicahq/monica

asbiin · 2017-12-27T12:33:15Z

This will close #164

Based on my fork of pragmarx/google2fa-laravel, see https://github.com/asbiin/google2fa-laravel

Be careful to update the time of the machine/VM to make it works :

sudo service ntp stop
sudo ntpd -gq
sudo service ntp start

Option is not activated by default, update your .env to add
2FA_ENABLED=true

This is a 1st release, which can:

add a new device
verify authentication code when logging
remove a device (checked with authentication code)

Still need to do :

using some Vue layout for the 2FA activation
Add recovery codes
Nice documentation to explain how to use it

Signed-off-by: Alexis Saettler <alexis@saettler.org>

djaiss

Excellent work, it works on my machine so far. I've added a few comments.
Also, don't forget the localization as well!

Finally, what do you think of moving both 2FA and password change to a new Security tab under Settings?

djaiss · 2017-12-27T13:58:43Z

resources/views/auth/validate2fa.blade.php

+              {{ csrf_field() }}
+
+              @if ($errors->has('totp'))
+                                <span class="help-block">


Indentation seems off here.

djaiss · 2017-12-27T13:59:41Z

resources/views/auth/validate2fa.blade.php

+                                @endif
+              <div class="form-group">
+                <label for="one_time_password">Enter code</label>
+                <input type="number" class="form-control" id="one_time_password" name="one_time_password" />


You could put required here, to enable browser validation of the field.

djaiss · 2017-12-27T14:00:49Z

resources/views/auth/validate2fa.blade.php

+              </div>
+
+              <div class="form-group actions">
+                <button type="submit" class="btn btn-primary">Validate</button>


Would be nice to provide a Cancel button here to let users go back to the Login screen.

djaiss · 2017-12-27T14:03:25Z

resources/views/settings/2fa/enable.blade.php

+          {{-- code --}}
+              <div class="form-group">
+                <label for="one_time_password">Enter code</label>
+                <input type="number" class="form-control" id="one_time_password" name="one_time_password" />


You can add required parameter here a well.

djaiss · 2017-12-27T14:06:46Z

.env.example

 AWS_BUCKET=
 AWS_SERVER=
+
+2FA_ENABLED=false


Can you add a comment here on what 2FA is and does?

…, indentation, etc) Signed-off-by: Alexis Saettler <alexis@saettler.org>

Signed-off-by: Alexis Saettler <alexis@saettler.org>

…to 164-2FA-two-factor-auth

asbiin · 2018-01-07T19:49:06Z

SonarQube analysis reported 3 issues

2 minor
1 info

Watch the comments in this conversation to review them.

djaiss · 2018-01-07T22:40:13Z

database/migrations/2017_12_21_170327_add_google2fa_secret_to_users.php

+    public function up()
+    {
+        Schema::table('users', function ($table) {
+            $table->string('google2fa_secret')->nullable();


Could you put $table->string('google2fa_secret')->after('remember_token')->nullable(); instead?

That way, the new field won't be at the latest position in the table.

muglug · 2018-02-04T04:43:40Z

app/Http/Controllers/Settings/MultiFAController.php

+        $user = $request->user();
+
+        //generate image for QR barcode
+        $imageDataUri = Google2FA::getQRCodeInline(


Not sure if it's an issue, but static analysis says that this is an instance method, called statically

You're right, we should use app('pragmarx.google2fa')->getQRCodeInline instead.
Thanks !
Do you want to open an issue ?

Or just add it to the (great) PR #878

github-actions · 2021-02-15T00:11:49Z

This pull request has been automatically locked since there
has not been any recent activity after it was closed.
Please open a new issue for related bugs.

asbiin added 10 commits December 22, 2017 08:27

[WIP] Add google2fa depend and views

7db3df7

[WIP] Add check for code when enabling 2FA, improve views

b8f3803

Signed-off-by: Alexis Saettler <alexis@saettler.org>

Update composer

280498a

Signed-off-by: Alexis Saettler <alexis@saettler.org>

Add a Command to deactivate 2FA for a user

f5ef3f9

Signed-off-by: Alexis Saettler <alexis@saettler.org>

Add a new conf in the .env file to enable/disable 2FA

9c956a5

Signed-off-by: Alexis Saettler <alexis@saettler.org>

Use the login function from Authenticator

70cb16b

Signed-off-by: Alexis Saettler <alexis@saettler.org>

Add some tests

a6cbd94

Signed-off-by: Alexis Saettler <alexis@saettler.org>

Updating composer

b0ef8cc

Signed-off-by: Alexis Saettler <alexis@saettler.org>

Merge branch 'master' into 164-2FA-two-factor-auth

df3b0a4

Commit StyleCI analysis

a1c1468

Signed-off-by: Alexis Saettler <alexis@saettler.org>

djaiss requested changes Dec 27, 2017

View reviewed changes

.env.example

AWS_BUCKET=

AWS_SERVER=

2FA_ENABLED=false

Copy link

Member

djaiss Dec 27, 2017

Choose a reason for hiding this comment

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

Can you add a comment here on what 2FA is and does?

djaiss assigned asbiin Dec 27, 2017

djaiss added feature request security labels Dec 27, 2017

asbiin added 15 commits January 1, 2018 21:40

Add a Security item in the sidebar, and fix other stuffs (translation…

e49f927

…, indentation, etc) Signed-off-by: Alexis Saettler <alexis@saettler.org>

Update lang file (en) that was missed on the last commit

63bc2f3

Signed-off-by: Alexis Saettler <alexis@saettler.org>

Add a comment to the env example file

6f44483

Signed-off-by: Alexis Saettler <alexis@saettler.org>

Renaming controller, add good return message

0d37211

Signed-off-by: Alexis Saettler <alexis@saettler.org>

StyleCI recommandations

c8c8266

Signed-off-by: Alexis Saettler <alexis@saettler.org>

Merge branch 'master' into 164-2FA-two-factor-auth

db98f2c

Merge branch 'master' into 164-2FA-two-factor-auth

b81a417

Update messages

6006cf6

Use app laravel function

663db7c

Change route for 2fa validation

42260cf

Fix layout and some messages

2c5b1db

Merge branch 'master' into 164-2FA-two-factor-auth

a740b68

Merge branch 'master' into 164-2FA-two-factor-auth

b37c399

Fix styleci

28380f2

Merge branch '164-2FA-two-factor-auth' of github.com:asbiin/monica in…

fbe4586

…to 164-2FA-two-factor-auth

asbiin added 2 commits January 6, 2018 02:58

Add texts to every lang files

efe87c5

Fix stylceci

8d0e1bf

asbiin changed the title ~~[wip] 2FA Two Authentication Factor #164~~ 2FA Two Authentication Factor #164 Jan 6, 2018

asbiin added 4 commits January 6, 2018 03:17

Add some infos

151535b

Merge branch 'master' into 164-2FA-two-factor-auth

a7c0cfa

Merge branch 'master' into 164-2FA-two-factor-auth

0b0f393

Merge branch 'master' into 164-2FA-two-factor-auth

d2159bb

djaiss requested changes Jan 7, 2018

View reviewed changes

Review AddGoogle2faSecretToUsers

06612eb

djaiss merged commit 620c5de into monicahq:master Jan 8, 2018

turtles2 pushed a commit to turtles2/monica that referenced this pull request Jan 9, 2018

Add Two Factor Authentication (2FA) (monicahq#724)

29b9e3d

asbiin deleted the 164-2FA-two-factor-auth branch January 9, 2018 08:33

muglug reviewed Feb 4, 2018

View reviewed changes

github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2021

Uh oh!

Conversation

asbiin commented Dec 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

djaiss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asbiin commented Jan 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

asbiin commented Dec 27, 2017 •

edited

Loading