From: Lucas Di Pentima Date: Mon, 4 Jul 2022 14:29:43 +0000 (-0300) Subject: 18858: Adds username-based test cases. X-Git-Tag: 2.5.0~124^2~3 X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/36f730574cf6d5f720656de6a102963af5e15cab 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 --- diff --git a/services/api/app/controllers/database_controller.rb b/services/api/app/controllers/database_controller.rb index fa1e1ca43c..69453959d2 100644 --- 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 1cf79b1a50..1d9bcbb040 100644 --- 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 5cf3410365..001014255d 100644 --- 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 8b93d32e4d..5c272b0da0 100644 --- 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 } }