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

Move BasicAuthenticationMechanism to synthetic bean #45442

Merged
merged 1 commit into from
Jan 9, 2025
+26 −19
Merged

Move BasicAuthenticationMechanism to synthetic bean #45442

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
30 changes: 16 additions & 14 deletions ...http/deployment/src/main/java/io/quarkus/vertx/http/deployment/HttpSecurityProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@

import org.jboss.jandex.AnnotationInstance;
import org.jboss.jandex.AnnotationTarget;
import org.jboss.jandex.AnnotationTransformation;
import org.jboss.jandex.ClassInfo;
import org.jboss.jandex.DotName;
import org.jboss.jandex.IndexView;
Expand All @@ -37,9 +36,7 @@
import org.jboss.jandex.TypeVariable;
import org.objectweb.asm.Opcodes;

import io.quarkus.arc.DefaultBean;
import io.quarkus.arc.deployment.AdditionalBeanBuildItem;
import io.quarkus.arc.deployment.AnnotationsTransformerBuildItem;
import io.quarkus.arc.deployment.BeanContainerBuildItem;
import io.quarkus.arc.deployment.BeanRegistrationPhaseBuildItem;
import io.quarkus.arc.deployment.GeneratedBeanBuildItem;
Expand Down Expand Up @@ -187,23 +184,28 @@ void detectBasicAuthImplicitlyRequired(HttpBuildTimeConfig buildTimeConfig,
}

@BuildStep(onlyIf = IsApplicationBasicAuthRequired.class)
AdditionalBeanBuildItem initBasicAuth(HttpBuildTimeConfig buildTimeConfig,
BuildProducer<AnnotationsTransformerBuildItem> annotationsTransformerProducer,
@Record(ExecutionTime.RUNTIME_INIT)
SyntheticBeanBuildItem initBasicAuth(HttpSecurityRecorder recorder,
HttpConfiguration runtimeConfig,
HttpBuildTimeConfig buildTimeConfig,
BuildProducer<SecurityInformationBuildItem> securityInformationProducer) {

if (makeBasicAuthMechDefaultBean(buildTimeConfig)) {
//if not explicitly enabled we make this a default bean, so it is the fallback if nothing else is defined
annotationsTransformerProducer.produce(new AnnotationsTransformerBuildItem(AnnotationTransformation
.forClasses()
.whenClass(BASIC_AUTH_MECH_NAME)
.transform(ctx -> ctx.add(DefaultBean.class))));
}

if (buildTimeConfig.auth.basic.isPresent() && buildTimeConfig.auth.basic.get()) {
securityInformationProducer.produce(SecurityInformationBuildItem.BASIC());
}

return AdditionalBeanBuildItem.builder().setUnremovable().addBeanClass(BasicAuthenticationMechanism.class).build();
SyntheticBeanBuildItem.ExtendedBeanConfigurator configurator = SyntheticBeanBuildItem
Copy link
Member

@michalvavrik michalvavrik Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think default bean scope is @Dependent and you dropped @Singleton so you are making it dependent. It could be actually good idea, but first, I'd need to inspect our code and probably make same change for all the auth mechanisms. Is this intentional? if not, let's keep it a singleton for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I picked up on that and have the change done locally

Copy link
Member

Choose a reason for hiding this comment

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

BTW I glanced at current impl. and I actually think all the HTTP auth mechanisms would be in 99 % cases better off as dependent (sometimes users can inject them if they provide alternatives, but how many users does it?), I think we just use them once. I'll re-check later this week if I didn't miss something and open PR when Sergey is back.

Copy link
Member

@michalvavrik michalvavrik Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Provided that it is worse to have beans we don't need at CDI container, because these java class instances will live anyway. And with the dependent, we won't be proxying beans either. I am not sure if it is worth it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍🏽

.configure(BasicAuthenticationMechanism.class)
.types(io.quarkus.vertx.http.runtime.security.HttpAuthenticationMechanism.class)
.scope(Singleton.class)
.supplier(recorder.basicAuthenticationMechanismBean(runtimeConfig, buildTimeConfig.auth.form.enabled))
.setRuntimeInit()
.unremovable();
if (makeBasicAuthMechDefaultBean(buildTimeConfig)) {
configurator.defaultBean();
}

return configurator.done();
}

private static boolean makeBasicAuthMechDefaultBean(HttpBuildTimeConfig buildTimeConfig) {
Expand Down
5 changes: 0 additions & 5 deletions ...me/src/main/java/io/quarkus/vertx/http/runtime/security/BasicAuthenticationMechanism.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,6 @@
import java.util.Set;
import java.util.regex.Pattern;

import jakarta.inject.Inject;
import jakarta.inject.Singleton;

import org.jboss.logging.Logger;

import io.netty.handler.codec.http.HttpHeaderNames;
Expand All @@ -51,7 +48,6 @@
* The authentication handler responsible for BASIC authentication as described by RFC2617
*
*/
@Singleton
public class BasicAuthenticationMechanism implements HttpAuthenticationMechanism {

private static final Logger log = Logger.getLogger(BasicAuthenticationMechanism.class);
Expand All @@ -75,7 +71,6 @@ public class BasicAuthenticationMechanism implements HttpAuthenticationMechanism
private final Charset charset;
private final Map<Pattern, Charset> userAgentCharsets;

@Inject
BasicAuthenticationMechanism(HttpConfiguration runtimeConfig, HttpBuildTimeConfig buildTimeConfig) {
this(runtimeConfig.auth.realm.orElse(null), buildTimeConfig.auth.form.enabled);
}
Expand Down
10 changes: 10 additions & 0 deletions ...tp/runtime/src/main/java/io/quarkus/vertx/http/runtime/security/HttpSecurityRecorder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -514,4 +514,14 @@ private static Set<String> parseRoles(String value) {
return Set.copyOf(roles);
}

public Supplier<BasicAuthenticationMechanism> basicAuthenticationMechanismBean(HttpConfiguration runtimeConfig,
boolean formAuthEnabled) {
return new Supplier<>() {
@Override
public BasicAuthenticationMechanism get() {
return new BasicAuthenticationMechanism(runtimeConfig.auth.realm.orElse(null), formAuthEnabled);
}
};
}

}
Loading