Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
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
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.jenkinsci.plugins.oic;

import edu.umd.cs.findbugs.annotations.NonNull;
import hudson.model.AbstractDescribableImpl;
import hudson.model.Descriptor;
import hudson.util.FormValidation;
Expand All @@ -10,7 +11,10 @@
public abstract class AbstractKeyValueDescribable<T extends AbstractKeyValueDescribable<T>>
extends AbstractDescribableImpl<T> {

@NonNull
private final String key;

@NonNull
private final String value;

/**
Expand Down Expand Up @@ -52,10 +56,12 @@ public AbstractKeyValueDescribable(String key, String value, boolean allowBlankV
this.value = value == null ? "" : value.trim();
}

@NonNull
public String getKey() {
return key;
}

@NonNull
public String getValue() {
return value;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.jenkinsci.plugins.oic;

import edu.umd.cs.findbugs.annotations.NonNull;
import hudson.model.Descriptor;
import java.net.URLEncoder;
import java.nio.charset.StandardCharsets;
Expand All @@ -25,6 +26,7 @@ public String getURLEncodedKey() {
/**
* Return {@link #getValue()} encoded with {@code application/x-www-form-urlencoded} in {@code UTF-8}
*/
@NonNull
public String getURLEncodedValue() {
return URLEncoder.encode(getValue(), StandardCharsets.UTF_8);
}
Expand Down

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Actually moved to EscapeHatch

Comment thread
Vlatombe marked this conversation as resolved.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

inlined into EscapHatch

This file was deleted.

38 changes: 38 additions & 0 deletions src/main/java/org/jenkinsci/plugins/oic/OicProperty.java
Comment thread
Vlatombe marked this conversation as resolved.
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
package org.jenkinsci.plugins.oic;

Check warning on line 1 in src/main/java/org/jenkinsci/plugins/oic/OicProperty.java

View check run for this annotation

ci.jenkins.io / Java Compiler

checkstyle:check

ERROR: (misc) NewlineAtEndOfFile: Expected line ending for file is LF(\n), but CRLF(\r\n) is detected.

import edu.umd.cs.findbugs.annotations.NonNull;
import hudson.model.AbstractDescribableImpl;
import java.util.List;
import java.util.Optional;
import org.springframework.security.core.Authentication;

/**
* Represents a property that can be configured for OIDC authentication.
*/
public abstract class OicProperty extends AbstractDescribableImpl<OicProperty> {
/**
* @return a new execution for this property, holding any required state.
*/
@NonNull
public OicPropertyExecution newExecution(@NonNull OicServerConfiguration serverConfiguration) {
return new EmptyExecution();
}

private record EmptyExecution() implements OicPropertyExecution {}

/**
* Allows a property to authenticate the user.
* @see org.jenkinsci.plugins.oic.properties.EscapeHatch
*/
public Optional<Authentication> authenticate(Authentication authentication) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm confused as to how this can work generically in practice with multiple authenticators?

Would this work with say bearer tokens?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

For bearer token, I think you would need to add a way to plug a new filter into the authentication chain (doesn't exist currently, I only added the bare minumum for EscapeHatch to work), to convert the http headers to a BearerTokenAuthentication (to be written). And then an implementation of this method does what https://github.com/jenkinsci/oic-auth-plugin/pull/632/files#diff-9c2c6436b143fde652866ea92ee9bec48671b094dbb23ae4805621c5d40ed59bR1514-R1587 currently does I guess ?

return Optional.empty();
}

/**
* Allows a property to contribute additional query parameters to the logout request.
*/
@NonNull
public List<LogoutQueryParameter> contributeLogoutQueryParameters() {
return List.of();
}
}
28 changes: 28 additions & 0 deletions src/main/java/org/jenkinsci/plugins/oic/OicPropertyDescriptor.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package org.jenkinsci.plugins.oic;

Check warning on line 1 in src/main/java/org/jenkinsci/plugins/oic/OicPropertyDescriptor.java

View check run for this annotation

ci.jenkins.io / Java Compiler

checkstyle:check

ERROR: (misc) NewlineAtEndOfFile: Expected line ending for file is LF(\n), but CRLF(\r\n) is detected.

import edu.umd.cs.findbugs.annotations.NonNull;
import hudson.ExtensionList;
import hudson.ExtensionPoint;
import hudson.model.Descriptor;
import org.pac4j.oidc.config.OidcConfiguration;

public abstract class OicPropertyDescriptor extends Descriptor<OicProperty> implements ExtensionPoint {
public static ExtensionList<OicPropertyDescriptor> all() {
return ExtensionList.lookup(OicPropertyDescriptor.class);
}

/**
* Allows the property to restrict its applicability depending on the context (for example, FIPS)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When I first saw this I was confused, perhaps some more docs on why this is here and when to use it vs not registering your Descriptor would help? e.g.

Suggested change
* Allows the property to restrict its applicability depending on the context (for example, FIPS)
* Allows the property to restrict its applicability depending on the context (for example, FIPS).
* This is different to not registering the {@code Descriptor} in Jenkins so that default (fallback) configuration can be used.
* @see #getFallbackConfiguration

*/
public boolean isApplicable() {
return true;

Check warning on line 18 in src/main/java/org/jenkinsci/plugins/oic/OicPropertyDescriptor.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line

Line 18 is not covered by tests
}

/**
* This method gets called if the property is not configured explicitly. For example, providing a default value.
*/
public void getFallbackConfiguration(
@NonNull OicServerConfiguration serverConfiguration, @NonNull OidcConfiguration configuration) {
// no-op
}
Comment on lines +24 to +

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

seems strange to handle this differently to an actual property. Was there a reason that an OicPropertyExecution was not used here instead?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

An OicPropertyExecution is created only if an instance is added explicitly in the UI. For properties with default values (pkce, token expiration clock skew, disable nonce), I wanted something managed in the same class. An alternative would be to loop on all descriptors to create "default" property executions ? It did seem to add more boilerplate for not obvious benefit.

}
23 changes: 23 additions & 0 deletions src/main/java/org/jenkinsci/plugins/oic/OicPropertyExecution.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package org.jenkinsci.plugins.oic;

Check warning on line 1 in src/main/java/org/jenkinsci/plugins/oic/OicPropertyExecution.java

View check run for this annotation

ci.jenkins.io / Java Compiler

checkstyle:check

ERROR: (misc) NewlineAtEndOfFile: Expected line ending for file is LF(\n), but CRLF(\r\n) is detected.

import edu.umd.cs.findbugs.annotations.NonNull;
import org.pac4j.oidc.client.OidcClient;
import org.pac4j.oidc.config.OidcConfiguration;

public interface OicPropertyExecution {
/**
* Customize the OIDC configuration.
*
* @param configuration the OIDC configuration to customize
*/
default void customizeConfiguration(@NonNull OidcConfiguration configuration) {}

/**
* Customize the OIDC client.
* <br/>
* Always called after {@link #customizeConfiguration(OidcConfiguration)}.
*
* @param client the OIDC client to customize
*/
default void customizeClient(@NonNull OidcClient client) {}
}
Loading
Loading