From 8e0c574f4a93a7c7b27c29a469a7737489f4aee8 Mon Sep 17 00:00:00 2001 From: "F.D.Castel" Date: Wed, 25 Mar 2026 19:03:29 -0300 Subject: [PATCH 1/2] refactor: unify MockUserLookup into shared test helper package Consolidate duplicated MockUserLookup structs from commands/add_test.go and policy/policyloader_test.go into a single struct in test/testutil/. The unified MockUserLookup supports both single-user and multi-user lookup scenarios via User (default) and Users (map) fields. Local type aliases keep existing test code concise. Closes #439 --- commands/add_test.go | 22 +++--------- policy/policyloader_test.go | 22 +++--------- test/testutil/mockuserlookup.go | 63 +++++++++++++++++++++++++++++++++ 3 files changed, 71 insertions(+), 36 deletions(-) create mode 100644 test/testutil/mockuserlookup.go diff --git a/commands/add_test.go b/commands/add_test.go index 222a8ecf..3a75d709 100644 --- a/commands/add_test.go +++ b/commands/add_test.go @@ -17,33 +17,19 @@ package commands import ( - "os/user" "testing" "github.com/openpubkey/opkssh/policy" "github.com/openpubkey/opkssh/policy/files" + "github.com/openpubkey/opkssh/test/testutil" "github.com/spf13/afero" "github.com/stretchr/testify/require" ) -// Duplicates code from multipolicyloader_test.go -type MockUserLookup struct { - // User is returned on any call to Lookup() if Error is nil - User *user.User - // Error is returned on any call to Lookup() if non-nil - Error error -} - -// Lookup implements policy.UserLookup -func (m *MockUserLookup) Lookup(username string) (*user.User, error) { - if m.Error == nil { - return m.User, nil - } else { - return nil, m.Error - } -} +// MockUserLookup is an alias for testutil.MockUserLookup to keep test code concise. +type MockUserLookup = testutil.MockUserLookup -var ValidUser *user.User = &user.User{HomeDir: "/home/foo", Username: "foo"} +var ValidUser = testutil.ValidUser func MockAddCmd(mockFs afero.Fs) *AddCmd { mockUserLookup := &MockUserLookup{User: ValidUser} diff --git a/policy/policyloader_test.go b/policy/policyloader_test.go index 1c0cfcf2..a9eefdcb 100644 --- a/policy/policyloader_test.go +++ b/policy/policyloader_test.go @@ -25,27 +25,13 @@ import ( "github.com/openpubkey/opkssh/policy" "github.com/openpubkey/opkssh/policy/files" + "github.com/openpubkey/opkssh/test/testutil" "github.com/spf13/afero" "github.com/stretchr/testify/require" ) -type MockUserLookup struct { - // User is returned on any call to Lookup() if Error is nil - User *user.User - // Error is returned on any call to Lookup() if non-nil - Error error -} - -var _ policy.UserLookup = &MockUserLookup{} - -// Lookup implements policy.UserLookup -func (m *MockUserLookup) Lookup(username string) (*user.User, error) { - if m.Error == nil { - return m.User, nil - } else { - return nil, m.Error - } -} +// MockUserLookup is an alias for testutil.MockUserLookup to keep test code concise. +type MockUserLookup = testutil.MockUserLookup // MockFsOpenError embeds an afero.MemMapFs (implements afero.Fs) but allows for // finer control on when an error should be returned on a specific filepath @@ -96,7 +82,7 @@ func NewTestSystemPolicyLoader(fs afero.Fs, userLookup policy.UserLookup) *polic } } -var ValidUser *user.User = &user.User{HomeDir: "/home/foo", Username: "foo"} +var ValidUser = testutil.ValidUser func TestLoadUserPolicy_FailUserLookup(t *testing.T) { // Test that LoadUserPolicy returns an error when user lookup fails diff --git a/test/testutil/mockuserlookup.go b/test/testutil/mockuserlookup.go new file mode 100644 index 00000000..d690b70b --- /dev/null +++ b/test/testutil/mockuserlookup.go @@ -0,0 +1,63 @@ +// Copyright 2026 OpenPubkey +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// SPDX-License-Identifier: Apache-2.0 + +package testutil + +import ( + "fmt" + "os/user" + + "github.com/openpubkey/opkssh/policy" +) + +// ValidUser is a shared test fixture representing a valid OS user. +var ValidUser = &user.User{HomeDir: "/home/foo", Username: "foo"} + +// MockUserLookup implements [policy.UserLookup] for testing. +// It supports both single-user and multi-user lookup scenarios: +// - Set [User] for a default user returned on any Lookup call. +// - Set [Users] to map specific usernames to user records. +// - Set [Error] to force every Lookup call to fail. +// +// When both Users and User are set, Users is checked first; if the username +// is not found in the map, User is returned as a fallback. +type MockUserLookup struct { + // Users, if non-nil, maps usernames to user records. + Users map[string]*user.User + // User is returned on any call to Lookup() when Users is nil or the + // username is not found in Users. + User *user.User + // Error, if non-nil, is returned on any call to Lookup(). + Error error +} + +var _ policy.UserLookup = &MockUserLookup{} + +// Lookup implements [policy.UserLookup]. +func (m *MockUserLookup) Lookup(username string) (*user.User, error) { + if m.Error != nil { + return nil, m.Error + } + if m.Users != nil { + if u, ok := m.Users[username]; ok { + return u, nil + } + } + if m.User != nil { + return m.User, nil + } + return nil, fmt.Errorf("user %q not found", username) +} From 3ff6b7fd79aa3333362eaf98eb5096a682f170ca Mon Sep 17 00:00:00 2001 From: "F.D.Castel" Date: Fri, 27 Mar 2026 22:10:33 -0300 Subject: [PATCH 2/2] refactor: remove unused Users map from MockUserLookup Keep the refactor focused on consolidating the existing functionality. The Users map can be added later when there is a concrete use case. --- test/testutil/mockuserlookup.go | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/test/testutil/mockuserlookup.go b/test/testutil/mockuserlookup.go index d690b70b..a5da1535 100644 --- a/test/testutil/mockuserlookup.go +++ b/test/testutil/mockuserlookup.go @@ -27,18 +27,10 @@ import ( var ValidUser = &user.User{HomeDir: "/home/foo", Username: "foo"} // MockUserLookup implements [policy.UserLookup] for testing. -// It supports both single-user and multi-user lookup scenarios: // - Set [User] for a default user returned on any Lookup call. -// - Set [Users] to map specific usernames to user records. // - Set [Error] to force every Lookup call to fail. -// -// When both Users and User are set, Users is checked first; if the username -// is not found in the map, User is returned as a fallback. type MockUserLookup struct { - // Users, if non-nil, maps usernames to user records. - Users map[string]*user.User - // User is returned on any call to Lookup() when Users is nil or the - // username is not found in Users. + // User is returned on any call to Lookup() if Error is nil. User *user.User // Error, if non-nil, is returned on any call to Lookup(). Error error @@ -51,11 +43,6 @@ func (m *MockUserLookup) Lookup(username string) (*user.User, error) { if m.Error != nil { return nil, m.Error } - if m.Users != nil { - if u, ok := m.Users[username]; ok { - return u, nil - } - } if m.User != nil { return m.User, nil }