Adding rudimentary whitelist support in lieu of globally available ge… #49
Conversation
pbrown-d2l
requested review from
alogan519,
awikkerink and
mdgbayly
as code owners
|
@omsmith - Here is the PR for what we talked about earlier |
|
If I run npm test locally on Master, it is also failing at the same place... any advice there would be helpful too. |
|
Code looks good but failing tests look not fun. Perhaps dummy URLs are being used somewhere and are now being rejected? I'll check |
|
Yup looks like it. Those domains can probably be changed to ones from the whitelist. |
| @@ -126,6 +127,9 @@ window.D2L.Siren.EntityStore = { | |||
| if (!entityId) { | |||
| return Promise.reject(new Error('Cannot fetch undefined entityId')); | |||
| } | |||
| if (!window.D2L.Siren.WhitelistBehavior.isWhitelisted(entityId)) { | |||
| return Promise.reject(new Error('Invalid request url; must be a valid whitelisted domain.')); | |||
There was a problem hiding this comment.
This isn't the approach we've taken generally. It might be fine, but it also might not be. Other libraries we've made have opted to just not add the Authorization header, as opposed to making the library only work at all for d2l urls.
There was a problem hiding this comment.
@pbrown-d2l @awikkerink Are we sure this is the approach we want to release?
There was a problem hiding this comment.
This was sort of a breaking change for my repo. Tests began failing. I'm guessing the recommended way is to setup test mode during setup? Let me know if there is a different suggestion. Related PR is https://github.com/BrightspaceHypermediaComponents/activities/pull/243
There was a problem hiding this comment.
This has broken a lot of demos and tests :/ on most of the BHC org.
There was a problem hiding this comment.
If you are making calls to sites not on the whitelist in your tests and demos it is very easy to switch it off as Miles mentioned. Luckily, now your users tokens aren't at risk,
…neric whitelist