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 the permissions of the contactsmanager #12369

Closed
wants to merge 1 commit into from
Closed

Fix the permissions of the contactsmanager #12369

wants to merge 1 commit into from

Conversation

LEDfan
Copy link
Member

@LEDfan LEDfan commented Nov 23, 2014

AFAIK the permissions are wrong, and the delete or createOrUpdate methods would never been called.

CC @DeepDiver1975

@scrutinizer-notifier
Copy link

The inspection completed: No new issues

@ghost
Copy link

ghost commented Nov 23, 2014

🚀 Test PASSed. 🚀
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/3091/
🚀 Test PASSed. 🚀

@DeepDiver1975
Copy link
Member

no - the old implementation is correct - only if the permissions returned by the address book contain the necessary permission the action is executed.

@LEDfan
Copy link
Member Author

LEDfan commented Nov 24, 2014

Mm okay.
If the getPermissions() function returns 32 (\OCP\PERMISSION_ALL), and you need PERMISSION_CREATE (4), then the method should be called right?

I have made a test: http://3v4l.org/B8HqW .

@DeepDiver1975
Copy link
Member

Bloody php ....

31 & 4 = 4

// but
PERMISSION_ALL & PERMISSION_CREATE = 0

@DeepDiver1975
Copy link
Member

@bantu

@bantu
Copy link

bantu commented Nov 24, 2014

@DeepDiver1975 Looking.

@bantu
Copy link

bantu commented Nov 24, 2014

@DeepDiver1975 Conceptionally, this patch looks correct.

$bitfield & $bitmask will answer whether the bit represented by $bitmask is set in $bitfield. The result is an integer, either 0 for no or $bitmask for yes. This can be implicitly cast to boolean via use in an if statement.

I have the feeling you're just seeing #6101 in action here.

@DeepDiver1975
Copy link
Member

@LEDfan sorry for the confusion - I mixed up the diff - yes your patch looks valid

Can I ask you to add a unit test? We don't want to run into this again - THX

@DeepDiver1975
Copy link
Member

@bantu thanks for having a look - regarding #6101 - a PR would be great 😉

@nickvergessen
Copy link
Contributor

PR has been merged: #12421 @LEDfan can you rebase this on master?

@nickvergessen nickvergessen added this to the 8.0-current milestone Nov 26, 2014
@LEDfan
Copy link
Member Author

LEDfan commented Dec 6, 2014

@nickvergessen I'll rebase. @DeepDiver1975 I'm going to write the unit tests :)

@LEDfan LEDfan closed this Dec 7, 2014
@lock lock bot locked as resolved and limited conversation to collaborators Aug 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants