Adding Quotes to Sidebar#90
Conversation
.gitignore
Outdated
| yearbook/ | ||
|
|
||
| # Ignore Config File from git | ||
| data/config.json No newline at end of file |
There was a problem hiding this comment.
Could you have the file end with a newline? For reference on why this is important please visit: https://stackoverflow.com/questions/729692/why-should-text-files-end-with-a-newline
data/config.tmp.json
Outdated
| @@ -0,0 +1,3 @@ | |||
| { | |||
| "quotefaultAPI": "keyherepls" | |||
| } No newline at end of file | |||
There was a problem hiding this comment.
See above comment on ending files with newlines.
|
|
||
| // Get the quotes | ||
| $scope.quote = []; | ||
| $http.get("./data/config.json").success(function (response) { |
There was a problem hiding this comment.
Doesn't this pose a security vulnerability where I could visit https://members.csh.rit.edu/data/config.json and then read out your API information and gain access to use that API as your user?
There was a problem hiding this comment.
Thinking back I think this concern could be fairly dismissed since only people who would already have quotefault access would get to this point. The only route that would really lead to any questionable activity would be adding quotes, but if that ends up being a concern a good solution may be to add read-only keys to QuotefaultAPI (although I don't think we'll get to that point).
There was a problem hiding this comment.
I can likely add readonly keys, but yeah in the meantime I'm not really worried about people accessing the key. Also couldn't we do something with permissions around the file to protect it? I don't really know what other way to store the key that makes sense.
There was a problem hiding this comment.
Since you'd be trying to access the file from client-side JS you'd need to make the file readable over HTTP to authenticated clients. I think that designing a way for that file to be secure would require more effort than would be worth it for protecting a database of quotes.
If you end up adding readonly keys I think that would be the simplest way to get a secure solution (from the perspective of someone with organization-level access accessing the API and mutating data through a key that 'anonymizes' them).
I'm fine with this change if @mbillow and @stevenmirabito also agree that this concern isn't high enough priority to block adding in this feature.
templates/quote.html
Outdated
| </div> | ||
| </div> | ||
| </div> | ||
|
|
There was a problem hiding this comment.
Could you not have this file end with a double-newline?
liam-middlebrook
left a comment
liam-middlebrook
left a comment
There was a problem hiding this comment.
I totally messed up and didn't use the review feature to properly batch my comments together, sorry about that. Could you fix the changes / address the concerns I pointed out above?
|
@stevenmirabito and I talked about this and the best way to do it is to grab the |
|
@mbillow sounds good. I'll dismiss my pending review for that then and defer to you and @stevenmirabito. |
liam-middlebrook
dismissed
their stale review
Deferring to @mbillow and @stevenmirabito for additional review for JWT integration with Keycloak for security purposes.
|
@devinmatte with the new quotefault API, this will probably have to be refactored. |
4 participants
Added Quotes to the Sidebar of members using the new Quotefault API!
This pull request includes a few changes:
Fixes #54