Add Chef Automate support to `inspec compliance login` #2203

jerryaldrichiii · 2017-10-02T22:01:26Z

This provides a single interface for logging into either Chef Automate or Chef Compliance servers. Server type is evaluated at run time via HTTP responses from designated endpoints.

This also moves the login logic from Compliance::ComplianceCLI to a separate set of modules in Compliance::API. Thus removing logic from Thor and allowing for more in depth Unit testing.

This also adds deprecation warnings for login_automate and --usertoken.

This fixes #1795.

Signed-off-by: Jerry Aldrich jerryaldrichiii@gmail.com

This provides a single interface for logging into either Chef Automate or Chef Compliance servers. Server type is evaluated at run time via HTTP responses from designated endpoints. This also moves the login logic from `Compliance::ComplianceCLI` to a separate set of modules in `Compliance::API`. This removes logic from Thor and allows for more in depth Unit testing. Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

adamleff

Just flipped through this quickly, some comments and questions, probably worth a Zoom review when it's no longer WIP.

adamleff · 2017-10-03T22:18:40Z

lib/bundles/inspec-compliance/api.rb


  # API Implementation does not hold any state by itself,
  # everything will be stored in local Configuration store
  class API # rubocop:disable Metrics/ClassLength
+


nit: extra newline at class beginning

Pick all the nits! Fixed.

adamleff · 2017-10-03T22:19:16Z

lib/bundles/inspec-compliance/api.rb

@@ -238,5 +243,11 @@ def self.server_version_from_config(config)
      return nil unless config['version'].is_a?(Hash)
      config['version']['version']
    end
+
+    def self.determine_server_type(url, insecure)
+      return 'automate' if Compliance::HTTP.get(url + '/compliance/version', nil, insecure).code == '401'


It's more idiomatic to return symbols for something like this rather than strings.

return :automate if ...

I always prefer going the idiomatic route. Fixed. Thanks @adamleff!

adamleff · 2017-10-03T22:20:23Z

lib/bundles/inspec-compliance/api.rb

+    def self.determine_server_type(url, insecure)
+      return 'automate' if Compliance::HTTP.get(url + '/compliance/version', nil, insecure).code == '401'
+      return 'compliance' if Compliance::HTTP.get(url + '/api/version', nil, insecure).code == '200'
+      raise CannotDetermineServerType


You should also add an exception message when raising.

raise CannotDetermineServerType, "Unable to determine if #{url} is a Compliance or Automate server"

Agreed. Fixed.

adamleff · 2017-10-03T22:21:50Z

lib/bundles/inspec-compliance/api/login.rb

+
+    options['url'] = options['server'] + '/compliance'
+    token = options['dctoken'] || options['token']
+    puts store_access_token(options, token)


I know it's a WIP, to just a reminder to remove this :)

store_access_token returns a string at this time. I will look at the flow and see if we should pass this up as a return from login.

Will investigate.

adamleff · 2017-10-03T22:24:28Z

lib/bundles/inspec-compliance/api/login.rb

+  def login(options)
+    options['server_type'] = Compliance::API.determine_server_type(options['server'], options['insecure'])
+
+    return Login::ComplianceServer.login(options) if options['server_type'] == 'compliance'


Will you be doing the Automate login here too? Probably worth Zooming to review the expected flow, etc. would be good when you're ready :) This mid-method return with no comments as to why is confusing me as to your intentions for this method and how Automate login will be handled.

This redirects to the login method of the ComplianceServer module if the server type is a Compliance server. I was returning here, since this will complete the login. Initially I had it just exit 0 at the end, but that was causing me issues with the Unit tests.

Will investigate.

I'd second @adamleff here - If you're switching between two sets of logic, and you've encapsulated one in a module, I'd expect the other logic - for Automate login - to also be encapsulated in a module (presumably Login::AutomateServer).

Originally I thought only to encapsulate the Compliance server stuff because it would be deprecated in the future leaving only Login...but the future is not now. I see your point.

Fixed.

@adamleff let me know what you think of the flow now that I've modified the namespacing.

adamleff · 2017-10-03T22:25:56Z

lib/bundles/inspec-compliance/api/login.rb

+  private
+
+  def store_access_token(options, token)
+    token_info = { type: 'dctoken', msg: 'data colletor token' } if options['dctoken']


Will you ever not have either a dctoken or a token? What happens if token_info is not set?

I'd recommend changing it to something like:

token_info = if options['dctoken'] { # dctoken stuff } else { # non-dctoken stuff } end

I agree. Fixed.

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

jerryaldrichiii · 2017-10-03T23:37:55Z

Agreed, a Zoom review would be helpful! Thanks for the initial feedback @adamleff.

clintoncwolfe · 2017-10-04T14:00:39Z

We should also update the README docs to reflect that the inspec compliance login_automate command is no longer present. See the README in the bundle. You've already removed it from the Thor-based CLI help, awesome!

clintoncwolfe · 2017-10-04T14:14:56Z

lib/bundles/inspec-compliance/cli.rb

-    option :insecure, aliases: :k, type: :boolean,
-      desc: 'Explicitly allows InSpec to perform "insecure" SSL connections and transfers'
-    def login_automate(server) # rubocop:disable Metrics/AbcSize
+    def login(server) # rubocop:disable Metrics/AbcSize


You don't need the rubocop disable flag here anymore (when you refactored this out, the method became much simpler). You can run a rubocop scan with
bundle exec rake lint

Great find! Fixed.

Agreed, nice catch! Fixed.

clintoncwolfe · 2017-10-04T14:30:49Z

lib/bundles/inspec-compliance/api.rb


  # API Implementation does not hold any state by itself,
  # everything will be stored in local Configuration store
  class API # rubocop:disable Metrics/ClassLength
+    extend Login


This may be a matter of taste, but I'd usually have the module name here more deeply namespaced - like Compliance::API::Login . I don't know that we'd ever (within InSpec) have a name collision with a module named Login, but being specific avoids that possibility.

Agreed! Fixed.

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

jerryaldrichiii · 2017-10-04T16:20:31Z

@clintoncwolfe I was certain I did a grep -R login_automate at one point and removed all references! Must have stashed and forgot about it somewhere.

Great catch! README.md in bundle is updated. I also removed references to --usertoken in all the places.

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

jerryaldrichiii · 2017-10-04T20:42:26Z

Changing this PR to target master. This will not remove support for login_automate or --usertoken. Instead it will provide a deprecation warning. We will remove both in InSpec 2.0.

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

adamleff

This is in great shape, @jerryaldrichiii. I'd like to see a bit of cleanup around where we raise exceptions and keeping method purposes focused. Let me know if any of my feedback is unclear.

adamleff · 2017-10-16T16:25:19Z

lib/bundles/inspec-compliance/api/login.rb

+  class API
+    module Login
+      def login(options)
+        options['server_type'] = Compliance::API.determine_server_type(options['server'], options['insecure']).to_s


Coercing to a string here seems... weird? We don't need it to be a string here or most of these methods -- only when we store the config to disk. Can we move the coercion there?

adamleff · 2017-10-16T16:29:06Z

lib/bundles/inspec-compliance/api.rb

+    def self.determine_server_type(url, insecure)
+      return :automate if Compliance::HTTP.get(url + '/compliance/version', nil, insecure).code == '401'
+      return :compliance if Compliance::HTTP.get(url + '/api/version', nil, insecure).code == '200'
+      raise CannotDetermineServerType, "Unable to determine if #{url} is a Chef Automate or Chef Compliance server"


Just tossing out a thought here... I prefer to have exceptions raised at the point of the implementation where "it matters" (for lack of a better phrase). I'd almost prefer to see this method return nil if it can't figure out the server type, and let the caller determine how to handle. That way other methods can use this if needed without having to worry about the stack unwinding on them.

def self.determine_server_type(url, insecure) if Compliance::HTTP.get(url + '/compliance/version', nil, insecure).code == '401' :automate elsif Compliance::HTTP.get(url + '/api/version', nil, insecure).code == '200' :compliance end end

adamleff · 2017-10-16T16:30:45Z

lib/bundles/inspec-compliance/api/login.rb

+      def login(options)
+        options['server_type'] = Compliance::API.determine_server_type(options['server'], options['insecure']).to_s
+
+        return Login::ComplianceServer.login(options) if options['server_type'] == 'compliance'


Here is where I'd raise an exception if we don't know the server type since this is actually where an error should be raised to the user:

if options['server_type'] == :compliance Login::ComplianceServer.login(options) elsif options['server_type'] == :automate Login::AutomateServer.login(options) else raise CannotDetermineServerType, # ..... end

adamleff · 2017-10-16T16:36:06Z

lib/bundles/inspec-compliance/api/login.rb

+
+          options['url'] = options['server'] + '/compliance'
+          token = options['dctoken'] || options['token']
+          puts store_access_token(options, token)


I realize I'm being a bit pedantic here, but it feels strange to me to have a method called store_access_token return a string intended to be user-facing messaging.

I'd rather see store_access_token return the Compliance::Configuration object it generated to successfully write the config to disk, and then a method that takes that config object and formats a message to the user. It will keep the concerns of the methods succinct and clear. This honestly just reads like a debug message to me. :)

config = store_access_token(options, token) # print out a success message to the user puts format_status_message(config)

adamleff · 2017-10-16T16:36:59Z

lib/bundles/inspec-compliance/api/login.rb

+
+          raise msg unless success
+
+          puts msg


Same feedback as above - I bet there's a good opportunity for reuse here, actually.

adamleff · 2017-10-16T16:41:37Z

lib/bundles/inspec-compliance/cli.rb

-        msg = login_automate_config(url, options['user'], options['dctoken'], options['usertoken'], options['ent'], options['insecure'])
-        puts '', msg
-        exit 0
+      if options['usertoken']


Despite what was in the issue that preceded this PR, we probably don't need this deprecation here since the whole command is deprecated. As long as the inspec compliance login command doesn't accept a --usertoken flag, we're covered.

This does the following: - Moves `CannotDetermineServerType` error to `.login` - Changes methods that store configuration to return the configuration - Moves user output to one location in `.login` - Makes other small improvements Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

jerryaldrichiii · 2017-10-16T21:44:10Z

I made all the changes you suggested @adamleff. Feel free to give it another look! 😄

adamleff

Lookin' good, @jerryaldrichiii - thanks!

chris-rock

@jerryaldrichiii Awesome work. I just have some mini-questions before we can go forward

chris-rock · 2017-10-17T12:55:44Z

lib/bundles/inspec-compliance/api.rb

@@ -5,13 +5,16 @@
 require 'net/http'
 require 'uri'

+require_relative 'api/login'


should we move this file to the api directory too? Feels strange to have a api.rb and api directory. What do you think?

That's actually pretty standard in Ruby.

Yeah, it's common in Ruby. I'm fine with it if you are @chris-rock.

chris-rock · 2017-10-17T12:56:33Z

lib/bundles/inspec-compliance/api.rb

@@ -238,5 +241,13 @@ def self.server_version_from_config(config)
      return nil unless config['version'].is_a?(Hash)
      config['version']['version']
    end
+
+    def self.determine_server_type(url, insecure)
+      if Compliance::HTTP.get(url + '/compliance/version', nil, insecure).code == '401'


Could this also return 200 if the user is already logged in?

I'm not passing any headers to the GET request so it should always act as if the user is not logged in.

chris-rock · 2017-10-17T12:56:52Z

lib/bundles/inspec-compliance/api/login.rb

+
+        case options['server_type']
+        when :automate
+          config = Login::AutomateServer.login(options)


Nice separation!

Thanks! Took some work that's for sure 😅

chris-rock · 2017-10-17T12:57:24Z

lib/bundles/inspec-compliance/cli.rb

-      end
-
-      puts '', msg
+      Compliance::API.login(options)


Nice clean-up! 👍

Thanks! 😃

chris-rock

Works like a charm!

jerryaldrichiii requested a review from a team as a code owner October 2, 2017 22:01

jerryaldrichiii added the in progress label Oct 2, 2017

adamleff reviewed Oct 3, 2017

View reviewed changes

jerryaldrichiii added 3 commits October 3, 2017 18:23

Remove empty line below class definition

223020a

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

Add message to raise CannotDetermineServerType

54d78d5

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

Refactor token_info assignment

192b275

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

clintoncwolfe reviewed Oct 4, 2017

View reviewed changes

jerryaldrichiii added 3 commits October 4, 2017 09:54

Remove unnecessary rubocop disable

6f3d299

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

Modify Login module namespacing

3e0fbdd

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

Remove mentions of login_automate and --usertoken

f7d36f1

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

jerryaldrichiii force-pushed the ja/merge-login-commands branch from 7627db0 to f7d36f1 Compare October 4, 2017 16:19

Modify determine_server_type to return a symbol

bf139b0

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

jerryaldrichiii changed the title ~~WIP: Merge login and login_automate commands~~ Merge login and login_automate commands Oct 4, 2017

jerryaldrichiii changed the base branch from release-2.0 to master October 4, 2017 20:40

jerryaldrichiii changed the title ~~Merge login and login_automate commands~~ Add Chef Automate support to inspec compliance login Oct 4, 2017

jerryaldrichiii added 2 commits October 4, 2017 15:42

Add support for login_automate and --usertoken

c0a0a67

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

Fix encoding typo

5448731

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

jerryaldrichiii force-pushed the ja/merge-login-commands branch from f1f9c49 to 5448731 Compare October 4, 2017 20:55

adamleff added the Type: Enhancement Improves an existing feature label Oct 5, 2017

adamleff suggested changes Oct 16, 2017

View reviewed changes

adamleff approved these changes Oct 17, 2017

View reviewed changes

chris-rock reviewed Oct 17, 2017

View reviewed changes

chris-rock approved these changes Oct 26, 2017

View reviewed changes

chris-rock merged commit 91403d8 into master Oct 26, 2017

chris-rock deleted the ja/merge-login-commands branch October 26, 2017 15:32

chris-rock removed the in progress label Oct 26, 2017

jerryaldrichiii mentioned this pull request Feb 1, 2018

Remove deprecations for InSpec 2.0 #2506

Merged

Add Chef Automate support to inspec compliance login #2203

Add Chef Automate support to inspec compliance login #2203

Uh oh!

Conversation

jerryaldrichiii commented Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamleff 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!

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!

jerryaldrichiii Oct 4, 2017 • 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!

jerryaldrichiii commented Oct 3, 2017

Uh oh!

clintoncwolfe commented Oct 4, 2017

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!

jerryaldrichiii commented Oct 4, 2017

Uh oh!

jerryaldrichiii commented Oct 4, 2017

Uh oh!

adamleff 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!

Choose a reason for hiding this comment

Uh oh!

jerryaldrichiii commented Oct 16, 2017

Uh oh!

adamleff left a comment

Choose a reason for hiding this comment

Uh oh!

Add Chef Automate support to `inspec compliance login` #2203

Add Chef Automate support to `inspec compliance login` #2203

jerryaldrichiii commented Oct 2, 2017 •

edited

Loading

jerryaldrichiii Oct 4, 2017 •

edited

Loading