Skip to content
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

CDS Service Request fhirAuthorization broken in 0.5.1 and above #678

Open
aeyates opened this issue Dec 12, 2022 · 2 comments
Open

CDS Service Request fhirAuthorization broken in 0.5.1 and above #678

aeyates opened this issue Dec 12, 2022 · 2 comments

Comments

@aeyates
Copy link

aeyates commented Dec 12, 2022

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
Copy link
Author

aeyates commented Dec 14, 2022

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
Copy link
Author

aeyates commented Apr 9, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant