[Ready] fix 5288 removal of accept header options in retrieve metadata (#5293)

* chore: Minor adjustment to generateAcceptHeader function signature to reflect expected return type.

Signed-off-by: Luis M. Santos <[email protected]>

* fix: Added switch to skip the Accept header generation when requesting metadata. WADO metadata request is more likely to return JSON and some VNAs do not like the extra options in the Accept header. Also, passing an empty array is not sufficient because somewhere we still include a comma that breaks the Accept header. It's better to omit it for the metadata request only.

Signed-off-by: Luis M. Santos <[email protected]>

* chore: Added new HeadersInterface interface type docstrings.

Signed-off-by: Luis M. Santos <[email protected]>

* chore: Refactored getAuthorizationHeader() function signature so it is typed checked. Of course, I upgraded the module to a TypeScript module.
Moved the request header interfaces into its own TypeScript module (RequestHeaders.ts) in the core types.

Signed-off-by: Luis M. Santos <[email protected]>

* chore: Refactored the user logic to include a TypeScript interface. As a result, upgraded the source file to TypeScript.
Removed the User interface from RequestHeaders module and moved them to the user module.

Signed-off-by: Luis M. Santos <[email protected]>

* chore: Updated function signatures, user import, and confirmed unit tests are passing.

Signed-off-by: Luis M. Santos <[email protected]>

* chore: Minor stylistic adjustment to getAuthorizationHeader.test.ts

Signed-off-by: Luis M. Santos <[email protected]>

* chore: Added missing comments.

Signed-off-by: Luis M. Santos <[email protected]>

* chore adjusted generateWadoHeader parameter's name and added comment about the expected default header.

Signed-off-by: Luis M. Santos <[email protected]>

* Just tweaking the interface a bit to be more consistent.

* fix authorization header signature change

---------

Signed-off-by: Luis M. Santos <[email protected]>
Co-authored-by: Bill Wallace <[email protected]>
Co-authored-by: Alireza <[email protected]>

Author: 3 people

extensions/default/src/DicomWebDataSource/index.ts

@@ -16,6 +16,7 @@ import { retrieveStudyMetadata, deleteStudyMetadataPromise } from './retrieveStu
 import StaticWadoClient from './utils/StaticWadoClient';
 import getDirectURL from '../utils/getDirectURL';
 import { fixBulkDataURI } from './utils/fixBulkDataURI';
+import {HeadersInterface} from '@ohif/core/src/types/RequestHeaders';
 
 const { DicomMetaDictionary, DicomDict } = dcmjs.data;
 
@@ -97,6 +98,20 @@ export type BulkDataURIConfig = {
   relativeResolution?: 'studies' | 'series';
 };
 
+/**
+ * The header options are the options passed into the generateWadoHeader
+ * command.  This takes an extensible set of attributes to allow future enhancements.
+ */
+export interface HeaderOptions {
+  includeTransferSyntax?: boolean;
+}
+
+/**
+ * Metadata and some other requests don't permit the transfer syntax to be included,
+ * so pass in the excludeTransferSyntax parameter.
+ */
+export const excludeTransferSyntax: HeaderOptions = { includeTransferSyntax: false };
+
 /**
  * Creates a DICOM Web API based on the provided configuration.
  *
@@ -128,27 +143,41 @@ function createDicomWebApi(dicomWebConfig: DicomWebConfig, servicesManager) {
       dicomWebConfigCopy = JSON.parse(JSON.stringify(dicomWebConfig));
 
       getAuthorizationHeader = () => {
-        const xhrRequestHeaders = {};
+        const xhrRequestHeaders: HeadersInterface = {};
         const authHeaders = userAuthenticationService.getAuthorizationHeader();
         if (authHeaders && authHeaders.Authorization) {
           xhrRequestHeaders.Authorization = authHeaders.Authorization;
         }
         return xhrRequestHeaders;
       };
 
-      generateWadoHeader = () => {
+      /**
+       * Generates the wado header for requesting resources from DICOMweb.
+       * These are classified into those that are dependent on the transfer syntax
+       * and those that aren't, as defined by the include transfer syntax attribute.
+       */
+      generateWadoHeader = (options: HeaderOptions): HeadersInterface => {
         const authorizationHeader = getAuthorizationHeader();
-        //Generate accept header depending on config params
-        const formattedAcceptHeader = utils.generateAcceptHeader(
-          dicomWebConfig.acceptHeader,
-          dicomWebConfig.requestTransferSyntaxUID,
-          dicomWebConfig.omitQuotationForMultipartRequest
-        );
-
-        return {
-          ...authorizationHeader,
-          Accept: formattedAcceptHeader,
-        };
+        if (options?.includeTransferSyntax!==false) {
+          //Generate accept header depending on config params
+          const formattedAcceptHeader = utils.generateAcceptHeader(
+            dicomWebConfig.acceptHeader,
+            dicomWebConfig.requestTransferSyntaxUID,
+            dicomWebConfig.omitQuotationForMultipartRequest
+          );
+          return {
+            ...authorizationHeader,
+            Accept: formattedAcceptHeader,
+          };
+        } else {
+          // The base header will be included in the request. We simply skip customization options around
+          // transfer syntaxes and whether the request is multipart. In other words, a request in
+          // which the server expects Accept: application/dicom+json will still include that in the
+          // header.
+          return {
+            ...authorizationHeader
+          };
+        }
       };
 
       qidoConfig = {
@@ -410,7 +439,7 @@ function createDicomWebApi(dicomWebConfig: DicomWebConfig, servicesManager) {
       madeInClient
     ) => {
       const enableStudyLazyLoad = false;
-      wadoDicomWebClient.headers = generateWadoHeader();
+      wadoDicomWebClient.headers = generateWadoHeader(excludeTransferSyntax);
       // data is all SOPInstanceUIDs
       const data = await retrieveStudyMetadata(
         wadoDicomWebClient,
@@ -484,7 +513,7 @@ function createDicomWebApi(dicomWebConfig: DicomWebConfig, servicesManager) {
       returnPromises = false
     ) => {
       const enableStudyLazyLoad = true;
-      wadoDicomWebClient.headers = generateWadoHeader();
+      wadoDicomWebClient.headers = generateWadoHeader(excludeTransferSyntax);
       // Get Series
       const { preLoadData: seriesSummaryMetadata, promises: seriesPromises } =
         await retrieveStudyMetadata(

platform/core/src/DICOMWeb/getAuthorizationHeader.test.js renamed to platform/core/src/DICOMWeb/getAuthorizationHeader.test.ts

@@ -1,7 +1,8 @@
 import getAuthorizationHeader from './getAuthorizationHeader';
 import user from './../user';
+import { HeadersInterface, RequestOptions } from '../types/RequestHeaders';
 
-jest.mock('./../user.js');
+jest.mock('../user');
 
 describe('getAuthorizationHeader', () => {
   it('should return a HTTP Basic Auth when server contains requestOptions.auth', () => {
@@ -15,7 +16,7 @@ describe('getAuthorizationHeader', () => {
       Authorization: `Basic ${btoa(validServer.requestOptions.auth)}`,
     };
 
-    const authentication = getAuthorizationHeader(validServer);
+    const authentication: HeadersInterface = getAuthorizationHeader(validServer);
 
     expect(authentication).toEqual(expectedAuthorizationHeader);
   });
@@ -31,13 +32,13 @@ describe('getAuthorizationHeader', () => {
       Authorization: `Basic ${btoa(validServerWithoutPassword.requestOptions.auth)}`,
     };
 
-    const authentication = getAuthorizationHeader(validServerWithoutPassword);
+    const authentication: HeadersInterface = getAuthorizationHeader(validServerWithoutPassword);
 
     expect(authentication).toEqual(expectedAuthorizationHeader);
   });
 
   it('should return a HTTP Basic Auth when server contains requestOptions.auth custom function', () => {
-    const validServerCustomAuth = {
+    const validServerCustomAuth: RequestOptions = {
       requestOptions: {
         auth: options => `Basic ${options.token}`,
         token: 'ZHVtbXlfdXNlcjpkdW1teV9wYXNzd29yZA==',
@@ -48,21 +49,21 @@ describe('getAuthorizationHeader', () => {
       Authorization: `Basic ${validServerCustomAuth.requestOptions.token}`,
     };
 
-    const authentication = getAuthorizationHeader(validServerCustomAuth);
+    const authentication: HeadersInterface = getAuthorizationHeader(validServerCustomAuth);
 
     expect(authentication).toEqual(expectedAuthorizationHeader);
   });
 
   it('should return an empty object when there is no either server.requestOptions.auth or accessToken', () => {
-    const authentication = getAuthorizationHeader({});
+    const authentication: HeadersInterface = getAuthorizationHeader({});
 
     expect(authentication).toEqual({});
   });
 
   it('should return an Authorization with accessToken when server is not defined and there is an accessToken', () => {
     user.getAccessToken.mockImplementationOnce(() => 'MOCKED_TOKEN');
 
-    const authentication = getAuthorizationHeader({}, user);
+    const authentication: HeadersInterface = getAuthorizationHeader({}, user);
     const expectedHeaderBasedOnUserAccessToken = {
       Authorization: 'Bearer MOCKED_TOKEN',
     };

platform/core/src/DICOMWeb/getAuthorizationHeader.js renamed to platform/core/src/DICOMWeb/getAuthorizationHeader.ts

@@ -1,33 +1,38 @@
 import 'isomorphic-base64';
-import user from '../user';
+import {UserAccountInterface} from '../user';
+import { HeadersInterface, RequestOptions } from '../types/RequestHeaders';
 
 /**
  * Returns the Authorization header as part of an Object.
  *
  * @export
  * @param {Object} [server={}]
- * @param {Object} [server.requestOptions]
- * @param {string|function} [server.requestOptions.auth]
+ * @param {Object} [requestOptions]
+ * @param {string|function} [requestOptions.auth]
+ * @param {Object} [user]
+ * @param {function} [user.getAccessToken]
  * @returns {Object} { Authorization }
  */
-export default function getAuthorizationHeader({ requestOptions } = {}, user) {
-  const headers = {};
+export default function getAuthorizationHeader(
+  {requestOptions}: RequestOptions = {},
+  user: UserAccountInterface = {}): HeadersInterface
+{
+  const headers: HeadersInterface = {};
 
   // Check for OHIF.user since this can also be run on the server
   const accessToken = user && user.getAccessToken && user.getAccessToken();
 
   // Auth for a specific server
-  if (requestOptions && requestOptions.auth) {
+  if (requestOptions?.auth) {
     if (typeof requestOptions.auth === 'function') {
       // Custom Auth Header
       headers.Authorization = requestOptions.auth(requestOptions);
     } else {
       // HTTP Basic Auth (user:password)
       headers.Authorization = `Basic ${btoa(requestOptions.auth)}`;
     }
-  }
-  // Auth for the user's default
-  else if (accessToken) {
+  } else if (accessToken) {
+    // Auth for the user's default
     headers.Authorization = `Bearer ${accessToken}`;
   }
 

platform/core/src/DICOMWeb/index.js

@@ -1,5 +1,5 @@
 import getAttribute from './getAttribute.js';
-import getAuthorizationHeader from './getAuthorizationHeader.js';
+import getAuthorizationHeader from './getAuthorizationHeader';
 import getModalities from './getModalities.js';
 import getName from './getName.js';
 import getNumber from './getNumber.js';

platform/core/src/index.ts

@@ -9,7 +9,7 @@ import errorHandler from './errorHandler.js';
 import log from './log.js';
 import object from './object.js';
 import string from './string.js';
-import user from './user.js';
+import user from './user';
 import utils from './utils';
 import defaults from './defaults';
 import * as Types from './types';

platform/core/src/types/RequestHeaders.ts

@@ -0,0 +1,35 @@
+/**
+ * Interface to clearly present the expected fields to linters when building a request header.
+ */
+export interface HeadersInterface {
+  /**
+   * Request Accept options. For example,
+   * `['multipart/related; type=application/octet-stream; transfer-syntax=1.2.840.10008.1.2.1.99',]`.
+   *
+   * Defines to the server the formats it can use to deliver data to us.
+   */
+  Accept?: string[];
+  /**
+   * Request Authorization field. It can be overridden with the `requestOptions.auth` config item.
+   * Contains the authorization credentials or tokens necessary to authorize the request with the
+   * server.
+   */
+  Authorization?: string;
+}
+
+/**
+ * Interface to clearly present the expected fields to linters when passing the configuration's
+ * requestOptions struct.
+ */
+export interface RequestOptions {
+  requestOptions?: {
+    /**
+     * Authentication options to include. Can be a function.
+     */
+    auth?: Function | string;
+    /**
+     * Authentication token. Satisfies the test requirement?
+     */
+    token?: string;
+  }
+}

platform/core/src/user.js

@@ -0,0 +1,31 @@
+/**
+ * Global user information, to be replaced with a  specific version which
+ * applies the methods.
+ */
+export let user = {
+  userLoggedIn: (): boolean => false,
+  getUserId: () => null,
+  getName: () => null,
+  getAccessToken: () => null,
+  login: () => new Promise((resolve, reject) => reject()),
+  logout: () => new Promise((resolve, reject) => reject()),
+  getData: key => null,
+  setData: (key, value) => null,
+};
+
+/**
+ * Interface to clearly present the expected fields to linters when passing the user account
+ * struct.
+ */
+export interface UserAccountInterface {
+  userLoggedIn?: () => boolean;
+  getUserId?: () => null;
+  getName?: () => null;
+  getAccessToken?: () => null;
+  login?: () => Promise<any>;
+  logout?: () => Promise<any>;
+  getData?: (key: any) => null;
+  setData?: (key: any, value: any) => null;
+}
+
+export default user;

platform/core/src/user.ts

@@ -1,5 +1,5 @@
 const generateAcceptHeader = (
-  configAcceptHeader = [],
+  configAcceptHeader: string[] = [],
   requestTransferSyntaxUID = '*', //default to accept all transfer syntax
   omitQuotationForMultipartRequest = false
 ): string[] => {