Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Swashbuckle instead of NSwag #13350

Merged

Conversation

kjac
Copy link
Contributor

@kjac kjac commented Nov 3, 2022

Prerequisites

  • I have added steps to test this contribution in the description below

Description

This PR replaces NSwag with Swashbuckle for OpenAPI schema generation.

Schema/model name generation

In order to get a more reasonable schema, a little bit of magic has also been introduced in ths PR, in order to shorten the generated schema/model names - i.e.:

  • RelationItemViewModel becomes RelationItem
  • PagedViewModelOfRelationItemViewModel becomes PagedRelationItem

This magic is performed by the SchemaIdGenerator. It is deliberately as static implementation, as it should never ever be replaced by means of DI. Refer to the implementation for details, it should be documented appropriately.

We may consider replacing the magic with explicit naming at some point. The magic path was chosen to achieve more reasonable schema as fast as possible, allowing the client to start using the schema sooner. There currently are some 100+ references to PagedViewModel<T>, most of which will have to be replaced if we go with explicit naming - and that's not even counting the "ViewModel" classes, which will also required explicit renaming.

Alternatively we could go the opt-in route where the auto generated schema names can be overridden by means of attribute decoration, but all this is subject to another discussion for another PR.

Included schemas

Endpoints returning CreatedResult and NotFoundObjectResult (basically any implementation of ObjectResult) cause the generated OpenAPI schema to contain the System.Type, System.Reflection.Assembly and all their referenced types (which is a LOT).

Upon closer inspection it turns out that NSwag deals with this by creating an "application/octet-stream" response, which is pretty useless to the client.

image

Swashbuckle on the other hand returns the correct type (i.e. CreatedResult), with the resulting overload of additional types being included in the schema.

This PR replaces a few of the CreatedResult and NotFoundObjectResult response definitions for this very reason, but not all of them - so System.Type etc. is still contained in the generated schema. We need to look into replacing CreatedResult ect. with custom models for all affected endpoints, in order to trim down the schema to that which is relevant for the Umbraco API. This task will be discussed elsewhere and handled separately from this PR, so for the time being we have to accept the additional schema types as-is.

Other points of interest

  • TelemetryLevelViewModel has been renamed to TelemetryViewModel because the new, trimmed down model name (the output from SchemaIdGenerator) became TelemetryLevel which clashed with the referenced Umbraco.Cms.Core.Models.TelemetryLevel enum.
  • Swashbuckle (and/or the bundled Swagger UI) does not seem to require a client secret for Authorization Code flows, so the ClientSecretManager (introduced with the new backoffice authentication solely for this specific purpose) has been removed. Yay!
  • Swashbuckle refers to the default JSON serializer for supported MIME types. This means that "text/plain" and "text/json" start appearing in the OpenAPI schema as accepted MIME types for requests and responses, and we probably don't want to support that explicitly. Thus the MimeTypeDocumentFilter has been introduced to filter out all unwanted MIME types.

Testing this PR

  1. Verify that the Swagger UI (/umbraco/swagger/index.html) still works.
  2. Protect an endpoint (e.g. add [Authorize(Policy = $"New{AuthorizationPolicies.SectionAccessForMediaTree}")] to MediaTreeControllerBase) and verify that the Swagger authentication still works:
    auth-swagger-swashbuckle
  3. Verify that the schema does not contain any "ViewModel" postfixes.
  4. Verify that paged list endpoints define a nice and tidy return model name - i.e. PagedLanguage instead of PagedViewModelOfLanguageViewModel:
    image

# Conflicts:
#	src/Umbraco.Cms.ManagementApi/ManagementApiComposer.cs
… use default login screen for back-office logins
…OTE: this is potentially behaviorally breaking!)
…aks everything for V11 without the new management APIs) and introduce a dedicated PoC policy setup for OpenIddict.
…-nswag

# Conflicts:
#	src/Umbraco.Cms.ManagementApi/Controllers/Security/BackOfficeController.cs
#	src/Umbraco.Cms.ManagementApi/DependencyInjection/BackOfficeAuthBuilderExtensions.cs
#	src/Umbraco.Cms.ManagementApi/ManagementApiComposer.cs
#	src/Umbraco.Cms.ManagementApi/OpenApi.json
#	src/Umbraco.Cms.ManagementApi/Security/BackOfficeApplicationManager.cs
Copy link
Contributor

@nikolajlauridsen nikolajlauridsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just had a peek through the code, and overall this looks good, I actually think the SchemaIdGenerator and MimeTypeDocumentFilter makes good sense even though it is slightly magic, but at least the code is reasonably easy to read and understand 🎉 But we can always discuss this a bit more with the entire team.

I have however noticed that our old nemesis: serializing enums has returned again leading to a mismatch between our schema and actual return value in modelsbuilder/dashboard for instance (it's present more places).

So in the swagger UI the example response is:

{
  "mode": 0,            <---- EnumAsInt
  "canGenerate": true,
  "outOfDateModels": true,
  "lastError": "string",
  "version": "string",
  "modelsNamespace": "string",
  "trackingOutOfDateModels": true
}

But the actual response is:

{
  "mode": "InMemoryAuto",    <--- EnumAsString
  "canGenerate": false,
  "outOfDateModels": false,
  "lastError": null,
  "version": "11.1.0--rc.preview.13+43ad93c",
  "modelsNamespace": "Umbraco.Cms.Web.Common.PublishedModels",
  "trackingOutOfDateModels": false
}

@nikolajlauridsen
Copy link
Contributor

One thin that initially confused me a bit too was this code in the composer:

swaggerGenOptions.AddSecurityRequirement(new OpenApiSecurityRequirement  
{  
    {  
        new OpenApiSecurityScheme  
        {  
            Reference = new OpenApiReference  
            {  
                Id = "OAuth",  
                Type = ReferenceType.SecurityScheme  
            }  
        },  
        new List<string> { }  
    }  
});

It wasn't until I noticed that OpenApiSecurityRequirement inherits from a dictionary that it made sense, so that might be worth mentioning in a comment, mostly just because it looks quite weird otherwise

@kjac
Copy link
Contributor Author

kjac commented Nov 4, 2022

@nikolajlauridsen good grief, right you are! Good thing one of us is awake 😆

The enum serialization is fixed now - by means of yet another filter 🙈 the EnumSchemaFilter.

Here's what the Swagger output for the TelemetryLevel enum looks like with this filter applied:

image

The filter also supports the usage of EnumMemberAttribute decoration, meaning that this:

image

...will yield this Swagger output:

image

@kjac
Copy link
Contributor Author

kjac commented Nov 4, 2022

It wasn't until I noticed that OpenApiSecurityRequirement inherits from a dictionary that it made sense, so that might be worth mentioning in a comment, mostly just because it looks quite weird otherwise

Done 👍

Copy link
Contributor

@nikolajlauridsen nikolajlauridsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a happy hippo now, everything looks good to me 👍so let's get this merged, great work as always 😊

@nikolajlauridsen nikolajlauridsen merged commit 2a5c70a into v11/dev Nov 4, 2022
@nikolajlauridsen nikolajlauridsen deleted the v11/new-backoffice/swashbuckle-instead-of-nswag branch November 4, 2022 13:46
Zeegaan added a commit that referenced this pull request Nov 8, 2022
* Bump version

* Add IContextCache to deploy connectors (#13287)

* Add IContextCache and implementations

* Update connector interfaces to use IContextCache

* Minor cleanup

* Move DeployContextCache prefix to constant

* Move default implementations to obsolete methods

* Remove DeployContextCache and DictionaryCache

Co-authored-by: Andy Butland <abutland73@gmail.com>

* Add IContextCache to deploy connectors (#13287)

* Add IContextCache and implementations

* Update connector interfaces to use IContextCache

* Minor cleanup

* Move DeployContextCache prefix to constant

* Move default implementations to obsolete methods

* Remove DeployContextCache and DictionaryCache

Co-authored-by: Andy Butland <abutland73@gmail.com>

* Parse lockId as invariant (#13284)

Co-authored-by: Zeegaan <nge@umbraco.dk>

* Fix Sqlite database locking issue (#13246)

* Add locking for creating scope

* Lock the repository instead

* Add scope in action instead of locking in service

* Fix up post-merge

Co-authored-by: Zeegaan <nge@umbraco.dk>

* Bump version to next minor

* Fix for UseExceptionHandler no longer working since v10.3 RC (#13218)

* Fix for UseExceptionHandler no longer working since v10.3 RC

* Update the management api path to match the new one

Co-authored-by: Nikolaj <nikolajlauridsen@protonmail.ch>

* New backoffice: Cleanup management API routes (#13296)

* Rename ModelsBuilderDashboard folder to ModelsBuilder

* Fix modelsbuilder paths and related naming

* Rename analytics route to telemetry

* Fix controller bases - routes and tags

* Fix items route

* Fix more controllerbase routes

* Fix route

* Fix OpenApi file

* Merging DictionaryItem and Dictionary

* Fix TrackedReferences naming

* Update OpenApi file

* Rename Analytics* related types to Telemetry*

* New Backoffice: Return AnalyticsLevelViewModel from Telemetry/ (#13298)

* Return TelemetryLevelViewModel instead of TelemetryLevel

* Fix schema

* Change telemetry/current to telemetry/level

(cherry picked from commit f2b8494)

* Add contants for tree and recycle-bin subpaths

(cherry picked from commit 4449f56)

Co-authored-by: Mole <nikolajlauridsen@protonmail.ch>

* Updated Smidge, Npoco and MailKit (#13310)

* Updated Smidge, Npoco and MailKit

* Added missing command (after breaking interface in npoco)

* OpenId Connect authentication for new management API (#13318)

* First attempt at OpenIddict

* Making headway and more TODOs

* Redo current policies for multiple schemas + clean up auth controller

* Fix bad merge

* Clean up some more test code

* Fix spacing

* Include AddAuthentication() in OpenIddict addition

* A little more clean-up

* Move application creation to its own implementation + prepare for middleware to handle valid callback URL

* Enable refresh token flow

* Fix bad merge from v11/dev

* Support auth for Swagger and Postman in non-production environments + use default login screen for back-office logins

* Add workaround to client side login handling so the OAuth return URL is not corrupted before redirection

* Add temporary configuration handling for new backoffice

* Restructure the code somewhat, move singular responsibility from management API project

* Add recurring task for cleaning up old tokens in the DB

* Fix bad merge + make auth controller align with the new management API structure

* Explicitly handle the new management API path as a backoffice path (NOTE: this is potentially behaviorally breaking!)

* Redo handle the new management API requests as backoffice requests, this time in a non-breaking way

* Add/update TODOs

* Revert duplication of current auth policies for OpenIddict (as it breaks everything for V11 without the new management APIs) and introduce a dedicated PoC policy setup for OpenIddict.

* Fix failing unit tests

* Update src/Umbraco.Cms.ManagementApi/Security/BackOfficeApplicationManager.cs

Co-authored-by: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com>

* Update src/Umbraco.Cms.ManagementApi/Security/BackOfficeApplicationManager.cs

Co-authored-by: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com>

* Update src/Umbraco.Cms.ManagementApi/Security/BackOfficeApplicationManager.cs

Co-authored-by: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com>

* Update src/Umbraco.Core/Routing/UmbracoRequestPaths.cs

Co-authored-by: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com>

* V11: Using IFileProvider to access assets added from packages (#13141)

* Creating a FileProviderFactory for getting the package.manifest and grid.editors.config.js files through a file provider

* Collecting the package.manifest-s from different sources

* Searching different sources for grid.editors.config.js

* Using an IFileProvider to collect all tours

* Refactoring IconService.cs

* Typo

* Optimizations when looping through the file system

* Moving WebRootFileProviderFactory to Umbraco.Web.Common proj

* Removes double registering

* pluginLangFileSources includes the localPluginFileSources

* Comments

* Remove linq from foreach

* Change workflow for grid.editors.config.js so we check first physical file, then RCL, then Embedded

* Clean up

* Check if config dir exists

* Discover nested package.manifest files

* Fix IFileInfo.PhysicalPath check

* Revert 712810e as that way files in content root are preferred over those in web root

* Adding comments

* Refactoring

* Remove PhysicalPath check

* Fix registration of WebRootFileProviderFactory

* Use Swashbuckle instead of NSwag (#13350)

* First attempt at OpenIddict

* Making headway and more TODOs

* Redo current policies for multiple schemas + clean up auth controller

* Fix bad merge

* Clean up some more test code

* Fix spacing

* Include AddAuthentication() in OpenIddict addition

* A little more clean-up

* Move application creation to its own implementation + prepare for middleware to handle valid callback URL

* Enable refresh token flow

* Fix bad merge from v11/dev

* Support auth for Swagger and Postman in non-production environments + use default login screen for back-office logins

* Add workaround to client side login handling so the OAuth return URL is not corrupted before redirection

* Add temporary configuration handling for new backoffice

* Restructure the code somewhat, move singular responsibility from management API project

* Add recurring task for cleaning up old tokens in the DB

* Fix bad merge + make auth controller align with the new management API structure

* Explicitly handle the new management API path as a backoffice path (NOTE: this is potentially behaviorally breaking!)

* Redo handle the new management API requests as backoffice requests, this time in a non-breaking way

* Add/update TODOs

* Replace NSwag with Swashbuckle and clean up unnecessary client secret workaround

* Revert duplication of current auth policies for OpenIddict (as it breaks everything for V11 without the new management APIs) and introduce a dedicated PoC policy setup for OpenIddict.

* Fix failing unit tests

* A little niceness + export new OpenApi.json and fix path in contract unit test

* Redo after merge with v11/dev + filter out unwanted mime types

* Remove CreatedResult and NotFoundObjectResult where possible

* Custom schema IDs - no more "ViewModel" postfix and make generic lists look less clunky too

* A little more explanation for generic schema ID generation

* Force Swashbuckle to use enum string names

* Update OpenApi.json to match new enum string values

* Add clarifying comment about weird looking construct

* add workflow to schema (#13349)

* add workflow to schema

* add licenses to CMSDefinition - intentionally only adding to schema, not registered as options

Co-authored-by: Nikolaj <nikolajlauridsen@protonmail.ch>
Co-authored-by: Ronald Barendse <ronald@barend.se>
Co-authored-by: Andy Butland <abutland73@gmail.com>
Co-authored-by: Zeegaan <nge@umbraco.dk>
Co-authored-by: Justin Neville <67802060+justin-nevitech@users.noreply.github.com>
Co-authored-by: Elitsa Marinovska <21998037+elit0451@users.noreply.github.com>
Co-authored-by: Bjarke Berg <mail@bergmania.dk>
Co-authored-by: Kenn Jacobsen <kja@umbraco.dk>
Co-authored-by: Nathan Woulfe <nathan@nathanw.com.au>
nikolajlauridsen pushed a commit that referenced this pull request Nov 8, 2022
* First attempt at OpenIddict

* Making headway and more TODOs

* Redo current policies for multiple schemas + clean up auth controller

* Fix bad merge

* Clean up some more test code

* Fix spacing

* Include AddAuthentication() in OpenIddict addition

* A little more clean-up

* Move application creation to its own implementation + prepare for middleware to handle valid callback URL

* Enable refresh token flow

* Fix bad merge from v11/dev

* Support auth for Swagger and Postman in non-production environments + use default login screen for back-office logins

* Add workaround to client side login handling so the OAuth return URL is not corrupted before redirection

* Add temporary configuration handling for new backoffice

* Restructure the code somewhat, move singular responsibility from management API project

* Add recurring task for cleaning up old tokens in the DB

* Fix bad merge + make auth controller align with the new management API structure

* Explicitly handle the new management API path as a backoffice path (NOTE: this is potentially behaviorally breaking!)

* Redo handle the new management API requests as backoffice requests, this time in a non-breaking way

* Add/update TODOs

* Replace NSwag with Swashbuckle and clean up unnecessary client secret workaround

* Revert duplication of current auth policies for OpenIddict (as it breaks everything for V11 without the new management APIs) and introduce a dedicated PoC policy setup for OpenIddict.

* Fix failing unit tests

* A little niceness + export new OpenApi.json and fix path in contract unit test

* Redo after merge with v11/dev + filter out unwanted mime types

* Remove CreatedResult and NotFoundObjectResult where possible

* Custom schema IDs - no more "ViewModel" postfix and make generic lists look less clunky too

* A little more explanation for generic schema ID generation

* Force Swashbuckle to use enum string names

* Update OpenApi.json to match new enum string values

* Add clarifying comment about weird looking construct
nikolajlauridsen pushed a commit that referenced this pull request Nov 8, 2022
* First attempt at OpenIddict

* Making headway and more TODOs

* Redo current policies for multiple schemas + clean up auth controller

* Fix bad merge

* Clean up some more test code

* Fix spacing

* Include AddAuthentication() in OpenIddict addition

* A little more clean-up

* Move application creation to its own implementation + prepare for middleware to handle valid callback URL

* Enable refresh token flow

* Fix bad merge from v11/dev

* Support auth for Swagger and Postman in non-production environments + use default login screen for back-office logins

* Add workaround to client side login handling so the OAuth return URL is not corrupted before redirection

* Add temporary configuration handling for new backoffice

* Restructure the code somewhat, move singular responsibility from management API project

* Add recurring task for cleaning up old tokens in the DB

* Fix bad merge + make auth controller align with the new management API structure

* Explicitly handle the new management API path as a backoffice path (NOTE: this is potentially behaviorally breaking!)

* Redo handle the new management API requests as backoffice requests, this time in a non-breaking way

* Add/update TODOs

* Replace NSwag with Swashbuckle and clean up unnecessary client secret workaround

* Revert duplication of current auth policies for OpenIddict (as it breaks everything for V11 without the new management APIs) and introduce a dedicated PoC policy setup for OpenIddict.

* Fix failing unit tests

* A little niceness + export new OpenApi.json and fix path in contract unit test

* Redo after merge with v11/dev + filter out unwanted mime types

* Remove CreatedResult and NotFoundObjectResult where possible

* Custom schema IDs - no more "ViewModel" postfix and make generic lists look less clunky too

* A little more explanation for generic schema ID generation

* Force Swashbuckle to use enum string names

* Update OpenApi.json to match new enum string values

* Add clarifying comment about weird looking construct
Zeegaan added a commit that referenced this pull request Nov 8, 2022
* New backoffice: Cleanup management API routes (#13296)

* Rename ModelsBuilderDashboard folder to ModelsBuilder

* Fix modelsbuilder paths and related naming

* Rename analytics route to telemetry

* Fix controller bases - routes and tags

* Fix items route

* Fix more controllerbase routes

* Fix route

* Fix OpenApi file

* Merging DictionaryItem and Dictionary

* Fix TrackedReferences naming

* Update OpenApi file

* Rename Analytics* related types to Telemetry*

* New Backoffice: Return AnalyticsLevelViewModel from Telemetry/ (#13298)

* Return TelemetryLevelViewModel instead of TelemetryLevel

* Fix schema

* Change telemetry/current to telemetry/level

(cherry picked from commit f2b8494)

* Add contants for tree and recycle-bin subpaths

(cherry picked from commit 4449f56)

Co-authored-by: Mole <nikolajlauridsen@protonmail.ch>

* OpenId Connect authentication for new management API (#13318)

* First attempt at OpenIddict

* Making headway and more TODOs

* Redo current policies for multiple schemas + clean up auth controller

* Fix bad merge

* Clean up some more test code

* Fix spacing

* Include AddAuthentication() in OpenIddict addition

* A little more clean-up

* Move application creation to its own implementation + prepare for middleware to handle valid callback URL

* Enable refresh token flow

* Fix bad merge from v11/dev

* Support auth for Swagger and Postman in non-production environments + use default login screen for back-office logins

* Add workaround to client side login handling so the OAuth return URL is not corrupted before redirection

* Add temporary configuration handling for new backoffice

* Restructure the code somewhat, move singular responsibility from management API project

* Add recurring task for cleaning up old tokens in the DB

* Fix bad merge + make auth controller align with the new management API structure

* Explicitly handle the new management API path as a backoffice path (NOTE: this is potentially behaviorally breaking!)

* Redo handle the new management API requests as backoffice requests, this time in a non-breaking way

* Add/update TODOs

* Revert duplication of current auth policies for OpenIddict (as it breaks everything for V11 without the new management APIs) and introduce a dedicated PoC policy setup for OpenIddict.

* Fix failing unit tests

* Update src/Umbraco.Cms.ManagementApi/Security/BackOfficeApplicationManager.cs

Co-authored-by: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com>

* Update src/Umbraco.Cms.ManagementApi/Security/BackOfficeApplicationManager.cs

Co-authored-by: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com>

* Update src/Umbraco.Cms.ManagementApi/Security/BackOfficeApplicationManager.cs

Co-authored-by: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com>

* Update src/Umbraco.Core/Routing/UmbracoRequestPaths.cs

Co-authored-by: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com>

* Use Swashbuckle instead of NSwag (#13350)

* First attempt at OpenIddict

* Making headway and more TODOs

* Redo current policies for multiple schemas + clean up auth controller

* Fix bad merge

* Clean up some more test code

* Fix spacing

* Include AddAuthentication() in OpenIddict addition

* A little more clean-up

* Move application creation to its own implementation + prepare for middleware to handle valid callback URL

* Enable refresh token flow

* Fix bad merge from v11/dev

* Support auth for Swagger and Postman in non-production environments + use default login screen for back-office logins

* Add workaround to client side login handling so the OAuth return URL is not corrupted before redirection

* Add temporary configuration handling for new backoffice

* Restructure the code somewhat, move singular responsibility from management API project

* Add recurring task for cleaning up old tokens in the DB

* Fix bad merge + make auth controller align with the new management API structure

* Explicitly handle the new management API path as a backoffice path (NOTE: this is potentially behaviorally breaking!)

* Redo handle the new management API requests as backoffice requests, this time in a non-breaking way

* Add/update TODOs

* Replace NSwag with Swashbuckle and clean up unnecessary client secret workaround

* Revert duplication of current auth policies for OpenIddict (as it breaks everything for V11 without the new management APIs) and introduce a dedicated PoC policy setup for OpenIddict.

* Fix failing unit tests

* A little niceness + export new OpenApi.json and fix path in contract unit test

* Redo after merge with v11/dev + filter out unwanted mime types

* Remove CreatedResult and NotFoundObjectResult where possible

* Custom schema IDs - no more "ViewModel" postfix and make generic lists look less clunky too

* A little more explanation for generic schema ID generation

* Force Swashbuckle to use enum string names

* Update OpenApi.json to match new enum string values

* Add clarifying comment about weird looking construct

Co-authored-by: Elitsa Marinovska <21998037+elit0451@users.noreply.github.com>
Co-authored-by: Kenn Jacobsen <kja@umbraco.dk>
Co-authored-by: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants