Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix blogging module post detail page permission check. #3099

Closed
wants to merge 8 commits into from
Closed

Fix blogging module post detail page permission check. #3099

wants to merge 8 commits into from

Conversation

gdlcf88
Copy link
Contributor

@gdlcf88 gdlcf88 commented Mar 12, 2020

Fix #3098

@gdlcf88 gdlcf88 changed the title Fix blogging post detail page Fix blogging module post detail page Mar 12, 2020
@maliming
Copy link
Member

maliming commented Mar 13, 2020 •
edited
Loading

hi @gdlcf88

Can you resolve the conflict?

Conflicting files

modules/blogging/src/Volo.Blogging.Web/Pages/Blog/Posts/Detail.cshtml

@maliming maliming added this to the 2.3 milestone Mar 13, 2020
@maliming maliming self-requested a review March 13, 2020 02:45
@gdlcf88
Copy link
Contributor Author

gdlcf88 commented Mar 13, 2020

I will do it.

@gdlcf88
Copy link
Contributor Author

gdlcf88 commented Mar 13, 2020

Done.

@gdlcf88 gdlcf88 changed the title Fix blogging module post detail page Fix blogging module post detail page permission check. Mar 13, 2020
maliming
maliming reviewed Mar 13, 2020
View reviewed changes
Copy link
Member

@maliming maliming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When hasCommentingPermission is false, a link in the page will redirect the user to the login page.
We should handle this situation. It may be not logged in or without permissions.

modules/blogging/src/Volo.Blogging.Web/Pages/Blogs/Posts/Detail.cshtml
Comment on lines +67 to 68
@if (await Authorization.IsGrantedAsync(BloggingPermissions.Posts.Delete))
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Users are not allowed to delete their own Posts?

abp/modules/blogging/src/Volo.Blogging.Application/Volo/Blogging/Posts/PostAuthorizationHandler.cs

Line 38 in b0adb9f

if (resource.CreatorId != null && resource.CreatorId == context.User.FindUserId())

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Users are not allowed to delete their own Posts?

abp/modules/blogging/src/Volo.Blogging.Application/Volo/Blogging/Posts/PostAuthorizationHandler.cs

Line 38 in b0adb9f

if (resource.CreatorId != null && resource.CreatorId == context.User.FindUserId())

Sorry, my branch missed this commit:
b0adb9f#diff-e461a1b98fc07088163606c19001917c

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can merge dev into your branch.

modules/blogging/src/Volo.Blogging.Web/Pages/Blogs/Posts/Detail.cshtml
@@ -57,14 +57,14 @@
<i class="fa fa-comment"></i> @L["CommentWithCount", @Model.CommentCount]


@if (await Authorization.IsGrantedAsync(BloggingPermissions.Posts.Update))
@if (await Authorization.IsGrantedAsync(BloggingPermissions.Posts.Update) || (CurrentUser.Id.HasValue && CurrentUser.Id == Model.Post.CreatorId))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(CurrentUser.Id.HasValue && CurrentUser.Id == Model.Post.CreatorId) can be CurrentUser.Id == Model.Post.CreatorId

modules/blogging/src/Volo.Blogging.Web/Pages/Blogs/Posts/Detail.cshtml
{
<span class="seperator">|</span>
<a href="#" class="tag deleteLink" data-deleteid="@commentWithRepliesDto.Comment.Id">
<i class="fa fa-trash" aria-hidden="true"></i> @L["Delete"]
</a>
}

@if (await Authorization.IsGrantedAsync(BloggingPermissions.Comments.Update) || (CurrentUser.Id == commentWithRepliesDto.Comment.CreatorId))
@if (await Authorization.IsGrantedAsync(BloggingPermissions.Comments.Update) || (CurrentUser.Id.HasValue && CurrentUser.Id == commentWithRepliesDto.Comment.CreatorId))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be CurrentUser.Id == commentWithRepliesDto.Comment.CreatorId

modules/blogging/src/Volo.Blogging.Web/Pages/Blogs/Posts/Detail.cshtml
@@ -200,7 +200,7 @@
</div>
</div>
}
@if (await Authorization.IsGrantedAsync(BloggingPermissions.Comments.Update) || (CurrentUser.Id == commentWithRepliesDto.Comment.CreatorId))
@if (await Authorization.IsGrantedAsync(BloggingPermissions.Comments.Update) || (CurrentUser.Id.HasValue && CurrentUser.Id == commentWithRepliesDto.Comment.CreatorId))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be CurrentUser.Id == commentWithRepliesDto.Comment.CreatorId

modules/blogging/src/Volo.Blogging.Web/Pages/Blogs/Posts/Detail.cshtml
{
<span class="seperator">|</span>
<a href="#" class="tag deleteLink" data-deleteid="@reply.Id">
<i class="fa fa-trash" aria-hidden="true"></i> @L["Delete"]
</a>
}

@if (await Authorization.IsGrantedAsync(BloggingPermissions.Comments.Update) || (CurrentUser.Id == commentWithRepliesDto.Comment.CreatorId))
@if (await Authorization.IsGrantedAsync(BloggingPermissions.Comments.Update) || (CurrentUser.Id.HasValue && CurrentUser.Id == commentWithRepliesDto.Comment.CreatorId))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be CurrentUser.Id == commentWithRepliesDto.Comment.CreatorId

modules/blogging/src/Volo.Blogging.Web/Pages/Blogs/Posts/Detail.cshtml
@@ -276,7 +276,7 @@
</div>
</div>
}
@if (await Authorization.IsGrantedAsync(BloggingPermissions.Comments.Update) || (CurrentUser.Id == commentWithRepliesDto.Comment.CreatorId))
@if (await Authorization.IsGrantedAsync(BloggingPermissions.Comments.Update) || (CurrentUser.Id.HasValue && CurrentUser.Id == commentWithRepliesDto.Comment.CreatorId))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be CurrentUser.Id == commentWithRepliesDto.Comment.CreatorId

yekalkan
yekalkan reviewed Mar 13, 2020
View reviewed changes
modules/blogging/src/Volo.Blogging.Web/Pages/Blogs/Posts/Detail.cshtml
@@ -238,15 +238,15 @@
<i class="fa fa-reply" aria-hidden="true"></i> @L["Reply"]
</a>
}
@if (await Authorization.IsGrantedAsync(BloggingPermissions.Comments.Delete) || (CurrentUser.Id == commentWithRepliesDto.Comment.CreatorId))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove CurrentUser.Id == commentWithRepliesDto.Comment.CreatorId?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to merge the latest branch since the creator check was added into AuthorizationHandler 4 days ago. 😅

yekalkan
yekalkan reviewed Mar 13, 2020
View reviewed changes
modules/blogging/src/Volo.Blogging.Web/Pages/Blogs/Posts/Detail.cshtml
@@ -160,15 +160,15 @@
</a>
}

@if (await Authorization.IsGrantedAsync(BloggingPermissions.Comments.Delete) || (CurrentUser.Id == commentWithRepliesDto.Comment.CreatorId))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove CurrentUser.Id == commentWithRepliesDto.Comment.CreatorId?

@gdlcf88 gdlcf88 closed this Mar 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maliming maliming maliming left review comments

@yekalkan yekalkan yekalkan left review comments

Assignees
No one assigned
Labels
abp-module-blogging enhancement
Projects
None yet
Milestone
2.3
Development

Successfully merging this pull request may close these issues.

Blogging post detail page may have wrong creator authority check.
3 participants
@gdlcf88 @maliming @yekalkan