cqframework / cqf-ruler

FHIR Clinical Reasoning Module Server
Apache License 2.0
64 stars 49 forks source link

CDS Service Request fhirAuthorization broken in 0.5.1 and above #678

Open aeyates opened 1 year ago

aeyates commented 1 year ago

In attempting to upgrade from 0.5.0, I started receiving a 401 Unauthorized Exception when executing a plan definition. After debugging, I found that the fhirAuthorization clause specified by CDS Hooks (https://cds-hooks.hl7.org/ballots/2018May/specification/1.0/#fhir-resource-access) is no longer forming a proper request to the EHR. The hook request used to have the following format:

"fhirAuthorization": {
     "access_token": "blah",
     "token_type": "Bearer",
     "expires_in": 300,
     "scope": "patient/*.read",
     "subject": "cds-service4"
 }

Through a lot of trial and error, I eventually found I could modify the hook request like this and it would properly authenticate:

"fhirAuthorization": {
        "access_token": "Bearer blah",
        "token_type": "Authorization",
        "expires_in": 300,
        "scope": "patient/*.read",
        "subject": "cds-service4"
    }

This does not match the 1.0 or current draft specification for CDS Hooks. If CQF Ruler requires a different syntax, can you add documentation somewhere? This is unexpected.

aeyates commented 1 year ago

I tracked down the original code in 0.5.0 that processed the Authorization header properly. It was in the (now removed) class EvaluationContext, and here's the relevant method:

public IGenericClient getHookFhirClient() {
        IGenericClient client = this.fhirContext.newRestfulGenericClient(this.hook.getRequest().getFhirServerUrl());
        if (this.hook.getRequest().getFhirAuthorization() != null
                && hook.getRequest().getFhirAuthorization().getTokenType().equals("Bearer")) {
            BearerTokenAuthInterceptor authInterceptor = new BearerTokenAuthInterceptor(
                    hook.getRequest().getFhirAuthorization().getAccessToken());
            client.registerInterceptor(authInterceptor);

            // TODO: account for the expires_in, scope and subject properties within
            // workflow
        }

        LoggingInterceptor loggingInterceptor = new LoggingInterceptor();
        loggingInterceptor.setLogRequestSummary(true);
        loggingInterceptor.setLogRequestHeaders(true);
        loggingInterceptor.setLogRequestBody(true);
        loggingInterceptor.setLogResponseSummary(true);
        loggingInterceptor.setLogResponseHeaders(true);
        loggingInterceptor.setLogResponseBody(true);
        client.registerInterceptor(loggingInterceptor);

        return client;
    }

This was replaced by the method resolveRemoteClient in the CQLEvaluationHelper. But that method no longer has the full hook information, only the EndpointInfo:

public IGenericClient resolveRemoteClient(EndpointInfo endpoint) {
        IGenericClient remoteClient = fhirContext.newRestfulGenericClient(endpoint.getAddress());
        endpoint.
        if (endpoint.getHeaders() != null) {
            AdditionalRequestHeadersInterceptor headerInterceptor = new AdditionalRequestHeadersInterceptor();
            for (HeaderInfo header : getHeaderNameValuePairs(endpoint.getHeaders())) {
                headerInterceptor.addHeaderValue(header.getName(), header.getValue());
            }
            remoteClient.registerInterceptor(headerInterceptor);
        }
        return remoteClient;
    }

One simple way I found to fix this is to add a check for the header name "Bearer" in this method and do the correct conversion. I also found it useful to reintroduce the LoggingInterceptor so the requests were transparent.

    public IGenericClient resolveRemoteClient(EndpointInfo endpoint) {
        IGenericClient remoteClient = fhirContext.newRestfulGenericClient(endpoint.getAddress());
        if (endpoint.getHeaders() != null) {
            AdditionalRequestHeadersInterceptor headerInterceptor = new AdditionalRequestHeadersInterceptor();
            for (HeaderInfo header : getHeaderNameValuePairs(endpoint.getHeaders())) {
                if (header.getName().equals("Bearer")) {
                    Clients.registerBearerTokenAuth(remoteClient, header.getValue());
                } else {
                    headerInterceptor.addHeaderValue(header.getName(), header.getValue());
                }
            }
            remoteClient.registerInterceptor(headerInterceptor);
        }
        LoggingInterceptor loggingInterceptor = new LoggingInterceptor();
        loggingInterceptor.setLogRequestSummary(true);
        loggingInterceptor.setLogRequestHeaders(true);
        loggingInterceptor.setLogRequestBody(true);
        loggingInterceptor.setLogResponseSummary(true);
        loggingInterceptor.setLogResponseHeaders(true);
        loggingInterceptor.setLogResponseBody(true);
        remoteClient.registerInterceptor(loggingInterceptor);
        return remoteClient;
    }
aeyates commented 5 months ago

For anyone encountering this, I have not verified personally but a colleague has reported this issue was fixed in 0.13.0.