[+] Authentication - Add two factor authentication on forest-rails #285

ArnaudValensi · 2018-09-03T10:09:27Z

To test

Use feature/2fa-rails on the front, and errors will be better with feature/2fa-rails on the server.

and set the premiumTwoFactor in the project model.

Litterature

Wrapped exceptions: https://pupeno.com/2014/02/05/wrapped-exceptions-in-ruby/

Pull Request checklist:

Write an explicit title for the Pull Request
Write changes made in the CHANGELOG.md
Create automatic tests
No automatic tests failures
Test manually the implemented changes
Review my own code (indentation, syntax, style, simplicity, readability)
Wonder if you can improve the existing code

ArnaudValensi · 2018-09-03T10:09:45Z

(8) 2FA - Rails - Registration & Login

ArnaudValensi · 2018-09-06T15:22:45Z

lib/forest_liana/version.rb

@@ -1,3 +1,3 @@
 module ForestLiana
-  VERSION = "2.11.11"


I will remove the commit with this change before merging. But with this it should work out of the box.

ArnaudValensi · 2018-09-06T16:00:15Z

config/initializers/errors.rb

+        recursively_print(self)
+      end
+
+      def recursively_print(error, margin="")


It will display recursively all the resons and then the stack trace of the deeper exception (since the higher stacks are contained in the deepest).

Example:

Cannot reach the forest server which was caused by: Unauthorized ...then all stacktrace...

ArnaudValensi · 2018-09-06T16:01:46Z

app/controllers/forest_liana/sessions_controller.rb

-    end
+      rescue ForestLiana::Errors::ExpectedError => error
+        error.display_error
+        error_data = JSONAPI::Serializer.serialize_errors([{ detail: error.message }])


Only the higher exception message is returned, so we just need to catch and rethrow to hide a sensitive exception message.

arnaudbesnier

The feature works well.

arnaudbesnier · 2018-09-06T20:46:55Z

CHANGELOG.md

@@ -1,6 +1,8 @@
 # Change Log

 ## [Unreleased]
+### Added
+- Authentication - Add two factor authentication on forest-rails.


We are in the Forest Rails liana, not necessary to specify this here ;)
Authentication - Add two factor authentication

arnaudbesnier · 2018-09-07T06:40:56Z

config/routes.rb

-  post 'sessions' => 'sessions#create'
-  post 'sessions-google' => 'sessions#create_with_google'
+  post 'sessions' => 'sessions#login_with_password'
+  post 'sessions-google' => 'sessions#login_with_google'


It is a session object creation so sessions#create was a better wording in the REST logic.
=> create_with_password

arnaudbesnier · 2018-09-07T06:50:11Z

app/controllers/forest_liana/login_handler.rb

+        end
+      end
+
+      return { 'token' => create_token(user, @rendering_id) };


{ token: create_token(user, @rendering_id) }?
(without trailing semicolon)

arnaudbesnier · 2018-09-07T06:55:40Z

app/controllers/forest_liana/login_handler.rb

+        FOREST_LOGGER.error 'Cannot use the two factor authentication because the environment variable "FOREST_2FA_SECRET_SALT" is not set.'
+        FOREST_LOGGER.error 'You can generate it using this command: `$ openssl rand -hex 10`'
+
+        raise ForestLiana::Errors::HTTP401Error.new('Invalid 2FA configuration, please ask more information to your admin')


As you already are in the ForestLiana module, I guess you can simplify with raise Errors::HTTP401Error.new('...')

arnaudbesnier · 2018-09-07T07:10:54Z

app/services/forest_liana/forest_api_requester.rb

      end
-      route
+
+      response


def self.check_configuration_errors(response) if response.is_a?(Net::HTTPNotFound) error_message = 'Cannot retrieve the data from the Forest server. Can you check that you properly copied the Forest envSecret in the Liana initializer?' elsif response.is_a?(Net::HTTPUnprocessableEntity) error_message = 'Cannot retrieve the project you\'re trying to unlock. The envSecret and renderingId seems to be missing or inconsistent.' end raise Errors::HTTP401Error.new(error_message) if error_message response end

looks simpler to me

app/controllers/forest_liana/sessions_controller.rb

arnaudbesnier · 2018-09-07T08:02:35Z

app/controllers/forest_liana/login_handler.rb

+            TwoFactorRegistrationConfirmer.new(@project_id, @use_google_authentication, @auth_data)
+              .perform
+
+            return { 'token' => create_token(user, @rendering_id) };


{ token: create_token(...) }

app/services/forest_liana/google_authorized_user_getter.rb

config/initializers/errors.rb

arnaudbesnier · 2018-09-07T09:18:13Z

config/initializers/errors.rb

+      def initialize(message = "Unauthorized")
+        super(401, :unauthorized, message)
+      end
+    end
  end
 end


Another case, on TOTP token submit if Forest Server is down. Here is the logs we have:

The technical error is not really interesting, the only thing we should say is that the service didn't work because Forest looks to be down or in maintenance. Providing traces will make the developer find the reason for the crash.
But he/she will work for nothing.

Yes, in this case I agree, I will improve this.

Fixed:

And about the trace, it is in debug, this mean if it appears it is because someone want to see a maximum of informations.

arnaudbesnier · 2018-09-10T20:59:58Z

config/initializers/errors.rb

+          FOREST_LOGGER.info "#{margin}which was caused by:"
+          recursively_print(error.cause, "#{margin} ")
+        else
+          FOREST_LOGGER.debug error.backtrace.join("\n")


Is it suppose to handle Ruby < 2.1.0 case? It looks like it prints something at the end of the recursion too.

I think it's good, if we use ruby < 2.1.0, we display the exception message and the stack trace.

app/services/forest_liana/forest_api_requester.rb

arnaudbesnier · 2018-09-10T21:03:08Z

app/services/forest_liana/forest_api_requester.rb

+        body: body.to_json,
+      }).response
+
+      check_configuration_errors(response)


Shouldn't be the same pattern here with a begin/rescue like you did in the self.get?

arnaudbesnier · 2018-09-10T21:12:26Z

app/services/forest_liana/permissions_getter.rb

+          raise "Cannot retrieve the data from the Forest server. An error occured in Forest API."
+        end
+      rescue
+        raise Errors::HTTP401Error


Why would the permission getter send a 401 if some internal error occurs on Forest Server?
It seems that this error will be rescued in the resources_controller and sent an internal error (500) if I read well the code. I am not sure we really want that.

I've improved and I've decided too also display the errors sent by the server to have a maximum of presision.

Here for permissions:
Server down

Bad secret

Here for authentication:

arnaudbesnier · 2018-09-11T05:08:30Z

app/controllers/forest_liana/sessions_controller.rb

+        use_google_authentication: false,
+        rendering_id: rendering_id,
+        project_id: project_id,
+        auth_data: { :email => email, :password => password },


foo: is just a simpler syntax for :foo =>

arnaudbesnier · 2018-09-11T05:13:03Z

app/services/forest_liana/forest_api_requester.rb

      end
-      route
+
+      raise Errors::HTTP401Error.new(error_message) if error_message


For a generic service, I am not sure it is a good idea to respond with a 401. Depending on the usage not sure we want to disconnect the end user.

app/services/forest_liana/two_factor_registration_confirmer.rb

arnaudbesnier · 2018-09-11T05:24:02Z

config/initializers/errors.rb

+
+        # NOTICE: Ruby < 2.1.0 doesn't have `cause`
+        if error.respond_to?(:cause) && !error.cause.nil?
+          FOREST_LOGGER.info "#{margin}which was caused by:"


Why not FOREST_LOGGER.error?

Because if a user us not authorized, it is not an error in the perspective of the liana server. This is like the expected/unexpected errors in forestadmin-server.

config/initializers/errors.rb

app/controllers/forest_liana/login_handler.rb

config/initializers/errors.rb

app/controllers/forest_liana/sessions_controller.rb

ArnaudValensi self-assigned this Sep 3, 2018

ArnaudValensi force-pushed the feature/2fa-rails branch 21 times, most recently from 27ed514 to 52675df Compare September 6, 2018 15:06

ArnaudValensi commented Sep 6, 2018

View reviewed changes

ArnaudValensi force-pushed the feature/2fa-rails branch from 52675df to 7a66900 Compare September 6, 2018 15:23

ArnaudValensi requested a review from arnaudbesnier September 6, 2018 15:26

ArnaudValensi assigned arnaudbesnier and unassigned ArnaudValensi Sep 6, 2018

ArnaudValensi force-pushed the feature/2fa-rails branch 2 times, most recently from 7381d31 to 4528e8d Compare September 6, 2018 15:43

ArnaudValensi commented Sep 6, 2018

View reviewed changes

arnaudbesnier suggested changes Sep 7, 2018

View reviewed changes

arnaudbesnier assigned ArnaudValensi and unassigned arnaudbesnier Sep 7, 2018

ArnaudValensi requested a review from arnaudbesnier September 10, 2018 09:38

ArnaudValensi assigned arnaudbesnier and unassigned ArnaudValensi Sep 10, 2018

arnaudbesnier force-pushed the feature/2fa-rails branch from d9bb270 to 79faed0 Compare September 10, 2018 20:52

arnaudbesnier suggested changes Sep 11, 2018

View reviewed changes

arnaudbesnier assigned ArnaudValensi and unassigned arnaudbesnier Sep 11, 2018

ArnaudValensi requested a review from arnaudbesnier September 11, 2018 14:51

ArnaudValensi assigned arnaudbesnier and unassigned ArnaudValensi Sep 11, 2018

ArnaudValensi commented Sep 12, 2018

View reviewed changes

app/controllers/forest_liana/login_handler.rb Outdated Show resolved Hide resolved

ArnaudValensi commented Sep 12, 2018

View reviewed changes

config/initializers/errors.rb Outdated Show resolved Hide resolved

ArnaudValensi commented Sep 12, 2018

View reviewed changes

app/controllers/forest_liana/sessions_controller.rb Outdated Show resolved Hide resolved

arnaudbesnier approved these changes Sep 12, 2018

View reviewed changes

arnaudbesnier assigned ArnaudValensi and unassigned arnaudbesnier Sep 12, 2018

ArnaudValensi force-pushed the feature/2fa-rails branch 2 times, most recently from 4e8f686 to 35d10c2 Compare September 12, 2018 15:11

[+] Authentication - Add two factor authentication on forest-rails

f3f1515

ArnaudValensi force-pushed the feature/2fa-rails branch 2 times, most recently from d7f6515 to f3f1515 Compare September 12, 2018 16:00

ArnaudValensi merged commit 6d22f38 into devel Sep 13, 2018

arnaudbesnier deleted the feature/2fa-rails branch January 30, 2019 16:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[+] Authentication - Add two factor authentication on forest-rails #285

[+] Authentication - Add two factor authentication on forest-rails #285

ArnaudValensi commented Sep 3, 2018 •

edited

Loading

ArnaudValensi commented Sep 3, 2018

ArnaudValensi Sep 6, 2018 •

edited

Loading

ArnaudValensi Sep 6, 2018 •

edited

Loading

ArnaudValensi Sep 6, 2018 •

edited

Loading

arnaudbesnier left a comment

arnaudbesnier Sep 6, 2018

arnaudbesnier Sep 7, 2018

arnaudbesnier Sep 7, 2018

arnaudbesnier Sep 7, 2018

arnaudbesnier Sep 7, 2018

arnaudbesnier Sep 7, 2018

arnaudbesnier Sep 7, 2018

ArnaudValensi Sep 7, 2018

ArnaudValensi Sep 10, 2018

arnaudbesnier Sep 10, 2018

ArnaudValensi Sep 11, 2018

arnaudbesnier Sep 10, 2018

arnaudbesnier Sep 10, 2018

ArnaudValensi Sep 11, 2018

arnaudbesnier Sep 11, 2018

arnaudbesnier Sep 11, 2018

arnaudbesnier Sep 11, 2018

ArnaudValensi Sep 11, 2018

[+] Authentication - Add two factor authentication on forest-rails #285

[+] Authentication - Add two factor authentication on forest-rails #285

Conversation

ArnaudValensi commented Sep 3, 2018 • edited Loading

To test

Litterature

Pull Request checklist:

ArnaudValensi commented Sep 3, 2018

ArnaudValensi Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

ArnaudValensi Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

ArnaudValensi Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

arnaudbesnier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArnaudValensi commented Sep 3, 2018 •

edited

Loading

ArnaudValensi Sep 6, 2018 •

edited

Loading

ArnaudValensi Sep 6, 2018 •

edited

Loading

ArnaudValensi Sep 6, 2018 •

edited

Loading