From f4ee28c618f0989beff70ca72e59073d29639e88 Mon Sep 17 00:00:00 2001 From: Riley Davis Date: Thu, 15 Dec 2016 13:05:59 -0700 Subject: [PATCH 01/11] allow non-admin folks to list their comments --- client/coral-framework/actions/user.js | 2 +- routes/api/comments/index.js | 23 +++++++++++++++++++---- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/client/coral-framework/actions/user.js b/client/coral-framework/actions/user.js index 69f4882bb..0ee8659d8 100644 --- a/client/coral-framework/actions/user.js +++ b/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'))); diff --git a/routes/api/comments/index.js b/routes/api/comments/index.js index 8f21a9488..a14b6bba0 100644 --- a/routes/api/comments/index.js +++ b/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; + } + + // only return comment lists for the owner of the comments + if (req.user.id !== user_id) { + 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) From 39fdd168fae3d07e0210fb6be8a5e5ead4ee08ff Mon Sep 17 00:00:00 2001 From: Riley Davis Date: Thu, 15 Dec 2016 13:18:28 -0700 Subject: [PATCH 02/11] admins can still view all comment streams --- routes/api/comments/index.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/routes/api/comments/index.js b/routes/api/comments/index.js index a14b6bba0..90b8b40f2 100644 --- a/routes/api/comments/index.js +++ b/routes/api/comments/index.js @@ -24,8 +24,8 @@ router.get('/', (req, res, next) => { return; } - // only return comment lists for the owner of the comments - if (req.user.id !== user_id) { + // 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; } From 1ea284d4282bc98f2c7cfdce4e7cc0360dd4c70b Mon Sep 17 00:00:00 2001 From: Riley Davis Date: Thu, 15 Dec 2016 13:43:06 -0700 Subject: [PATCH 03/11] remove fromJS call --- client/coral-framework/reducers/user.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/client/coral-framework/reducers/user.js b/client/coral-framework/reducers/user.js index 6e9f3529c..6bdf2a9d6 100644 --- a/client/coral-framework/reducers/user.js +++ b/client/coral-framework/reducers/user.js @@ -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; } From 6f0de84144671bc9d8c8118bd0993d25a5c3d4ca Mon Sep 17 00:00:00 2001 From: Riley Davis Date: Thu, 15 Dec 2016 14:03:54 -0700 Subject: [PATCH 04/11] fix lint --- client/coral-framework/reducers/user.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/coral-framework/reducers/user.js b/client/coral-framework/reducers/user.js index 6bdf2a9d6..bd5f78e87 100644 --- a/client/coral-framework/reducers/user.js +++ b/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'; From 203d35e6d10b7281e02aae0b77c21f6239b1549f Mon Sep 17 00:00:00 2001 From: Riley Davis Date: Thu, 15 Dec 2016 14:33:33 -0700 Subject: [PATCH 05/11] link to comment instead of article --- client/coral-plugin-history/Comment.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/coral-plugin-history/Comment.js b/client/coral-plugin-history/Comment.js index c54a0feef..37a93df13 100644 --- a/client/coral-plugin-history/Comment.js +++ b/client/coral-plugin-history/Comment.js @@ -6,7 +6,7 @@ const Comment = props => { return ( From 181e558b286e481c2ad6370e984c6a0ee740f1b7 Mon Sep 17 00:00:00 2001 From: Riley Davis Date: Thu, 15 Dec 2016 14:36:14 -0700 Subject: [PATCH 06/11] comments not showing on embed page --- client/coral-embed-stream/src/CommentStream.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/client/coral-embed-stream/src/CommentStream.js b/client/coral-embed-stream/src/CommentStream.js index c052e5aa1..e6cc8fbbc 100644 --- a/client/coral-embed-stream/src/CommentStream.js +++ b/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; From 632956e0847a4abefdd4621fb7873556c612bd87 Mon Sep 17 00:00:00 2001 From: riley Date: Thu, 15 Dec 2016 15:20:02 -0700 Subject: [PATCH 07/11] failed to update tests. working now --- client/coral-plugin-history/Comment.js | 1 + tests/client/coral-plugin-history/Comment.spec.js | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/client/coral-plugin-history/Comment.js b/client/coral-plugin-history/Comment.js index 37a93df13..fd515e628 100644 --- a/client/coral-plugin-history/Comment.js +++ b/client/coral-plugin-history/Comment.js @@ -15,6 +15,7 @@ const Comment = props => { Comment.propTypes = { comment: PropTypes.shape({ + id: PropTypes.string, body: PropTypes.string }).isRequired, asset: PropTypes.shape({ diff --git a/tests/client/coral-plugin-history/Comment.spec.js b/tests/client/coral-plugin-history/Comment.spec.js index 7d49630d6..2098fa1b3 100644 --- a/tests/client/coral-plugin-history/Comment.spec.js +++ b/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(); 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', () => { From c7d4bb6961cb7d8651ff3f7b12606e07b361a09f Mon Sep 17 00:00:00 2001 From: riley Date: Thu, 15 Dec 2016 15:20:50 -0700 Subject: [PATCH 08/11] add a newline so lint doesn't complain --- client/coral-framework/constants/assets.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/coral-framework/constants/assets.js b/client/coral-framework/constants/assets.js index 0224e0945..3883ee835 100644 --- a/client/coral-framework/constants/assets.js +++ b/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'; \ No newline at end of file +export const MULTIPLE_ASSSETS_FAILURE = 'MULTIPLE_ASSSETS_FAILURE'; From 115d19945be7cc8a2927bba77be74933a8303397 Mon Sep 17 00:00:00 2001 From: riley Date: Thu, 15 Dec 2016 15:43:22 -0700 Subject: [PATCH 09/11] add tests for comment stream for non-admins --- tests/routes/api/comments/index.js | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/routes/api/comments/index.js b/tests/routes/api/comments/index.js index fdd4e4bbe..f07610bbd 100644 --- a/tests/routes/api/comments/index.js +++ b/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[0]).to.have.property('id', comments[1].id); + }); + }); + + 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') From 833094b6dddb1210604a074465c335ea290adbfc Mon Sep 17 00:00:00 2001 From: riley Date: Thu, 15 Dec 2016 15:53:47 -0700 Subject: [PATCH 10/11] check the author_id instead of comment.id --- tests/routes/api/comments/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/routes/api/comments/index.js b/tests/routes/api/comments/index.js index f07610bbd..8e5632d26 100644 --- a/tests/routes/api/comments/index.js +++ b/tests/routes/api/comments/index.js @@ -92,7 +92,7 @@ describe('/api/v1/comments', () => { .then(res => { expect(res).to.have.status(200); expect(res.body.comments).to.have.length(2); - expect(res.body.comments[0]).to.have.property('id', comments[1].id); + expect(res.body.comments[0]).to.have.property('author_id', '456'); }); }); From 64ac1610ebf7da3589b2fd76a736d99ec0e48c9a Mon Sep 17 00:00:00 2001 From: Riley Davis Date: Thu, 15 Dec 2016 15:58:56 -0700 Subject: [PATCH 11/11] address different comment in result --- tests/routes/api/comments/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/routes/api/comments/index.js b/tests/routes/api/comments/index.js index 8e5632d26..1d7ad5321 100644 --- a/tests/routes/api/comments/index.js +++ b/tests/routes/api/comments/index.js @@ -92,7 +92,7 @@ describe('/api/v1/comments', () => { .then(res => { expect(res).to.have.status(200); expect(res.body.comments).to.have.length(2); - expect(res.body.comments[0]).to.have.property('author_id', '456'); + expect(res.body.comments[1]).to.have.property('author_id', '456'); }); });