diff --git a/C4/SIP/SIPServer.pm b/C4/SIP/SIPServer.pm index 97f452e2cb..dff3af28b1 100644 --- a/C4/SIP/SIPServer.pm +++ b/C4/SIP/SIPServer.pm @@ -423,9 +423,8 @@ sub telnet_transport { $pwd = get_clean_string($pwd); siplog( "LOG_DEBUG", "telnet_transport 2: uid length %s, pwd length %s", length($uid), length($pwd) ); - if ( exists( $config->{accounts}->{$uid} ) - && ( $pwd eq $config->{accounts}->{$uid}->{password} ) ) - { + # Check if user is authorized for SIP access, then authenticate via login_core + if ( exists( $config->{accounts}->{$uid} ) ) { $account = $config->{accounts}->{$uid}; if ( C4::SIP::Sip::MsgType::login_core( $self, $uid, $pwd ) ) { last; diff --git a/C4/SIP/Sip/MsgType.pm b/C4/SIP/Sip/MsgType.pm index 9f1cdd5460..d77c8088d1 100644 --- a/C4/SIP/Sip/MsgType.pm +++ b/C4/SIP/Sip/MsgType.pm @@ -927,59 +927,59 @@ sub login_core { my $uid = shift; my $pwd = shift; my $status = 1; # Assume it all works + + # Check if this userid is authorized for SIP access if ( !exists( $server->{config}->{accounts}->{$uid} ) ) { - siplog( "LOG_WARNING", "MsgType::login_core: Unknown login '$uid'" ); - $status = 0; - } elsif ( $server->{config}->{accounts}->{$uid}->{password} ne $pwd ) { - siplog( "LOG_WARNING", "MsgType::login_core: Invalid password for login '$uid'" ); - $status = 0; - } else { - - # Store the active account someplace handy for everybody else to find. - $server->{account} = $server->{config}->{accounts}->{$uid}; - my $inst = $server->{account}->{institution}; - $server->{institution} = $server->{config}->{institutions}->{$inst}; - $server->{policy} = $server->{institution}->{policy}; - $server->{sip_username} = $uid; - $server->{sip_password} = $pwd; - - my $auth_status = api_auth( $uid, $pwd, $inst ); - if ( !$auth_status or $auth_status !~ /^ok$/i ) { - siplog( - "LOG_WARNING", "api_auth failed for SIP terminal '%s' of '%s': %s", $uid, $inst, - ( $auth_status || 'unknown' ) - ); - $status = 0; - } else { - siplog( "LOG_INFO", "Successful login/auth for '%s' of '%s'", $server->{account}->{id}, $inst ); - - # - # initialize connection to ILS - # - my $module = $server->{config}->{institutions}->{$inst}->{implementation}; - siplog( "LOG_DEBUG", 'login_core: ' . Dumper($module) ); - - # Suspect this is always ILS but so we don't break any eccentic install (for now) - if ( $module eq 'ILS' ) { - $module = 'C4::SIP::ILS'; - } - $module->use; - if ($@) { - siplog( - "LOG_ERR", "%s: Loading ILS implementation '%s' for institution '%s' failed", - $server->{service}, $module, $inst - ); - die("Failed to load ILS implementation '$module' for $inst"); - } - - # like ILS->new(), I think. - $server->{ils} = $module->new( $server->{institution}, $server->{account} ); - if ( !$server->{ils} ) { - siplog( "LOG_ERR", "%s: ILS connection to '%s' failed", $server->{service}, $inst ); - die("Unable to connect to ILS '$inst'"); - } - } + siplog( "LOG_WARNING", "MsgType::login_core: SIP access not authorized for user '$uid'" ); + return 0; } + + # Store the active account configuration + $server->{account} = $server->{config}->{accounts}->{$uid}; + my $inst = $server->{account}->{institution}; + $server->{institution} = $server->{config}->{institutions}->{$inst}; + $server->{policy} = $server->{institution}->{policy}; + $server->{sip_username} = $uid; + $server->{sip_password} = $pwd; + + # Authenticate using Koha's internal authentication (checks hashed password in borrowers table) + my $auth_status = api_auth( $uid, $pwd, $inst ); + if ( !$auth_status or $auth_status !~ /^ok$/i ) { + siplog( + "LOG_WARNING", "Authentication failed for SIP terminal '%s' of '%s': %s", + $uid, $inst, ( $auth_status || 'unknown' ) + ); + return 0; + } + + siplog( "LOG_INFO", "Successful login/auth for '%s' of '%s'", $server->{account}->{id}, $inst ); + + # + # initialize connection to ILS + # + my $module = $server->{config}->{institutions}->{$inst}->{implementation}; + siplog( "LOG_DEBUG", 'login_core: ' . Dumper($module) ); + + # Suspect this is always ILS but so we don't break any eccentic install (for now) + if ( $module eq 'ILS' ) { + $module = 'C4::SIP::ILS'; + } + $module->use; + if ($@) { + siplog( + "LOG_ERR", "%s: Loading ILS implementation '%s' for institution '%s' failed", + $server->{service}, $module, $inst + ); + die("Failed to load ILS implementation '$module' for $inst"); + } + + # like ILS->new(), I think. + $server->{ils} = $module->new( $server->{institution}, $server->{account} ); + if ( !$server->{ils} ) { + siplog( "LOG_ERR", "%s: ILS connection to '%s' failed", $server->{service}, $inst ); + die("Unable to connect to ILS '$inst'"); + } + return $status; } diff --git a/Koha/SIP2/Account.pm b/Koha/SIP2/Account.pm index 20bcdbd5c6..e4524fb4ad 100644 --- a/Koha/SIP2/Account.pm +++ b/Koha/SIP2/Account.pm @@ -69,7 +69,6 @@ sub get_for_config { # Map database columns to their expected C4/SIP counterparts $unblessed->{institution} = $self->institution->name; $unblessed->{id} = delete $unblessed->{login_id}; - $unblessed->{password} = delete $unblessed->{login_password}; if ( $unblessed->{error_detect} ) { $unblessed->{'error-detect'} = 'enabled'; delete $unblessed->{error_detect}; diff --git a/api/v1/swagger/definitions/sip2_account.yaml b/api/v1/swagger/definitions/sip2_account.yaml index 27b7a357f6..ca13a4e6ef 100644 --- a/api/v1/swagger/definitions/sip2_account.yaml +++ b/api/v1/swagger/definitions/sip2_account.yaml @@ -124,10 +124,7 @@ properties: - string - 'null' login_id: - description: login id - type: string - login_password: - description: login password + description: Staff userid for SIP2 authentication type: string lost_block_checkout: description: lost status for missing @@ -238,4 +235,3 @@ additionalProperties: false required: - sip_account_id - login_id - - login_password diff --git a/installer/data/mysql/atomicupdate/bug_37893.pl b/installer/data/mysql/atomicupdate/bug_37893.pl index dbdc86a5c7..2f14fcc9ee 100755 --- a/installer/data/mysql/atomicupdate/bug_37893.pl +++ b/installer/data/mysql/atomicupdate/bug_37893.pl @@ -63,8 +63,7 @@ return { `holds_get_captured` tinyint(1) NULL, `inhouse_item_types` varchar(255) NULL, `inhouse_patron_categories` varchar(255) NULL, - `login_id` varchar(255) NOT NULL COMMENT 'PREVIOUSLY id in Sipconfig.xml', - `login_password` varchar(255) NULL, + `login_id` varchar(255) NOT NULL COMMENT 'Staff userid for SIP2 authentication', `lost_block_checkout` tinyint(1) NULL COMMENT 'actual tinyint, not boolean', `lost_block_checkout_value` tinyint(1) NULL COMMENT 'actual tinyint, not boolean', `lost_status_for_missing` tinyint(1) NULL COMMENT 'actual tinyint, not boolean', @@ -290,7 +289,7 @@ return { next unless $sip_institution; my $insert_accounts = $dbh->prepare( - q{INSERT IGNORE INTO sip_accounts (sip_institution_id, ae_field_template, allow_additional_materials_checkout, allow_empty_passwords, allow_fields, av_field_template, blocked_item_types, checked_in_ok, convert_nonprinting_characters, cr_item_field, ct_always_send, cv_send_00_on_success, cv_triggers_alert, da_field_template, delimiter, disallow_overpayment, encoding, error_detect, format_due_date, hide_fields, holds_block_checkin, holds_get_captured, inhouse_item_types, inhouse_patron_categories, login_id, login_password, lost_block_checkout, lost_block_checkout_value, lost_status_for_missing, overdues_block_checkout, payment_type_writeoff, prevcheckout_block_checkout, register_id, seen_on_item_information, send_patron_home_library_in_af, show_checkin_message, show_outstanding_amount, terminator) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?,?, ?, ?, ?, ?, ?, ?, ?,?,?,?,?, ?,?,?,?,?,?,?, ?,?,?,?,?)} + q{INSERT IGNORE INTO sip_accounts (sip_institution_id, ae_field_template, allow_additional_materials_checkout, allow_empty_passwords, allow_fields, av_field_template, blocked_item_types, checked_in_ok, convert_nonprinting_characters, cr_item_field, ct_always_send, cv_send_00_on_success, cv_triggers_alert, da_field_template, delimiter, disallow_overpayment, encoding, error_detect, format_due_date, hide_fields, holds_block_checkin, holds_get_captured, inhouse_item_types, inhouse_patron_categories, login_id, lost_block_checkout, lost_block_checkout_value, lost_status_for_missing, overdues_block_checkout, payment_type_writeoff, prevcheckout_block_checkout, register_id, seen_on_item_information, send_patron_home_library_in_af, show_checkin_message, show_outstanding_amount, terminator) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?,?, ?, ?, ?, ?, ?, ?, ?,?,?,?, ?,?,?,?,?,?,?, ?,?,?,?,?)} ); my $sip_institution_id = $sip_institution->get_column('sip_institution_id'); @@ -325,7 +324,6 @@ return { my $inhouse_item_types = $SIPconfig->{accounts}->{$account_key}->{inhouse_item_types}; my $inhouse_patron_categories = $SIPconfig->{accounts}->{$account_key}->{inhouse_patron_categories}; my $login_id = $SIPconfig->{accounts}->{$account_key}->{id}; - my $login_password = $SIPconfig->{accounts}->{$account_key}->{password}; my $lost_block_checkout = $SIPconfig->{accounts}->{$account_key}->{lost_block_checkout}; my $lost_block_checkout_value = $SIPconfig->{accounts}->{$account_key}->{lost_block_checkout_value}; my $lost_status_for_missing = $SIPconfig->{accounts}->{$account_key}->{lost_status_for_missing}; @@ -366,7 +364,6 @@ return { $inhouse_item_types, $inhouse_patron_categories, $login_id, - $login_password, $lost_block_checkout, $lost_block_checkout_value, $lost_status_for_missing, diff --git a/installer/data/mysql/kohastructure.sql b/installer/data/mysql/kohastructure.sql index aec42e1357..7346e931a1 100644 --- a/installer/data/mysql/kohastructure.sql +++ b/installer/data/mysql/kohastructure.sql @@ -6107,8 +6107,7 @@ CREATE TABLE `sip_accounts` ( `holds_get_captured` tinyint(1) NULL, `inhouse_item_types` varchar(255) NULL, `inhouse_patron_categories` varchar(255) NULL, - `login_id` varchar(255) NOT NULL COMMENT 'PREVIOUSLY id in Sipconfig.xml', - `login_password` varchar(255) NULL, + `login_id` varchar(255) NOT NULL COMMENT 'Staff userid for SIP2 authentication', `lost_block_checkout` tinyint(1) NULL COMMENT 'actual tinyint, not boolean', `lost_block_checkout_value` tinyint(1) NULL COMMENT 'actual tinyint, not boolean', `lost_status_for_missing` tinyint(1) NULL COMMENT 'actual tinyint, not boolean', diff --git a/koha-tmpl/intranet-tmpl/prog/js/vue/components/SIP2/SIP2AccountResource.vue b/koha-tmpl/intranet-tmpl/prog/js/vue/components/SIP2/SIP2AccountResource.vue index 6c91bad6f9..34c1e7eff1 100644 --- a/koha-tmpl/intranet-tmpl/prog/js/vue/components/SIP2/SIP2AccountResource.vue +++ b/koha-tmpl/intranet-tmpl/prog/js/vue/components/SIP2/SIP2AccountResource.vue @@ -109,17 +109,9 @@ export default { name: "login_id", required: true, type: "text", - label: __("Account login id"), + label: __("Staff userid for SIP2 authentication"), group: "Details", }, - { - name: "login_password", - required: true, - type: "text", - label: __("Account login password"), - group: "Details", - hideIn: ["List"], - }, { name: "sip_institution_id", type: "relationshipSelect", diff --git a/t/cypress/integration/SIP2/Accounts.ts b/t/cypress/integration/SIP2/Accounts.ts index 94e29a3926..25711ef768 100644 --- a/t/cypress/integration/SIP2/Accounts.ts +++ b/t/cypress/integration/SIP2/Accounts.ts @@ -48,7 +48,6 @@ function get_account() { timeout: 100, }, login_id: "asdas", - login_password: "dasdasd", lost_block_checkout: null, lost_block_checkout_value: null, lost_status_for_missing: null, @@ -142,7 +141,6 @@ describe("Accounts", () => { 3 ); cy.get("#login_id").type(account.login_id); - cy.get("#login_password").type(account.login_password); cy.wait("@getSIP2Institutions"); cy.get("#sip_institution_id .vs__search").type( @@ -284,10 +282,6 @@ describe("Accounts", () => { // Form has been correctly filled in cy.get("#login_id").should("have.value", accounts[0].login_id); - cy.get("#login_password").should( - "have.value", - accounts[0].login_password - ); // cy.get("#checkin_yes").should("be.checked"); // cy.get("#checkout_yes").should("be.checked"); diff --git a/t/db_dependent/Koha/SIP2/SIP2ModuleMigration.t b/t/db_dependent/Koha/SIP2/SIP2ModuleMigration.t index 56f399751c..f43e5987d4 100755 --- a/t/db_dependent/Koha/SIP2/SIP2ModuleMigration.t +++ b/t/db_dependent/Koha/SIP2/SIP2ModuleMigration.t @@ -66,6 +66,10 @@ subtest 'Config from XML matches config from database' => sub { { delete $fileSIPconfig->{accounts}->{$key}{'register_id'}; } + + # Remove 'password' from file config as it's no longer migrated (security improvement) + # Passwords are now validated against the borrowers table using bcrypt hashes + delete $fileSIPconfig->{accounts}->{$key}{'password'}; } #Test accounts @@ -124,7 +128,6 @@ subtest 'config_timestamp is updated when database configuration changes' => sub class => 'Koha::SIP2::Accounts', value => { login_id => $seen_patron->userid, - login_password => '123456', sip_institution_id => $sip_institution->sip_institution_id, allow_fields => '', hide_fields => '', diff --git a/t/db_dependent/api/v1/sip2_accounts.t b/t/db_dependent/api/v1/sip2_accounts.t index ac3112ffb4..c25e55480d 100755 --- a/t/db_dependent/api/v1/sip2_accounts.t +++ b/t/db_dependent/api/v1/sip2_accounts.t @@ -165,7 +165,7 @@ subtest 'get() tests' => sub { subtest 'add() tests' => sub { - plan tests => 17; + plan tests => 16; $schema->storage->txn_begin; @@ -192,7 +192,6 @@ subtest 'add() tests' => sub { my $account = { login_id => "koha", - login_password => "koha", sip_institution_id => $account_obj->sip_institution_id, }; @@ -203,7 +202,6 @@ subtest 'add() tests' => sub { my $account_with_invalid_field = { blah => "Account Blah", login_id => "koha", - login_password => "koha", sip_institution_id => $account->{sip_institution_id}, }; @@ -223,7 +221,7 @@ subtest 'add() tests' => sub { ->header_like( Location => qr|^/api/v1/sip2/accounts/\d*|, 'REST3.4.1' - )->json_is( '/login_id' => $account->{login_id} )->json_is( '/login_password' => $account->{login_password} ) + )->json_is( '/login_id' => $account->{login_id} ) ->json_is( '/sip_institution_id' => $account->{sip_institution_id} )->tx->res->json->{sip_account_id}; # Authorized attempt to create with null id @@ -281,7 +279,6 @@ subtest 'update() tests' => sub { # Attempt partial update on a PUT my $account_with_missing_field = { - login_password => "koha", sip_institution_id => $account->sip_institution_id, }; @@ -291,7 +288,6 @@ subtest 'update() tests' => sub { # Full object update on PUT my $account_with_updated_field = { login_id => "new koha", - login_password => "koha", sip_institution_id => $account->sip_institution_id, }; @@ -302,7 +298,6 @@ subtest 'update() tests' => sub { my $account_with_invalid_field = { blah => "Account Blah", login_id => "koha", - login_password => "koha", sip_institution_id => $account->sip_institution_id, };