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.

Sign up for GitHub

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

OpenAPI: enable auto security filter for auth policy via configuration #36460

Closed
wants to merge 1 commit into from
+77 −23
Closed

OpenAPI: enable auto security filter for auth policy via configuration #36460

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 22 additions & 7 deletions ...oyment/src/main/java/io/quarkus/smallrye/openapi/deployment/SmallRyeOpenApiProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@
import io.quarkus.vertx.http.deployment.RouteBuildItem;
import io.quarkus.vertx.http.deployment.SecurityInformationBuildItem;
import io.quarkus.vertx.http.deployment.devmode.NotFoundPageDisplayableEndpointBuildItem;
import io.quarkus.vertx.http.runtime.HttpBuildTimeConfig;
import io.quarkus.vertx.http.runtime.management.ManagementInterfaceBuildTimeConfig;
import io.quarkus.vertx.http.runtime.management.ManagementInterfaceConfiguration;
import io.smallrye.openapi.api.OpenApiConfig;
Expand Down Expand Up @@ -222,15 +223,15 @@ void registerAutoSecurityFilter(BuildProducer<SyntheticBeanBuildItem> syntheticB
SmallRyeOpenApiConfig openApiConfig,
OpenApiFilteredIndexViewBuildItem apiFilteredIndexViewBuildItem,
List<SecurityInformationBuildItem> securityInformationBuildItems,
OpenApiRecorder recorder) {
OpenApiRecorder recorder,
HttpBuildTimeConfig httpConfig) {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it can complicate a planned migration of this configuration to Runtime ? CC @michalvavrik

Copy link
Member

Choose a reason for hiding this comment

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

There will always need to be at least one build time property that will allow to you decide, if HTTP permissions are used not, for we need to decide if we produce beans (sure, we could also keep them there, in which case these changes are blocker). I'll comment below as well.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, but we produce it now anyway, so no build time properties needed, sorry.

Copy link
Member

Choose a reason for hiding this comment

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

So, answer is yes.


OASFilter autoSecurityFilter = null;
if (openApiConfig.autoAddSecurity) {

if (openApiConfig.autoAddSecurity
Copy link
Member

@michalvavrik michalvavrik Oct 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Now you are able to enable security with io.quarkus.smallrye.openapi.common.deployment.SmallRyeOpenApiConfig#autoAddSecurity and including security extension and either use RolessAlowed-Authenticated or HTTP permissions to have security enabled, correct?

I wonder what happens when user uses other HTTP Security Policies like Keycloak Authorization or has their own one because they implemented global policy due to our limited path matching capabilities? Users will have to specify securityScheme when they want it generated, rather then it being detected?

I can see why this is an improvement, but it will block moving HTTP permissions to runtime. Isn't that rather rare to have security extension added (you need to add it explicitly, right?) without securing anything? Why can't you simply add it always when security extension is present and if user don't want to add these schemes, they will set auto-add-security to false.

Also, the config property description says nothing about actually securing endpoints, therefore my proposed change would conform description This will automatically add security based on the security extension included (if any).. If you don't agree, please change the description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now you are able to enable security with io.quarkus.smallrye.openapi.common.deployment.SmallRyeOpenApiConfig#autoAddSecurity and including security extension and either use RolessAlowed-Authenticated or HTTP permissions to have security enabled, correct?

@michalvavrik , that's correct. For your question on other policies, I think it depends on what HttpBuildTimeConfig and the SecurityInformationBuildItem look like for other/custom security policies. I'm thinking that the answer is what you said - the user would need to specify their own securityScheme and this approach only works when SecurityInformationBuildItem has one of the known SecurityModels.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for reply @MikeEdgar. I don't see any good reason why scheme shouldn't be generated when the security extension is present and SecurityInformationBuildItem is present as well. Just because someone doesn't use HTTP permissions and RBAC annotations, it doesn't mean they don't do security. Is there a point why you want to check HTTP permissions explicitly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a point why you want to check HTTP permissions explicitly?

Only because it's what I needed for another project, but I think some more general approach would be better if it's possible.

Copy link
Member

Choose a reason for hiding this comment

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

Well, it's not a deal break, I suppose we can still migrate HTTP permissions and just add some flag for OpenAPI, but I'd appreciate if it was possible to not use HTTP permissions during the build time unless you need to. Whatever works for you, thanks.

Copy link
Member

Choose a reason for hiding this comment

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

No objections there :-) I simply relied on docs in this for that's as much as I know about this thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should fix the configuration property description then :)

I'm not following why this would be changed. If there are multiple ways to configure the security - annotations, configuration, KC authorizer, etc. - why wouldn't it be preferable to use auto-add-security=false to disable the automatic security when not desired?

Copy link
Member

Choose a reason for hiding this comment

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

Auto security adds security (in OpenAPI) when nothing is configured in OpenAPI (annotations, configuration) but only if

  • Security extensions are added
  • Other (security) annotations and configuration are detected.

The second part are not in the description ....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, got it 👍. I think @michalvavrik's suggestion earlier was to remove the second part from the extension's code and allow users not wanting security to disable it using false. The issue I see is that if we require annotations, there are other possible ways to setup security that aren't supported.

Copy link
Member

Choose a reason for hiding this comment

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

Also remember, auto security for OpenAPI only run if there is not OpenAPI Security annotations. The user can always still add their own OpenAPI Security annotation when it's a special case.

&& hasEnabledAuthPermission(httpConfig, openApiConfig, apiFilteredIndexViewBuildItem)) {
// Only add the security if there are secured endpoints
OASFilter autoRolesAllowedFilter = getAutoRolesAllowedFilter(openApiConfig.securitySchemeName,
apiFilteredIndexViewBuildItem, openApiConfig);
if (autoRolesAllowedFilter != null) {
autoSecurityFilter = getAutoSecurityFilter(securityInformationBuildItems, openApiConfig);
}
autoSecurityFilter = getAutoSecurityFilter(securityInformationBuildItems, openApiConfig);
}

syntheticBeans.produce(SyntheticBeanBuildItem.configure(OASFilter.class).setRuntimeInit()
Expand Down Expand Up @@ -543,6 +544,20 @@ private OASFilter getAutoSecurityFilter(List<SecurityInformationBuildItem> secur
return null;
}

private boolean hasEnabledAuthPermission(HttpBuildTimeConfig httpConfig,
sberyozkin marked this conversation as resolved.
Show resolved Hide resolved
SmallRyeOpenApiConfig openApiConfig,
OpenApiFilteredIndexViewBuildItem apiFilteredIndexViewBuildItem) {
return httpConfig.auth.permissions.values()
.stream()
.map(mapping -> mapping.enabled)
// By default, if the permission set is defined, it is enabled.
.map(enabled -> enabled.orElse(Boolean.TRUE))
.filter(Boolean.TRUE::equals)
.findFirst()
.orElseGet(() -> getAutoRolesAllowedFilter(openApiConfig.securitySchemeName,
apiFilteredIndexViewBuildItem, openApiConfig) != null);
}

private OASFilter getAutoRolesAllowedFilter(String securitySchemeName,
OpenApiFilteredIndexViewBuildItem apiFilteredIndexViewBuildItem,
SmallRyeOpenApiConfig config) {
Expand Down
29 changes: 29 additions & 0 deletions ...ent/src/test/java/io/quarkus/smallrye/openapi/test/jaxrs/OIDCSecurityAutoAddTestTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package io.quarkus.smallrye.openapi.test.jaxrs;

import java.util.List;

import org.jboss.shrinkwrap.api.asset.StringAsset;
import org.junit.jupiter.api.extension.RegisterExtension;

import io.quarkus.builder.Version;
import io.quarkus.maven.dependency.Dependency;
import io.quarkus.test.QuarkusUnitTest;

class OIDCSecurityAutoAddTestTest extends OIDCSecurityTestBase {

@RegisterExtension
static QuarkusUnitTest runner = new QuarkusUnitTest()
.withApplicationRoot((jar) -> jar
.addClasses(OpenApiResource.class, ResourceBean.class)
.addAsResource(
new StringAsset(""
+ "quarkus.smallrye-openapi.security-scheme-name=OIDCCompanyAuthentication\n"
+ "quarkus.smallrye-openapi.security-scheme-description=OIDC Authentication\n"
+ "quarkus.http.auth.permission.\"oidc\".policy=authenticated\n"
+ "quarkus.http.auth.permission.\"oidc\".paths=/resource/*\n"
+ "quarkus.oidc.auth-server-url=http://localhost:8081/auth/realms/OpenAPIOIDC"),
"application.properties"))
.setForcedDependencies(List.of(
Dependency.of("io.quarkus", "quarkus-oidc", Version.getVersion())));

}
24 changes: 24 additions & 0 deletions ...deployment/src/test/java/io/quarkus/smallrye/openapi/test/jaxrs/OIDCSecurityTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package io.quarkus.smallrye.openapi.test.jaxrs;

import static org.hamcrest.Matchers.allOf;
import static org.hamcrest.Matchers.hasEntry;

import org.junit.jupiter.api.Test;

import io.restassured.RestAssured;

abstract class OIDCSecurityTestBase {

@Test
void testOIDCAuthentication() {
RestAssured.given().header("Accept", "application/json")
.when().get("/q/openapi")
.then().body("components.securitySchemes.OIDCCompanyAuthentication",
allOf(
hasEntry("type", "openIdConnect"),
hasEntry("description", "OIDC Authentication"),
hasEntry("openIdConnectUrl",
"http://localhost:8081/auth/realms/OpenAPIOIDC/.well-known/openid-configuration")));
}

}
18 changes: 2 additions & 16 deletions .../src/test/java/io/quarkus/smallrye/openapi/test/jaxrs/OIDCSecurityWithConfigTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,12 @@
package io.quarkus.smallrye.openapi.test.jaxrs;

import org.hamcrest.Matchers;
import org.jboss.shrinkwrap.api.asset.StringAsset;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;

import io.quarkus.test.QuarkusUnitTest;
import io.restassured.RestAssured;

public class OIDCSecurityWithConfigTestCase {
class OIDCSecurityWithConfigTestCase extends OIDCSecurityTestBase {

@RegisterExtension
static QuarkusUnitTest runner = new QuarkusUnitTest()
.withApplicationRoot((jar) -> jar
Expand All @@ -18,18 +16,6 @@ public class OIDCSecurityWithConfigTestCase {
+ "quarkus.smallrye-openapi.security-scheme-name=OIDCCompanyAuthentication\n"
+ "quarkus.smallrye-openapi.security-scheme-description=OIDC Authentication\n"
+ "quarkus.smallrye-openapi.oidc-open-id-connect-url=http://localhost:8081/auth/realms/OpenAPIOIDC/.well-known/openid-configuration"),

"application.properties"));

@Test
public void testOIDCAuthentication() {
RestAssured.given().header("Accept", "application/json")
.when().get("/q/openapi")
.then().body("components.securitySchemes.OIDCCompanyAuthentication", Matchers.hasEntry("type", "openIdConnect"))
.and()
.body("components.securitySchemes.OIDCCompanyAuthentication",
Matchers.hasEntry("description", "OIDC Authentication"))
.and().body("components.securitySchemes.OIDCCompanyAuthentication", Matchers.hasEntry("openIdConnectUrl",
"http://localhost:8081/auth/realms/OpenAPIOIDC/.well-known/openid-configuration"));
}
}
Loading