From c5e5a9439664a3a9514af6862f5ea16a091376c7 Mon Sep 17 00:00:00 2001 From: Jakub Skoczen Date: Wed, 30 Sep 2026 00:58:46 +0200 Subject: [PATCH 1/2] Use NCIP patron info --- broker/lms/lms_adapter.go | 16 ++- broker/lms/lms_adapter_manual.go | 4 +- broker/lms/lms_adapter_ncip.go | 110 ++++++++++++++----- broker/lms/lms_adapter_test.go | 100 ++++++++++++----- broker/patron_request/service/action.go | 57 +++++++++- broker/patron_request/service/action_test.go | 72 ++++++++++-- 6 files changed, 285 insertions(+), 74 deletions(-) diff --git a/broker/lms/lms_adapter.go b/broker/lms/lms_adapter.go index d9cafa4d6..dbc1dad19 100644 --- a/broker/lms/lms_adapter.go +++ b/broker/lms/lms_adapter.go @@ -35,12 +35,26 @@ type CheckedOutItem struct { DueDate *time.Time } +// LookupUserOptions controls which user data an LMS lookup should validate and return. +type LookupUserOptions struct { + ValidatePatronProfile bool + IncludePatronInfo bool +} + +// LookupUserResult contains the canonical user identifier and optional patron details. +type LookupUserResult struct { + UserID string + GivenName string + Surname string + EmailAddresses []string +} + // LmsAdapter is an interface defining methods for interacting with a Library Management System (LMS) // https://github.com/openlibraryenvironment/mod-rs/blob/master/service/src/main/groovy/org/olf/rs/lms/HostLMSActions.groovy type LmsAdapter interface { SetLogFunc(logFunc ncipclient.NcipLogFunc) - LookupUser(patron string, validatePatronProfile bool) (userId string, err error) + LookupUser(patron string, options LookupUserOptions) (LookupUserResult, error) // Operations without a response payload return performed=false when skipped. // Errors never confirm progress; explicit manual confirmations do. diff --git a/broker/lms/lms_adapter_manual.go b/broker/lms/lms_adapter_manual.go index a7b16e4ca..97c5429d4 100644 --- a/broker/lms/lms_adapter_manual.go +++ b/broker/lms/lms_adapter_manual.go @@ -8,8 +8,8 @@ type LmsAdapterManual struct { func (l *LmsAdapterManual) SetLogFunc(logFunc ncipclient.NcipLogFunc) { } -func (l *LmsAdapterManual) LookupUser(patron string, validatePatronProfile bool) (string, error) { - return patron, nil +func (l *LmsAdapterManual) LookupUser(patron string, options LookupUserOptions) (LookupUserResult, error) { + return LookupUserResult{UserID: patron}, nil } // AcceptItem skips requester LMS item creation in manual workflows. diff --git a/broker/lms/lms_adapter_ncip.go b/broker/lms/lms_adapter_ncip.go index c75474e1f..568949b8e 100644 --- a/broker/lms/lms_adapter_ncip.go +++ b/broker/lms/lms_adapter_ncip.go @@ -4,11 +4,11 @@ import ( "encoding/xml" "errors" "fmt" - "github.com/indexdata/crosslink/broker/profiles" "net/http" "strings" "github.com/indexdata/crosslink/broker/ncipclient" + "github.com/indexdata/crosslink/broker/profiles" dirapi "github.com/indexdata/crosslink/directory/api" "github.com/indexdata/crosslink/ncip" ) @@ -16,9 +16,11 @@ import ( type NcipUserElement string const ( - NCIPUserId string = "User Id" - NCIPUserPrivilege string = "User Privilege" - NCIPItemBarcode string = "Item Barcode" + NCIPUserId string = "User Id" + NCIPUserPrivilege string = "User Privilege" + NCIPNameInformation string = "Name Information" + NCIPUserAddressInformation string = "User Address Information" + NCIPItemBarcode string = "Item Barcode" ) type NcipItemElement string @@ -83,26 +85,26 @@ func (l *LmsAdapterNcip) SetLogFunc(logFunc ncipclient.NcipLogFunc) { l.ncipClient.SetLogFunc(logFunc) } -func (l *LmsAdapterNcip) LookupUser(patron string, validatePatronProfile bool) (string, error) { +func (l *LmsAdapterNcip) LookupUser(patron string, options LookupUserOptions) (LookupUserResult, error) { if l.config.LookupUserEnabled != nil && !*l.config.LookupUserEnabled { - return patron, nil // could even be empty + return LookupUserResult{UserID: patron}, nil // could even be empty } if patron == "" { - return "", fmt.Errorf("empty patron identifier") + return LookupUserResult{}, fmt.Errorf("empty patron identifier") } // first try to check if patron is actually user Id arg := ncip.LookupUser{ UserId: &ncip.UserId{UserIdentifierValue: patron}, - UserElementType: l.getUserElements(false, validatePatronProfile), + UserElementType: l.getUserElements(false, options), } response, err := l.ncipClient.LookupUser(arg) if err == nil { - if validatePatronProfile { + if options.ValidatePatronProfile { if err = l.validatePatronProfile(response); err != nil { - return "", err + return LookupUserResult{}, err } } - return patron, nil + return lookupUserResult(patron, response, options.IncludePatronInfo), nil } // then try by user username // a better solution would be that the LookupUser had type argument (eg barcode or PIN) @@ -114,39 +116,91 @@ func (l *LmsAdapterNcip) LookupUser(patron string, validatePatronProfile bool) ( }) arg = ncip.LookupUser{ AuthenticationInput: authenticationInput, - UserElementType: l.getUserElements(true, validatePatronProfile), + UserElementType: l.getUserElements(true, options), } response, err = l.ncipClient.LookupUser(arg) if err != nil { - return "", err + return LookupUserResult{}, err } - if validatePatronProfile { + if options.ValidatePatronProfile { if err = l.validatePatronProfile(response); err != nil { - return "", err + return LookupUserResult{}, err } } + userID := "" if response != nil && response.UserOptionalFields != nil && len(response.UserOptionalFields.UserId) != 0 { - return response.UserOptionalFields.UserId[0].UserIdentifierValue, nil + userID = response.UserOptionalFields.UserId[0].UserIdentifierValue + } else if response != nil && response.UserId != nil { + userID = response.UserId.UserIdentifierValue + } + if userID == "" { + return LookupUserResult{}, fmt.Errorf("missing User ID in LookupUser response") + } + return lookupUserResult(userID, response, options.IncludePatronInfo), nil +} + +func (l *LmsAdapterNcip) getUserElements(userID bool, options LookupUserOptions) []ncip.SchemeValuePair { + var elements []ncip.SchemeValuePair + if userID || (options.ValidatePatronProfile && l.config.PatronProfiles != nil && len(*l.config.PatronProfiles) > 0) { + elements = append(elements, ncip.SchemeValuePair{Text: NCIPUserId}) + } + if options.ValidatePatronProfile && l.config.PatronProfiles != nil && len(*l.config.PatronProfiles) > 0 { + elements = append(elements, ncip.SchemeValuePair{Text: NCIPUserPrivilege}) } - if response != nil && response.UserId != nil { - return response.UserId.UserIdentifierValue, nil + if options.IncludePatronInfo { + elements = append(elements, + ncip.SchemeValuePair{Text: NCIPNameInformation}, + ncip.SchemeValuePair{Text: NCIPUserAddressInformation}, + ) } - return "", fmt.Errorf("missing User ID in LookupUser response") + return elements } -func (l *LmsAdapterNcip) getUserElements(userId bool, validatePatronProfile bool) []ncip.SchemeValuePair { - if validatePatronProfile && l.config.PatronProfiles != nil && len(*l.config.PatronProfiles) > 0 { - return []ncip.SchemeValuePair{ - {Text: NCIPUserId}, - {Text: NCIPUserPrivilege}, +func lookupUserResult(userID string, response *ncip.LookupUserResponse, includePatronInfo bool) LookupUserResult { + result := LookupUserResult{UserID: userID} + if !includePatronInfo || response == nil || response.UserOptionalFields == nil { + return result + } + + optional := response.UserOptionalFields + if optional.NameInformation != nil && optional.NameInformation.PersonalNameInformation != nil { + name := optional.NameInformation.PersonalNameInformation.StructuredPersonalUserName + if name != nil { + result.GivenName = strings.TrimSpace(name.GivenName) + result.Surname = strings.TrimSpace(name.Surname) } } - if userId { - return []ncip.SchemeValuePair{ - {Text: NCIPUserId}, + + seen := make(map[string]struct{}) + for _, address := range optional.UserAddressInformation { + email := ncipEmailAddress(address.ElectronicAddress) + if email == "" { + continue } + key := strings.ToLower(email) + if _, ok := seen[key]; ok { + continue + } + seen[key] = struct{}{} + result.EmailAddresses = append(result.EmailAddresses, email) } - return nil + return result +} + +func ncipEmailAddress(address *ncip.ElectronicAddress) string { + if address == nil { + return "" + } + data := strings.TrimSpace(address.ElectronicAddressData) + if data == "" { + return "" + } + addressType := strings.ToLower(strings.TrimSpace(address.ElectronicAddressType.Text)) + // Match common NCIP/FOLIO email type variants such as "mailto", "Email", and "electronic mail address". + if strings.Contains(addressType, "mail") { + return data + } + return "" } func (l *LmsAdapterNcip) validatePatronProfile(response *ncip.LookupUserResponse) error { diff --git a/broker/lms/lms_adapter_test.go b/broker/lms/lms_adapter_test.go index 17f0789d3..c2de28d3d 100644 --- a/broker/lms/lms_adapter_test.go +++ b/broker/lms/lms_adapter_test.go @@ -51,21 +51,22 @@ func TestLookupUser(t *testing.T) { ncipClient: mock, config: config, } - _, err := ad.LookupUser("", true) + validateOptions := LookupUserOptions{ValidatePatronProfile: true} + _, err := ad.LookupUser("", validateOptions) assert.Error(t, err) assert.Equal(t, "empty patron identifier", err.Error()) - userId, err := ad.LookupUser("testuser", true) + result, err := ad.LookupUser("testuser", validateOptions) assert.NoError(t, err) - assert.Equal(t, "testuser", userId) + assert.Equal(t, "testuser", result.UserID) request := mock.(*ncipClientMock).lastRequest.(ncip.LookupUser) assert.Equal(t, []ncip.SchemeValuePair{{Text: NCIPUserId}, {Text: NCIPUserPrivilege}}, request.UserElementType) - userId, err = ad.LookupUser("staff-profile", true) + result, err = ad.LookupUser("staff-profile", validateOptions) assert.NoError(t, err) - assert.Equal(t, "staff-profile", userId) + assert.Equal(t, "staff-profile", result.UserID) - _, err = ad.LookupUser("blocked-profile", true) + _, err = ad.LookupUser("blocked-profile", validateOptions) assert.EqualError(t, err, `patron profile with code "BLOCKED" and name "Blocked patrons" is not eligible to create ILL requests`) var ineligibleErr *PatronProfileIneligibleError if assert.ErrorAs(t, err, &ineligibleErr) { @@ -73,58 +74,58 @@ func TestLookupUser(t *testing.T) { assert.Equal(t, "Blocked patrons", ineligibleErr.ProfileName) } - _, err = ad.LookupUser("blocked user", true) + _, err = ad.LookupUser("blocked user", validateOptions) assert.EqualError(t, err, `patron profile with code "BLOCKED" and name "Blocked patrons" is not eligible to create ILL requests`) request = mock.(*ncipClientMock).lastRequest.(ncip.LookupUser) assert.Equal(t, []ncip.SchemeValuePair{{Text: NCIPUserId}, {Text: NCIPUserPrivilege}}, request.UserElementType) - userId, err = ad.LookupUser("blocked-profile", false) + result, err = ad.LookupUser("blocked-profile", LookupUserOptions{}) assert.NoError(t, err) - assert.Equal(t, "blocked-profile", userId) + assert.Equal(t, "blocked-profile", result.UserID) request = mock.(*ncipClientMock).lastRequest.(ncip.LookupUser) assert.Empty(t, request.UserElementType) - userId, err = ad.LookupUser("blocked user", false) + result, err = ad.LookupUser("blocked user", LookupUserOptions{}) assert.NoError(t, err) - assert.Equal(t, "blocked-user-id", userId) + assert.Equal(t, "blocked-user-id", result.UserID) request = mock.(*ncipClientMock).lastRequest.(ncip.LookupUser) assert.Equal(t, []ncip.SchemeValuePair{{Text: NCIPUserId}}, request.UserElementType) - _, err = ad.LookupUser("bad user", true) + _, err = ad.LookupUser("bad user", validateOptions) assert.Error(t, err) assert.Equal(t, "unknown user name", err.Error()) - _, err = ad.LookupUser("problem user", true) + _, err = ad.LookupUser("problem user", validateOptions) var ncipErr *ncipclient.NcipError assert.ErrorAs(t, err, &ncipErr) assert.Equal(t, string(ncip.UnknownUser), ncipErr.Problem.ProblemType.Text) assert.Equal(t, "patron was not found", ncipErr.Problem.ProblemDetail) - userId, err = ad.LookupUser("pass", true) + result, err = ad.LookupUser("pass", validateOptions) assert.NoError(t, err) - assert.Equal(t, "pass", userId) + assert.Equal(t, "pass", result.UserID) - _, err = ad.LookupUser("missing data", true) + _, err = ad.LookupUser("missing data", validateOptions) assert.Error(t, err) assert.Equal(t, "missing User ID in LookupUser response", err.Error()) - userId, err = ad.LookupUser("good user", true) + result, err = ad.LookupUser("good user", validateOptions) assert.NoError(t, err) - assert.Equal(t, "user124", userId) + assert.Equal(t, "user124", result.UserID) - userId, err = ad.LookupUser("other user", true) + result, err = ad.LookupUser("other user", validateOptions) assert.NoError(t, err) - assert.Equal(t, "user123", userId) + assert.Equal(t, "user123", result.UserID) b = false - userId, err = ad.LookupUser("", true) + result, err = ad.LookupUser("", validateOptions) assert.NoError(t, err) - assert.Equal(t, "", userId) + assert.Equal(t, "", result.UserID) mock.(*ncipClientMock).lastRequest = nil - userId, err = ad.LookupUser("anyuser", true) + result, err = ad.LookupUser("anyuser", validateOptions) assert.NoError(t, err) - assert.Equal(t, "anyuser", userId) + assert.Equal(t, "anyuser", result.UserID) assert.Nil(t, mock.(*ncipClientMock).lastRequest) // not called } @@ -165,22 +166,22 @@ func TestLookupUserElements(t *testing.T) { config: dirapi.LmsConfig{PatronProfiles: test.profiles}, } - _, err := adapter.LookupUser("testuser", true) + _, err := adapter.LookupUser("testuser", LookupUserOptions{ValidatePatronProfile: true}) assert.NoError(t, err) directRequest := mock.lastRequest.(ncip.LookupUser) assert.Equal(t, test.directElements, directRequest.UserElementType) - _, err = adapter.LookupUser("other user", true) + _, err = adapter.LookupUser("other user", LookupUserOptions{ValidatePatronProfile: true}) assert.NoError(t, err) fallbackRequest := mock.lastRequest.(ncip.LookupUser) assert.Equal(t, test.fallbackElements, fallbackRequest.UserElementType) - _, err = adapter.LookupUser("testuser", false) + _, err = adapter.LookupUser("testuser", LookupUserOptions{}) assert.NoError(t, err) directRequest = mock.lastRequest.(ncip.LookupUser) assert.Empty(t, directRequest.UserElementType) - _, err = adapter.LookupUser("other user", false) + _, err = adapter.LookupUser("other user", LookupUserOptions{}) assert.NoError(t, err) fallbackRequest = mock.lastRequest.(ncip.LookupUser) assert.Equal(t, userIDElement, fallbackRequest.UserElementType) @@ -188,6 +189,24 @@ func TestLookupUserElements(t *testing.T) { } } +func TestLookupUserPatronInfo(t *testing.T) { + mock := new(ncipClientMock) + adapter := &LmsAdapterNcip{ncipClient: mock} + + result, err := adapter.LookupUser("patron-info", LookupUserOptions{IncludePatronInfo: true}) + + assert.NoError(t, err) + assert.Equal(t, "patron-info", result.UserID) + assert.Equal(t, "Jane", result.GivenName) + assert.Equal(t, "Doe", result.Surname) + assert.Equal(t, []string{"jane@example.org"}, result.EmailAddresses) + request := mock.lastRequest.(ncip.LookupUser) + assert.Equal(t, []ncip.SchemeValuePair{ + {Text: NCIPNameInformation}, + {Text: NCIPUserAddressInformation}, + }, request.UserElementType) +} + func TestPatronProfile(t *testing.T) { tests := []struct { name string @@ -758,6 +777,31 @@ func (n *ncipClientMock) SetLogFunc(logFunc ncipclient.NcipLogFunc) { func (n *ncipClientMock) LookupUser(lookup ncip.LookupUser) (*ncip.LookupUserResponse, error) { n.lastRequest = lookup if lookup.UserId != nil { + if lookup.UserId.UserIdentifierValue == "patron-info" { + return &ncip.LookupUserResponse{ + UserId: &ncip.UserId{UserIdentifierValue: "patron-info"}, + UserOptionalFields: &ncip.UserOptionalFields{ + NameInformation: &ncip.NameInformation{ + PersonalNameInformation: &ncip.PersonalNameInformation{ + StructuredPersonalUserName: &ncip.StructuredPersonalUserName{ + GivenName: " Jane ", + Surname: " Doe ", + }, + }, + }, + UserAddressInformation: []ncip.UserAddressInformation{ + {ElectronicAddress: &ncip.ElectronicAddress{ + ElectronicAddressType: ncip.SchemeValuePair{Text: "mailto"}, + ElectronicAddressData: " jane@example.org ", + }}, + {ElectronicAddress: &ncip.ElectronicAddress{ + ElectronicAddressType: ncip.SchemeValuePair{Text: "TEL"}, + ElectronicAddressData: "+1 555 0100", + }}, + }, + }, + }, nil + } if lookup.UserId.UserIdentifierValue == "staff-profile" { return lookupUserResponseWithProfile(lookup.UserId.UserIdentifierValue, "PROFILE", "STAFF", "Staff"), nil } diff --git a/broker/patron_request/service/action.go b/broker/patron_request/service/action.go index 61d6aacd2..d582493fe 100644 --- a/broker/patron_request/service/action.go +++ b/broker/patron_request/service/action.go @@ -873,7 +873,10 @@ func (a *PatronRequestActionService) validatePatronBorrowingRequest(ctx common.E if pr.Patron.Valid { patron = pr.Patron.String } - userId, err := lmsAdapter.LookupUser(patron, true) + lookupResult, err := lmsAdapter.LookupUser(patron, lms.LookupUserOptions{ + ValidatePatronProfile: true, + IncludePatronInfo: true, + }) if err != nil { var ncipErr *ncipclient.NcipError if errors.As(err, &ncipErr) { @@ -900,11 +903,55 @@ func (a *PatronRequestActionService) validatePatronBorrowingRequest(ctx common.E } // change patron to canonical user id // perhaps it would be better to have both original and canonical id stored? - pr.Patron = pgtype.Text{String: userId, Valid: true} + pr.Patron = pgtype.Text{String: lookupResult.UserID, Valid: true} + pr.IllRequest = enrichPatronInfo(illRequest, lookupResult) return actionExecutionResult{status: events.EventStatusSuccess, pr: pr} } +func enrichPatronInfo(illRequest iso18626.Request, lookupResult lms.LookupUserResult) iso18626.Request { + givenName := strings.TrimSpace(lookupResult.GivenName) + surname := strings.TrimSpace(lookupResult.Surname) + hasEmail := false + for _, emailAddress := range lookupResult.EmailAddresses { + if strings.TrimSpace(emailAddress) != "" { + hasEmail = true + break + } + } + if givenName == "" && surname == "" && !hasEmail { + return illRequest + } + if illRequest.PatronInfo == nil { + illRequest.PatronInfo = &iso18626.PatronInfo{} + } + if strings.TrimSpace(illRequest.PatronInfo.GivenName) == "" { + illRequest.PatronInfo.GivenName = givenName + } + if strings.TrimSpace(illRequest.PatronInfo.Surname) == "" { + illRequest.PatronInfo.Surname = surname + } + + for _, address := range illRequest.PatronInfo.Address { + if address.ElectronicAddress != nil && + address.ElectronicAddress.ElectronicAddressType.Text == string(iso18626.ElectronicAddressTypeEmail) && + strings.TrimSpace(address.ElectronicAddress.ElectronicAddressData) != "" { + return illRequest + } + } + for _, emailAddress := range lookupResult.EmailAddresses { + if emailAddress = strings.TrimSpace(emailAddress); emailAddress != "" { + illRequest.PatronInfo.Address = append(illRequest.PatronInfo.Address, iso18626.Address{ + ElectronicAddress: &iso18626.ElectronicAddress{ + ElectronicAddressType: iso18626.TypeSchemeValuePair{Text: string(iso18626.ElectronicAddressTypeEmail)}, + ElectronicAddressData: emailAddress, + }, + }) + } + } + return illRequest +} + func (a *PatronRequestActionService) updateMetadataBorrowingRequest(ctx common.ExtendedContext, pr pr_db.PatronRequest, illRequest iso18626.Request) actionExecutionResult { peers, _, peerErr := a.illRepo.GetCachedPeersBySymbols(ctx, []string{pr.RequesterSymbol.String}, a.directoryLookupAdapter) if peerErr != nil { @@ -1444,9 +1491,9 @@ func (a *PatronRequestActionService) fillLocallyBorrowingRequest(ctx common.Exte return actionResultFromIllSend(ctx, sendStatus, sendResult, sendErr, pr) } -func (a *PatronRequestActionService) validatePatronLenderRequest(ctx common.ExtendedContext, pr pr_db.PatronRequest, lms lms.LmsAdapter) actionExecutionResult { - institutionalPatron := lms.InstitutionalPatron(pr.RequesterSymbol.String) - _, err := lms.LookupUser(institutionalPatron, false) +func (a *PatronRequestActionService) validatePatronLenderRequest(ctx common.ExtendedContext, pr pr_db.PatronRequest, lmsAdapter lms.LmsAdapter) actionExecutionResult { + institutionalPatron := lmsAdapter.InstitutionalPatron(pr.RequesterSymbol.String) + _, err := lmsAdapter.LookupUser(institutionalPatron, lms.LookupUserOptions{}) if err != nil { status, result := logActionErrorAndReturnResult(ctx, "LMS LookupUser failed", err) return actionExecutionResult{status: status, result: result, pr: pr} diff --git a/broker/patron_request/service/action_test.go b/broker/patron_request/service/action_test.go index 681266374..9de3c47a3 100644 --- a/broker/patron_request/service/action_test.go +++ b/broker/patron_request/service/action_test.go @@ -1210,6 +1210,52 @@ func TestHandleInvokeActionValidateLookupFailed(t *testing.T) { assert.Equal(t, string(events.EventStatusError), mockPrRepo.savedPr.LastActionResult.String) } +func TestValidatePatronBorrowingRequestEnrichesMissingPatronInfo(t *testing.T) { + adapter := &MockLmsAdapterLog{lookupResult: lms.LookupUserResult{ + UserID: "canonical-user-id", + GivenName: "NCIP given name", + Surname: "NCIP surname", + EmailAddresses: []string{"first@example.org", "second@example.org"}, + }} + illRequest := iso18626.Request{PatronInfo: &iso18626.PatronInfo{ + GivenName: "Existing given name", + Surname: " ", + Address: []iso18626.Address{makePhysicalAddress()}, + }} + pr := pr_db.PatronRequest{Patron: getDbText("submitted-user-id"), IllRequest: illRequest} + + result := (&PatronRequestActionService{}).validatePatronBorrowingRequest(appCtx, pr, adapter, illRequest) + + require.Equal(t, events.EventStatusSuccess, result.status) + assert.True(t, adapter.validatePatronProfile) + assert.True(t, adapter.includePatronInfo) + assert.Equal(t, "canonical-user-id", result.pr.Patron.String) + require.NotNil(t, result.pr.IllRequest.PatronInfo) + assert.Equal(t, "Existing given name", result.pr.IllRequest.PatronInfo.GivenName) + assert.Equal(t, "NCIP surname", result.pr.IllRequest.PatronInfo.Surname) + assert.Equal(t, []string{"first@example.org", "second@example.org"}, patronEmail(result.pr)) + assert.Len(t, result.pr.IllRequest.PatronInfo.Address, 3) +} + +func TestEnrichPatronInfoKeepsExistingEmailAndNames(t *testing.T) { + illRequest := iso18626.Request{PatronInfo: &iso18626.PatronInfo{ + GivenName: "Existing given name", + Surname: "Existing surname", + Address: []iso18626.Address{makeAddress(string(iso18626.ElectronicAddressTypeEmail), "existing@example.org")}, + }} + + result := enrichPatronInfo(illRequest, lms.LookupUserResult{ + GivenName: "NCIP given name", + Surname: "NCIP surname", + EmailAddresses: []string{"ncip@example.org"}, + }) + + require.NotNil(t, result.PatronInfo) + assert.Equal(t, "Existing given name", result.PatronInfo.GivenName) + assert.Equal(t, "Existing surname", result.PatronInfo.Surname) + assert.Equal(t, []string{"existing@example.org"}, patronEmail(pr_db.PatronRequest{IllRequest: result})) +} + func TestHandleInvokeActionValidatePatronProblem(t *testing.T) { mockPrRepo := new(MockPrRepo) lmsCreator := new(MockLmsCreator) @@ -5624,18 +5670,24 @@ type MockLmsAdapterLog struct { logFunc ncipclient.NcipLogFunc requestItemErr error validatePatronProfile bool + includePatronInfo bool + lookupResult lms.LookupUserResult } func (l *MockLmsAdapterLog) SetLogFunc(logFunc ncipclient.NcipLogFunc) { l.logFunc = logFunc } -func (l *MockLmsAdapterLog) LookupUser(patron string, validatePatronProfile bool) (string, error) { - l.validatePatronProfile = validatePatronProfile +func (l *MockLmsAdapterLog) LookupUser(patron string, options lms.LookupUserOptions) (lms.LookupUserResult, error) { + l.validatePatronProfile = options.ValidatePatronProfile + l.includePatronInfo = options.IncludePatronInfo if l.logFunc != nil { l.logFunc(map[string]any{"patron": patron}, map[string]any{"patron": patron}, nil) } - return patron, nil + if l.lookupResult.UserID == "" { + l.lookupResult.UserID = patron + } + return l.lookupResult, nil } func (l *MockLmsAdapterLog) RequestItem( @@ -5679,8 +5731,8 @@ type MockLmsAdapterPatronProfileIneligible struct { validatePatronProfile bool } -func (l *MockLmsAdapterPatronProblem) LookupUser(patron string, validatePatronProfile bool) (string, error) { - return "", &ncipclient.NcipError{ +func (l *MockLmsAdapterPatronProblem) LookupUser(patron string, options lms.LookupUserOptions) (lms.LookupUserResult, error) { + return lms.LookupUserResult{}, &ncipclient.NcipError{ Message: "NCIP user lookup failed", Problem: ncip.Problem{ ProblemType: ncip.SchemeValuePair{Text: string(ncip.UnknownUser)}, @@ -5689,9 +5741,9 @@ func (l *MockLmsAdapterPatronProblem) LookupUser(patron string, validatePatronPr } } -func (l *MockLmsAdapterPatronProfileIneligible) LookupUser(patron string, validatePatronProfile bool) (string, error) { - l.validatePatronProfile = validatePatronProfile - return "", &lms.PatronProfileIneligibleError{ +func (l *MockLmsAdapterPatronProfileIneligible) LookupUser(patron string, options lms.LookupUserOptions) (lms.LookupUserResult, error) { + l.validatePatronProfile = options.ValidatePatronProfile + return lms.LookupUserResult{}, &lms.PatronProfileIneligibleError{ ProfileCode: "BLOCKED", ProfileName: "Blocked patrons", } @@ -5700,8 +5752,8 @@ func (l *MockLmsAdapterPatronProfileIneligible) LookupUser(patron string, valida func (l *MockLmsAdapterFail) SetLogFunc(logFunc ncipclient.NcipLogFunc) { } -func (l *MockLmsAdapterFail) LookupUser(patron string, validatePatronProfile bool) (string, error) { - return "", errors.New("LookupUser failed") +func (l *MockLmsAdapterFail) LookupUser(patron string, options lms.LookupUserOptions) (lms.LookupUserResult, error) { + return lms.LookupUserResult{}, errors.New("LookupUser failed") } func (l *MockLmsAdapterFail) AcceptItem( From 290606090125965c7701893b7d48a6b22903ebd6 Mon Sep 17 00:00:00 2001 From: Jakub Skoczen Date: Wed, 30 Sep 2026 01:28:31 +0200 Subject: [PATCH 2/2] CoPilot --- broker/ncipclient/ncipclient_impl.go | 71 ++++++++-------------- broker/ncipclient/ncipclient_test.go | 90 +++++++++++++++++++++++----- 2 files changed, 102 insertions(+), 59 deletions(-) diff --git a/broker/ncipclient/ncipclient_impl.go b/broker/ncipclient/ncipclient_impl.go index 8f4f5ac24..03d2ae5f1 100644 --- a/broker/ncipclient/ncipclient_impl.go +++ b/broker/ncipclient/ncipclient_impl.go @@ -6,7 +6,6 @@ import ( "fmt" "io" "net/http" - "reflect" "github.com/indexdata/crosslink/broker/common" "github.com/indexdata/crosslink/httpclient" @@ -247,14 +246,14 @@ func (n *NcipClientImpl) logOperation(outgoingMessage *ncip.NCIPMessage, incomin return } - hideSensitive(outgoingMessage) outgoing, outgoingErr := common.StructToMap(outgoingMessage) + hideSensitive(outgoing) var incoming map[string]any var incomingErr error if incomingMessage != nil { - hideSensitive(incomingMessage) incoming, incomingErr = common.StructToMap(incomingMessage) + hideSensitive(incoming) } logErr := operationErr @@ -267,56 +266,38 @@ func (n *NcipClientImpl) logOperation(outgoingMessage *ncip.NCIPMessage, incomin n.logFunc(outgoing, incoming, logErr) } -func hideSensitive(message *ncip.NCIPMessage) { - traverse(reflect.ValueOf(message), 0) +func hideSensitive(message map[string]any) { + traverse(message, 0) } // removes values from the FromAgencyAuthentication and FromSystemAuthentication fields -// as well as AuthenticationInput fields except if type is "username" -func traverse(v reflect.Value, level int) { +// as well as user name/address information and AuthenticationInput fields except +// if type is "username" +func traverse(value any, level int) { if level > 20 { return } - level = level + 1 - if !v.IsValid() { - return - } - if v.Kind() == reflect.Pointer { - if v.IsNil() { - return - } - traverse(v.Elem(), level) - return - } - if v.Kind() == reflect.Slice { - if v.IsNil() { - return - } - for i := 0; i < v.Len(); i++ { - traverse(v.Index(i), level) + level++ + switch v := value.(type) { + case map[string]any: + if authenticationType, ok := v["AuthenticationInputType"].(map[string]any); ok { + if authenticationType["#text"] != "username" { + if _, exists := v["AuthenticationInputData"]; exists { + v["AuthenticationInputData"] = "***" + } + } } - return - } - if v.Kind() != reflect.Struct { - return - } - t := v.Type() - if t == reflect.TypeOf(ncip.AuthenticationInput{}) { - ncipAuthenticationInput := v.Interface().(ncip.AuthenticationInput) - exclude := ncipAuthenticationInput.AuthenticationInputType.Text != "username" - if exclude { - ncipAuthenticationInput.AuthenticationInputData = "***" - v.Set(reflect.ValueOf(ncipAuthenticationInput)) + for key, field := range v { + if key == "FromAgencyAuthentication" || key == "FromSystemAuthentication" || + key == "NameInformation" || key == "UserAddressInformation" { + v[key] = "***" + continue + } + traverse(field, level) } - return - } - for i := 0; i < t.NumField(); i++ { - field := t.Field(i) - if field.Type.Kind() == reflect.String && - (field.Name == "FromAgencyAuthentication" || field.Name == "FromSystemAuthentication") { - v.Field(i).SetString("***") - } else { - traverse(v.Field(i), level) + case []any: + for _, item := range v { + traverse(item, level) } } } diff --git a/broker/ncipclient/ncipclient_test.go b/broker/ncipclient/ncipclient_test.go index aace398ce..5b55c58a6 100644 --- a/broker/ncipclient/ncipclient_test.go +++ b/broker/ncipclient/ncipclient_test.go @@ -6,13 +6,13 @@ import ( "net/http" "net/http/httptest" "os" - "reflect" "strconv" "testing" "github.com/indexdata/go-utils/utils" "github.com/stretchr/testify/assert" + "github.com/indexdata/crosslink/broker/common" mockapp "github.com/indexdata/crosslink/illmock/app" "github.com/indexdata/crosslink/illmock/netutil" "github.com/indexdata/crosslink/ncip" @@ -526,23 +526,85 @@ func TestHideSensitive(t *testing.T) { }, }, } - hideSensitive(sampleMessage) - assert.Equal(t, "ILL-MOCK", sampleMessage.LookupUser.InitiationHeader.ToAgencyId.AgencyId.Text) - assert.Equal(t, "***", sampleMessage.LookupUser.InitiationHeader.FromAgencyAuthentication) - assert.Equal(t, "***", sampleMessage.LookupUser.InitiationHeader.FromSystemAuthentication) - assert.Equal(t, "validuser", sampleMessage.LookupUser.UserId.UserIdentifierValue) - assert.Equal(t, "***", sampleMessage.LookupUser.AuthenticationInput[0].AuthenticationInputData) - assert.Equal(t, "myuser", sampleMessage.LookupUser.AuthenticationInput[1].AuthenticationInputData) + messageMap, err := common.StructToMap(sampleMessage) + assert.NoError(t, err) + hideSensitive(messageMap) + messageJSON, err := json.Marshal(messageMap) + assert.NoError(t, err) + assert.Contains(t, string(messageJSON), "ILL-MOCK") + assert.Contains(t, string(messageJSON), "validuser") + assert.Contains(t, string(messageJSON), "myuser") + assert.NotContains(t, string(messageJSON), "supersecret") + assert.NotContains(t, string(messageJSON), "othersecret") + assert.NotContains(t, string(messageJSON), "1234") + assert.Equal(t, "supersecret", sampleMessage.LookupUser.InitiationHeader.FromAgencyAuthentication) + assert.Equal(t, "1234", sampleMessage.LookupUser.AuthenticationInput[0].AuthenticationInputData) } -func TestHideSensitiveLevel(t *testing.T) { - sampleMessage := &ncip.NCIPMessage{} - traverse(reflect.ValueOf(sampleMessage), 30) +func TestHideSensitiveRedactsRoutingNameWithoutMutatingMessage(t *testing.T) { + sampleMessage := &ncip.NCIPMessage{ + CheckInItemResponse: &ncip.CheckInItemResponse{ + RoutingInformation: &ncip.RoutingInformation{ + NameInformation: &ncip.NameInformation{ + PersonalNameInformation: &ncip.PersonalNameInformation{ + StructuredPersonalUserName: &ncip.StructuredPersonalUserName{ + GivenName: "Jane", + Surname: "Doe", + }, + }, + }, + }, + }, + } + messageMap, err := common.StructToMap(sampleMessage) + assert.NoError(t, err) + + hideSensitive(messageMap) + + response := messageMap["CheckInItemResponse"].(map[string]any) + routing := response["RoutingInformation"].(map[string]any) + assert.Equal(t, "***", routing["NameInformation"]) + assert.Equal(t, "Jane", sampleMessage.CheckInItemResponse.RoutingInformation.NameInformation.PersonalNameInformation.StructuredPersonalUserName.GivenName) } -func TestHideSensitiveInvalid(t *testing.T) { - invalid := reflect.Value{} - traverse(invalid, 0) +func TestLogOperationRedactsLookupUserDetailsWithoutMutatingResponse(t *testing.T) { + client := &NcipClientImpl{} + var loggedIncoming map[string]any + client.SetLogFunc(func(_ map[string]any, incoming map[string]any, _ error) { + loggedIncoming = incoming + }) + response := &ncip.NCIPMessage{ + LookupUserResponse: &ncip.LookupUserResponse{ + UserOptionalFields: &ncip.UserOptionalFields{ + NameInformation: &ncip.NameInformation{ + PersonalNameInformation: &ncip.PersonalNameInformation{ + StructuredPersonalUserName: &ncip.StructuredPersonalUserName{ + GivenName: "Jane", + Surname: "Doe", + }, + }, + }, + UserAddressInformation: []ncip.UserAddressInformation{{ + ElectronicAddress: &ncip.ElectronicAddress{ + ElectronicAddressType: ncip.SchemeValuePair{Text: "mailto"}, + ElectronicAddressData: "jane@example.org", + }, + }}, + }, + }, + } + + client.logOperation(&ncip.NCIPMessage{}, response, nil) + + loggedJSON, err := json.Marshal(loggedIncoming) + assert.NoError(t, err) + assert.Contains(t, string(loggedJSON), `"NameInformation":"***"`) + assert.Contains(t, string(loggedJSON), `"UserAddressInformation":"***"`) + assert.NotContains(t, string(loggedJSON), "Jane") + assert.NotContains(t, string(loggedJSON), "Doe") + assert.NotContains(t, string(loggedJSON), "jane@example.org") + assert.Equal(t, "Jane", response.LookupUserResponse.UserOptionalFields.NameInformation.PersonalNameInformation.StructuredPersonalUserName.GivenName) + assert.Equal(t, "jane@example.org", response.LookupUserResponse.UserOptionalFields.UserAddressInformation[0].ElectronicAddress.ElectronicAddressData) } func TestSetLogFunc(t *testing.T) {