papsign / Ktor-OpenAPI-Generator

Ktor OpenAPI/Swagger 3 Generator
Apache License 2.0
241 stars 42 forks source link

Scopes enum is required even for Bearer authentication #65

Open sigmanil opened 4 years ago

sigmanil commented 4 years ago

As far as I understand the OpenAPI documentation, Scopes should be an empty list when using Bearer authentication. See: https://swagger.io/docs/specification/authentication/bearer-authentication/

In Ktor-OpenAPI-Generator, due to the way com.papsign.ktor.openapigen.modules.providers.AuthProvider.Security is defined, it seems that we must still have an Enum class with scopes even when only supporting Bearer authentication: requierments must be a List, "where T: Enum" and T is a Described.

Intuitively, I think a bearer scheme should be possible to define either as follows, skipping the requirements parameter:

class JwtProvider : AuthProvider<UserPrincipal> {
    override val security: Iterable<Iterable<Security<*>>> =
        listOf(listOf(Security(SecuritySchemeModel(SecuritySchemeType.openIdConnect, scheme = HttpSecurityScheme.bearer, bearerFormat = "JWT", name = "JWT"))))

Or as follows, providing an empty list:

class JwtProvider : AuthProvider<UserPrincipal> {
    override val security: Iterable<Iterable<Security<*>>> =
        listOf(listOf(Security(SecuritySchemeModel(SecuritySchemeType.openIdConnect, scheme = HttpSecurityScheme.bearer, bearerFormat = "JWT", name = "JWT"), emptyList()))

But both of these result in compilation errors.

The solution for now has been to have "dummy enum" that's a subtype of Described, as follows:

enum class Scopes(override val description: String) : Described {
    DummyScope("Some scope")
}

...and then using that to define an empty list that can be sent to the Security class:

    override val security: Iterable<Iterable<Security<*>>> =
        listOf(listOf(Security(SecuritySchemeModel(SecuritySchemeType.openIdConnect, scheme = HttpSecurityScheme.bearer, bearerFormat = "JWT", name = "JWT"), emptyList<Scopes>())))

But the "dummy enum" feels out of place, and is never used anywhere apart from placating the type system. Unless I'm missing something?

Wicpar commented 4 years ago

Yes, that restriction is quite bothersome and should be relaxed. I unfortunately haven't gotten around to it. The security was designed with OAuth2 in mind, that is why typed enforcement of flows seemed a good idea.

sigmanil commented 4 years ago

I haven't really thought this through very thoroughly, but would an easy improvement be to change the type parameter in AuthProvider.Security, SecurityShemeModel to just require a Described, and not an Enum? For example:

Security<TScope : Described>(val scheme: SecuritySchemeModel<TScope>, val requirements: List<TScope>)

Or are you using the fact that it's an Enum for something somewhere? If not, that seems something that could be left to the discretion of whoever is using the library. In my head it wouldn't be a breaking change for any current users (though again, I might easily be missing something). And in my case it would allow me to say emptyList<Described>(), and forego the dummy enum.

Wicpar commented 4 years ago

Enum is used for the ".name" and serialization, but besides that it was only meant as type enforcement. I think the issue with removing the Enum would be that the security model would be improperly generated. Replacing it with a String is possible, but generally using language types directly is unadvisable. Maybe a wrapper that handles the serialization properly ?