Skip to content
Permalink
Browse files Browse the repository at this point in the history
[#2052] Fix improper access control leads to admin self-banning
  • Loading branch information
julianguyen committed Nov 19, 2021
1 parent df4986f commit d1f570c
Show file tree
Hide file tree
Showing 8 changed files with 103 additions and 38 deletions.
4 changes: 3 additions & 1 deletion app/controllers/notifications_controller.rb
Expand Up @@ -47,7 +47,9 @@ def render_notification(notification)
uniqueid = notification[:uniqueid]
data = convert_to_hash(notification[:data])
# In case data[:name] is invalid
data[:email] = User.find(data[:user_id]).email
if data[:user_id]
data[:email] = User.find(data[:user_id]).email
end
case data[:type]
when /comment/ then comment_link(uniqueid, data)
when /accepted_ally_request/ then accepted_ally_link(uniqueid, data)
Expand Down
1 change: 1 addition & 0 deletions app/helpers/comments_helper.rb
Expand Up @@ -42,6 +42,7 @@ def comment_hash(comment, user)
commentByUid: user.uid,
commentByName: user.name,
commentByAvatar: user.avatar.url,
commentByAdmin: user.admin?,
comment: sanitize(comment.comment),
viewers: CommentViewersService.viewers(comment, current_user),
createdAt: created_at(comment.created_at),
Expand Down
8 changes: 5 additions & 3 deletions app/views/allies/index.html.erb
Expand Up @@ -56,9 +56,11 @@
<div>
<%= link_to t('common.actions.remove'), remove_allies_path(ally_id: ally.id), method: :post, data: { confirm: t('common.actions.confirm') } %>
</div>
<div>
<%= link_to t('common.actions.report'), new_report_path(uid: ally.uid) %>
</div>
<% if !ally.admin? && ally != current_user %>
<div>
<%= link_to t('common.actions.report'), new_report_path(uid: ally.uid) %>
</div>
<% end %>
</div>
<% end %>
</div>
Expand Down
10 changes: 6 additions & 4 deletions app/views/profile/index.html.erb
Expand Up @@ -26,9 +26,11 @@
<div>
<%= link_to t('common.actions.remove'), remove_allies_path(ally_id: @profile.id), method: :post, data: { confirm: t('common.actions.confirm') } %>
</div>
<div>
<%= link_to t('common.actions.report'), new_report_path(uid: @profile.uid) %>
</div>
<% if !@profile.admin? && @profile != current_user %>
<div>
<%= link_to t('common.actions.report'), new_report_path(uid: @profile.uid) %>
</div>
<% end %>
<% elsif current_user.allies_by_status(:pending_from_user).include? @profile %>
<div>
<%= link_to t('allies.accept'), add_allies_path(ally_id: @profile.id), method: :post %> | <%= link_to t('allies.reject'), remove_allies_path(ally_id: @profile.id), method: :post, data: { confirm: t('common.actions.confirm') } %>
Expand All @@ -39,7 +41,7 @@
</div>
<% end %>
<% end %>
<% if current_user.admin? %>
<% if @profile != current_user && current_user.admin? %>
<% if @profile.banned? %>
<div>
<%= link_to t('reports.remove_ban'), remove_ban_profile_index_path(user_id: @profile.id), method: :post %>
Expand Down
8 changes: 5 additions & 3 deletions app/views/search/index.html.erb
Expand Up @@ -15,9 +15,11 @@
<div>
<%= link_to t('common.actions.remove'), remove_allies_path(ally_id: user.id), method: :post, data: { confirm: t('common.actions.confirm') } %>
</div>
<div>
<%= link_to t('common.actions.report'), new_report_path(uid: user.uid) %>
</div>
<% if !user.admin? && user != current_user %>
<div>
<%= link_to t('common.actions.report'), new_report_path(uid: user.uid) %>
</div>
<% end %>
<% else %>
<%= link_to t('allies.add_ally'), add_allies_path(ally_id: user.id), method: :post %>
<% end %>
Expand Down
94 changes: 68 additions & 26 deletions client/app/widgets/Comments/__tests__/Comments.spec.jsx
Expand Up @@ -64,50 +64,92 @@ const value = 'Hey';

const id = 1;

const comment = {
const getComment = ({ commentByAdmin } = {}) => ({
comment: value,
currentUserId: 'some-uid',
currentUserUid: 'some-uid',
commentByAvatar: null,
commentByAdmin: Boolean(commentByAdmin),
commentByName: 'Kind Human',
commentByUid: 'uid',
createdAt: 'Created less than a minute ago',
deleteAction: '/comments/delete?comment_id=1',
id,
viewers: 'Visible only between you and Lane Kim',
};

const component = <Comments formProps={formProps} />;
});

describe('Comments', () => {
beforeEach(() => {
jest.spyOn(axios, 'post').mockResolvedValue({
data: { comment },
describe('when written by a non-admin user', () => {
it('renders correctly with a report link', () => {
render(<Comments formProps={formProps} comments={[getComment()]} />);
expect(screen.getByRole('textbox')).toBeInTheDocument();
expect(screen.getByRole('article')).toBeInTheDocument();
expect(
screen.getByRole('button', { name: 'Submit' }),
).toBeInTheDocument();
expect(screen.getByRole('link', { name: 'Report' })).toBeInTheDocument();
});
jest.spyOn(axios, 'delete').mockResolvedValue({
data: { id },

it('add and delete a comment', async () => {
jest.spyOn(axios, 'post').mockResolvedValue({
data: { comment: getComment() },
});
jest.spyOn(axios, 'delete').mockResolvedValue({
data: { id },
});
render(<Comments formProps={formProps} />);
expect(screen.queryByRole('article')).not.toBeInTheDocument();

userEvent.type(screen.getByRole('textbox'));
userEvent.selectOptions(screen.getByRole('combobox'), 'private');
userEvent.click(screen.getByRole('button', { name: 'Submit' }));

await waitFor(() => expect(screen.getByRole('article')).toBeInTheDocument());
expect(screen.getByRole('article')).toHaveTextContent('Hey');

userEvent.click(screen.getByRole('link', { name: 'Delete' }));

await waitFor(() => expect(screen.queryByRole('article')).not.toBeInTheDocument());
});
});

it('renders correctly', () => {
render(component);
expect(screen.getByRole('textbox')).toBeInTheDocument();
expect(screen.queryByRole('article')).not.toBeInTheDocument();
expect(screen.getByRole('button', { name: 'Submit' })).toBeInTheDocument();
});
describe('when written by an admin user', () => {
it('renders correctly without a report link', () => {
render(
<Comments
formProps={formProps}
comments={[getComment({ commentByAdmin: true })]}
/>,
);
expect(screen.getByRole('textbox')).toBeInTheDocument();
expect(screen.getByRole('article')).toBeInTheDocument();
expect(
screen.getByRole('button', { name: 'Submit' }),
).toBeInTheDocument();
expect(
screen.queryByRole('link', { name: 'Report' }),
).not.toBeInTheDocument();
});

it('add and delete a comment', async () => {
render(component);
expect(screen.queryByRole('article')).not.toBeInTheDocument();
it('add and delete a comment', async () => {
jest.spyOn(axios, 'post').mockResolvedValue({
data: { comment: getComment({ commentByAdmin: true }) },
});
jest.spyOn(axios, 'delete').mockResolvedValue({
data: { id },
});
render(<Comments formProps={formProps} />);
expect(screen.queryByRole('article')).not.toBeInTheDocument();

userEvent.type(screen.getByRole('textbox'));
userEvent.selectOptions(screen.getByRole('combobox'), 'private');
userEvent.click(screen.getByRole('button', { name: 'Submit' }));
userEvent.type(screen.getByRole('textbox'));
userEvent.selectOptions(screen.getByRole('combobox'), 'private');
userEvent.click(screen.getByRole('button', { name: 'Submit' }));

await waitFor(() => expect(screen.getByRole('article')).toBeInTheDocument());
expect(screen.getByRole('article')).toHaveTextContent('Hey');
await waitFor(() => expect(screen.getByRole('article')).toBeInTheDocument());
expect(screen.getByRole('article')).toHaveTextContent('Hey');

userEvent.click(screen.getByRole('link', { name: 'Delete' }));
userEvent.click(screen.getByRole('link', { name: 'Delete' }));

await waitFor(() => expect(screen.queryByRole('article')).not.toBeInTheDocument());
await waitFor(() => expect(screen.queryByRole('article')).not.toBeInTheDocument());
});
});
});
6 changes: 5 additions & 1 deletion client/app/widgets/Comments/index.jsx
Expand Up @@ -24,6 +24,7 @@ type Comment = {
commentByUid: string,
commentByName: string,
commentByAvatar?: string,
commentByAdmin: Boolean,
comment: any,
viewers?: string,
createdAt: string,
Expand Down Expand Up @@ -68,9 +69,10 @@ export const Comments = ({ comments, formProps }: Props) => {
currentUserUid: string,
uid: string,
id: number,
commentByAdmin: Boolean,
) => {
const actions = {};
if (currentUserUid !== uid) {
if (currentUserUid !== uid && !commentByAdmin) {
actions.report = reportAction(uid, id);
}
if (viewers) {
Expand All @@ -94,6 +96,7 @@ export const Comments = ({ comments, formProps }: Props) => {
commentByUid,
commentByName,
commentByAvatar,
commentByAdmin,
comment,
viewers,
createdAt,
Expand All @@ -113,6 +116,7 @@ export const Comments = ({ comments, formProps }: Props) => {
currentUserUid,
commentByUid,
id,
commentByAdmin,
)}
hasStory
/>
Expand Down
10 changes: 10 additions & 0 deletions spec/helpers/comments_helper_spec.rb
Expand Up @@ -32,6 +32,7 @@
commentByUid: user1.uid,
commentByName: user1.name,
commentByAvatar: user1.avatar.url,
commentByAdmin: false,
comment: comment,
createdAt: created_at,
viewers: nil,
Expand All @@ -47,6 +48,7 @@
commentByUid: user1.uid,
commentByName: user1.name,
commentByAvatar: user1.avatar.url,
commentByAdmin: false,
comment: comment,
createdAt: created_at,
viewers: "Visible only between you and #{user2.name}",
Expand All @@ -68,6 +70,7 @@
commentByUid: user2.uid,
commentByName: user2.name,
commentByAvatar: user2.avatar.url,
commentByAdmin: false,
comment: comment,
createdAt: created_at,
viewers: nil,
Expand All @@ -83,6 +86,7 @@
commentByUid: user2.uid,
commentByName: user2.name,
commentByAvatar: user2.avatar.url,
commentByAdmin: false,
comment: comment,
createdAt: created_at,
viewers: "Visible only between you and #{user1.name}",
Expand All @@ -108,6 +112,7 @@
commentByUid: user1.uid,
commentByName: user1.name,
commentByAvatar: user1.avatar.url,
commentByAdmin: false,
comment: comment,
createdAt: created_at,
viewers: nil,
Expand All @@ -123,6 +128,7 @@
commentByUid: user1.uid,
commentByName: user1.name,
commentByAvatar: user1.avatar.url,
commentByAdmin: false,
comment: comment,
createdAt: created_at,
viewers: "Visible only between you and #{user2.name}",
Expand All @@ -144,6 +150,7 @@
commentByUid: user2.uid,
commentByName: user2.name,
commentByAvatar: user2.avatar.url,
commentByAdmin: false,
comment: comment,
createdAt: created_at,
viewers: nil,
Expand All @@ -159,6 +166,7 @@
commentByUid: user2.uid,
commentByName: user2.name,
commentByAvatar: user2.avatar.url,
commentByAdmin: false,
comment: comment,
createdAt: created_at,
viewers: "Visible only between you and #{user1.name}",
Expand Down Expand Up @@ -186,6 +194,7 @@
commentByUid: user1.uid,
commentByName: user1.name,
commentByAvatar: user1.avatar.url,
commentByAdmin: false,
comment: comment,
createdAt: created_at,
viewers: nil,
Expand All @@ -204,6 +213,7 @@
commentByUid: user2.uid,
commentByName: user2.name,
commentByAvatar: user2.avatar.url,
commentByAdmin: false,
comment: comment,
createdAt: created_at,
viewers: nil,
Expand Down

0 comments on commit d1f570c

Please sign in to comment.