From f2f62585100c78bf7b121cfec9927c55bacacbf8 Mon Sep 17 00:00:00 2001 From: Agustin Moyano Date: Wed, 2 Nov 2022 21:44:32 -0300 Subject: [PATCH] Bug 31378: (follow-up) Fix QA concerns Several FIXME comments added on the report addressed here. Signed-off-by: Tomas Cohen Arazi Signed-off-by: Nick Clemens Signed-off-by: Martin Renvoize --- Koha/Auth/Client.pm | 34 +- Koha/Auth/Client/OAuth.pm | 4 +- Koha/REST/V1/OAuth/Client.pm | 49 +-- admin/identity_providers.pl | 18 +- .../admin/identity_provider_domains.tt | 391 +++++++++-------- .../en/modules/admin/identity_providers.tt | 392 ++++++++++-------- t/db_dependent/Koha/Auth/Client.t | 6 +- t/db_dependent/Koha/REST/Plugin/Auth/IdP.t | 0 t/db_dependent/api/v1/idp.t | 79 ++-- 9 files changed, 523 insertions(+), 450 deletions(-) mode change 100644 => 100755 t/db_dependent/Koha/Auth/Client.t mode change 100644 => 100755 t/db_dependent/Koha/REST/Plugin/Auth/IdP.t mode change 100644 => 100755 t/db_dependent/api/v1/idp.t diff --git a/Koha/Auth/Client.pm b/Koha/Auth/Client.pm index 4bb71f628e..159dd40c8f 100644 --- a/Koha/Auth/Client.pm +++ b/Koha/Auth/Client.pm @@ -62,16 +62,16 @@ sub get_user { my ( $mapped_data, $patron ) = $self->_get_data_and_patron({ provider => $provider, data => $data, config => $config }); - if ($mapped_data) { - my $domain = $self->has_valid_domain_config({ provider => $provider, email => $mapped_data->{email}, interface => $interface}); + $mapped_data //= {}; - $mapped_data->{categorycode} = $domain->default_category_id; - $mapped_data->{branchcode} = $domain->default_library_id; + my $domain = $self->has_valid_domain_config({ provider => $provider, email => $mapped_data->{email}, interface => $interface}); - $patron->set($mapped_data)->store if $patron && $domain->update_on_auth; + $patron->set($mapped_data)->store if $patron && $domain->update_on_auth; - return ( $patron, $mapped_data, $domain ); - } + $mapped_data->{categorycode} = $domain->default_category_id; + $mapped_data->{branchcode} = $domain->default_library_id; + + return ( $patron, $mapped_data, $domain ); } =head3 get_valid_domain_config @@ -97,15 +97,15 @@ sub get_valid_domain_config { my $domains = $provider->domains; my $allow = "allow_$interface"; my @subdomain_matches; - my $default_match; + my $perfect_match; + my $response; while ( my $domain = $domains->next ) { - next unless $domain->$allow; my $pattern = '@'; my $domain_text = $domain->domain; unless ( defined $domain_text && $domain_text ne '') { - $default_match = $domain; + $response = $domain; next; } my ( $asterisk, $domain_name ) = ( $domain_text =~ /^(\*)?(.+)$/ ); @@ -118,20 +118,22 @@ sub get_valid_domain_config { if ( defined $asterisk && $asterisk eq '*' ) { push @subdomain_matches, { domain => $domain, match_length => length $domain_name }; } else { - - # Perfect match.. return this one. - return $domain; + $perfect_match = $domain; } } } - if ( @subdomain_matches ) { + if ( !$perfect_match && @subdomain_matches ) { @subdomain_matches = sort { $b->{match_length} <=> $a->{match_length} } @subdomain_matches unless scalar @subdomain_matches == 1; - return $subdomain_matches[0]->{domain}; + $response = $subdomain_matches[0]->{domain}; + } elsif ($perfect_match) { + $response = $perfect_match; } - return $default_match; + return unless $response && $response->$allow; + + return $response; } =head3 has_valid_domain_config diff --git a/Koha/Auth/Client/OAuth.pm b/Koha/Auth/Client/OAuth.pm index 3ea30f79b2..fe1fba306d 100644 --- a/Koha/Auth/Client/OAuth.pm +++ b/Koha/Auth/Client/OAuth.pm @@ -65,6 +65,7 @@ sub _get_data_and_patron { my ( $header_part, $claims_part, $footer_part ) = split( /\./, $data->{id_token} ); my $claim = decode_json( decode_base64url($claims_part) ); + foreach my $key ( keys %$mapping ) { my $pkey = $mapping->{$key}; $mapped_data->{$key} = $claim->{$pkey} @@ -111,8 +112,7 @@ sub _get_data_and_patron { } - return ( $mapped_data, $patron ) - if $patron; + return ( $mapped_data, $patron ); } 1; diff --git a/Koha/REST/V1/OAuth/Client.pm b/Koha/REST/V1/OAuth/Client.pm index 36cca8aac6..f3256ce3f5 100644 --- a/Koha/REST/V1/OAuth/Client.pm +++ b/Koha/REST/V1/OAuth/Client.pm @@ -50,22 +50,6 @@ sub login { my $provider_config = $c->oauth2->providers->{$provider}; - unless ( $provider_config ) { - return $c->render( - status => 404, - openapi => { - error => 'Object not found', - error_code => 'not_found', - } - ); - } - - unless ( $provider_config->{authorize_url} =~ /response_type=code/ ) { - my $authorize_url = Mojo::URL->new($provider_config->{authorize_url}); - $authorize_url->query->append(response_type => 'code'); - $provider_config->{authorize_url} = $authorize_url->to_string; - } - my $uri; if ( $interface eq 'opac' ) { @@ -78,20 +62,30 @@ sub login { $uri = '/cgi-bin/koha/mainpage.pl'; } + unless ( $provider_config ) { + my $error = "No configuration found for your provider"; + return $c->redirect_to($uri."?auth_error=$error"); + } + + unless ( $provider_config->{authorize_url} =~ /response_type=code/ ) { + my $authorize_url = Mojo::URL->new($provider_config->{authorize_url}); + $authorize_url->query->append(response_type => 'code'); + $provider_config->{authorize_url} = $authorize_url->to_string; + } + return $c->oauth2->get_token_p($provider)->then( sub { return unless my $response = shift; - my ( $patron, $mapped_data, $domain ) = Koha::Auth::Client::OAuth->new->get_user( - { provider => $provider, - data => $response, - interface => $interface, - config => $c->oauth2->providers->{$provider} - } - ); - try { - # FIXME: We could check if the backend allows registering + my ( $patron, $mapped_data, $domain ) = Koha::Auth::Client::OAuth->new->get_user( + { provider => $provider, + data => $response, + interface => $interface, + config => $c->oauth2->providers->{$provider} + } + ); + if ( !$patron ) { $patron = $c->auth->register( { @@ -113,7 +107,10 @@ sub login { # TODO: Review behavior if ( blessed $error ) { if ( $error->isa('Koha::Exceptions::Auth::Unauthorized') ) { - $error = "$error"; + $error = "User cannot access this resource"; + } + if ( $error->isa('Koha::Exceptions::Auth::NoValidDomain') ) { + $error = "No configuration found for your email domain"; } } diff --git a/admin/identity_providers.pl b/admin/identity_providers.pl index 2f7a433683..77d78c90f4 100644 --- a/admin/identity_providers.pl +++ b/admin/identity_providers.pl @@ -95,14 +95,14 @@ if ( !$domain_ops && $op eq 'add' ) { } elsif ( $domain_ops && $op eq 'add' ) { - my $allow_opac = $input->param('allow_opac'); - my $allow_staff = $input->param('allow_staff'); + my $allow_opac = $input->param('allow_opac'); + my $allow_staff = $input->param('allow_staff'); my $identity_provider_id = $input->param('identity_provider_id'); - my $auto_register = $input->param('auto_register'); - my $default_category_id = $input->param('default_category_id'); - my $default_library_id = $input->param('default_library_id'); - my $domain = $input->param('domain'); - my $update_on_auth = $input->param('update_on_auth'); + my $auto_register = $input->param('auto_register'); + my $default_category_id = $input->param('default_category_id') || undef; + my $default_library_id = $input->param('default_library_id') || undef; + my $domain = $input->param('domain'); + my $update_on_auth = $input->param('update_on_auth'); try { @@ -237,8 +237,8 @@ elsif ( $domain_ops && $op eq 'edit_save' ) { my $domain = $input->param('domain'); my $auto_register = $input->param('auto_register'); my $update_on_auth = $input->param('update_on_auth'); - my $default_library_id = $input->param('default_library_id'); - my $default_category_id = $input->param('default_category_id'); + my $default_library_id = $input->param('default_library_id') || undef; + my $default_category_id = $input->param('default_category_id') || undef; my $allow_opac = $input->param('allow_opac'); my $allow_staff = $input->param('allow_staff'); diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/identity_provider_domains.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/identity_provider_domains.tt index 7a7cd85eb5..3ad8593025 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/identity_provider_domains.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/identity_provider_domains.tt @@ -33,7 +33,7 @@ [% IF op == 'add_form' %]
  • - Domains for [%- identity_provider_name | html -%] + Domains for [%- identity_provider_code | html -%]
  • @@ -43,7 +43,7 @@ [% ELSIF op == 'edit_form' %]
  • - Domains for [%- identity_provider_name | html -%] + Domains for [%- identity_provider_code | html -%]
  • @@ -87,198 +87,225 @@ [% IF op == 'add_form' %] -

    New identity provider domain

    -
    - - - -
    -
      -
    1. - - -
      2. -
    -
    +

    New email domain

    +
    + + + + +
    +
      +
    1. + + +
      Email domain to match this rule.
      + +
      2. +
    +
    -
    -
      -
    1. - - - user data on login -
      2. -
    2. - - - users to auto register on login -
      3. -
    3. - - -
      4. -
    4. - - [% SET categories = Categories.all() %] - -
      5. -
    5. - - - opac users of this domain to login with this identity provider -
      6. -
    6. - - - of this domain -
      7. -
    -
    -
    - - Cancel -
    - +
    +
      +
    1. + + + user data on login +
      2. +
    2. + + + users to auto register on login +
      3. +
    3. + + +
      Use this library for the patron on auto register
      +
      4. +
    4. + + [% SET categories = Categories.all() %] + +
      Use this category for the patron on auto register
      +
      5. +
    5. + + + opac users of this domain to login with this identity provider +
      6. +
    6. + + + of this domain to login with this identity provider +
      7. +
    +
    +
    + + Cancel +
    + +
    [% END %] [% IF op == 'edit_form' %]

    Edit identity provider domain

    -
    - - - - -
    -
      -
    1. - - -
      2. -
    -
    +
    + + + + + +
    +
      +
    1. + + +
      Email domain to match this rule.
      + +
      2. +
    +
    -
    -
      -
    1. - - - user data on login -
      2. -
    2. - - - users to auto register on login -
      3. -
    3. - - -
      4. -
    4. - - [% SET categories = Categories.all() %] - + [% IF identity_provider_domain.update_on_auth == "1" %] + + + [% ELSE %] + + + [% END %] + + user data on login +
      5. +
    5. + + + users to auto register on login +
      6. +
    6. + + +
      Use this library for the patron on auto register
      +
      7. +
    7. + + [% SET categories = Categories.all() %] + +
      Use this category for the patron on auto register
      +
      8. +
    8. + + + opac users of this domain to login with this identity provider +
      9. +
    9. + + -
      10. -
    10. - - - opac users of this domain to login with this identity provider -
      11. -
    11. - - - staff users of this domain to login with this identity provider -
      12. -
    -
    -
    - - Cancel -
    - + + staff users of this domain to login with this identity provider +
    • + + +
    + + Cancel +
    + + [% END %] [% IF op == 'list' %] -

    Identity provider domains

    - - - - - - - - - - - - - - -
    DomainUpdate on loginAuto registerDefault libraryDefault categoryAllow opacAllow staffActions
    +

    Identity provider email domains

    +
    + + + + + + + + + + + + + +
    DomainUpdate on loginAuto registerDefault libraryDefault categoryAllow opacAllow staffActions
    +
    [% END %] -
    - - -
    - -
  • - - - Required -
    • -
  • - - -
    • - - -
    - - Cancel -
    - +
    Map provider's result to Koha patron's fields.
    + +
    + +
    + +
  • + + +
    Koha patron's field that will be used to match provider's user with Koha's
    +
    It must be present in mapping
    +
    • +
  • + + +
    • + + +
    + + Cancel +
    + + [% END %] [% IF op == 'edit_form' %]

    Edit identity provider

    -
    - - -
    -
      -
    1. - - - Required -
      2. -
    2. - - - Required -
      3. -
    3. - - -
      4. -
    -
    - -
    -
      -
    1. -
      +
      + + + +
      +
        +
      1. + + + Required +
        Code that identifies this provider. Only alphanumeric and "_" characters are allowed
        +
        2. +
      2. + + + Required +
        User friendly name of this provider
        +
        3. +
      3. + + +
        Choose the protocol this external identity provider uses
        +
        4. +
      +
      + +
      +
        +
      1. Required -
      -
      - - -
      -
      2. -
    2. -
      +
      Provider's main configuration.
      + +
      + +
      +
      3. +
    3. Required - -
      - - -
      -
      4. -
    4. - - - Required -
      5. -
    5. - - -
      6. -
    -
    -
    - - Cancel -
    -
    +
    Map provider's result to Koha patron's fields.
    + +
    + +
    + +
  • + + +
    Koha patron's field that will be used to match provider's user with Koha's
    +
    It must be present in mapping
    +
    • +
  • + + +
    • + + +
    + + Cancel +
    + + [% END %] [% IF op == 'list' %] @@ -261,17 +288,18 @@

    Identity providers

    - - - - - - - - - - -
    CodeDescriptionProtocolActions
    +
    + + + + + + + + + +
    CodeDescriptionProtocolActions
    +
    [% END %]