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

Jump to bottom

Miscellaneous refactors #2608

Merged
merged 2 commits into from
Nov 20, 2023
Merged

Miscellaneous refactors #2608

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bf19628
refactor: simplify assertions in tests
peterhaochen47 Nov 17, 2023
c8ab93f
refactor: simplify code
peterhaochen47 Nov 17, 2023
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
82 changes: 4 additions & 78 deletions ...g/cloudfoundry/identity/uaa/provider/AbstractExternalOAuthIdentityProviderDefinition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,14 @@
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
import com.fasterxml.jackson.annotation.JsonInclude;
import lombok.Getter;

import java.lang.reflect.ParameterizedType;
import java.net.URL;
import java.util.List;
import java.util.Objects;

@Getter
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! I'm a big fan of Lombok.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not really a fan of converting existing class to use Lombok. To me, it is most useful for new code. Regardless, if you wanted to change it to use Lombok, why stop at Getter? Why not also use @Setter?

Copy link
Member Author

@peterhaochen47 peterhaochen47 Nov 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

For the Setters, our existing implementations of the setters in this class is not "standard", e.g.:

    public T setTokenKeyUrl(URL tokenKeyUrl) {
        this.tokenKeyUrl = tokenKeyUrl;
        return (T) this;
    }

So I can't use the lombok setter without more code changes. It can be done though, just leaving it aside for now.

As of the motivation of this refactor - it's mainly sort of a response to this prior discussion with Bruce. With this PR, I'm making the point that the "isSomeBoolean" getter pattern is now a common pattern, since it's what lombok would generate, though it might not be necessarily grammatically correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see.

@JsonIgnoreProperties(ignoreUnknown = true)
public abstract class AbstractExternalOAuthIdentityProviderDefinition<T extends AbstractExternalOAuthIdentityProviderDefinition> extends ExternalIdentityProviderDefinition {
public enum OAuthGroupMappingMode {
Expand All @@ -39,165 +41,97 @@ public enum OAuthGroupMappingMode {
private boolean clientAuthInBody = false;
private boolean skipSslValidation;
private String relyingPartyId;
@JsonInclude(JsonInclude.Include.NON_NULL)
private String relyingPartySecret;
private List<String> scopes;
private String issuer;
private String responseType = "code";
private String userPropagationParameter;
@JsonInclude(JsonInclude.Include.NON_NULL)
private OAuthGroupMappingMode groupMappingMode;
private boolean pkce = true;
private boolean performRpInitiatedLogout = true;

public URL getAuthUrl() {
return authUrl;
}

public T setAuthUrl(URL authUrl) {
this.authUrl = authUrl;
return (T) this;
}

public URL getTokenUrl() {
return tokenUrl;
}

public T setTokenUrl(URL tokenUrl) {
this.tokenUrl = tokenUrl;
return (T) this;
}

public URL getTokenKeyUrl() {
return tokenKeyUrl;
}

public T setTokenKeyUrl(URL tokenKeyUrl) {
this.tokenKeyUrl = tokenKeyUrl;
return (T) this;
}

public String getTokenKey() {
return tokenKey;
}

public T setTokenKey(String tokenKey) {
this.tokenKey = tokenKey;
return (T) this;
}

public URL getUserInfoUrl() {
return userInfoUrl;
}

public T setUserInfoUrl(URL userInfoUrl) {
this.userInfoUrl = userInfoUrl;
return (T) this;
}

public URL getLogoutUrl() {
return logoutUrl;
}

public T setLogoutUrl(URL logoutUrl) {
this.logoutUrl = logoutUrl;
return (T) this;
}

public String getLinkText() {
return linkText;
}

public T setLinkText(String linkText) {
this.linkText = linkText;
return (T) this;
}

public boolean isClientAuthInBody() {
return clientAuthInBody;
}

public T setClientAuthInBody(boolean clientAuthInBody) {
this.clientAuthInBody = clientAuthInBody;
return (T) this;
}

public boolean isShowLinkText() {
return showLinkText;
}

public T setShowLinkText(boolean showLinkText) {
this.showLinkText = showLinkText;
return (T) this;
}

public String getRelyingPartyId() {
return relyingPartyId;
}

public T setRelyingPartyId(String relyingPartyId) {
this.relyingPartyId = relyingPartyId;
return (T) this;
}

@JsonInclude(JsonInclude.Include.NON_NULL)
public String getRelyingPartySecret() {
return relyingPartySecret;
}

public T setRelyingPartySecret(String relyingPartySecret) {
this.relyingPartySecret = relyingPartySecret;
return (T) this;
}

public boolean isSkipSslValidation() {
return skipSslValidation;
}

public T setSkipSslValidation(boolean skipSslValidation) {
this.skipSslValidation = skipSslValidation;
return (T) this;
}

public List<String> getScopes() {
return scopes;
}

public T setScopes(List<String> scopes) {
this.scopes = scopes;
return (T) this;
}

public String getIssuer() {
return issuer;
}

public T setIssuer(String issuer) {
this.issuer = issuer;
return (T) this;
}

public String getResponseType() {
return responseType;
}

public T setResponseType(String responseType) {
this.responseType = responseType;
return (T) this;
}

public String getUserPropagationParameter() {
return userPropagationParameter;
}

public T setUserPropagationParameter(String userPropagationParameter) {
this.userPropagationParameter = userPropagationParameter;
return (T) this;
}

@JsonInclude(JsonInclude.Include.NON_NULL) // prevent json data for default
public OAuthGroupMappingMode getGroupMappingMode() {
return groupMappingMode;
}

public T setGroupMappingMode(OAuthGroupMappingMode externalGroupMappingMode) {
if (externalGroupMappingMode == OAuthGroupMappingMode.EXPLICITLY_MAPPED) {
this.groupMappingMode = null;
Expand All @@ -211,14 +145,6 @@ public void setPkce(final boolean pkce) {
this.pkce = pkce;
}

public boolean isPkce() {
return this.pkce;
}

public boolean isPerformRpInitiatedLogout() {
return performRpInitiatedLogout;
}

public void setPerformRpInitiatedLogout(boolean performRpInitiatedLogout) {
this.performRpInitiatedLogout = performRpInitiatedLogout;
}
Expand Down
15 changes: 9 additions & 6 deletions ...est/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthLogoutHandlerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@
import java.util.Set;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.times;
Expand Down Expand Up @@ -113,15 +116,15 @@ void getLogoutUrl() throws OidcMetadataFetchingException {
@Test
void getNewFetchedLogoutUrl() throws OidcMetadataFetchingException {
oAuthIdentityProviderDefinition.setLogoutUrl(null);
assertEquals(null, oAuthLogoutHandler.getLogoutUrl(oAuthIdentityProviderDefinition));
assertNull(oAuthLogoutHandler.getLogoutUrl(oAuthIdentityProviderDefinition));
verify(oidcMetadataFetcher, times(1)).fetchMetadataAndUpdateDefinition(oAuthIdentityProviderDefinition);
}

@Test
void getNewInvalidFetchedLogoutUrl() throws OidcMetadataFetchingException {
oAuthIdentityProviderDefinition.setLogoutUrl(null);
doThrow(new OidcMetadataFetchingException("")).when(oidcMetadataFetcher).fetchMetadataAndUpdateDefinition(oAuthIdentityProviderDefinition);
assertEquals(null, oAuthLogoutHandler.getLogoutUrl(oAuthIdentityProviderDefinition));
assertNull(oAuthLogoutHandler.getLogoutUrl(oAuthIdentityProviderDefinition));
verify(oidcMetadataFetcher, times(1)).fetchMetadataAndUpdateDefinition(oAuthIdentityProviderDefinition);
}

Expand All @@ -132,17 +135,17 @@ void getOAuthProviderForAuthentication() {

@Test
void getNullOAuthProviderForAuthentication() {
assertEquals(null, oAuthLogoutHandler.getOAuthProviderForAuthentication(null));
assertNull(oAuthLogoutHandler.getOAuthProviderForAuthentication(null));
}

@Test
void getPerformRpInitiatedLogout() {
oAuthIdentityProviderDefinition.setPerformRpInitiatedLogout(true);
assertEquals(true, oAuthLogoutHandler.getPerformRpInitiatedLogout(oAuthIdentityProviderDefinition));
assertTrue(oAuthLogoutHandler.getPerformRpInitiatedLogout(oAuthIdentityProviderDefinition));

oAuthIdentityProviderDefinition.setPerformRpInitiatedLogout(false);
assertEquals(false, oAuthLogoutHandler.getPerformRpInitiatedLogout(oAuthIdentityProviderDefinition));
assertFalse(oAuthLogoutHandler.getPerformRpInitiatedLogout(oAuthIdentityProviderDefinition));

assertEquals(false, oAuthLogoutHandler.getPerformRpInitiatedLogout(null));
assertFalse(oAuthLogoutHandler.getPerformRpInitiatedLogout(null));
}
}