Two-factor authentication feature in UI part. by ecnelises · Pull Request #1729 · rubygems/rubygems.org

ecnelises · 2018-06-03T12:08:46Z

This pull request is related to Issue 1725, and my GSoC 2018 project, which has implemented features below.

Things to be done next:

Expire time of 2fa generated qr-code.
Two-factor authentication in sites' API part.
Settings on the authentication level (auth-only and auth-and-write).

sonalkr132 · 2018-06-11T09:40:47Z

+    else
+      do_login
+    end
+  end


This if else logic could be simpler if 'sessions/otp_prompt' made request to a new method like mfa_create.

yeah. and in mfa_create check session[:mfa_user] seems can achieve the same functionality

sonalkr132 · 2018-06-11T09:41:23Z


 BUNDLED WITH
-   1.16.0
+   1.16.1


Please use bundler 1.16.0

Do I need to specify it in Gemfile? Or just downgrade my Bundler locally and Gemfile.lock will be changed? Also, is there any difference between the two versions?

just downgrade my Bundler locally and Gemfile.lock will be changed?

yes.

is there any difference between the two versions?

changing bundler version here would mean we need to update bundler on our servers. we prefer to do that separately (not as part of any PR)

I've got it done. Thanks

sonalkr132 · 2018-06-11T09:49:07Z

+      flash[:success] = t('.disable_success')
+      current_user.disable_mfa!
+    else
+      flash[:error] = t('profiles.mfa_enable.otp_auth_failed')


I got t('profiles.mfa_enable.otp_auth_failed') as not found.

oh! sorry, that should be t('two_factor_auths.create.otp_auth_failed')

Is semantics of flash[:notice] more suitable here?

error is fine.

sonalkr132 · 2018-06-11T09:57:34Z

+    return unless current_user.no_auth?
+    flash[:error] = t('two_factor_auths.no_auth_no_access')
+    redirect_to edit_profile_path
+  end


Perhaps, require_mfa_enabled and require_mfa_disabled are better names for these functions. Also, it would make more sense to use current_user.mfa_enabled? here instead of current_user.no_auth?.

that really makes more sense, thanks

sonalkr132 · 2018-06-13T05:46:42Z

  end

-  private
+  def verifying_otp?


I don't think this method is used.

That should be deleted.

sonalkr132 · 2018-06-13T05:48:54Z

+
+  def require_mfa_enabled
+    return if current_user.mfa_enabled?
+    flash[:error] = t('two_factor_auths.no_auth_no_access')


Can we please update name of these i18n keys as well?

Yes. I'm curious that is there any tool for such i18n sync/generation (between multi-lang versions)?

I am not sure what do you mean by i18n sync/generation. Can you please elaborate?

Can we please update name of these i18n keys as well?

no_auth_no_access -> access_denied (or something more appropriate).

Maybe my words are not accurate, I just wanted to know whether there's tool for checking whether any part in i18n of some language is missing and automatically filling them.

Thanks for the advice, I'll do checks about all i18n names I created.

sonalkr132 · 2018-06-13T05:58:44Z

  validates :password, length: { within: 10..200 }, allow_nil: true, unless: :skip_password_validation?
  validate :unconfirmed_email_uniqueness

+  enum mfa_level: { no_auth: 0, auth_only: 1, auth_and_write: 2 }


Would renaming no_auth to mfa_disabled or just disabled make it easier to understand?

Since Rails enums generates methods like no_auth? and no_auth! for class User automatically, I think using disabled will be a little bit ambiguous

no_auth is not representing (mfa being disabled) what is says it is (no_auth ~ no authentication?).

(Also reply for the comment on mfa/two_factor_auth names)
Are tfa and two_factor_auth easily recognized as the same? If so, I think something like no_tfa is better name.

Because two_factor_auth and two_factor_authentication is too lengty...

no_tfa or no_mfa is better.

sonalkr132 · 2018-06-13T06:08:03Z

    @user = find_user(params.require(:session))

+    if mfa_enabled? && @user&.mfa_enabled?
+      session[:mfa_user] = @user.id


Can we please continue using handle (session[:who]) to find users?

You mean store who in session and find user with who, not id? That seems better in security.

Since we should not store passwords in session, here we just needs to store session[:who]. But in find_user, session is just a param, not real sessions. So it seems that we do this to just hide user's id.

sonalkr132 · 2018-06-13T15:43:17Z

You have used TwoFactor in controller name and mfa in function names. please use either one.

sonalkr132 · 2018-06-13T15:44:37Z

+      @request.cookies[:mfa_feature] = 'true'
+    end
+
+    context 'when 2fa already enabled' do


when 2fa is enabled

sonalkr132 · 2018-06-13T15:47:36Z

+      end
+    end
+
+    context 'when 2fa not enabled' do


when 2fa is disabled.

sonalkr132 · 2018-06-13T15:54:03Z

+          assert @user.no_auth?
+        end
+      end
+    end


Please add a test for mfa being disabled by default (without @user.disable_mfa! in setup).

got it, thanks.

sonalkr132 · 2018-06-13T15:57:17Z

Thank you for following single and double quotes as per file 😆 we will enforce one of them soon.

Can you please write integration tests as well?

ecnelises · 2018-06-13T16:50:55Z

Thank you for following single and double quotes as per file 😆 we will enforce one of them soon.

It would be so careful to find that! I also want to know if we have such spec on commit messages. (Forgive me for recent commits with bunch of unrelated changes, I'll take care of it)

Integration tests are similar to tests on controllers, it focuses on 'a feature' or 'a workflow'?

sonalkr132 · 2018-06-15T17:53:43Z

whether there's tool for checking whether any part in i18n of some language is missing and automatically filling them.

Not that I know of, however it should be too difficult to come up with a script of your own. We don't really need to create keys in all languages as rails falls back to default language where keys in other languages are missing. Since #1522 we started requiring that all keys be created.
Feel free to open a issue explaining your problem and need.

sonalkr132 · 2018-06-21T07:10:18Z

+        true
+      else
+        false
+      end


Please move the code else to a new method. Also, we would want to do mfa_recovery_codes.include?(otp) at last, ie this logic should be in else and totp.verify.. should be elsif.

sonalkr132 · 2018-06-21T07:12:40Z

+
+    should 'disable mfa by default' do
+      refute @user.mfa_enabled?
+    end


this belongs in unit test.

Right. I forgot to move it.

sonalkr132 · 2018-06-21T10:26:22Z

+        should redirect_to('the dashboard') { dashboard_path }
+        should "clear user name in session" do
+          assert @controller.session[:mfa_user].nil?
+        end


Please add an assert in this and one above (when OTP is correct), for user being logged in.

sonalkr132 · 2018-06-21T10:27:20Z

+      should respond_with :success
+      should "save user name in session" do
+        assert @controller.session[:mfa_user] == @user.handle
+      end


Please add test for assert page.has_content?

sonalkr132 · 2018-06-24T12:46:46Z

+      if is_recovery
+        mfa_recovery_codes.delete(otp)
+        save!(validate: false)
+      end


please change this to return false unless is_recovery. last line in otp_verified? could be just save!(validate: false)

sonalkr132 · 2018-06-24T12:51:26Z

+  end
+
+  def otp_verified?(otp)
+    if no_mfa? || verify_digit_otp(otp)


I don't think no_mfa? is needed here. As far as I can tell, opt_verified? is always called only after ensuring that mfa is enabled.

Yes. If remove no_mfa? here, a user who has not enabled mfa will get false for this method, seems more reasonable.

sonalkr132 · 2018-06-24T12:54:38Z

+      self.last_otp_at = Time.at(last_success).utc.to_datetime
+      save!(validate: false)
+    end
+    !last_success.nil?


if block here could be changed return false unless last_success. IMHO, guard statements are more readable.

sonalkr132 · 2018-06-24T12:58:32Z

+  def change
+    add_column :users, :last_otp_at, :datetime
+  end
+end


Please merge these two migration. You can fix ActiveRecord::Schema.define version: manually.

OK. I'll use the older version number.

sonalkr132 · 2018-06-24T13:03:21Z

+      end
+
+      should "show OTP prompt" do
+        assert page.has_content? "Multifactor authentication"


You don't need to do it twice.

sonalkr132 · 2018-06-30T16:51:06Z

+      mfa:
+        multifactor_auth: Multifactor authentication
+        disabled: You have not yet enabled multifactor authentication.
+        go_settings: Go to setting page.


Please change this to Register a new device

Add methods and migrations related to 2fa on user model.

Check user's auth settings before entering 2fa settings.

Correct warnings from Rubocop.

Fix bugs when checking users' auth state.

Change 2fa issuer to host of request. Add text 2fa key for manual input.

- Bundler version is reverted back to 1.16.0 in Gemfile.lock. - Add feature flag on 2FA, just set 'mfa_feature=true' in cookie to turn it on. - Move OTP post action into a single method `mfa_create`. - Change two filter names in 2fa settings controller to make it more readable.

- Test OTP requirement in login. - Test model methods of user on verifying OTP. - Test settings of 2fa.

Remove unused 2fa controller method and i18n entry.

- Locale item names is changed from 'two_factor_auths' to 'multifactor_auths'. - `TwoFactorAuths` are renamed into `MultifactorAuths`. - '2fa' in test descriptions is changed into 'mfa'. - `mfa_level` enum cases are now `no_mfa`, `mfa_login_only` and `mfa_login_and_write`.

- Add integration tests for user login with mfa and 2fa settings. - Move mfa default state test into user test. - Add controller test for otp prompt when user login with mfa enabled.

- Merge two mfa related migrations into one. - Refactor `otp_verified?` method using guard clauses. - Remove duplicate test case on OTP prompt. - Default returns false for `otp_verified?` if mfa not enabled yet.

sonalkr132 · 2018-07-04T14:36:07Z

Thanks @ecnelises

Kholoudatef-90 · 2018-07-04T16:33:03Z

Please help me to solve this problem, it doesn't create a .css file.

sonalkr132 · 2018-07-04T17:32:35Z

Hi @Kholoudatef-90
Welcome to github 🎉 I am afraid this is not the right place to get answers to problems you are facing. I think you will find this useful https://yihui.name/en/2017/08/so-gh-email/

2369: [GSoC] Multi-factor feature for RubyGems. r=hsbt a=ecnelises # Description: Hello. This is my GSoC project, dedicated to add multifactor authentication to both RubyGems command program and Gemcutter ([RubyGems.org](https://rubygems.org)). Work for the Gemcutter part has almost been finished (see [PR #1753](rubygems/rubygems.org#1753), [PR #1729](rubygems/rubygems.org#1729) and a series of [my progress reports](https://ecnelises.github.io/)). ## Content: This PR will contain my changes to RubyGems client, adding multifactor auth for `gem push`, `gem signin` and `gem owner` commands. Since no command for editing profile, adding command for changing multifactor auth settings seems unnecessary. ## Workflow: - User set up multifactor auth well in the site. (into `mfa_login_and_write` level) - When user does the actions requiring MFA, an OTP prompt is shown. Or user can add `--otp` option into command, like `gem push mygem-0.0.0.gem --otp 123456`. - If the OTP is incorrect, operation fails with failure text. ______________ # Tasks: - [x] Add OTP requirement to `push_command`. - [x] Add OTP requirement to `owner_command`. - [x] Add OTP prompt to `sign_in`. - [ ] Support for `yank_command`. - [x] Write related tests. I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md). Co-authored-by: Qiu Chaofan <fwage73@gmail.com> Co-authored-by: SHIBATA Hiroshi <hsbt@ruby-lang.org>

ecnelises force-pushed the feature-2fa branch 2 times, most recently from dd0ca63 to eba6f1b Compare June 6, 2018 01:52

sonalkr132 reviewed Jun 11, 2018

View reviewed changes

sonalkr132 added the needs-review label Jun 11, 2018

sonalkr132 reviewed Jun 13, 2018

View reviewed changes

ecnelises force-pushed the feature-2fa branch from ab6d95f to 3f44bfb Compare June 20, 2018 10:04

sonalkr132 reviewed Jun 21, 2018

View reviewed changes

ecnelises force-pushed the feature-2fa branch from ae020b3 to d84f2ac Compare June 24, 2018 06:31

sonalkr132 reviewed Jun 24, 2018

View reviewed changes

sonalkr132 reviewed Jun 30, 2018

View reviewed changes

ecnelises added 18 commits July 4, 2018 00:03

Add necessary gems dependency for otp and qr-code.

9363eb9

Add methods and migrations related to 2fa on user model.

Added 2fa settings page.

f0f1022

Move two factor auth stuff to a single controller.

32c9c8c

Check user's auth settings before entering 2fa settings.

Fill missing locale items.

38ab940

Correct warnings from Rubocop.

Add otp prompt page when enabled 2fa and do login.

40efa62

Fix bugs when checking users' auth state.

Show recovery codes after user successfully enabled 2fa.

184d49d

Change 2fa issuer to host of request. Add text 2fa key for manual input.

Add unit tests regarding 2fa feature in UI part.

b1e50dc

- Test OTP requirement in login. - Test model methods of user on verifying OTP. - Test settings of 2fa.

Improve 2fa related i18n entry names.

319dc0c

Remove unused 2fa controller method and i18n entry.

Add drift and prior attempt record for otp verification.

9b6d0e1

Store user name instead of id in 2fa login session.

1f30246

Change 2fa prompting words from 'two factor' to 'multifactor'.

2bf7bcf

Tweak on mfa settings page UI.

7cb8996

Move otp code verification into a single method.

68491f0

Add tests regarding UI 2fa.

82bae0c

- Add integration tests for user login with mfa and 2fa settings. - Move mfa default state test into user test. - Add controller test for otp prompt when user login with mfa enabled.

Tweaks on otp verification, test and migration.

5e26d87

- Merge two mfa related migrations into one. - Refactor `otp_verified?` method using guard clauses. - Remove duplicate test case on OTP prompt. - Default returns false for `otp_verified?` if mfa not enabled yet.

Change style and text of mfa-enabling entry.

a3d7883

ecnelises force-pushed the feature-2fa branch from 1b74e77 to a3d7883 Compare July 3, 2018 16:03

sonalkr132 merged commit 09fd787 into rubygems:master Jul 4, 2018

bundlerbot mentioned this pull request Jul 4, 2018

Add more zh_CN translate #1712

Closed

rubygems-deployer deployed to staging July 4, 2018 14:39 Active

sonalkr132 deployed to production July 9, 2018 18:20 Active

connorshea mentioned this pull request Jul 12, 2018

RFC: Add 2FA auth to RubyGems.org #1252

Closed

ecnelises mentioned this pull request Jul 27, 2018

[GSoC] Multi-factor feature for RubyGems. ruby/rubygems#2369

Merged

5 tasks

Uh oh!

Conversation

ecnelises commented Jun 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

sonalkr132 Jun 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

sonalkr132 commented Jun 13, 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!

sonalkr132 commented Jun 13, 2018

Uh oh!

ecnelises commented Jun 13, 2018

Uh oh!

sonalkr132 commented Jun 15, 2018

Uh oh!

Choose a reason for hiding this comment

ecnelises commented Jun 3, 2018 •

edited

Loading

sonalkr132 Jun 11, 2018 •

edited

Loading

ecnelises Jun 25, 2018 •

edited

Loading