Differentiate cases when using default SecurityExpression and Authori…

[patternknife]

• When the Authorization header is missing, the process does not proceed to AuthenticationProvider, which is supposed to result in a 401 error rather than a 403 error due to insufficient permissions.

• As it does not seem to be appropriate to directly throw an AuthenticationException from ThrowingMethodAuthorizationDeniedHandler, that codes only allows developers to distinguish and handle such cases in @ExceptionHandler.

• This issue was discovered during testing by calling oauth2/token with spring-security-oauth2-authorization-server(1.2.3).

[pivotal-cla]

@patternknife Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-cla]

@patternknife Thank you for signing the Contributor License Agreement!

[jzheaux]

Hi, @patternknife, thanks for the PR. I don't understand, though, why this change is necessary. When an application throws an AccessDeniedException, the ExceptionTranslationFilter catches it and, if the user isn't logged in, translates it into a 401.

It sounds like something else may be going on. Can you produce a minimal GitHub sample that reproduces the issue you are seeing?

[patternknife]

Hi, @jzheaux , as the abstract class SecurityExpressionRoot, which implements SecurityExpressionOperations, only returns a boolean type:

1. Inspecting roles at the point returns an "AccessDeniedException."
2. No Authorization header throws an "AccessDeniedException" as well.

Consequently, I can't distinguish whether the header is omitted or if the user doesn't have the proper roles.

• curl --location 'http://localhost:8370/systemProfile' : 403 (Forbidden, No Authorization header, which is supposed to be 401, as this is NOT a permission issue.)
• curl --location 'http://localhost:8370/systemProfile'
  --header 'Authorization: wrongvalue' : 401 (Unauthorizaed, Wrong or Expired Token Value in Authorization header)

You can check the OAuth2 Resource Server source code at "https://github.com/patternknife/spring-security-oauth2-password-jpa-implementation/blob/main/src/main/java/io/github/patternknife/securityhelper/oauth2/api/config/security/server/ServerConfig.java."

(Here, KnifeOauth2AuthenticationException is a custom exception that I created, which implements OAuth2AuthenticationException in 'Spring Authorization Server'. )

package com.patternknife.securityhelper.oauth2.client.domain.common.api;

@RestController
public class BaseApi {

    @Value("${spring.profiles.active}")
    private String activeProfile;


	// AccessDeniedException
    @PreAuthorize("isAuthenticated()")
    @GetMapping("/systemProfile")
    public String checkAuthenticated () {
        return activeProfile;
    }

	// AccessDeniedException
    @PreAuthorize("@customAuthorityService.hasAnyUserRole()")
    @GetMapping("/systemProfile2")
    public String checkPermissions () {
        return activeProfile;
    }

}

package com.patternknife.securityhelper.oauth2.client.config.securityimpl.serivce.permission;


@Service
public class CustomAuthorityService {
    
    public boolean hasAnyUserRole() {
        Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
        if(authentication == null){
            throw new KnifeOauth2AuthenticationException();
        }
        if (authentication.getAuthorities() == null) {
            return false;
        }
        return authentication.getAuthorities().stream()
                .anyMatch(authority -> authority.getAuthority().endsWith("_USER"));
    }

}


```java

package io.github.patternknife.securityhelper.oauth2.api.config.security.server;


@Bean
@Order(2)
public SecurityFilterChain resourceServerSecurityFilterChain(HttpSecurity http, OAuth2AuthorizationServiceImpl authorizationService,
															 ConditionalDetailsService conditionalDetailsService,
															 ISecurityUserExceptionMessageService iSecurityUserExceptionMessageService,
															 AuthenticationEntryPoint iAuthenticationEntryPoint

) throws Exception {

	DefaultBearerTokenResolver resolver = new DefaultBearerTokenResolver();
	resolver.setAllowFormEncodedBodyParameter(true);

	http.csrf(AbstractHttpConfigurer::disable)
			.oauth2ResourceServer(oauth2 -> oauth2
					.bearerTokenResolver(resolver)
					.authenticationEntryPoint(iAuthenticationEntryPoint)
					.opaqueToken(opaqueToken -> opaqueToken.introspector(tokenIntrospector(authorizationService, conditionalDetailsService, iSecurityUserExceptionMessageService))));

	return http.build();
}

[jzheaux]

Consequently, I can't distinguish whether the header is omitted or if the user doesn't have the proper roles.

I agree that if you don't have an authorization header, you don't know if the principal has the appropriate roles. However, it's also true that if you don't have an authorization header, then you don't know who the principal is at all. This makes it a concern for authentication, not authorization.

IOW, you already have enough to differentiate between a 401 and a 403.

401 means either no credentials were provided or the credentials are bad
403 means the credentials are good, but the principal doesn't have the needed roles

This is how Spring Security works by default. Is there a reason you need to deviate from that?

[patternknife]

Well, if I inject a customized function into @PreAuthorize, I can distinguish the two cases.

public boolean isAuthenticated() throws OAuth2AuthenticationException {
	Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
	if(authentication == null || authentication instanceof AnonymousAuthenticationToken){
		throw new CustomOauth2AuthenticationException(); // extending OAuth2AuthenticationException extending AuthenticationException
	}
	return true;
    }

However, by default, is the case of the omitted Authorization header in a normal OAuth2 setup (not only in Spring Security) regarded as an AuthenticationException (401), not an AccessDeniedException (403)?

I think the expressions "isAuthenticated" and "isFullyAuthenticated" should throw a 401 by default, but ThrowingMethodAuthorizationDeniedHandler only throws a 403.

If someone uses the default (@PreAuthorize("isAuthenticated()")) and omits the header, they will face a 403.

The committed code should also face a 403, but with different messages, as the class was restricted to throwing only an AccessDeniedException.

I don't know, but I was just wondering if the default omitted Authorization header case needed to be handled as a 401 by default or be with a different message.

[jzheaux]

Thanks for the extra info, @patternknife.

Well, if I inject a customized function into @PreAuthorize, I can distinguish the two cases

This shouldn't be necessary as the framework does this for you. When @PreAuthorize throws an AccessDeniedException, it is caught by ExceptionTranslationFilter, which performs the differentiating logic you have described (it tests if authentication is null or is of type AnonymousAuthenticationToken) and then returns a 401 in that case.

Maybe I've made an assumption. Are you getting responses from your application that don't align with the following expectations:

401 means either no credentials were provided or the credentials are bad
403 means the credentials are good, but the principal doesn't have the needed roles

If so, please try this:

@Service
public class CustomAuthorityService {
    
    public boolean hasAnyUserRole() {
        Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
        if(authentication == null){
            throw new AccessDeniedException("access is denied");
        }
        if (authentication.getAuthorities() == null) {
            return false;
        }
        return authentication.getAuthorities().stream()
                .anyMatch(authority -> authority.getAuthority().endsWith("_USER"));
    }

}

AccessDeniedException/AuthorizationDeniedException will play nicer with Spring Security as it expects an authorization exception from its authorization APIs.

If that doesn't address your concern, please respond with a GitHub sample project that demonstrates what appears to be broken.

[patternknife]

"AuthenticationEntryPoint" here seems to catch only an AuthenticationException implemented exceptions, not an AccessDeniedException which occurs when there is NO header for protected APIs.

https://docs.spring.io/spring-security/reference/servlet/oauth2/resource-server/index.html

@RequiredArgsConstructor
public class DefaultAuthenticationEntryPoint implements AuthenticationEntryPoint {

    private final HandlerExceptionResolver resolver;

    @Override
    public void commence(HttpServletRequest request, HttpServletResponse response, AuthenticationException ex) throws IOException {
        resolver.resolveException(request, response, null, ex);
    }
}

@Bean
@Order(2)
public SecurityFilterChain resourceServerSecurityFilterChain(HttpSecurity http, OAuth2AuthorizationServiceImpl authorizationService,
															 ConditionalDetailsService conditionalDetailsService,
															 ISecurityUserExceptionMessageService iSecurityUserExceptionMessageService,
															 AuthenticationEntryPoint iAuthenticationEntryPoint
															 ) throws Exception {

	DefaultBearerTokenResolver resolver = new DefaultBearerTokenResolver();
	resolver.setAllowFormEncodedBodyParameter(true);

	http.csrf(AbstractHttpConfigurer::disable)
					.oauth2ResourceServer(oauth2 -> oauth2
					.bearerTokenResolver(resolver)
							.authenticationEntryPoint(iAuthenticationEntryPoint)
					.opaqueToken(opaqueToken -> opaqueToken.introspector(tokenIntrospector(authorizationService, conditionalDetailsService, iSecurityUserExceptionMessageService))));

	return http.build();
}

So the following throws an AuthenticationException.

public boolean hasAnyUserRole() {
	Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
	if(authentication == null || authentication instanceof AnonymousAuthenticationToken){
		throw new CustomOauth2AuthenticationException();
	}
	if (authentication.getAuthorities() == null) {
		return false;
	}
	return authentication.getAuthorities().stream()
			.anyMatch(authority -> authority.getAuthority().endsWith("_USER"));
}

Your code doesn't seem to be caught by the "AuthenticationEntryPoint" in the ExceptionTranslationFilter, as it throws an AccessDeniedException.

[jzheaux]

Gotcha. Yes, AuthenticationEntryPoint is for handling authentication failures and AccessDeniedHandler is for handling authorization failures.

Does that help? If not, please provide a sample and I'll see if I can better understand what changes are needed if any.

[patternknife]

I see. But this code is customized.

public boolean hasAnyUserRole() {
	Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
	if(authentication == null || authentication instanceof AnonymousAuthenticationToken){
		throw new CustomOauth2AuthenticationException();
	}
	if (authentication.getAuthorities() == null) {
		return false;
	}
	return authentication.getAuthorities().stream()
			.anyMatch(authority -> authority.getAuthority().endsWith("_USER"));
}

The following default class doesn't send the missing authorization header case to AuthenticationEntryPoint.

public abstract class SecurityExpressionRoot implements SecurityExpressionOperations {
    private final Supplier<Authentication> authentication;
    private AuthenticationTrustResolver trustResolver;
    private RoleHierarchy roleHierarchy;
    private Set<String> roles;
    private String defaultRolePrefix;
    public final boolean permitAll;
    public final boolean denyAll;
    private PermissionEvaluator permissionEvaluator;
    public final String read;
    public final String write;
    public final String create;
    public final String delete;
    public final String admin;

    public SecurityExpressionRoot(Authentication authentication) {
        this(() -> {
            return authentication;
        });
    }

    public SecurityExpressionRoot(Supplier<Authentication> authentication) {
        this.defaultRolePrefix = "ROLE_";
        this.permitAll = true;
        this.denyAll = false;
        this.read = "read";
        this.write = "write";
        this.create = "create";
        this.delete = "delete";
        this.admin = "administration";
        this.authentication = SingletonSupplier.of(() -> {
            Authentication value = (Authentication)authentication.get();
            Assert.notNull(value, "Authentication object cannot be null");
            return value;
        });
    }

    public final boolean hasAuthority(String authority) {
        return this.hasAnyAuthority(authority);
    }

    public final boolean hasAnyAuthority(String... authorities) {
        return this.hasAnyAuthorityName((String)null, authorities);
    }

    public final boolean hasRole(String role) {
        return this.hasAnyRole(role);
    }

    public final boolean hasAnyRole(String... roles) {
        return this.hasAnyAuthorityName(this.defaultRolePrefix, roles);
    }

    private boolean hasAnyAuthorityName(String prefix, String... roles) {
        Set<String> roleSet = this.getAuthoritySet();
        String[] var4 = roles;
        int var5 = roles.length;

        for(int var6 = 0; var6 < var5; ++var6) {
            String role = var4[var6];
            String defaultedRole = getRoleWithDefaultPrefix(prefix, role);
            if (roleSet.contains(defaultedRole)) {
                return true;
            }
        }

        return false;
    }

    public final Authentication getAuthentication() {
        return (Authentication)this.authentication.get();
    }

    public final boolean permitAll() {
        return true;
    }

    public final boolean denyAll() {
        return false;
    }

    public final boolean isAnonymous() {
        return this.trustResolver.isAnonymous(this.getAuthentication());
    }

    public final boolean isAuthenticated() {
        return this.trustResolver.isAuthenticated(this.getAuthentication());
    }
 ...

}

curl --location 'http://localhost:8370/systemProfile' : 403 (Forbidden, No Authorization header, which is supposed to be 401, as this is NOT a permission issue.)
curl --location 'http://localhost:8370/systemProfile'
--header 'Authorization: wrongvalue' : 401 (Unauthorizaed, Wrong or Expired Token Value in Authorization header)

...
    //  sent to the SecurityExpressionRoot
    @PreAuthorize("isAuthenticated()")
    @GetMapping("/systemProfile")
...

I think the case of a missing Authorization header should be handled as an AuthenticationException sent to an AuthenticationEntryPoint, same to the case with an incorrect Authorization value, but not at this point.

[jzheaux]

I think at this point I'm going to need a sample. Would you post something to GitHub that demonstrates the request/response behavior you would like and what you are needing to do to achieve that? I'd be happy to push a PR to that repo if a more idiomatic arrangement is possible.

[jzheaux]

@patternknife, I'm sorry that we weren't able to get aligned on this PR. At this point, I'm going to close it. If you'd like to continue collaborating, please post a question to StackOverflow and paste the link here.