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.

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

Fix for #8076 #7945 and #5558 #8169

Closed
wants to merge 5 commits into from
Closed

Fix for #8076 #7945 and #5558 #8169

wants to merge 5 commits into from

Conversation

Joly0
Copy link
Contributor

@Joly0 Joly0 commented Jun 23, 2020

Fixes #8076 , #7945 and #5558

@Joly0 Joly0 requested a review from snipe as a code owner June 23, 2020 13:29
@Joly0 Joly0 changed the title Fix for #8076 Fix for #8076 #7945 and 5558 Jun 23, 2020
@Joly0 Joly0 changed the title Fix for #8076 #7945 and 5558 Fix for #8076 #7945 and #5558 Jun 23, 2020


$permissions = config('permissions');
$permissions_array = Helper::selectedPermissionsArray($permissions, Input::old('permissions', array()));
Copy link
Owner

Choose a reason for hiding this comment

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

We should be using the old() helper here, instead of the Input:: facade.

Also where is this old data coming from? This is a console command, so there isn't any request data being passed.

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 have literally no knowledge of PHP, although i still understand what is happening in the code and what you mean. I just had this problem and i tried to fix it by looking up, how a user was normally created and from there it was just copy-paste with some testing until i found this to be working. But your totally right, that the old input is not needed. I even think, that the "use Input;" line is unnecessary then aswell.

@ghost
Copy link

ghost commented Oct 4, 2021

Hey @Joly0 - after reading the code and the permissions array (and the groups()->sync() BelongsToMany relationship docs), this fix seems to make sense.

It's creating a populated permissions array, with the default of no permissions active (which seems the safest approach), and then stores that on the User.

Do you know if this could/should be reopened or merged to fix the issues you mention?

@Joly0
Copy link
Contributor Author

Joly0 commented Oct 4, 2021

Hey @Joly0 - after reading the code and the permissions array (and the groups()->sync() BelongsToMany relationship docs), this fix seems to make sense.

It's creating a populated permissions array, with the default of no permissions active (which seems the safest approach), and then stores that on the User.

Do you know if this could/should be reopened or merged to fix the issues you mention?

As far as i can tell, this problem does no longer occur in v5 and higher, so opening a pr or reopening the old one wouldnt make any sense, as the codebase for v4 doesnt exist anymore. Though the fix is just a few lines of code most people should be able to implement. If there is any problem, i can write a short guide which files beed to be edited

@ghost
Copy link

ghost commented Oct 4, 2021

Thanks @Joly0! I think we should be OK without the patch and guide, in that case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LDAP Sync users with bulk edit permissions have no effective permissions upon first login
2 participants