canvas-rce-api icon indicating copy to clipboard operation
canvas-rce-api copied to clipboard

Added the RCE_API_HOST environment variable

Open jrherskovic-mda opened this issue 5 years ago • 8 comments

Hi,

We run our Canvas instances on servers not directly exposed to the internet and without publicly routable addresses. They are behind a load balancer/nat appliance.

The canvas-rce-api proxy was getting 127.0.0.1 when calling request.get("host") which meant that the flies browser was getting unusable JSON from app/api/folders.js and unable to work.

To get around the problem, we added an environment variable RCE_API_HOST that allows the sysadmin to force the public URL to a setting of their choosing. If the variable is not set, the previous behavior (using request.get("host")) is preserved.

Best, Jorge

jrherskovic-mda avatar May 29 '19 01:05 jrherskovic-mda

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar May 29 '19 01:05 CLAassistant

@jrherskovic-mda It seems that this patchset doesn't pass the test suite. I'm getting the following two errors:

  272 passing (615ms)
  2 failing

  1) Link API response handlers
       successful canvas responses
         bookmark construction
           keeps the request's protocol and host:
     AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:

  assert.ok(bookmark.match("^http://rce.example"))

      at Context.it (test/service/api/linksResponseHandler.test.js:103:16)

  2) packageBookmark
       forms corect url:

      AssertionError [ERR_ASSERTION]: 'http://false/a/b/c?id=47&bookmark=canvasurl' == 'http://somehost:3000/a/b/c?id=47&bookmark=canvasurl'
      + expected - actual

      -http://false/a/b/c?id=47&bookmark=canvasurl
      +http://somehost:3000/a/b/c?id=47&bookmark=canvasurl
      
      at Context.it (test/service/api/packageBookmark.test.js:27:5)

Would you take a look at those and make adjustments as needed? You should be able to run the test suite by running npm test within the repo.

claydiffrient avatar May 30 '19 14:05 claydiffrient

Fixed the issue. Sorry about that - node isn't my strong suit. That said, the test suite behaves differently on my machine. Could you please verify again?

jherskovic avatar May 30 '19 18:05 jherskovic

@jherskovic I'm just auditing stuff that's outstanding here. There hasn't been any response since I last reviewed on June 3. Are you still interested in the getting this merged in?

claydiffrient avatar Sep 16 '19 17:09 claydiffrient

@jherskovic I'm just auditing stuff that's outstanding here. There hasn't been any response since I last reviewed on June 3. Are you still interested in the getting this merged in?

Yes, definitely. Sorry - I didn't realize that your comments on the test were meant for me. I can certainly attempt to fix the test, if that's what you need.

jherskovic avatar Sep 16 '19 17:09 jherskovic

Ended up ignoring casing of the Host variable - please LMK if this is acceptable.

jrherskovic-mda avatar Sep 16 '19 17:09 jrherskovic-mda

Hi, as @claydiffrient said in https://github.com/instructure/canvas-rce-api/pull/5#issuecomment-531869048, is this change going to be applied?, we're working Apache with a proxy to serve the RCE API and the problem is the same, so we need to be able to pass the RCE_API_HOST in the .env

thattejada avatar Sep 30 '19 23:09 thattejada

This PR is great. I tested in out production environment. I suggest this PR be accepted except "package-lock.json"

Gao-Jun avatar Feb 14 '20 09:02 Gao-Jun