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.

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

mod_acl_user_groups: Fix non-admins denied permission to insert file #1665

Merged
merged 1 commit into from May 1, 2017

Conversation

Projects
None yet
4 participants
@ddeboer
Member

ddeboer commented Apr 20, 2017

Description

Fix driebit/ginger#283.

#1640 introduced a bug: non-admins no longer have permission to insert files. This is because Props can contain content_group_id (with value undefined) when coming from action_admin_dialog_media_upload, so proplists:lookup is not none.

Checklist

  • no BC breaks

@ddeboer ddeboer added this to the 0.28 milestone Apr 20, 2017

@ddeboer ddeboer requested review from mworrell and mmzeeman Apr 20, 2017

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Apr 20, 2017

@ddeboer, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mworrell, @arjan and @mmzeeman to be potential reviewers.

mention-bot commented Apr 20, 2017

@ddeboer, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mworrell, @arjan and @mmzeeman to be potential reviewers.

@mworrell

This comment has been minimized.

Show comment
Hide comment
@mworrell

mworrell Apr 21, 2017

Member

Shouldn't the undefined content-group be mapped to the default content group? Or is there some content group selection made later?

Member

mworrell commented Apr 21, 2017

Shouldn't the undefined content-group be mapped to the default content group? Or is there some content group selection made later?

@mmzeeman

This comment has been minimized.

Show comment
Hide comment
@mmzeeman

mmzeeman Apr 21, 2017

Member

Until somebody makes a content group named undefined ;-)

Member

mmzeeman commented Apr 21, 2017

Until somebody makes a content group named undefined ;-)

@mmzeeman

This comment has been minimized.

Show comment
Hide comment
@mmzeeman

mmzeeman Apr 21, 2017

Member

Shouldn't https://github.com/zotonic/zotonic/blob/master/modules/mod_admin/actions/action_admin_dialog_media_upload.erl#L111 be adapted in such a way that it actually adds a content_group_id, instead of inserting undefined?

Member

mmzeeman commented Apr 21, 2017

Shouldn't https://github.com/zotonic/zotonic/blob/master/modules/mod_admin/actions/action_admin_dialog_media_upload.erl#L111 be adapted in such a way that it actually adds a content_group_id, instead of inserting undefined?

@mworrell

This comment has been minimized.

Show comment
Hide comment
@mworrell

mworrell Apr 21, 2017

Member

Agree, we should be able to upload a file to a specified content group (aka with ACL specific settings)

Member

mworrell commented Apr 21, 2017

Agree, we should be able to upload a file to a specified content group (aka with ACL specific settings)

@mworrell

This comment has been minimized.

Show comment
Hide comment
@mworrell

mworrell Apr 21, 2017

Member

@mmzeeman the undefined content group is during reads mapped to the default content group. This is used for situations where content was created without the mod_content_groups enabled.

Member

mworrell commented Apr 21, 2017

@mmzeeman the undefined content group is during reads mapped to the default content group. This is used for situations where content was created without the mod_content_groups enabled.

@mmzeeman

This comment has been minimized.

Show comment
Hide comment
@mmzeeman

mmzeeman Apr 21, 2017

Member

Check, good to know.

We recently started using mod_content_groups, without mod_acl_user_groups, only recently.

Member

mmzeeman commented Apr 21, 2017

Check, good to know.

We recently started using mod_content_groups, without mod_acl_user_groups, only recently.

@ddeboer

This comment has been minimized.

Show comment
Hide comment
@ddeboer

ddeboer Apr 24, 2017

Member

So, to summarise, should acl_user_groups_checks or should it not support {content_group_id, undefined}? If it should not, I can change action_admin_dialog_media_upload to remove the content_group_id key in case no (or the default) content group applies.

Member

ddeboer commented Apr 24, 2017

So, to summarise, should acl_user_groups_checks or should it not support {content_group_id, undefined}? If it should not, I can change action_admin_dialog_media_upload to remove the content_group_id key in case no (or the default) content group applies.

@mworrell

This comment has been minimized.

Show comment
Hide comment
@mworrell

mworrell Apr 25, 2017

Member

I think content group id undefined should map to the default content group, as that is also the behavior when reading resources (it maps undefined to default content group).

So for the fix it would be better to just not pass the content group, or use an explicit content group.

Member

mworrell commented Apr 25, 2017

I think content group id undefined should map to the default content group, as that is also the behavior when reading resources (it maps undefined to default content group).

So for the fix it would be better to just not pass the content group, or use an explicit content group.

@ddeboer

This comment has been minimized.

Show comment
Hide comment
@ddeboer

ddeboer Apr 29, 2017

Member

Updated this PR: I replaced the changes in acl_user_groups_checks with a check around content_group_id in the media upload code.

Member

ddeboer commented Apr 29, 2017

Updated this PR: I replaced the changes in acl_user_groups_checks with a check around content_group_id in the media upload code.

@ddeboer ddeboer changed the title from mod_acl_user_groups: Fix non-admins denied permissions to insert file to mod_acl_user_groups: Fix non-admins denied permission to insert file Apr 29, 2017

@ddeboer ddeboer merged commit ac7094d into 0.x May 1, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ddeboer ddeboer deleted the fix-insert-media-non-admins branch May 1, 2017

ddeboer added a commit that referenced this pull request May 1, 2017

@mworrell

This comment has been minimized.

Show comment
Hide comment
@mworrell

mworrell May 1, 2017

Member

Good, then we can later (in the master) check how to set the content group in the dialogs for the media uploads. And what the handling of the undefined value should be.

Member

mworrell commented May 1, 2017

Good, then we can later (in the master) check how to set the content group in the dialogs for the media uploads. And what the handling of the undefined value should be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment