Merge pull request #868 from atmire/google-scholar-fixes

tdonohue · web-flow · commit 6511920d0c7e · 2020-09-30T09:48:17.000-05:00
Google scholar fixes
diff --git a/docs/Configuration.md b/docs/Configuration.md
@@ -63,3 +63,70 @@ Angulartics can be configured to work with a number of other services besides Go
 In order to start using one of these services, select it from the [Angulartics Providers page](https://angulartics.github.io/angulartics2/#providers), and follow the instructions on how to configure it.
 
 The Google Analytics script was added in [`main.browser.ts`](https://github.com/DSpace/dspace-angular/blob/ff04760f4af91ac3e7add5e7424a46cb2439e874/src/main.browser.ts#L33) instead of the `<head>` tag in `index.html` to ensure events get sent when the page is shown in a client's browser, and not when it's rendered on the universal server. Likely you'll want to do the same when adding a new service.
+
+## SEO when hosting REST Api and UI on different servers
+
+Indexers such as Google Scholar require that files are hosted on the same domain as the page that links them. In DSpace 7, Bitstreams are served from the REST server. So if you use different servers for the REST api and the UI you'll want to ensure that Bitstream downloads are proxied through the UI server. 
+
+In order to achieve this we'll need to do two things:
+- **Proxy the Bitstream downloads through the UI server.** You'll need to put a webserver such as httpd or nginx in front of the UI server in order to achieve this. [Below](#apache-http-server-config) you'll find a section explaining how to do it in httpd.
+- **Update the URLs for Bitstream downloads to match the UI server.** This can be done using a setting in the UI environment file. 
+
+### UI config
+If you set the property `rewriteDownloadUrls` to `true` in your `environment.prod.ts` file, the [origin](https://developer.mozilla.org/en-US/docs/Glossary/Origin) of any download URL will be replaced by the origin of the UI. This will also happen for the `citation_pdf_url` `<meta>` tag on Item pages.
+
+The app will determine the UI origin currently in use, so the external UI URL doesn't need to be configured anywhere and rewrites will still work if you host the UI from multiple domains.
+
+### Apache HTTP Server config
+
+#### Basics
+In order to be able to host bitstreams from the UI Server you'll need to enable mod_proxy and add the following to the httpd config of your UI server:
+
+```
+ProxyPassMatch    "/server/api/core/bitstreams/([^/]+)/content" "http://rest.api/server/api/core/bitstreams/$1/content" 
+ProxyPassReverse  "/server/api/core/bitstreams/([^/]+)/content" "http://rest.api/server/api/core/bitstreams/$1/content"
+```
+
+Replace http://rest.api in with the correct origin for your REST server.
+
+The `ProxyPassMatch` line forwards all requests matching the regular expression for a bitstream download URL to the corresponding path on the REST server
+
+The `ProxyPassReverse` ensures that if the REST server were to return redirect response, httpd would also swap out its hostname for the hostname of the UI before forwarding the response to the client.
+
+#### Using HTTPS
+If your REST server uses https, you'll need to enable mod_ssl and ensure `SSLProxyEngine on` is part of your UI server's httpd config as well
+
+If the UI hostname doesn't match the CN in the SSL certificate of the REST server (which is likely if they're on different domains), you'll also need to add the following lines
+
+```
+SSLProxyCheckPeerCN off
+SSLProxyCheckPeerName off
+```
+These are two names for [the same directive](https://httpd.apache.org/docs/trunk/mod/mod_ssl.html#sslproxycheckpeername) that have been used for various versions of httpd, old versions need the former, then some in-between versions need both, and newer versions only need the latter. Keeping them both doesn't harm anything.
+
+So the entire config becomes:
+
+```
+SSLProxyEngine on
+SSLProxyCheckPeerCN off
+SSLProxyCheckPeerName off
+ProxyPassMatch    "/server/api/core/bitstreams/([^/]+)/content" "https://rest.api/server/api/core/bitstreams/$1/content" 
+ProxyPassReverse  "/server/api/core/bitstreams/([^/]+)/content" "https://rest.api/server/api/core/bitstreams/$1/content"
+```
+
+If you don't want httpd to verify the certificate of the REST server, you can also turn all checks off with the following config:
+
+```
+SSLProxyEngine on
+SSLProxyVerify none
+SSLProxyCheckPeerCN off
+SSLProxyCheckPeerName off
+SSLProxyCheckPeerExpire off
+ProxyPassMatch    "/server/api/core/bitstreams/([^/]+)/content" "https://rest.api/server/api/core/bitstreams/$1/content" 
+ProxyPassReverse  "/server/api/core/bitstreams/([^/]+)/content" "https://rest.api/server/api/core/bitstreams/$1/content"
+```
+
+
+
+
+
diff --git a/scripts/set-env.ts b/scripts/set-env.ts
@@ -57,8 +57,8 @@ function generateEnvironmentFile(file: GlobalConfig): void {
 
   // TODO remove workaround in beta 5
   if (file.rest.nameSpace.match("(.*)/api/?$") !== null) {
-    const newValue = getNameSpace(file.rest.nameSpace);
-    console.log(colors.white.bgMagenta.bold(`The rest.nameSpace property in your environment file or in your DSPACE_REST_NAMESPACE environment variable ends with '/api'.\nThis is deprecated. As '/api' isn't configurable on the rest side, it shouldn't be repeated in every environment file.\nPlease change the rest nameSpace to '${newValue}'`));
+    file.rest.nameSpace = getNameSpace(file.rest.nameSpace);
+    console.log(colors.white.bgMagenta.bold(`The rest.nameSpace property in your environment file or in your DSPACE_REST_NAMESPACE environment variable ends with '/api'.\nThis is deprecated. As '/api' isn't configurable on the rest side, it shouldn't be repeated in every environment file.\nPlease change the rest nameSpace to '${file.rest.nameSpace}'`));
   }
 
   const contents = `export const environment = ` + JSON.stringify(file);
diff --git a/src/app/core/metadata/metadata.service.spec.ts b/src/app/core/metadata/metadata.service.spec.ts
@@ -52,10 +52,13 @@ import { UUIDService } from '../shared/uuid.service';
 import { MetadataService } from './metadata.service';
 import { environment } from '../../../environments/environment';
 import { storeModuleConfig } from '../../app.reducer';
+import { HardRedirectService } from '../services/hard-redirect.service';
+import { URLCombiner } from '../url-combiner/url-combiner';
 
 /* tslint:disable:max-classes-per-file */
 @Component({
-  template: `<router-outlet></router-outlet>`
+  template: `
+      <router-outlet></router-outlet>`
 })
 class TestComponent {
   constructor(private metadata: MetadataService) {
@@ -170,6 +173,7 @@ describe('MetadataService', () => {
         Title,
         // tslint:disable-next-line:no-empty
         { provide: ItemDataService, useValue: { findById: () => {} } },
+        { provide: HardRedirectService, useValue: { rewriteDownloadURL: (a) => a, getRequestOrigin: () => environment.ui.baseUrl }},
         BrowseService,
         MetadataService
       ],
@@ -208,7 +212,7 @@ describe('MetadataService', () => {
     tick();
     expect(tagStore.get('citation_dissertation_name')[0].content).toEqual('Test PowerPoint Document');
     expect(tagStore.get('citation_dissertation_institution')[0].content).toEqual('Mock Publisher');
-    expect(tagStore.get('citation_abstract_html_url')[0].content).toEqual([environment.ui.baseUrl, router.url].join(''));
+    expect(tagStore.get('citation_abstract_html_url')[0].content).toEqual(new URLCombiner(environment.ui.baseUrl, router.url).toString());
     expect(tagStore.get('citation_pdf_url')[0].content).toEqual('https://dspace7.4science.it/dspace-spring-rest/api/core/bitstreams/99b00f3c-1cc6-4689-8158-91965bee6b28/content');
   }));
 
diff --git a/src/app/core/metadata/metadata.service.ts b/src/app/core/metadata/metadata.service.ts
@@ -1,4 +1,4 @@
-import { Inject, Injectable } from '@angular/core';
+import { Injectable } from '@angular/core';
 
 import { Meta, MetaDefinition, Title } from '@angular/platform-browser';
 import { ActivatedRoute, NavigationEnd, Router } from '@angular/router';
@@ -20,7 +20,8 @@ import { Bitstream } from '../shared/bitstream.model';
 import { DSpaceObject } from '../shared/dspace-object.model';
 import { Item } from '../shared/item.model';
 import { getFirstSucceededRemoteDataPayload, getFirstSucceededRemoteListPayload } from '../shared/operators';
-import { environment } from '../../../environments/environment';
+import { HardRedirectService } from '../services/hard-redirect.service';
+import { URLCombiner } from '../url-combiner/url-combiner';
 
 @Injectable()
 export class MetadataService {
@@ -39,6 +40,7 @@ export class MetadataService {
     private dsoNameService: DSONameService,
     private bitstreamDataService: BitstreamDataService,
     private bitstreamFormatDataService: BitstreamFormatDataService,
+    private redirectService: HardRedirectService
   ) {
     // TODO: determine what open graph meta tags are needed and whether
     // the differ per route. potentially add image based on DSpaceObject
@@ -254,7 +256,7 @@ export class MetadataService {
    */
   private setCitationAbstractUrlTag(): void {
     if (this.currentObject.value instanceof Item) {
-      const value = [environment.ui.baseUrl, this.router.url].join('');
+      const value = new URLCombiner(this.redirectService.getRequestOrigin(), this.router.url).toString();
       this.addMetaTag('citation_abstract_html_url', value);
     }
   }
@@ -279,7 +281,8 @@ export class MetadataService {
               getFirstSucceededRemoteDataPayload()
             ).subscribe((format: BitstreamFormat) => {
               if (format.mimetype === 'application/pdf') {
-                this.addMetaTag('citation_pdf_url', bitstream._links.content.href);
+                const rewrittenURL= this.redirectService.rewriteDownloadURL(bitstream._links.content.href);
+                this.addMetaTag('citation_pdf_url', rewrittenURL);
               }
             });
           }
diff --git a/src/app/core/services/browser-hard-redirect.service.spec.ts b/src/app/core/services/browser-hard-redirect.service.spec.ts
@@ -2,11 +2,12 @@ import {TestBed} from '@angular/core/testing';
 import {BrowserHardRedirectService} from './browser-hard-redirect.service';
 
 describe('BrowserHardRedirectService', () => {
-
+  const origin = 'test origin';
   const mockLocation = {
     href: undefined,
     pathname: '/pathname',
     search: '/search',
+    origin
   } as Location;
 
   const service: BrowserHardRedirectService = new BrowserHardRedirectService(mockLocation);
@@ -38,4 +39,12 @@ describe('BrowserHardRedirectService', () => {
       expect(service.getCurrentRoute()).toEqual(mockLocation.pathname + mockLocation.search);
     });
   });
+
+  describe('when requesting the origin', () => {
+
+    it('should return the location origin', () => {
+      expect(service.getRequestOrigin()).toEqual(origin);
+    });
+  });
+
 });
diff --git a/src/app/core/services/browser-hard-redirect.service.ts b/src/app/core/services/browser-hard-redirect.service.ts
@@ -11,11 +11,12 @@ export function locationProvider(): Location {
  * Service for performing hard redirects within the browser app module
  */
 @Injectable()
-export class BrowserHardRedirectService implements HardRedirectService {
+export class BrowserHardRedirectService extends HardRedirectService {
 
   constructor(
     @Inject(LocationToken) protected location: Location,
   ) {
+    super();
   }
 
   /**
@@ -32,4 +33,11 @@ export class BrowserHardRedirectService implements HardRedirectService {
   getCurrentRoute() {
     return this.location.pathname + this.location.search;
   }
+
+  /**
+   * Get the hostname of the request
+   */
+  getRequestOrigin() {
+    return this.location.origin;
+  }
 }
diff --git a/src/app/core/services/hard-redirect.service.spec.ts b/src/app/core/services/hard-redirect.service.spec.ts
@@ -0,0 +1,57 @@
+import { HardRedirectService } from './hard-redirect.service';
+import { environment } from '../../../environments/environment';
+import { TestBed } from '@angular/core/testing';
+import { Injectable } from '@angular/core';
+
+const requestOrigin = 'http://dspace-angular-ui.dspace.com';
+
+describe('HardRedirectService', () => {
+  let service: TestHardRedirectService;
+
+  beforeEach(() => {
+    TestBed.configureTestingModule({ providers: [TestHardRedirectService] });
+    service = TestBed.get(TestHardRedirectService);
+  });
+
+  describe('when calling rewriteDownloadURL', () => {
+    let originalValue;
+    const relativePath = '/test/url/path';
+    const testURL = environment.rest.baseUrl + relativePath;
+    beforeEach(() => {
+      originalValue = environment.rewriteDownloadUrls;
+    });
+
+    it('it should return the same url when rewriteDownloadURL is false', () => {
+      environment.rewriteDownloadUrls = false;
+      expect(service.rewriteDownloadURL(testURL)).toEqual(testURL);
+    });
+
+    it('it should replace part of the url when rewriteDownloadURL is true', () => {
+      environment.rewriteDownloadUrls = true;
+      expect(service.rewriteDownloadURL(testURL)).toEqual(requestOrigin + environment.rest.nameSpace + relativePath);
+    });
+
+    afterEach(() => {
+      environment.rewriteDownloadUrls = originalValue;
+    })
+  });
+});
+
+@Injectable()
+class TestHardRedirectService extends HardRedirectService {
+  constructor() {
+    super();
+  }
+
+  redirect(url: string) {
+    return undefined;
+  }
+
+  getCurrentRoute() {
+    return undefined;
+  }
+
+  getRequestOrigin() {
+    return requestOrigin;
+  }
+}
diff --git a/src/app/core/services/hard-redirect.service.ts b/src/app/core/services/hard-redirect.service.ts
@@ -1,4 +1,6 @@
 import { Injectable } from '@angular/core';
+import { environment } from '../../../environments/environment';
+import { URLCombiner } from '../url-combiner/url-combiner';
 
 /**
  * Service to take care of hard redirects
@@ -19,4 +21,20 @@ export abstract class HardRedirectService {
    * e.g. /search?page=1&query=open%20access&f.dateIssued.min=1980&f.dateIssued.max=2020
    */
   abstract getCurrentRoute();
+
+  /**
+   * Get the hostname of the request
+   */
+  abstract getRequestOrigin();
+
+  public rewriteDownloadURL(originalUrl: string): string {
+    if (environment.rewriteDownloadUrls) {
+      const hostName = this.getRequestOrigin();
+      const namespace = environment.rest.nameSpace;
+      const rewrittenUrl = new URLCombiner(hostName, namespace).toString();
+      return originalUrl.replace(environment.rest.baseUrl, rewrittenUrl);
+    } else {
+      return originalUrl;
+    }
+  }
 }
diff --git a/src/app/core/services/server-hard-redirect.service.spec.ts b/src/app/core/services/server-hard-redirect.service.spec.ts
@@ -7,8 +7,13 @@ describe('ServerHardRedirectService', () => {
   const mockResponse = jasmine.createSpyObj(['redirect', 'end']);
 
   const service: ServerHardRedirectService = new ServerHardRedirectService(mockRequest, mockResponse);
+  const origin = 'test-host';
 
   beforeEach(() => {
+    mockRequest.headers = {
+      host: 'test-host',
+    };
+
     TestBed.configureTestingModule({});
   });
 
@@ -40,4 +45,12 @@ describe('ServerHardRedirectService', () => {
       expect(service.getCurrentRoute()).toEqual(mockRequest.originalUrl);
     });
   });
+
+  describe('when requesting the origin', () => {
+
+    it('should return the location origin', () => {
+      expect(service.getRequestOrigin()).toEqual(origin);
+    });
+  });
+
 });
diff --git a/src/app/core/services/server-hard-redirect.service.ts b/src/app/core/services/server-hard-redirect.service.ts
@@ -7,12 +7,13 @@ import { HardRedirectService } from './hard-redirect.service';
  * Service for performing hard redirects within the server app module
  */
 @Injectable()
-export class ServerHardRedirectService implements HardRedirectService {
+export class ServerHardRedirectService extends HardRedirectService {
 
   constructor(
     @Inject(REQUEST) protected req: Request,
     @Inject(RESPONSE) protected res: Response,
   ) {
+    super();
   }
 
   /**
@@ -59,4 +60,11 @@ export class ServerHardRedirectService implements HardRedirectService {
   getCurrentRoute() {
     return this.req.originalUrl;
   }
+
+  /**
+   * Get the hostname of the request
+   */
+  getRequestOrigin() {
+    return this.req.headers.host;
+  }
 }
diff --git a/src/app/shared/file-download-link/file-download-link.component.spec.ts b/src/app/shared/file-download-link/file-download-link.component.spec.ts
@@ -3,6 +3,7 @@ import { FileDownloadLinkComponent } from './file-download-link.component';
 import { AuthService } from '../../core/auth/auth.service';
 import { FileService } from '../../core/shared/file.service';
 import { of as observableOf } from 'rxjs';
+import { HardRedirectService } from '../../core/services/hard-redirect.service';
 
 describe('FileDownloadLinkComponent', () => {
   let component: FileDownloadLinkComponent;
@@ -23,13 +24,14 @@ describe('FileDownloadLinkComponent', () => {
   beforeEach(async(() => {
     init();
     TestBed.configureTestingModule({
-      declarations: [ FileDownloadLinkComponent ],
+      declarations: [FileDownloadLinkComponent],
       providers: [
         { provide: AuthService, useValue: authService },
-        { provide: FileService, useValue: fileService }
+        { provide: FileService, useValue: fileService },
+        { provide: HardRedirectService, useValue: { rewriteDownloadURL: (a) => a } },
       ]
     })
-    .compileComponents();
+      .compileComponents();
   }));
 
   beforeEach(() => {
diff --git a/src/app/shared/file-download-link/file-download-link.component.ts b/src/app/shared/file-download-link/file-download-link.component.ts
diff --git a/src/config/global-config.interface.ts b/src/config/global-config.interface.ts
diff --git a/src/environments/environment.common.ts b/src/environments/environment.common.ts

Original file line number	Diff line number	Diff line change
`@@ -57,8 +57,8 @@ function generateEnvironmentFile(file: GlobalConfig): void {`
`57`	`57`
`58`	`58`	`// TODO remove workaround in beta 5`
`59`	`59`	`if (file.rest.nameSpace.match("(.*)/api/?$") !== null) {`
`60`		`- const newValue = getNameSpace(file.rest.nameSpace);`
`61`		- console.log(colors.white.bgMagenta.bold(`The rest.nameSpace property in your environment file or in your DSPACE_REST_NAMESPACE environment variable ends with '/api'.\nThis is deprecated. As '/api' isn't configurable on the rest side, it shouldn't be repeated in every environment file.\nPlease change the rest nameSpace to '${newValue}'`));
	`60`	`+ file.rest.nameSpace = getNameSpace(file.rest.nameSpace);`
	`61`	+ console.log(colors.white.bgMagenta.bold(`The rest.nameSpace property in your environment file or in your DSPACE_REST_NAMESPACE environment variable ends with '/api'.\nThis is deprecated. As '/api' isn't configurable on the rest side, it shouldn't be repeated in every environment file.\nPlease change the rest nameSpace to '${file.rest.nameSpace}'`));
`62`	`62`	`}`
`63`	`63`
`64`	`64`	const contents = `export const environment = ` + JSON.stringify(file);