Skip to content

Remove internal Optional usage in favor of null checks #7295

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.

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

Merged
Show file tree
Hide file tree
Changes from all 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
Expand Up @@ -71,13 +71,19 @@ static class OAuth2ClientWebMvcSecurityConfiguration implements WebMvcConfigurer
@Override
public void addArgumentResolvers(List<HandlerMethodArgumentResolver> argumentResolvers) {
if (this.clientRegistrationRepository != null && this.authorizedClientRepository != null) {
OAuth2AuthorizedClientProvider authorizedClientProvider =
OAuth2AuthorizedClientProviderBuilder authorizedClientProviderBuilder =
OAuth2AuthorizedClientProviderBuilder.builder()
.authorizationCode()
.refreshToken()
.clientCredentials(configurer ->
Optional.ofNullable(this.accessTokenResponseClient).ifPresent(configurer::accessTokenResponseClient))
.build();
.refreshToken();

if (this.accessTokenResponseClient != null) {
authorizedClientProviderBuilder.clientCredentials(configurer ->
configurer.accessTokenResponseClient(this.accessTokenResponseClient));
} else {
authorizedClientProviderBuilder.clientCredentials();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This code changes the existing functionality. If accessTokenResponseClient is null, clientCredentials should still be enabled. Please add an else statement containing authorizedClientProviderBuilder.clientCredentials() to ensure clientCredentials is enabled by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 3fef167

OAuth2AuthorizedClientProvider authorizedClientProvider = authorizedClientProviderBuilder.build();

DefaultOAuth2AuthorizedClientManager authorizedClientManager = new DefaultOAuth2AuthorizedClientManager(
this.clientRegistrationRepository, this.authorizedClientRepository);
authorizedClientManager.setAuthorizedClientProvider(authorizedClientProvider);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.UUID;
import java.util.function.Function;
import java.util.function.Supplier;
Expand Down Expand Up @@ -2665,10 +2664,10 @@ public ServerHttpSecurity disable() {
}

protected void configure(ServerHttpSecurity http) {
Optional.ofNullable(this.csrfTokenRepository).ifPresent(serverCsrfTokenRepository -> {
this.filter.setCsrfTokenRepository(serverCsrfTokenRepository);
http.logout().addLogoutHandler(new CsrfServerLogoutHandler(serverCsrfTokenRepository));
});
if (this.csrfTokenRepository != null) {
this.filter.setCsrfTokenRepository(this.csrfTokenRepository);
http.logout().addLogoutHandler(new CsrfServerLogoutHandler(this.csrfTokenRepository));
}
http.addFilterAt(this.filter, SecurityWebFiltersOrder.CSRF);
}

Expand Down Expand Up @@ -3607,19 +3606,21 @@ public ServerHttpSecurity disable() {
return and();
}

private Optional<ServerLogoutHandler> createLogoutHandler() {
private ServerLogoutHandler createLogoutHandler() {
if (this.logoutHandlers.isEmpty()) {
return Optional.empty();
}
else if (this.logoutHandlers.size() == 1) {
return Optional.of(this.logoutHandlers.get(0));
return null;
} else if (this.logoutHandlers.size() == 1) {
return this.logoutHandlers.get(0);
} else {
return new DelegatingServerLogoutHandler(this.logoutHandlers);
}

return Optional.of(new DelegatingServerLogoutHandler(this.logoutHandlers));
}

protected void configure(ServerHttpSecurity http) {
createLogoutHandler().ifPresent(this.logoutWebFilter::setLogoutHandler);
ServerLogoutHandler logoutHandler = createLogoutHandler();
if (logoutHandler != null) {
this.logoutWebFilter.setLogoutHandler(logoutHandler);
}
http.addFilterAt(this.logoutWebFilter, SecurityWebFiltersOrder.LOGOUT);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@

import java.net.URI;
import java.nio.charset.StandardCharsets;
import java.util.Optional;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

Expand Down Expand Up @@ -52,25 +51,34 @@ public OidcClientInitiatedLogoutSuccessHandler(ClientRegistrationRepository clie
@Override
protected String determineTargetUrl(HttpServletRequest request,
HttpServletResponse response, Authentication authentication) {
String targetUrl = null;
URI endSessionEndpoint;
if (authentication instanceof OAuth2AuthenticationToken && authentication.getPrincipal() instanceof OidcUser) {
endSessionEndpoint = this.endSessionEndpoint((OAuth2AuthenticationToken) authentication);
if (endSessionEndpoint != null) {
targetUrl = endpointUri(endSessionEndpoint, authentication);
}
}
if (targetUrl == null) {
targetUrl = super.determineTargetUrl(request, response);
}

return Optional.of(authentication)
.filter(OAuth2AuthenticationToken.class::isInstance)
.filter(token -> authentication.getPrincipal() instanceof OidcUser)
.map(OAuth2AuthenticationToken.class::cast)
.flatMap(this::endSessionEndpoint)
.map(endSessionEndpoint -> endpointUri(endSessionEndpoint, authentication))
.orElseGet(() -> super.determineTargetUrl(request, response));
return targetUrl;
}

private Optional<URI> endSessionEndpoint(OAuth2AuthenticationToken token) {
private URI endSessionEndpoint(OAuth2AuthenticationToken token) {
String registrationId = token.getAuthorizedClientRegistrationId();
return Optional.of(
this.clientRegistrationRepository.findByRegistrationId(registrationId))
.map(ClientRegistration::getProviderDetails)
.map(ClientRegistration.ProviderDetails::getConfigurationMetadata)
.map(configurationMetadata -> configurationMetadata.get("end_session_endpoint"))
.map(Object::toString)
.map(URI::create);
ClientRegistration clientRegistration = this.clientRegistrationRepository.findByRegistrationId(registrationId);
Copy link
Contributor

Choose a reason for hiding this comment

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

clientRegistration may be null. Please add a null check here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 6a4425c


URI result = null;
if (clientRegistration != null) {
Object endSessionEndpoint = clientRegistration.getProviderDetails().getConfigurationMetadata().get("end_session_endpoint");
if (endSessionEndpoint != null) {
result = URI.create(endSessionEndpoint.toString());
}
}

return result;
}

private String endpointUri(URI endSessionEndpoint, Authentication authentication) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;

import com.nimbusds.oauth2.sdk.GrantType;
import com.nimbusds.oauth2.sdk.ParseException;
Expand Down Expand Up @@ -141,8 +140,11 @@ public static ClientRegistration.Builder fromIssuerLocation(String issuer) {
Map<String, Object> configuration = getConfiguration(issuer, oidc(uri), oidcRfc8414(uri), oauth(uri));
AuthorizationServerMetadata metadata = parse(configuration, AuthorizationServerMetadata::parse);
ClientRegistration.Builder builder = withProviderConfiguration(metadata, issuer);
return Optional.ofNullable((String) configuration.get("userinfo_endpoint"))
.map(builder::userInfoUri).orElse(builder);
String userinfoEndpoint = (String) configuration.get("userinfo_endpoint");
if (userinfoEndpoint != null) {
builder.userInfoUri(userinfoEndpoint);
}
return builder;
}

private static URI oidc(URI issuer) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@
package org.springframework.security.oauth2.server.resource.authentication;

import java.time.Instant;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.stream.Collectors;

import org.springframework.http.HttpStatus;
import org.springframework.security.authentication.AbstractAuthenticationToken;
Expand Down Expand Up @@ -128,11 +128,12 @@ private AbstractAuthenticationToken convert(String token, Map<String, Object> cl
}

private Collection<GrantedAuthority> extractAuthorities(Map<String, Object> claims) {
Collection<String> scopes = (Collection<String>) claims.get(SCOPE);
return Optional.ofNullable(scopes).orElse(Collections.emptyList())
.stream()
.map(authority -> new SimpleGrantedAuthority("SCOPE_" + authority))
.collect(Collectors.toList());
Collection<String> scopes = (Collection<String>) claims.getOrDefault(SCOPE, Collections.emptyList());
List<GrantedAuthority> authorities = new ArrayList<>();
for (String scope : scopes) {
authorities.add(new SimpleGrantedAuthority("SCOPE_" + scope));
}
return authorities;
}

private static BearerTokenError invalidToken(String message) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@
package org.springframework.security.oauth2.server.resource.authentication;

import java.time.Instant;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.stream.Collectors;

import org.springframework.security.oauth2.core.OAuth2TokenAttributes;
import reactor.core.publisher.Mono;
Expand Down Expand Up @@ -108,11 +108,12 @@ private Mono<OAuth2IntrospectionAuthenticationToken> authenticate(String token)
}

private Collection<GrantedAuthority> extractAuthorities(Map<String, Object> claims) {
Collection<String> scopes = (Collection<String>) claims.get(SCOPE);
return Optional.ofNullable(scopes).orElse(Collections.emptyList())
.stream()
.map(authority -> new SimpleGrantedAuthority("SCOPE_" + authority))
.collect(Collectors.toList());
Collection<String> scopes = (Collection<String>) claims.getOrDefault(SCOPE, Collections.emptyList());
List<GrantedAuthority> authorities = new ArrayList<>();
for (String scope : scopes) {
authorities.add(new SimpleGrantedAuthority("SCOPE_" + scope));
}
return authorities;
}

private static BearerTokenError invalidToken(String message) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.stream.Collectors;

import com.nimbusds.oauth2.sdk.TokenIntrospectionResponse;
Expand Down Expand Up @@ -124,16 +123,22 @@ private MultiValueMap<String, String> requestBody(String token) {
*/
@Override
public Map<String, Object> introspect(String token) {
TokenIntrospectionSuccessResponse response = Optional.of(token)
.map(this.requestEntityConverter::convert)
.map(this::makeRequest)
.map(this::adaptToNimbusResponse)
.map(this::parseNimbusResponse)
.map(this::castToNimbusSuccess)
// relying solely on the authorization server to validate this token (not checking 'exp', for example)
.filter(TokenIntrospectionSuccessResponse::isActive)
.orElseThrow(() -> new OAuth2IntrospectionException("Provided token [" + token + "] isn't active"));
return convertClaimsSet(response);
RequestEntity<?> requestEntity = this.requestEntityConverter.convert(token);
if (requestEntity == null) {
throw new OAuth2IntrospectionException("Provided token [" + token + "] isn't active");
}

ResponseEntity<String> responseEntity = makeRequest(requestEntity);
HTTPResponse httpResponse = adaptToNimbusResponse(responseEntity);
TokenIntrospectionResponse introspectionResponse = parseNimbusResponse(httpResponse);
TokenIntrospectionSuccessResponse introspectionSuccessResponse = castToNimbusSuccess(introspectionResponse);

// relying solely on the authorization server to validate this token (not checking 'exp', for example)
if (!introspectionSuccessResponse.isActive()) {
throw new OAuth2IntrospectionException("Provided token [" + token + "] isn't active");
}

return convertClaimsSet(introspectionSuccessResponse);
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2018 the original author or authors.
* Copyright 2002-2019 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -16,7 +16,6 @@

package org.springframework.security.web.server.csrf;

import java.util.Optional;
import java.util.UUID;

import org.springframework.http.HttpCookie;
Expand Down Expand Up @@ -69,14 +68,17 @@ public Mono<CsrfToken> generateToken(ServerWebExchange exchange) {
@Override
public Mono<Void> saveToken(ServerWebExchange exchange, CsrfToken token) {
return Mono.fromRunnable(() -> {
Optional<String> tokenValue = Optional.ofNullable(token).map(CsrfToken::getToken);
String tokenValue = token != null ? token.getToken() : "";
int maxAge = !tokenValue.isEmpty() ? -1 : 0;
String path = this.cookiePath != null ? this.cookiePath : getRequestContext(exchange.getRequest());
boolean secure = exchange.getRequest().getSslInfo() != null;

ResponseCookie cookie = ResponseCookie.from(this.cookieName, tokenValue.orElse(""))
ResponseCookie cookie = ResponseCookie.from(this.cookieName, tokenValue)
.domain(this.cookieDomain)
.httpOnly(this.cookieHttpOnly)
.maxAge(tokenValue.map(val -> -1).orElse(0))
.path(Optional.ofNullable(this.cookiePath).orElseGet(() -> getRequestContext(exchange.getRequest())))
.secure(Optional.ofNullable(exchange.getRequest().getSslInfo()).map(sslInfo -> true).orElse(false))
.maxAge(maxAge)
.path(path)
.secure(secure)
.build();

exchange.getResponse().addCookie(cookie);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2018 the original author or authors.
* Copyright 2002-2019 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -17,7 +17,6 @@
package org.springframework.security.web.server.transport;

import java.net.URI;
import java.util.Optional;

import reactor.core.publisher.Mono;

Expand Down Expand Up @@ -102,10 +101,11 @@ private URI createRedirectUri(ServerWebExchange exchange) {
UriComponentsBuilder.fromUri(exchange.getRequest().getURI());

if (port > 0) {
Optional.ofNullable(this.portMapper.lookupHttpsPort(port))
.map(builder::port)
.orElseThrow(() -> new IllegalStateException(
"HTTP Port '" + port + "' does not have a corresponding HTTPS Port"));
Integer httpsPort = this.portMapper.lookupHttpsPort(port);
if (httpsPort == null) {
throw new IllegalStateException("HTTP Port '" + port + "' does not have a corresponding HTTPS Port");
}
builder.port(httpsPort);
}

return builder.scheme("https").build().toUri();
Expand Down