Merge branch 'master' into login-fix

client/coral-embed-stream/src/CommentStream.js

@@ -61,7 +61,11 @@ class CommentStream extends Component {
     // Set up messaging between embedded Iframe an parent component
     this.pym = new Pym.Child({polling: 100});
 
-    const path = this.pym.parentUrl.split('#')[0];
+    let path = this.pym.parentUrl.split('#')[0];
+
+    if (!path) {
+      path = window.location.href.split('#')[0];
+    }
 
     this.props.getStream(path || window.location);
     this.path = path;

client/coral-framework/actions/user.js

@@ -32,7 +32,7 @@ export const saveBio = (user_id, formData) => dispatch => {
 export const fetchCommentsByUserId = userId => {
   return (dispatch) => {
     dispatch({type: actions.COMMENTS_BY_USER_REQUEST});
-    return coralApi(`/comments?user_id${userId}`)
+    return coralApi(`/comments?user_id=${userId}`)
       .then(({comments, assets}) => {
         comments.forEach(comment => dispatch(addItem(comment, 'comments')));
 

client/coral-framework/constants/assets.js

@@ -1,3 +1,3 @@
 export const MULTIPLE_ASSETS_REQUEST = 'MULTIPLE_ASSETS_REQUEST';
 export const MULTIPLE_ASSETS_SUCCESS = 'MULTIPLE_ASSETS_SUCCESS';
-export const MULTIPLE_ASSSETS_FAILURE = 'MULTIPLE_ASSSETS_FAILURE';
+export const MULTIPLE_ASSSETS_FAILURE = 'MULTIPLE_ASSSETS_FAILURE';

client/coral-framework/reducers/user.js

@@ -1,4 +1,4 @@
-import {Map, fromJS} from 'immutable';
+import {Map} from 'immutable';
 import * as authActions from '../constants/auth';
 import * as actions from '../constants/user';
 import * as assetActions from '../constants/assets';
@@ -34,9 +34,9 @@ export default function user (state = initialState, action) {
     return state
       .set('settings', action.settings);
   case actions.COMMENTS_BY_USER_SUCCESS:
-    return state.set('myComments', fromJS(action.comments));
+    return state.set('myComments', action.comments);
   case assetActions.MULTIPLE_ASSETS_SUCCESS:
-    return state.set('myAssets', fromJS(action.assets));
+    return state.set('myAssets', action.assets);
   default :
     return state;
   }

client/coral-plugin-history/Comment.js

@@ -6,7 +6,7 @@ const Comment = props => {
   return (
     <div>
       <p className="myCommentAsset">
-        <a className={`${styles.assetURL} myCommentAnchor`} href={props.asset.url}>{props.asset.url}</a>
+        <a className={`${styles.assetURL} myCommentAnchor`} href={`${props.asset.url}#${props.comment.id}`}>{`${props.asset.url}#${props.comment.id}`}</a>
       </p>
       <p className={`${styles.commentBody} myCommentBody`}>{props.comment.body}</p>
     </div>
@@ -15,6 +15,7 @@ const Comment = props => {
 
 Comment.propTypes = {
   comment: PropTypes.shape({
+    id: PropTypes.string,
     body: PropTypes.string
   }).isRequired,
   asset: PropTypes.shape({

routes/api/comments/index.js

@@ -9,7 +9,7 @@ const _ = require('lodash');
 
 const router = express.Router();
 
-router.get('/', authorization.needed('admin'), (req, res, next) => {
+router.get('/', (req, res, next) => {
 
   const {
     status = null,
@@ -18,6 +18,18 @@ router.get('/', authorization.needed('admin'), (req, res, next) => {
     user_id = null
   } = req.query;
 
+  // everything on this route requires admin privileges besides listing comments for owner of said comments
+  if (!authorization.has(req.user, 'admin') && !user_id) {
+    next(authorization.ErrNotAuthorized);
+    return;
+  }
+
+  // if the user is not an admin, only return comment list for the owner of the comments
+  if (req.user.id !== user_id && !authorization.has(req.user, 'admin')) {
+    next(authorization.ErrNotAuthorized);
+    return;
+  }
+
   /**
    * This adds the asset_id requirement to the query if the asset_id is defined.
    */
@@ -31,10 +43,13 @@ router.get('/', authorization.needed('admin'), (req, res, next) => {
 
   let query;
 
-  if (status) {
-    query = assetIDWrap(Comment.findByStatus(status === 'new' ? null : status));
-  } else if (user_id) {
+  // the check for user_id MUST be first here.
+  // otherwise this will be a vulnerability if you pass user_id and something else,
+  // the app will return admin-level data without the proper checks
+  if (user_id) {
     query = Comment.findByUserId(user_id);
+  } else if (status) {
+    query = assetIDWrap(Comment.findByStatus(status === 'new' ? null : status));
   } else if (action_type) {
     query = Comment
       .findIdsByActionType(action_type)

tests/client/coral-plugin-history/Comment.spec.js

@@ -5,7 +5,7 @@ import Comment from '../../../client/coral-plugin-history/Comment';
 
 describe('coral-plugin-history/Comment', () => {
   let render;
-  const comment = {body: 'this is a comment'};
+  const comment = {body: 'this is a comment', id: '123'};
   const asset = {url: 'https://google.com'};
 
   beforeEach(() => {
@@ -21,7 +21,7 @@ describe('coral-plugin-history/Comment', () => {
   it('should render the asset url as a link', () => {
     const wrapper = mount(<Comment asset={asset} comment={comment} />);
     expect(wrapper.find('.myCommentAnchor')).to.have.length(1);
-    expect(wrapper.find('.myCommentAnchor').text()).to.equal('https://google.com');
+    expect(wrapper.find('.myCommentAnchor').text()).to.equal('https://google.com#123');
   });
 
   it('should render the comment with styles', () => {

tests/routes/api/comments/index.js

@@ -85,6 +85,29 @@ describe('/api/v1/comments', () => {
       ]);
     });
 
+    it('should return only the owner’s comments if the user is not an admin', () => {
+      return chai.request(app)
+        .get('/api/v1/comments?user_id=456')
+        .set(passport.inject({id: '456', roles: []}))
+        .then(res => {
+          expect(res).to.have.status(200);
+          expect(res.body.comments).to.have.length(2);
+          expect(res.body.comments[1]).to.have.property('author_id', '456');
+        });
+    });
+
+    it('should fail if a non-admin requests comments not owned by them', () => {
+      return chai.request(app)
+        .get('/api/v1/comments?user_id=456')
+        .set(passport.inject({id: '123', roles: []}))
+        .then((res) => {
+          expect(res).to.be.empty;
+        })
+        .catch((err) => {
+          expect(err).to.have.property('status', 401);
+        });
+    });
+
     it('should return all the comments', () => {
       return chai.request(app)
         .get('/api/v1/comments')