Skip to content

Commit

Permalink
Fix 2.6 regression on group field and deprecate Jackson parser (fix #236
Browse files Browse the repository at this point in the history 
) (#280)
  • Loading branch information
@michael-doubez
michael-doubez committed Mar 20, 2024
1 parent 503b9a7 commit 62720cf
Show file tree
Hide file tree
Showing 5 changed files with 38 additions and 37 deletions.
4 changes: 2 additions & 2 deletions pom.xml
View file
Expand Up @@ -23,7 +23,7 @@

<scm>
<connection>scm:git:ssh://github.com/${gitHubRepo}.git</connection>
<developerConnection>scm:git:ssh://git@github.com/${gitHubRepo}.git</developerConnection>
<developerConnection>scm:git:git@github.com/${gitHubRepo}.git</developerConnection>
<url>https://github.com/${gitHubRepo}</url>
<tag>oic-auth-3.0</tag>
</scm>
Expand Down Expand Up @@ -58,7 +58,7 @@
</dependency>
<dependency>
<groupId>com.google.http-client</groupId>
<artifactId>google-http-client-jackson2</artifactId>
<artifactId>google-http-client-gson</artifactId>
<version>1.44.1</version>
</dependency>
<dependency>
Expand Down
41 changes: 26 additions & 15 deletions src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java
View file
Expand Up @@ -23,7 +23,6 @@
*/
package org.jenkinsci.plugins.oic;

import com.fasterxml.jackson.core.JsonParseException;
import com.google.api.client.auth.oauth2.AuthorizationCodeFlow;
import com.google.api.client.auth.oauth2.AuthorizationCodeTokenRequest;
import com.google.api.client.auth.oauth2.BearerToken;
Expand All @@ -41,13 +40,13 @@
import com.google.api.client.http.HttpTransport;
import com.google.api.client.http.javanet.NetHttpTransport;
import com.google.api.client.json.GenericJson;
import com.google.api.client.json.JsonFactory;
import com.google.api.client.json.JsonObjectParser;
import com.google.api.client.json.jackson2.JacksonFactory;
import com.google.api.client.json.gson.GsonFactory;
import com.google.api.client.json.webtoken.JsonWebSignature;
import com.google.api.client.util.ArrayMap;
import com.google.api.client.util.Data;
import com.google.common.base.Strings;
import com.google.gson.JsonParseException;
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import hudson.Extension;
import hudson.Util;
Expand Down Expand Up @@ -123,12 +122,10 @@ public class OicSecurityRealm extends SecurityRealm {
private static final Logger LOGGER = Logger.getLogger(OicSecurityRealm.class.getName());
public static enum TokenAuthMethod { client_secret_basic, client_secret_post };

private static final JsonFactory JSON_FACTORY = new JacksonFactory();
private static final String ID_TOKEN_REQUEST_ATTRIBUTE = "oic-id-token";
private static final String STATE_REQUEST_ATTRIBUTE = "oic-state";
private static final String NO_SECRET = "none";


private final String clientId;
private final Secret clientSecret;
private String wellKnownOpenIDConfigurationUrl = null;
Expand All @@ -142,8 +139,8 @@ public static enum TokenAuthMethod { client_secret_basic, client_secret_post };
private String fullNameFieldName = null;
private String emailFieldName = null;
private String groupsFieldName = null;
private String simpleGroupsFieldName = null;
private String nestedGroupFieldName = null;
private transient String simpleGroupsFieldName = null;
private transient String nestedGroupFieldName = null;
private String scopes = null;
private final boolean disableSslVerification;
private boolean logoutFromOpenidProvider = true;
Expand Down Expand Up @@ -191,6 +188,9 @@ public static enum TokenAuthMethod { client_secret_basic, client_secret_post };
private transient String endSessionUrl;

private transient HttpTransport httpTransport;

/** Random generator needed for robust random wait
*/
private static final Random RANDOM = new Random();

/**
Expand Down Expand Up @@ -227,11 +227,11 @@ public OicSecurityRealm(String clientId, String clientSecret, String wellKnownOp
this.wellKnownOpenIDConfigurationUrl = null; // Remove the autoconfig URL
}

this.tokenFieldToCheckKey = Util.fixEmpty(tokenFieldToCheckKey);
this.tokenFieldToCheckValue = Util.fixEmpty(tokenFieldToCheckValue);
this.userNameField = Util.fixEmpty(userNameField) == null ? "sub" : userNameField;
this.fullNameFieldName = Util.fixEmpty(fullNameFieldName);
this.emailFieldName = Util.fixEmpty(emailFieldName);
this.setTokenFieldToCheckKey(Util.fixEmpty(tokenFieldToCheckKey));
this.setTokenFieldToCheckValue(Util.fixEmpty(tokenFieldToCheckValue));
this.setUserNameField(Util.fixEmpty(userNameField) == null ? "sub" : userNameField);

Check warning on line 232 in src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line 

Line 232 is only partially covered, one branch is missing
this.setFullNameFieldName(Util.fixEmpty(fullNameFieldName));
this.setEmailFieldName(Util.fixEmpty(emailFieldName));
this.setGroupsFieldName(Util.fixEmpty(groupsFieldName));
this.logoutFromOpenidProvider = Util.fixNull(logoutFromOpenidProvider, Boolean.TRUE);
this.postLogoutRedirectUrl = postLogoutRedirectUrl;
Expand Down Expand Up @@ -276,6 +276,17 @@ protected Object readResolve() {
LOGGER.log(Level.SEVERE, "Can't set endSessionEndpoint from old value", e);
}
}

// backward compatibility with wrong groupsFieldName split
if(Strings.isNullOrEmpty(this.groupsFieldName) && !Strings.isNullOrEmpty(this.simpleGroupsFieldName)) {

Check warning on line 281 in src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line 

Line 281 is only partially covered, 2 branches are missing
String originalGroupFieldName = this.simpleGroupsFieldName;
if(!Strings.isNullOrEmpty(this.nestedGroupFieldName)) {
originalGroupFieldName += "[]." + this.nestedGroupFieldName;
}
this.setGroupsFieldName(originalGroupFieldName);
} else {

Check warning on line 287 in src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines 

Lines 282-287 are not covered by tests
this.setGroupsFieldName(this.groupsFieldName);
}
return this;
}

Expand Down Expand Up @@ -435,7 +446,7 @@ private void loadWellKnownOpenIDConfigurationUrl() {
.createRequestFactory().buildGetRequest(new GenericUrl(url));

com.google.api.client.http.HttpResponse response = request.execute();
WellKnownOpenIDConfigurationResponse config = OicSecurityRealm.JSON_FACTORY
WellKnownOpenIDConfigurationResponse config = GsonFactory.getDefaultInstance()
.fromInputStream(response.getContent(), Charset.defaultCharset(),
WellKnownOpenIDConfigurationResponse.class);

Expand Down Expand Up @@ -716,7 +727,7 @@ protected AuthorizationCodeFlow buildAuthorizationCodeFlow() {
AuthorizationCodeFlow.Builder builder = new AuthorizationCodeFlow.Builder(
tokenAccessMethod,
httpTransport,
JSON_FACTORY,
GsonFactory.getDefaultInstance(),
new GenericUrl(tokenServerUrl),
authInterceptor,
clientId,
Expand Down Expand Up @@ -1220,7 +1231,7 @@ public FormValidation doCheckWellKnownOpenIDConfigurationUrl(@QueryParameter Str

// Try to parse the response. If it's not valid, a JsonParseException will be thrown indicating
// that it's not a valid JSON describing an OpenID Connect endpoint
WellKnownOpenIDConfigurationResponse config = OicSecurityRealm.JSON_FACTORY
WellKnownOpenIDConfigurationResponse config = GsonFactory.getDefaultInstance()

Check warning on line 1234 in src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line 

Line 1234 is not covered by tests
.fromInputStream(response.getContent(), Charset.defaultCharset(),
WellKnownOpenIDConfigurationResponse.class);
if(config.getAuthorizationEndpoint() == null || config.getTokenEndpoint() == null) {
Expand Down
12 changes: 4 additions & 8 deletions src/test/java/org/jenkinsci/plugins/oic/OicTokenResponseTest.java
View file
@@ -1,7 +1,6 @@
package org.jenkinsci.plugins.oic;

import com.google.api.client.json.JsonFactory;
import com.google.api.client.json.jackson2.JacksonFactory;
import com.google.api.client.json.gson.GsonFactory;
import java.io.IOException;
import org.junit.Test;

Expand Down Expand Up @@ -31,8 +30,7 @@ public class OicTokenResponseTest {

@Test
public void parseLongLiteral() throws IOException {
JsonFactory jsonFactory = new JacksonFactory();
OicTokenResponse response = jsonFactory.fromString(JSON_WITH_LONG_LITERAL, OicTokenResponse.class);
OicTokenResponse response = GsonFactory.getDefaultInstance().fromString(JSON_WITH_LONG_LITERAL, OicTokenResponse.class);
assertEquals("2YotnFZFEjr1zCsicMWpAA", response.getAccessToken());
assertEquals("example", response.getTokenType());
assertEquals(3600L, response.getExpiresInSeconds().longValue());
Expand All @@ -42,8 +40,7 @@ public void parseLongLiteral() throws IOException {

@Test
public void parseStringWithLong() throws IOException {
JsonFactory jsonFactory = new JacksonFactory();
OicTokenResponse response = jsonFactory.fromString(JSON_WITH_LONG_AS_STRING, OicTokenResponse.class);
OicTokenResponse response = GsonFactory.getDefaultInstance().fromString(JSON_WITH_LONG_AS_STRING, OicTokenResponse.class);
assertEquals("2YotnFZFEjr1zCsicMWpAA", response.getAccessToken());
assertEquals("example", response.getTokenType());
assertEquals(3600L, response.getExpiresInSeconds().longValue());
Expand Down Expand Up @@ -81,8 +78,7 @@ public void testSetters() throws IOException {

@Test
public void parseAbsent() throws IOException {
JsonFactory jsonFactory = new JacksonFactory();
OicTokenResponse response = jsonFactory.fromString(JSON_WITH_ABSENT, OicTokenResponse.class);
OicTokenResponse response = GsonFactory.getDefaultInstance().fromString(JSON_WITH_ABSENT, OicTokenResponse.class);
assertEquals("2YotnFZFEjr1zCsicMWpAA", response.getAccessToken());
assertEquals("example", response.getTokenType());
assertEquals(null, response.getExpiresInSeconds());
Expand Down
9 changes: 3 additions & 6 deletions src/test/java/org/jenkinsci/plugins/oic/PluginTest.java
View file
Expand Up @@ -3,8 +3,7 @@
import com.github.tomakehurst.wiremock.core.WireMockConfiguration;
import com.github.tomakehurst.wiremock.junit.WireMockRule;
import com.google.api.client.auth.openidconnect.IdToken;
import com.google.api.client.json.JsonFactory;
import com.google.api.client.json.jackson2.JacksonFactory;
import com.google.api.client.json.gson.GsonFactory;
import com.google.api.client.json.webtoken.JsonWebSignature;
import com.google.api.client.json.webtoken.JsonWebToken;
import com.google.api.client.util.ArrayMap;
Expand Down Expand Up @@ -59,8 +58,6 @@
*/
@Url("https://jenkins.io/blog/2018/01/13/jep-200/")
public class PluginTest {
private static final JsonFactory JSON_FACTORY = new JacksonFactory();

private static final String TEST_USER_USERNAME = "testUser";
private static final String TEST_USER_EMAIL_ADDRESS = "test@jenkins.oic";
private static final String TEST_USER_FULL_NAME = "Oic Test User";
Expand Down Expand Up @@ -1164,7 +1161,7 @@ private String createIdToken(PrivateKey privateKey, Map<String, Object> keyValue
payload.set(keyValue.getKey(), keyValue.getValue());
}

return JsonWebSignature.signUsingRsaSha256(privateKey, JSON_FACTORY, header, payload);
return JsonWebSignature.signUsingRsaSha256(privateKey, GsonFactory.getDefaultInstance(), header, payload);
}

private String createUserInfoJWT(PrivateKey privateKey, String userInfo) throws Exception {
Expand All @@ -1177,7 +1174,7 @@ private String createUserInfoJWT(PrivateKey privateKey, String userInfo) throws
payload.set(keyValue.getKey(), keyValue.getValue().getAsString());
}

return JsonWebSignature.signUsingRsaSha256(privateKey, JSON_FACTORY, header, payload);
return JsonWebSignature.signUsingRsaSha256(privateKey, GsonFactory.getDefaultInstance(), header, payload);
}

/**
Expand Down
9 changes: 3 additions & 6 deletions src/test/java/org/jenkinsci/plugins/oic/WellKnownOpenIDConfigurationResponseTest.java
View file
@@ -1,7 +1,6 @@
package org.jenkinsci.plugins.oic;

import com.google.api.client.json.JsonFactory;
import com.google.api.client.json.jackson2.JacksonFactory;
import com.google.api.client.json.gson.GsonFactory;
import java.io.IOException;
import org.junit.Test;

Expand All @@ -13,8 +12,6 @@

public class WellKnownOpenIDConfigurationResponseTest {

private final JsonFactory jsonFactory = new JacksonFactory();

private static final String JSON_FROM_GOOGLE = "{"
+ " \"issuer\": \"https://accounts.google.com\","
+ " \"authorization_endpoint\": \"https://accounts.google.com/o/oauth2/v2/auth\","
Expand Down Expand Up @@ -69,7 +66,7 @@ public class WellKnownOpenIDConfigurationResponseTest {

@Test
public void parseExplicitKeys() throws IOException {
WellKnownOpenIDConfigurationResponse response = jsonFactory.fromString(JSON_FROM_GOOGLE, WellKnownOpenIDConfigurationResponse.class);
WellKnownOpenIDConfigurationResponse response = GsonFactory.getDefaultInstance().fromString(JSON_FROM_GOOGLE, WellKnownOpenIDConfigurationResponse.class);

assertThat(response.getAuthorizationEndpoint(), is("https://accounts.google.com/o/oauth2/v2/auth"));
assertThat(response.getTokenEndpoint(), is("https://www.googleapis.com/oauth2/v4/token"));
Expand All @@ -81,7 +78,7 @@ public void parseExplicitKeys() throws IOException {

@Test
public void parseWellKnownKeys() throws IOException {
WellKnownOpenIDConfigurationResponse response = jsonFactory.fromString(JSON_FROM_GOOGLE, WellKnownOpenIDConfigurationResponse.class);
WellKnownOpenIDConfigurationResponse response = GsonFactory.getDefaultInstance().fromString(JSON_FROM_GOOGLE, WellKnownOpenIDConfigurationResponse.class);
assertThat(response.getKnownKeys().keySet(), containsInAnyOrder(
"authorization_endpoint",
"token_endpoint",
Expand Down

0 comments on commit 62720cf

Please sign in to comment.