18858: Adds username-based test cases.

Also, fixes user fixtures and improves railsAPI's debugging logs on database
reset failures.

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas.dipentima@curii.com>

diff --git a/services/api/app/controllers/database_controller.rb b/services/api/app/controllers/database_controller.rb
index fa1e1ca43c64dc0b98a0587e703f0a075e890dae..69453959d262a792b7f09edca6b6557e8a5d8a4b 100644 (file)
--- a/services/api/app/controllers/database_controller.rb
+++ b/services/api/app/controllers/database_controller.rb
@@ -25,7 +25,7 @@ class DatabaseController < ApplicationController
     unexpected_uuids = user_uuids - fixture_uuids
     if unexpected_uuids.any?
       logger.error("Running in test environment, but non-fixture users exist: " +
-                   "#{unexpected_uuids}")
+                   "#{unexpected_uuids}" + "\nMaybe test users without @example.com email addresses were created?")
       raise ArvadosModel::PermissionDeniedError
     end
 

diff --git a/services/api/test/fixtures/users.yml b/services/api/test/fixtures/users.yml
index 1cf79b1a505405ca920de7cb71b85bb81fb90545..1d9bcbb040ab7c5dd955361704da7b9a81a3147b 100644 (file)
--- a/services/api/test/fixtures/users.yml
+++ b/services/api/test/fixtures/users.yml
@@ -12,6 +12,7 @@ system_user:
   modified_by_user_uuid: zzzzz-tpzed-000000000000000
   modified_at: 2014-11-27 06:38:21.208036000 Z
   email: root
+  username: root
   first_name: root
   last_name: ''
   identity_url:
@@ -193,6 +194,7 @@ inactive_uninvited:
   identity_url: https://inactive-uninvited-user.openid.local
   is_active: false
   is_admin: false
+  username: inactiveuninvited
   prefs: {}
 
 inactive:
@@ -216,6 +218,7 @@ inactive_but_signed_user_agreement:
   identity_url: https://inactive-but-agreeable-user.openid.local
   is_active: false
   is_admin: false
+  username: inactiveusersignedua
   prefs:
     profile:
       organization: example.com
@@ -230,6 +233,7 @@ anonymous:
   last_name: anonymouspublic
   is_active: false
   is_admin: false
+  username: anonymous
   prefs: {}
 
 job_reader:
@@ -273,6 +277,7 @@ active_no_prefs:
   identity_url: https://active_no_prefs.openid.local
   is_active: true
   is_admin: false
+  username: activenoprefs
   prefs: {}
 
 active_no_prefs_profile_no_getting_started_shown:
@@ -284,6 +289,7 @@ active_no_prefs_profile_no_getting_started_shown:
   identity_url: https://active_no_prefs_profile.openid.local
   is_active: true
   is_admin: false
+  username: activenoprefsprofilenogs
   prefs:
     test: abc
 
@@ -296,6 +302,7 @@ active_no_prefs_profile_with_getting_started_shown:
   identity_url: https://active_no_prefs_profile_seen_gs.openid.local
   is_active: true
   is_admin: false
+  username: activenoprefsprofile
   prefs:
     test: abc
     getting_started_shown: 2015-03-26 12:34:56.789000000 Z
@@ -308,6 +315,7 @@ active_with_prefs_profile_no_getting_started_shown:
   last_name: NoGettingStartedShown
   identity_url: https://active_nogettinstarted.openid.local
   is_active: true
+  username: activenogettinstarted
   prefs:
     profile:
       organization: example.com

diff --git a/tools/sync-users/sync-users.go b/tools/sync-users/sync-users.go
index 5cf3410365ddf28423f0783d27835fc00dae93a5..001014255d8c71bd3ed70b30e813e8196491e50f 100644 (file)
--- a/tools/sync-users/sync-users.go
+++ b/tools/sync-users/sync-users.go
@@ -94,8 +94,8 @@ func ParseFlags(cfg *ConfigParams) error {
 
        caseInsensitive := flags.Bool(
                "case-insensitive",
-               true,
-               "Performs case insensitive matching on user IDs. Always ON whe using 'email' user IDs.")
+               false,
+               "Performs case insensitive matching on user IDs. Always ON when using 'email' user IDs.")
        deactivateUnlisted := flags.Bool(
                "deactivate-unlisted",
                false,
@@ -107,7 +107,7 @@ func ParseFlags(cfg *ConfigParams) error {
        verbose := flags.Bool(
                "verbose",
                false,
-               "Log informational messages. Off by default.")
+               "Log informational messages.")
        getVersion := flags.Bool(
                "version",
                false,
@@ -366,7 +366,7 @@ func ProcessRecord(cfg *ConfigParams, record userRecord, userIDToUUID map[string
        }
 
        wantedActiveStatus := strconv.FormatBool(record.Active)
-       wantedAdminStatus := strconv.FormatBool(record.Admin)
+       wantedAdminStatus := strconv.FormatBool(record.Active && record.Admin)
        createRequired := false
        updateRequired := false
        // Check if user exists, set its active & admin status.

diff --git a/tools/sync-users/sync-users_test.go b/tools/sync-users/sync-users_test.go
index 8b93d32e4d3a3522e7d3aafbba7cd71315bfbd6d..5c272b0da0808ad8f4c0828fbc6bf0db5899ef70 100644 (file)
--- a/tools/sync-users/sync-users_test.go
+++ b/tools/sync-users/sync-users_test.go
@@ -98,6 +98,14 @@ func (s *TestSuite) TestParseFlagsWithoutPositionalArgument(c *C) {
        os.Args = []string{"cmd", "-verbose"}
        err := ParseFlags(&ConfigParams{})
        c.Assert(err, NotNil)
+       c.Assert(err, ErrorMatches, ".*please provide a path to an input file.*")
+}
+
+func (s *TestSuite) TestParseFlagsWrongUserID(c *C) {
+       os.Args = []string{"cmd", "-user-id=nickname", "/tmp/somefile.csv"}
+       err := ParseFlags(&ConfigParams{})
+       c.Assert(err, NotNil)
+       c.Assert(err, ErrorMatches, ".*user ID must be one of:.*")
 }
 
 func (s *TestSuite) TestParseFlagsWithPositionalArgument(c *C) {
@@ -108,16 +116,20 @@ func (s *TestSuite) TestParseFlagsWithPositionalArgument(c *C) {
        c.Assert(cfg.Path, Equals, "/tmp/somefile.csv")
        c.Assert(cfg.Verbose, Equals, false)
        c.Assert(cfg.DeactivateUnlisted, Equals, false)
+       c.Assert(cfg.UserID, Equals, "email")
+       c.Assert(cfg.CaseInsensitive, Equals, true)
 }
 
 func (s *TestSuite) TestParseFlagsWithOptionalFlags(c *C) {
        cfg := ConfigParams{}
-       os.Args = []string{"cmd", "-verbose", "-deactivate-unlisted", "/tmp/somefile.csv"}
+       os.Args = []string{"cmd", "-verbose", "-deactivate-unlisted", "-user-id=username", "/tmp/somefile.csv"}
        err := ParseFlags(&cfg)
        c.Assert(err, IsNil)
        c.Assert(cfg.Path, Equals, "/tmp/somefile.csv")
        c.Assert(cfg.Verbose, Equals, true)
        c.Assert(cfg.DeactivateUnlisted, Equals, true)
+       c.Assert(cfg.UserID, Equals, "username")
+       c.Assert(cfg.CaseInsensitive, Equals, false)
 }
 
 func (s *TestSuite) TestGetConfig(c *C) {
@@ -218,85 +230,118 @@ func (s *TestSuite) TestWrongDataFields(c *C) {
        }
 }
 
-// Activate and deactivate users
+// Create, activate and deactivate users
 func (s *TestSuite) TestUserCreationAndUpdate(c *C) {
-       testCases := []userRecord{{
-               UserID:    "user1@example.com",
-               FirstName: "Example",
-               LastName:  "User1",
-               Active:    true,
-               Admin:     false,
-       }, {
-               UserID:    "admin1@example.com",
-               FirstName: "Example",
-               LastName:  "Admin1",
-               Active:    true,
-               Admin:     true,
-       }}
-       // Make sure users aren't already there from fixtures
-       for _, user := range s.users {
-               e := user.Email
-               found := false
-               for _, r := range testCases {
-                       if e == r.UserID {
-                               found = true
-                               break
+       for _, tc := range []string{"email", "username"} {
+               uIDPrefix := tc
+               uIDSuffix := ""
+               if tc == "email" {
+                       uIDSuffix = "@example.com"
+               }
+               s.cfg.UserID = tc
+               records := []userRecord{{
+                       UserID:    fmt.Sprintf("%suser1%s", uIDPrefix, uIDSuffix),
+                       FirstName: "Example",
+                       LastName:  "User1",
+                       Active:    true,
+                       Admin:     false,
+               }, {
+                       UserID:    fmt.Sprintf("%suser2%s", uIDPrefix, uIDSuffix),
+                       FirstName: "Example",
+                       LastName:  "User2",
+                       Active:    false, // initially inactive
+                       Admin:     false,
+               }, {
+                       UserID:    fmt.Sprintf("%sadmin1%s", uIDPrefix, uIDSuffix),
+                       FirstName: "Example",
+                       LastName:  "Admin1",
+                       Active:    true,
+                       Admin:     true,
+               }, {
+                       UserID:    fmt.Sprintf("%sadmin2%s", uIDPrefix, uIDSuffix),
+                       FirstName: "Example",
+                       LastName:  "Admin2",
+                       Active:    false, // initially inactive
+                       Admin:     true,
+               }}
+               // Make sure users aren't already there from fixtures
+               for _, user := range s.users {
+                       uID, err := GetUserID(user, s.cfg.UserID)
+                       c.Assert(err, IsNil)
+                       found := false
+                       for _, r := range records {
+                               if uID == r.UserID {
+                                       found = true
+                                       break
+                               }
                        }
+                       c.Assert(found, Equals, false)
                }
-               c.Assert(found, Equals, false)
-       }
-       // User creation
-       tmpfile, err := MakeTempCSVFile(RecordsToStrings(testCases))
-       c.Assert(err, IsNil)
-       defer os.Remove(tmpfile.Name())
-       s.cfg.Path = tmpfile.Name()
-       err = doMain(s.cfg)
-       c.Assert(err, IsNil)
+               // User creation
+               tmpfile, err := MakeTempCSVFile(RecordsToStrings(records))
+               c.Assert(err, IsNil)
+               defer os.Remove(tmpfile.Name())
+               s.cfg.Path = tmpfile.Name()
+               err = doMain(s.cfg)
+               c.Assert(err, IsNil)
 
-       users, err := ListUsers(s.cfg.Client)
-       c.Assert(err, IsNil)
-       for _, tc := range testCases {
-               var foundUser arvados.User
-               for _, user := range users {
-                       if user.Email == tc.UserID {
-                               foundUser = user
-                               break
+               users, err := ListUsers(s.cfg.Client)
+               c.Assert(err, IsNil)
+               for _, r := range records {
+                       var foundUser arvados.User
+                       for _, user := range users {
+                               uID, err := GetUserID(user, s.cfg.UserID)
+                               c.Assert(err, IsNil)
+                               if uID == r.UserID {
+                                       // Add an @example.com email if missing
+                                       // (to avoid database reset errors)
+                                       if tc == "username" && user.Email == "" {
+                                               err := UpdateUser(s.cfg.Client, user.UUID, &user, map[string]string{
+                                                       "email": fmt.Sprintf("%s@example.com", user.Username),
+                                               })
+                                               c.Assert(err, IsNil)
+                                       }
+                                       foundUser = user
+                                       break
+                               }
                        }
+                       c.Assert(foundUser, NotNil)
+                       c.Logf("Checking creation for user %q", r.UserID)
+                       c.Assert(foundUser.FirstName, Equals, r.FirstName)
+                       c.Assert(foundUser.LastName, Equals, r.LastName)
+                       c.Assert(foundUser.IsActive, Equals, r.Active)
+                       c.Assert(foundUser.IsAdmin, Equals, (r.Active && r.Admin))
                }
-               c.Assert(foundUser, NotNil)
-               c.Logf("Checking recently created user %q", foundUser.Email)
-               c.Assert(foundUser.FirstName, Equals, tc.FirstName)
-               c.Assert(foundUser.LastName, Equals, tc.LastName)
-               c.Assert(foundUser.IsActive, Equals, true)
-               c.Assert(foundUser.IsAdmin, Equals, tc.Admin)
-       }
-       // User deactivation
-       for idx := range testCases {
-               testCases[idx].Active = false
-       }
-       tmpfile, err = MakeTempCSVFile(RecordsToStrings(testCases))
-       c.Assert(err, IsNil)
-       defer os.Remove(tmpfile.Name())
-       s.cfg.Path = tmpfile.Name()
-       err = doMain(s.cfg)
-       c.Assert(err, IsNil)
+               // User active status switch
+               for idx := range records {
+                       records[idx].Active = !records[idx].Active
+               }
+               tmpfile, err = MakeTempCSVFile(RecordsToStrings(records))
+               c.Assert(err, IsNil)
+               defer os.Remove(tmpfile.Name())
+               s.cfg.Path = tmpfile.Name()
+               err = doMain(s.cfg)
+               c.Assert(err, IsNil)
 
-       users, err = ListUsers(s.cfg.Client)
-       c.Assert(err, IsNil)
-       for _, tc := range testCases {
-               var foundUser arvados.User
-               for _, user := range users {
-                       if user.Email == tc.UserID {
-                               foundUser = user
-                               break
+               users, err = ListUsers(s.cfg.Client)
+               c.Assert(err, IsNil)
+               for _, r := range records {
+                       var foundUser arvados.User
+                       for _, user := range users {
+                               uID, err := GetUserID(user, s.cfg.UserID)
+                               c.Assert(err, IsNil)
+                               if uID == r.UserID {
+                                       foundUser = user
+                                       break
+                               }
                        }
+                       c.Assert(foundUser, NotNil)
+                       c.Logf("Checking update for user %q", r.UserID)
+                       c.Assert(foundUser.FirstName, Equals, r.FirstName)
+                       c.Assert(foundUser.LastName, Equals, r.LastName)
+                       c.Assert(foundUser.IsActive, Equals, r.Active)
+                       c.Assert(foundUser.IsAdmin, Equals, (r.Active && r.Admin))
                }
-               c.Assert(foundUser, NotNil)
-               c.Logf("Checking recently deactivated user %q", foundUser.Email)
-               c.Assert(foundUser.FirstName, Equals, tc.FirstName)
-               c.Assert(foundUser.LastName, Equals, tc.LastName)
-               c.Assert(foundUser.IsActive, Equals, false)
-               c.Assert(foundUser.IsAdmin, Equals, false) // inactive users cannot be admins
        }
 }