mitreid-connect / OpenID-Connect-Java-Spring-Server

An OpenID Connect reference implementation in Java on the Spring platform.
Other
1.48k stars 765 forks source link

SubjectIssuerGrantedAuthority cannot be cast to SimpleGrantedAuthority #1040

Open mobilars opened 8 years ago

mobilars commented 8 years ago

I've got a modified MitreID server, only modified in such a way that the authentication is set up to point to an external openid connect server. That part seems to work, since I'm able to log in to the web admin. But when trying to get an access_token through an authorization flow, the last piece failes (I get the consent page, but then I get an exception). There are some log files in another issuehttps://github.com/mitreid-connect/OpenID-Connect-Java-Spring-Server/issues/1037, which I closed since the question had been answered.

The exception I'm getting is as follows:

[EL Fine]: sql: 2016-03-15 21:19:10.934--ClientSession(1887131608)--Connection(976278464)--Thread(Thread[qtp1095849663-26,5,main])--INSERT INTO saved_user_auth (authenticated, name, source_class) VALUES (?, ?, ?) bind => [true, {sub=b3074c04-3a10-4709-b3d6-c35ca156cc61, iss=http://removed-server-url:80/opensso/oauth2/norge.no}, org.mitre.openid.connect.model.OIDCAuthenticationToken] [EL Finest]: query: 2016-03-15 21:19:10.936--ClientSession(1887131608)--Thread(Thread[qtp1095849663-26,5,main])--Execute query ValueReadQuery(name="SEQ_GEN_IDENTITY" sql="select lastval()") [EL Fine]: sql: 2016-03-15 21:19:10.936--ClientSession(1887131608)--Connection(976278464)--Thread(Thread[qtp1095849663-26,5,main])--select lastval() [EL Finest]: sequencing: 2016-03-15 21:19:10.937--UnitOfWork(1577044644)--Thread(Thread[qtp1095849663-26,5,main])--assign sequence to the object (11 -> org.mitre.oauth2.model.SavedUserAuthentication@680d0b6a) [EL Warning]: 2016-03-15 21:19:10.942--UnitOfWork(1577044644)--Thread(Thread[qtp1095849663-26,5,main])--javax.persistence.PersistenceException: An exception occurred while calling convertToDatabaseColumn on converter class org.mitre.oauth2.model.convert.SimpleGrantedAuthorityStringConverter with value OIDC_b3074c04-3a10-4709-b3d6-c35ca156cc61_http://removed-server-url:80/opensso/oauth2/norge.no at org.eclipse.persistence.mappings.converters.ConverterClass.convertObjectValueToDataValue(ConverterClass.java:138) at org.eclipse.persistence.mappings.DirectCollectionMapping.postInsert(DirectCollectionMapping.java:2216) at org.eclipse.persistence.descriptors.DescriptorQueryManager.postInsert(DescriptorQueryManager.java:999) at org.eclipse.persistence.internal.queries.DatabaseQueryMechanism.insertObjectForWrite(DatabaseQueryMechanism.java:498) at org.eclipse.persistence.queries.InsertObjectQuery.executeCommit(InsertObjectQuery.java:80) later followed by: Caused by: java.lang.ClassCastException: org.mitre.openid.connect.client.SubjectIssuerGrantedAuthority cannot be cast to org.springframework.security.core.authority.SimpleGrantedAuthority at org.mitre.oauth2.model.convert.SimpleGrantedAuthorityStringConverter.convertToDatabaseColumn(SimpleGrantedAuthorityStringConverter.java:29) at org.eclipse.persistence.mappings.converters.ConverterClass.convertObjectValueToDataValue(ConverterClass.java:136) ... 127 more

Any help would be appreciated. If you need more code, files etc, let me know.

jricher commented 8 years ago

This is caused by the storage subsystem which expects granted authorities to be SimpleGrantedAuthority, and not the special class authority given by the OIDC client. We store them as strings in the DB to avoid messy Java serialization issues that were present in 1.1 and before. If there's a better way to manage the translation class, we'll gladly take a pull request for it.

mobilars commented 8 years ago

OK. If modify SimpleGrantedAuthorityStringConverter as below, my exception goes away, and it seems to work (but I don't understand the effect of my change, so I wouldn't trust myself to add a patch). Other things may break.

/*******************************************************************************
 * Copyright 2016 The MITRE Corporation
 *   and the MIT Internet Trust Consortium
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *   http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 *******************************************************************************/

package org.mitre.oauth2.model.convert;

import javax.persistence.AttributeConverter;
import javax.persistence.Converter;

import org.springframework.security.core.GrantedAuthority;
import org.springframework.security.core.authority.SimpleGrantedAuthority;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

/**
 * @author jricher
 *
 */
@Converter
public class SimpleGrantedAuthorityStringConverter implements AttributeConverter<GrantedAuthority, String> {
//public class SimpleGrantedAuthorityStringConverter implements AttributeConverter<SimpleGrantedAuthority, String> {

    private static final Logger logger = LoggerFactory.getLogger(JsonElementStringConverter.class);

    // SubjectIssuerGrantedAuthority cannot be cast to org.springframework.security.core.authority.SimpleGrantedAuthority
    @Override
    //public String convertToDatabaseColumn(SimpleGrantedAuthority attribute) {
    public String convertToDatabaseColumn(GrantedAuthority attribute) {

        if (attribute != null) {
            logger.info("convertToDatabaseColumn:"+attribute.toString());
            return attribute.getAuthority();
        } else {
            logger.info("convertToDatabaseColumn: attribute=null");
            return null;
        }
    }

    @Override
    //public SimpleGrantedAuthority convertToEntityAttribute(String dbData) {
    public GrantedAuthority convertToEntityAttribute(String dbData) {
        if (dbData != null) {
            logger.info("convertToEntityAttribute:"+dbData);
            return new SimpleGrantedAuthority(dbData);
        } else {
            logger.info("convertToEntityAttribute: dbData == null");
            return null;
        }
    }

}
jricher commented 8 years ago

Please submit that change as a pull request and we can review it. It's been brought up before: #1021

Only problem is that you lose the original class on the way out (everything is converted to SimpleGrantedAuthority), but there may be no fixing that in a reasonable fashion without changing the data model.

mobilars commented 8 years ago

I'll do some more testing first.

I have one issue which I wonder if is a bug or a feature. My access_tokens don't contain scopes. On another server type the scopes were embedded in the JWT access token. Are they supposed to be here, and / or is there a way to include them?

My authorize-url:

http://access.ehelselab.com/authorize?client_id=client&redirect_uri=http://apps.ehelselab.com/oauthtest/&response_type=token&scope=patient/*.*&nonce=6130755535249629&state=14578213732110.3184424809526636

My access token:

eyJhbGciOiJSUzI1NiIsImtpZCI6InJzYTEifQ.eyJleHAiOjE0NTgxNTYzNjEsImlzcyI6Imh0dHA6XC9cL2FjY2Vzcy5laGVsc2VsYWIuY29tXC8iLCJhdWQiOiJjbGllbnQiLCJqdGkiOiJlY2FiZGQ4NC02ZmExLTQwYmUtYWViNS1mYmJmMjljNmUzMzgiLCJpYXQiOjE0NTgxNTI3NjF9.n-_3zRcKCuGkygYVOfT2Yn-0If3C4pSiLa7OKnnqfPOi6gvkAFvoCR6pctVMM1978IFcT_YFmHcj9_LdU7d2tGDRt8GQ1hf7pgxtJB8-s_qhIYnxBgu2uqOVN7yzL7SlPteqAoNmEroj-65eiAAkeWijddfp7GvHwbQ3n1nDJZE

{"exp":1458156361,"iss":"http://access.ehelselab.com/","aud":"client","jti":"ecabdd84-6fa1-40be-aeb5-fbbf29c6e338","iat":1458152761}

jricher commented 8 years ago

We do not include the scopes in the access token, on purpose. If you want to do so, you'll need to override the TokenEnhancer class and inject your own implementation that will add the scopes to the token.

ericktedeschi commented 8 years ago

Hi Richer, The reason why scopes aren't included in access_token is related to security threats? I'm working in a PoC with MitreID Connect where a specific kind of client (internal APIs) can use an cached key of AS to do an "offline introspection" (verify the JWT signature), and would be nice to have scopes inside the token. p.s.: I'm aware the risks around this.

mobilars commented 8 years ago

Do I understand correctly that the standard introspection endpoint needs the client-secret? What's the 'right' way of our API-server checking the scopes that an access-token it gets from a client? Up to now we've verified the signature of the access token offline and extracted the scopes. (with an access-token from another oauth-server we no longer want to use).

I tested overriding that class and that works nicely thanks. So I guess I can do this (but as opposed to ericktedeschi, I'm not aware of the risks :-/ )

jricher commented 8 years ago

The reason was to not expose too much information in the token itself and to keep the token size smaller. It's a tradeoff and we found having the issuer/expiration/signature combination was enough for our systems.