From 454980dcbe93b2b6e28f36910cab82805730b342 Mon Sep 17 00:00:00 2001 From: Olli-Antti Kivilahti Date: Wed, 12 Apr 2023 12:52:41 +0300 Subject: [PATCH] Bug 33503 - Plugin OpenAPI2.0 specification schema fragments are not resolved. Merge full schema definitions. Koha::REST::Plugin::PluginRoutes.pm simply injects a plugin's OpenAPI2.0 route definitions into the /paths -object. Possibly after Bug 30194, dereferencing OpenAPI2.0 schema fragments/components has started to fail. This plugin: https://github.com/Hypernova-Oy/koha-plugin-self-service/releases/tag/v1.0.11 used to get it's REST API paths injected and dereferenced/bundled correctly, for Koha versions 22.05 and before, but in the new Koha version the same schema definitions no longer work. This patch adds a new plugin hook, api_spec which injects a full OpenAPI2.0 compatible schema into the full Koha schema, avoiding to overwrite any existing definitions/parameters/info/paths/etc. This way we maintain backwards compatibility with existing plugins, which are not broken, and maintain updated plugins' ability to use complex schemas. We need to inject full schema definitions, because of the way how the new OpenAPI validator does dereferencing, by creating /parameters and /definitions -objects inside the plugin's OpenAPI schema object. The api_route()-plugin hook then proceeds to strip away only the paths/routes and loses the internal data structure references to the schema fragments/components. If the api_spec is defined in the plugin, the api_routes-hook is never called. TEST PLAN: a) Run the modified test. or b) Install this plugin, and observe the routes are not loaded into the REST API. https://github.com/Hypernova-Oy/koha-plugin-self-service/releases/tag/v1.0.11 b1) Deploying this patch doesnt fix the plugin, as the whole subsystem to load OpenAPI2 spec has changed, and the plugins updated with workarounds. Plugins can be updated to support api_spec once it has been pushed. Signed-off-by: Victor Grousset/tuxayo --- Koha/REST/Plugin/PluginRoutes.pm | 77 ++++++++++++++++++- .../Koha/REST/Plugin/PluginRoutes.t | 3 +- t/lib/plugins/Koha/Plugin/BundledApiSpec.pm | 46 +++++++++++ .../Koha/Plugin/BundledApiSpec/openapi.yaml | 69 +++++++++++++++++ .../BundledApiSpec/openapi/definitions.json | 5 ++ .../openapi/definitions/finger.json | 12 +++ .../BundledApiSpec/openapi/parameters.json | 18 +++++ 7 files changed, 226 insertions(+), 4 deletions(-) create mode 100644 t/lib/plugins/Koha/Plugin/BundledApiSpec.pm create mode 100644 t/lib/plugins/Koha/Plugin/BundledApiSpec/openapi.yaml create mode 100644 t/lib/plugins/Koha/Plugin/BundledApiSpec/openapi/definitions.json create mode 100644 t/lib/plugins/Koha/Plugin/BundledApiSpec/openapi/definitions/finger.json create mode 100644 t/lib/plugins/Koha/Plugin/BundledApiSpec/openapi/parameters.json diff --git a/Koha/REST/Plugin/PluginRoutes.pm b/Koha/REST/Plugin/PluginRoutes.pm index c275f2578c..fac7d48be1 100644 --- a/Koha/REST/Plugin/PluginRoutes.pm +++ b/Koha/REST/Plugin/PluginRoutes.pm @@ -102,11 +102,13 @@ sub inject_routes { sub merge_spec { my ( $spec, $plugin ) = @_; - if($plugin->can('api_routes')) { + # Prefer the new api_spec() to inject a full OpenAPI2 spec instead of just the route definitions, if both are defined, to prevent duplicate routes. + # This way we can use the same plugin release for the new and the old versions of Koha. + if($plugin->can('api_routes') && not($plugin->can('api_spec'))) { # Deprecated my $plugin_spec = $plugin->api_routes; foreach my $route ( keys %{ $plugin_spec } ) { - my $THE_route = '/contrib/' . $plugin->api_namespace . $route; + my $THE_route = get_normalized_plugin_path($plugin->api_namespace, $route); if ( exists $spec->{ $THE_route } ) { # Route exists, overwriting is forbidden Koha::Exceptions::Plugin::ForbiddenAction->throw( @@ -118,12 +120,57 @@ sub merge_spec { } } + if($plugin->can('api_spec')) { + my $plugin_namespace = $plugin->api_namespace; + my $plugin_spec = $plugin->api_spec; + my @errors; + if ($plugin_spec->{definitions}) { + while (my ($k, $v) = each %{$plugin_spec->{definitions}}) { + if ($spec->{definitions}->{$k}) { + push(@errors, "Duplicate OpenAPI definition '$k'. Plugin needs to rename one of it's definitions."); + } else { + $spec->{definitions}->{$k} = $v; + } + } + } + if ($plugin_spec->{parameters}) { + while (my ($k, $v) = each %{$plugin_spec->{parameters}}) { + if ($spec->{parameters}->{$k}) { + push(@errors, "Duplicate OpenAPI parameter '$k'. Plugin needs to rename one of it's parameters."); + } else { + $spec->{parameters}->{$k} = $v; + } + } + } + if ($plugin_spec->{tags}) { + if ($spec->{tags}) { + push(@{$spec->{tags}}, @{$plugin_spec->{tags}}); + } else { + $spec->{tags} = $plugin_spec->{tags}; + } + } + if ($plugin_spec->{paths}) { + while (my ($k, $v) = each %{$plugin_spec->{paths}}) { + if ($spec->{paths}->{$k}) { + push(@errors, "Duplicate OpenAPI parameter '$k'. Plugin needs to rename one of it's paths."); + } else { + $spec->{paths}->{get_normalized_plugin_path($plugin_namespace, $k)} = $v; + } + } + } + if (@errors) { + Koha::Exceptions::Plugin::ForbiddenAction->throw( + "Plugin '".$plugin->{class}."' had the following errors while merging OpenAPI specs:\n".join(', ', @errors) + ); + } + } + if($plugin->can('static_routes')) { my $plugin_spec = $plugin->static_routes; foreach my $route ( keys %{ $plugin_spec } ) { - my $THE_route = '/contrib/' . $plugin->api_namespace . '/static'.$route; + my $THE_route = get_normalized_plugin_static_path($plugin->api_namespace, $route); if ( exists $spec->{ $THE_route } ) { # Route exists, overwriting is forbidden Koha::Exceptions::Plugin::ForbiddenAction->throw( @@ -153,4 +200,28 @@ sub spec_ok { return !$schema->is_invalid; } +=head3 get_normalized_plugin_path + +public static subroutine +Normalizes a plugin's route path to /contrib// + +=cut + +sub get_normalized_plugin_path { + my ($plugin_api_namespace, $plugin_path_name) = @_; + return '/contrib/' . $plugin_api_namespace . $plugin_path_name; +} + +=head3 get_normalized_plugin_static_path + +public static subroutine +Normalizes a plugin's static route path to /contrib//static/ + +=cut + +sub get_normalized_plugin_static_path { + my ($plugin_api_namespace, $plugin_path_name) = @_; + return '/contrib/' . $plugin_api_namespace . '/static' . $plugin_path_name; +} + 1; diff --git a/t/db_dependent/Koha/REST/Plugin/PluginRoutes.t b/t/db_dependent/Koha/REST/Plugin/PluginRoutes.t index 66746508ed..98e7138a32 100755 --- a/t/db_dependent/Koha/REST/Plugin/PluginRoutes.t +++ b/t/db_dependent/Koha/REST/Plugin/PluginRoutes.t @@ -129,7 +129,7 @@ subtest 'Disabled plugins tests' => sub { subtest 'Permissions and access to plugin routes tests' => sub { - plan tests => 16; + plan tests => 17; $schema->storage->txn_begin; @@ -164,6 +164,7 @@ subtest 'Permissions and access to plugin routes tests' => sub { my $routes = get_defined_routes($t); ok( exists $routes->{'/contrib/testplugin/patrons/bother'}, 'Route exists' ); ok( exists $routes->{'/contrib/testplugin/public/patrons/bother'}, 'Route exists' ); + ok( exists $routes->{'/contrib/bundledass/patrons/<:patron_id>/finger'}, 'Bundled Route exists' ); C4::Context->set_preference( 'RESTPublicAnonymousRequests', 0 ); diff --git a/t/lib/plugins/Koha/Plugin/BundledApiSpec.pm b/t/lib/plugins/Koha/Plugin/BundledApiSpec.pm new file mode 100644 index 0000000000..368d8609e8 --- /dev/null +++ b/t/lib/plugins/Koha/Plugin/BundledApiSpec.pm @@ -0,0 +1,46 @@ +package Koha::Plugin::BundledApiSpec; + +use Modern::Perl; + +use base qw(Koha::Plugins::Base); + +our $VERSION = 0.01; +our $metadata = { + name => 'Bundled OpenAPI Spec Plugin', + author => 'Olli-Antti Kivilahti', + description => 'Test plugin for testing the bundling of OpenAPI schema fragments', + date_authored => '2023-', + date_updated => '2023-04-06', + minimum_version => '21.11', + maximum_version => undef, + version => $VERSION, + my_example_tag => 'bundled_api_spec', +}; + +sub new { + my ( $class, $args ) = @_; + $args->{'metadata'} = $metadata; + my $self = $class->SUPER::new($args); + return $self; +} + +sub api_namespace { + return "bundledass"; +} + +# One should never define these both, but only the other. +#sub api_route { +# my ( $self, $args ) = @_; +# return JSON::Validator->new->schema($self->mbf_dir() . "/openapi.yaml")->schema->{data}; +#} + +# One should never define these both, but only the other. Automated test suite should disable the other for testing. +sub api_spec { + my ( $self, $args ) = @_; + + my $schema2 = JSON::Validator::Schema::OpenAPIv2->new; + $schema2->resolve($self->mbf_dir() . "/openapi.yaml"); + return $schema2->bundle->data; +} + +1; diff --git a/t/lib/plugins/Koha/Plugin/BundledApiSpec/openapi.yaml b/t/lib/plugins/Koha/Plugin/BundledApiSpec/openapi.yaml new file mode 100644 index 0000000000..1cf2132118 --- /dev/null +++ b/t/lib/plugins/Koha/Plugin/BundledApiSpec/openapi.yaml @@ -0,0 +1,69 @@ +--- + +swagger: "2.0" +info: + title: API Title + version: "1.0" + +paths: + /patrons/{patron_id}/finger: + delete: + x-mojo-to: Koha::Plugin::BundledApiSpec#finger_delete + operationId: finger_delete + tags: + - plugin + produces: + - application/json + parameters: + - $ref: openapi/parameters.json#/patron_idPathParam + x-koha-authorization: + permissions: + borrowers: finger + responses: + "200": + description: Deleted ok + schema: + type: object + required: + - deleted_count + properties: + deleted_count: + description: How many Block-objects have been deleted in the DB + type: integer + get: + x-mojo-to: Koha::Plugin::BundledApiSpec#finger_get + operationId: finger_get + tags: + - plugin + produces: + - application/json + parameters: + - $ref: openapi/parameters.json#/patron_idPathParam + x-koha-authorization: + permissions: + borrowers: finger + responses: + "200": + description: List of fingers + schema: + type: array + items: + $ref: openapi/definitions.json#/finger + post: + x-mojo-to: Koha::Plugin::BundledApiSpec#finger_post + operationId: finger_post + tags: + - plugin + produces: + - application/json + parameters: + - $ref: openapi/parameters.json#/patron_idPathParam + - $ref: openapi/parameters.json#/finger + x-koha-authorization: + permissions: + borrowers: finger + responses: + "200": + description: The created self-service block + schema: + $ref: openapi/definitions.json#/finger diff --git a/t/lib/plugins/Koha/Plugin/BundledApiSpec/openapi/definitions.json b/t/lib/plugins/Koha/Plugin/BundledApiSpec/openapi/definitions.json new file mode 100644 index 0000000000..2ee34eac20 --- /dev/null +++ b/t/lib/plugins/Koha/Plugin/BundledApiSpec/openapi/definitions.json @@ -0,0 +1,5 @@ +{ + "finger": { + "$ref": "./definitions/finger.json" + } +} \ No newline at end of file diff --git a/t/lib/plugins/Koha/Plugin/BundledApiSpec/openapi/definitions/finger.json b/t/lib/plugins/Koha/Plugin/BundledApiSpec/openapi/definitions/finger.json new file mode 100644 index 0000000000..9adc5fb6fe --- /dev/null +++ b/t/lib/plugins/Koha/Plugin/BundledApiSpec/openapi/definitions/finger.json @@ -0,0 +1,12 @@ +{ + "type": "object", + "properties": { + "patron_id": { + "type": "integer", + "description": "The user that has the finger" + }, + "type": { + "type": "string" + } + } +} \ No newline at end of file diff --git a/t/lib/plugins/Koha/Plugin/BundledApiSpec/openapi/parameters.json b/t/lib/plugins/Koha/Plugin/BundledApiSpec/openapi/parameters.json new file mode 100644 index 0000000000..9beb538348 --- /dev/null +++ b/t/lib/plugins/Koha/Plugin/BundledApiSpec/openapi/parameters.json @@ -0,0 +1,18 @@ +{ + "patron_idPathParam": { + "name": "patron_id", + "in": "path", + "description": "Internal Patron identifier", + "required": true, + "type": "integer" + }, + "finger": { + "name": "finger", + "description": "Finger", + "in": "body", + "required": true, + "schema": { + "$ref": "./definitions.json#/finger" + } + } +} \ No newline at end of file -- 2.42.0