Improving flows and rules around user creation

CHANGELOG.md

@@ -6,6 +6,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 
 ## [Unreleased]
 
+### Changed
+- Improved flows and rules around user creation
+
 ## [1.4.6] - 2020-01-21
 
 ### Changed

app/controllers/credentials_controller.rb

@@ -35,9 +35,9 @@ class CredentialsController < ApplicationController
   # This method requires a PUT request. The new password is in the request body.
   def update_password
     password = request.body.read
-    
+
     @role.credentials.password = password
-    @role.credentials.save
+    @role.credentials.save(raise_on_save_failure: true)
     head 204
   end
 

app/domain/errors.rb

@@ -19,6 +19,12 @@ module Conjur
         code: "CONJ00037E"
       )
 
+      InsufficientPasswordComplexity = Util::TrackableErrorClass.new(
+        msg: "The password you have chosen does not meet the complexity requirements. " \
+             "Choose a password that includes: 12-128 characters, 2 uppercase letters, 2 lowercase letters, 1 digit, 1 special character",
+        code: "CONJ00046E"
+      )
+
     end
 
     module Authentication

app/models/credentials.rb

@@ -7,6 +7,13 @@ class Credentials < Sequel::Model
   # Bcrypt work factor, minimum recommended work factor is 12
   BCRYPT_COST = 12
 
+  # special characters according to https://www.owasp.org/index.php/Password_special_characters
+  VALID_PASSWORD_REGEX = %r{^(?=.*?[A-Z].*[A-Z])                             # 2 uppercase letters
+                             (?=.*?[a-z].*[a-z])                             # 2 lowercase letters
+                             (?=.*?[0-9])                                    # 1 digit
+                             (?=.*[ !"#$%&'()*+,-.\/:;<=>?@\[\\\]^_`{|}~]).  # 1 special character
+                             {12,128}$}x                                     # 12-128 characters
+
   plugin :validation_helpers
 
   unrestrict_primary_key
@@ -23,7 +30,7 @@ def random_api_key
       Slosilo::Random.salt.unpack("N*").map{|i| Base32::Crockford::encode(i)}.join.downcase
     end
   end
-  
+
   def as_json
     { }
   end
@@ -63,8 +70,7 @@ def validate
 
     validates_presence [ :api_key ]
 
-    errors.add(:password, 'must not be blank') if @plain_password && @plain_password.empty?
-    errors.add(:password, 'cannot contain a newline') if @plain_password && @plain_password.index("\n")
+    errors.add(:password, ::Errors::Conjur::InsufficientPasswordComplexity.new.to_s) if @plain_password && (@plain_password.index("\n") || @plain_password !~ VALID_PASSWORD_REGEX)
   end
 
   def before_validation

ci/docker-compose.yml

@@ -13,7 +13,7 @@ services:
     image: "conjur:${TAG}"
     environment:
       DATABASE_URL: postgres://postgres@pg/postgres
-      CONJUR_ADMIN_PASSWORD: admin
+      CONJUR_ADMIN_PASSWORD: ADmin123!!!!
       CONJUR_ACCOUNT: cucumber
       CONJUR_DATA_KEY:
       RAILS_ENV:

cucumber/api/features/change_password.feature

@@ -10,14 +10,26 @@ Feature: Change the password of a role
 
   Scenario: With basic authentication, users can update their own password using the current password.
 
-    Given I set the password for "alice" to "my-password"
-    When I successfully PUT "/authn/cucumber/password" with username "alice" and password "my-password" and plain text body "new-password"
-    Then I can GET "/authn/cucumber/login" with username "alice" and password "new-password"
+    Given I set the password for "alice" to "My-Password1"
+    When I successfully PUT "/authn/cucumber/password" with username "alice" and password "My-Password1" and plain text body "New-Password1"
+    Then I can GET "/authn/cucumber/login" with username "alice" and password "New-Password1"
 
   Scenario: With basic authentication, users can update their own password using the current API key.
 
-    When I successfully PUT "/authn/cucumber/password" with username "alice" and password ":alice_api_key" and plain text body "new-password"
-    Then I can GET "/authn/cucumber/login" with username "alice" and password "new-password"
+    When I successfully PUT "/authn/cucumber/password" with username "alice" and password ":alice_api_key" and plain text body "New-Password1"
+    Then I can GET "/authn/cucumber/login" with username "alice" and password "New-Password1"
+
+  Scenario: Succeed to set password with escaped special character applying to password complexity.
+
+    Given I set the password for "alice" to "My-Password1"
+    When I successfully PUT "/authn/cucumber/password" with username "alice" and password "My-Password1" and plain text body "NewPassword1/"
+    Then I can GET "/authn/cucumber/login" with username "alice" and password "NewPassword1/"
+
+  Scenario: Fail to set password with 11 characters not applying to password complexity.
+
+    Given I set the password for "alice" to "My-Password1"
+    When I PUT "/authn/cucumber/password" with username "alice" and password "My-Password1" and plain text body "My-Passwor1"
+    Then the HTTP response status code is 422
 
   Scenario: Bearer token cannot be used to change the password
 

cucumber/api/features/login.feature

@@ -7,8 +7,8 @@ Feature: Exchange a role's password for its API key
     Given I create a new user "alice"
 
   Scenario: Password can be used to obtain API key
-    Given I set the password for "alice" to "my-password"
-    Then I can GET "/authn/cucumber/login" with username "alice" and password "my-password"
+    Given I set the password for "alice" to "My-Password1"
+    When I can GET "/authn/cucumber/login" with username "alice" and password "My-Password1"
     Then the result is the API key for user "alice"
 
   @logged-in

cucumber/api/features/rotate_api_key.feature

@@ -9,8 +9,8 @@ Feature: Rotate the API key of a role
     Given I create a new user "alice"
 
   Scenario: Password can be used to rotate API key
-    Given I set the password for "alice" to "my-password"
-    Then I can PUT "/authn/cucumber/api_key" with username "alice" and password "my-password"
+    Given I set the password for "alice" to "My-Password1"
+    When I can PUT "/authn/cucumber/api_key" with username "alice" and password "My-Password1"
     Then the result is the API key for user "alice"
 
   @logged-in

dev/docker-compose.yml

@@ -19,9 +19,9 @@ services:
     environment:
       CONJUR_APPLIANCE_URL: http://localhost:3000
       DATABASE_URL: postgres://postgres@pg/postgres
-      CONJUR_ADMIN_PASSWORD: admin
+      CONJUR_ADMIN_PASSWORD: ADmin123!!!!
       CONJUR_ACCOUNT: cucumber
-      CONJUR_PASSWORD_ALICE: secret
+      CONJUR_PASSWORD_ALICE: SEcret12!!!!
       CONJUR_DATA_KEY:
       RAILS_ENV:
       CONJUR_LOG_LEVEL: debug
@@ -50,7 +50,7 @@ services:
       LDAP_BASE: dc=conjur,dc=net
       CONJUR_APPLIANCE_URL: http://conjur:3000
       DATABASE_URL: postgres://postgres@pg/postgres
-      CONJUR_ADMIN_PASSWORD: admin
+      CONJUR_ADMIN_PASSWORD: ADmin123!!!!
       CONJUR_DATA_KEY:
       RAILS_ENV:
     volumes:

spec/controllers/authenticate_controller_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 describe AuthenticateController, :type => :controller do
-  let(:password) { "password" }
+  let(:password) { "The-Password1" }
   let(:login) { "u-#{random_hex}" }
   let(:account) { "rspec" }
   let(:authenticator) { "authn" }

spec/controllers/credentials_controller_spec.rb

@@ -45,7 +45,8 @@
   end
 
   context "#update_password" do
-    let(:new_password) { +"new-password" }
+    let(:new_password) { +"New-Password1" }
+    let(:insufficient_msg) { ::Errors::Conjur::InsufficientPasswordComplexity.new.to_s }
     context "without auth" do
       it "is unauthorized" do
         post :update_password, account: account, authenticator: authenticator
@@ -61,12 +62,12 @@
           {
             error: {
               code: "validation_failed",
-              message: "password must not be blank",
+              message: "password #{insufficient_msg}",
               details: [
                 {
                   code: "validation_failed",
                   target: "password",
-                  message: "must not be blank"
+                  message: insufficient_msg
                 }
               ]
             }
@@ -94,12 +95,12 @@
             {
               error: {
                 code: "validation_failed",
-                message: "password cannot contain a newline",
+                message: "password #{insufficient_msg}",
                 details: [
                   {
                     code: "validation_failed",
                     target: "password",
-                    message: "cannot contain a newline"
+                    message: insufficient_msg
                   }
                 ]
               }

spec/models/credentials_spec.rb

@@ -21,7 +21,7 @@
     describe '#encrypted_hash' do
       subject { the_user.credentials.encrypted_hash }
       context "when password is specified" do
-        let(:password) { "the-password" }
+        let(:password) { "The-Password1" }
         it { is_expected.not_to be_blank }
       end
       context "when password is not specified" do
@@ -47,8 +47,9 @@
     end
 
     describe '#password=' do
+      let(:insufficient_msg) { ::Errors::Conjur::InsufficientPasswordComplexity.new.to_s }
       context "with password" do
-        let(:password) { "the-password" }
+        let(:password) { "The-Password1" }
         it 'sets no password when given nil' do
           credentials.password = nil
           credentials.save
@@ -57,10 +58,70 @@
           expect(credentials.valid_password?(nil)).to be_falsey
         end
       end
+      it 'allows passwords with 128 characters long' do
+        credentials.password = "My Password134567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678"
+        expect { credentials.save }.not_to raise_error
+      end
+      it 'allows passwords with all acceptable special characters' do
+        credentials.password = "New-Password1 !\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~]"
+        expect { credentials.save }.not_to raise_error
+      end
+      it 'allows passwords with mixed languages (Russian and French)' do
+        credentials.password = "New!Password1БбИиàâæ"
+        expect { credentials.save }.not_to raise_error
+      end
       it 'disallows passwords with newlines' do
-        credentials.password = "foo\nbar"
+        credentials.password = "My-\nPasswor1"
+        expect { credentials.save }.to raise_error(Sequel::ValidationFailed) do |e|
+          expect(e.errors.to_h).to eq({ password: [ insufficient_msg ]})
+        end
+      end
+      it 'disallows passwords with 11 characters long' do
+        credentials.password = "My-Passwor1"
+        expect { credentials.save }.to raise_error(Sequel::ValidationFailed) do |e|
+          expect(e.errors.to_h).to eq({ password: [ insufficient_msg ]})
+        end
+      end
+      it 'disallows passwords with 129 characters long' do
+        credentials.password = "My-Password1345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789"
+        expect { credentials.save }.to raise_error(Sequel::ValidationFailed) do |e|
+          expect(e.errors.to_h).to eq({ password: [ insufficient_msg ]})
+        end
+      end
+      it 'disallows passwords with only 1 upper case character' do
+        credentials.password = "My-password12"
+        expect { credentials.save }.to raise_error(Sequel::ValidationFailed) do |e|
+          expect(e.errors.to_h).to eq({ password: [ insufficient_msg ]})
+        end
+      end
+      it 'disallows passwords with only 1 lower case character' do
+        credentials.password = "My-P12345671"
+        expect { credentials.save }.to raise_error(Sequel::ValidationFailed) do |e|
+          expect(e.errors.to_h).to eq({ password: [ insufficient_msg ]})
+        end
+      end
+      it 'disallows passwords without a digit' do
+        credentials.password = "My-Passworda"
+        expect { credentials.save }.to raise_error(Sequel::ValidationFailed) do |e|
+          expect(e.errors.to_h).to eq({ password: [ insufficient_msg ]})
+        end
+      end
+      it 'disallows passwords without a special character' do
+        credentials.password = "NewPassword1"
+        expect { credentials.save }.to raise_error(Sequel::ValidationFailed) do |e|
+          expect(e.errors.to_h).to eq({ password: [ insufficient_msg ]})
+        end
+      end
+      it 'disallows passwords with non supported special character' do
+        credentials.password = "New¶Password1"
+        expect { credentials.save }.to raise_error(Sequel::ValidationFailed) do |e|
+          expect(e.errors.to_h).to eq({ password: [ insufficient_msg ]})
+        end
+      end
+      it 'disallows passwords with empty password' do
+        credentials.password = ""
         expect { credentials.save }.to raise_error(Sequel::ValidationFailed) do |e|
-          expect(e.errors.to_h).to eq({ password: [ "cannot contain a newline" ]})
+          expect(e.errors.to_h).to eq({ password: [ insufficient_msg ]})
         end
       end
     end
@@ -69,7 +130,7 @@
   describe "authenticate" do
     before { credentials.save }
     context "with password" do
-      let(:password) { "the-password" }
+      let(:password) { "The-Password1" }
       it "returns true on good password" do
         expect(credentials.authenticate(password)).to be_truthy
         expect(credentials.valid_password?(password)).to be_truthy
@@ -100,7 +161,7 @@
         end
       end
       context "with password" do
-        let(:password) { "the-password" }
+        let(:password) { "The-Password1" }
         describe "when expired" do
           let(:expiration_time) { past }
           it "has a valid password" do