Make FixedAuthoritiesExtractor more liberal in what it accepts from a user info endpoint

[ddewaele]

I think there's an issue in the FixedAuthoritiesExtractor class that allows us to do role based authorizations on OAuth2Authentication objects.

It seems FixedAuthoritiesExtractor should pass on a comma-separated string of roles from the Oauth2Authentication object, but instead it passes on this string : {authority=ROLE_ADMIN},{authority=ROLE_USER} (= the result of an ArrayList toString). See Update 4 for more info. If you think this is valid I'd be happy to log an issue somewhere.

After spending some quality time in my debugger, I noticed that at some point an accessDecisionManager needs to decide if I can acces the url based on the following expression :

#oauth2.throwOnError(hasRole('ROLE_USER'))

So for some reason this evaluates to false

        return ExpressionUtils.evaluateAsBoolean(weca.getAuthorizeExpression(), ctx) ? ACCESS_GRANTED
                : ACCESS_DENIED;

with my user

org.springframework.security.oauth2.provider.OAuth2Authentication@e6c61a39: Principal: user; Credentials: [PROTECTED]; Authenticated: true; Details: remoteAddress=127.0.0.1, tokenType=bearertokenValue=<TOKEN>; Granted Authorities: {authority=ROLE_USER}

I also noticed during debugging that the WebExpressionResolver seems to match the roles based on the string, but the Oauth2Authentication wraps it in {authority=ROLENAME}

Whereas a simple basic authentication flow uses this

So either something is wrong in the FixedAuthoritiesExtractor, or in the way the Principal is constructed that contains the authorities

{
   "details":{
      "remoteAddress":"127.0.0.1",
      "sessionId":null,
      "tokenValue":"bdb90e1a-233b-4c47-b899-2af1ab60ef23",
      "tokenType":"bearer",
      "decodedDetails":{
         "remoteAddress":"127.0.0.1",
         "sessionId":null,
         "tokenValue":"bdb90e1a-233b-4c47-b899-2af1ab60ef23",
         "tokenType":"Bearer",
         "decodedDetails":{
            "remoteAddress":"127.0.0.1",
            "sessionId":null,
            "tokenValue":"bdb90e1a-233b-4c47-b899-2af1ab60ef23",
            "tokenType":"bearer",
            "decodedDetails":null
         }
      }
   },
   "authorities":[
      {
         "authority":"ROLE_ADMIN"
      },
      {
         "authority":"ROLE_USER"
      }
   ],
   "authenticated":true,
   "userAuthentication":{
      "details":{
         "remoteAddress":"0:0:0:0:0:0:0:1",
         "sessionId":null
      },
      "authorities":[
         {
            "authority":"ROLE_ADMIN"
         },
         {
            "authority":"ROLE_USER"
         }
      ],
      "authenticated":true,
      "principal":{
         "password":null,
         "username":"admin",
         "authorities":[
            {
               "authority":"ROLE_ADMIN"
            },
            {
               "authority":"ROLE_USER"
            }
         ],
         "accountNonExpired":true,
         "accountNonLocked":true,
         "credentialsNonExpired":true,
         "enabled":true
      },
      "credentials":null,
      "name":"admin"
   },
   "credentials":"",
   "principal":{
      "password":null,
      "username":"admin",
      "authorities":[
         {
            "authority":"ROLE_ADMIN"
         },
         {
            "authority":"ROLE_USER"
         }
      ],
      "accountNonExpired":true,
      "accountNonLocked":true,
      "credentialsNonExpired":true,
      "enabled":true
   },
   "clientOnly":false,
   "oauth2Request":{
      "clientId":"acme",
      "scope":[
         "openid"
      ],
      "requestParameters":{
         "response_type":"code",
         "redirect_uri":"http://localhost:8888/login",
         "state":"VyGyQ0",
         "code":"XbAHuC",
         "grant_type":"authorization_code",
         "client_id":"acme"
      },
      "resourceIds":[

      ],
      "authorities":[
         {
            "authority":"ROLE_USER"
         }
      ],
      "approved":true,
      "refresh":false,
      "redirectUri":"http://localhost:8888/login",
      "responseTypes":[
         "code"
      ],
      "extensions":{

      },
      "grantType":"authorization_code",
      "refreshTokenRequest":null
   },
   "name":"admin"
}

[caelcs]

It is happening to me as well, issue #5499. It is related to the serialisation of the json to a map.

[philwebb]

/cc @dsyer @rwinch

[dsyer]

Duplicates #5499 (so one of them can be closed). It's something that users can easily fix by providing a user info endpoint (which they have to do anyway) which sends back the user authorities as an array or collection of String. I believe a small change to the authorities extractor that we provide by default will also make it a bit easier to do (in the sense that the user info endpoint can be implemented with less care, if the consumer is more liberal in what it accepts).

[caelcs]

Thank you for your suggestion, I've already implemented the user endpoint like this:

@RequestMapping("/user")
public Principal user(Principal user) {
return user;
}

I will try to apply for the time been the fix that you suggested but I guess that it will require to define a custom Jackson Serializer.

But what I would like to understand if there is any reason why the extractor is expecting an array of String, while UserDetails interface has a attribute '''Collection authorities''' and this will be serialise to json like this:

{  
   "username":"randomUser",
   ....
   "authorities":[  
      {  
         "authority":"ROLE_CLIENT"
      }
   ],
 ....
}

As you said making the extractor more aware about this scenario will fix the issue.

[caelcs]

To fix this issue I have implemented my custom authority extractor. All you have to do is declare it as a Bean.

import org.springframework.boot.autoconfigure.security.oauth2.resource.AuthoritiesExtractor;
import org.springframework.security.core.GrantedAuthority;
import org.springframework.security.core.authority.AuthorityUtils;

import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.Map;

import static org.springframework.util.CollectionUtils.isEmpty;

public class CustomAuthorityExtractor implements AuthoritiesExtractor {

    private static final String AUTHORITIES = "authorities";
    private static final String AUTHORITY = "authority";

    @Override
    public List<GrantedAuthority> extractAuthorities(Map<String, Object> map) {
        List<String> authorities = new ArrayList<>();
        authorities.add("ROLE_USER");

        if (map.containsKey(AUTHORITIES)) {
            authorities = asAuthorities(map.get(AUTHORITIES));
        }
        return AuthorityUtils.createAuthorityList(authorities.toArray(new String[authorities.size()]));
    }

    private List<String> asAuthorities(Object object) {
        List<String> result = new ArrayList<>();
        if (object instanceof Collection) {
            final Collection sourceCol = (Collection) object;
            if (!isEmpty(sourceCol)) {
                for (Object o : sourceCol) {
                    final Map<String, String> authorityMap = (Map) o;
                    result.add(authorityMap.get(AUTHORITY));
                }
            }
        }
        return result;
    }
}

[dsyer]

@caelwinner that's possibly useful information, but for the sake of clarity for others reading the discussion: its' not really a workaround and there isn't really an issue. The user info endpoint is not one that is provided by Spring Boot, or any other Spring project, so the user has to provide one, or use the one that is provided by the external OAuth2 provider. When you provide that endpoint it makes sense to provide information that is useful to you in the consumer apps (and that can mean anything you like, including, but not limited to user authorities). In my opinion, the "workaround" is misguided because it puts the onus to implement it on the client apps (of which there might be many) instead of the user info provider (of which there can only be one). Having said that, I concede that the extension point is there, and also the default behaviour of a client app could be different, and more likely to produce a successful outcome for a not-very-thoughtful user info producer.