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

Check addressbook permission before moving #297

Merged
merged 7 commits into from
Aug 25, 2017
Merged

Check addressbook permission before moving #297

merged 7 commits into from
Aug 25, 2017

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Aug 22, 2017 •
edited
Loading

Fix #174

Testing:

  1. Create an addressbook with another user in read only mode and share it with your current user.
  2. Try to move a contact to this addressbook.

@skjnldsv skjnldsv added 2. developing Work in progress bug Something isn't working high High priority labels Aug 22, 2017
@skjnldsv skjnldsv added this to the 2.0.0 milestone Aug 22, 2017
@skjnldsv skjnldsv self-assigned this Aug 22, 2017
@skjnldsv skjnldsv changed the title Fix correct addressbook owner Check addressbook permission before moving Aug 22, 2017
@skjnldsv skjnldsv force-pushed the check-if-move-to-ab-is-allowed branch from db6abc0 to 86a221b Compare August 22, 2017 16:11
@skjnldsv
Fix correct addressbook owner
8e2d679 
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv skjnldsv force-pushed the check-if-move-to-ab-is-allowed branch from 86a221b to 8e2d679 Compare August 22, 2017 16:12
@codecov
Copy link

codecov bot commented Aug 22, 2017 •
edited
Loading

Codecov Report

Merging #297 into master will decrease coverage by 0.08%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #297      +/-   ##
==========================================
- Coverage   14.82%   14.73%   -0.09%     
==========================================
  Files          55       55              
  Lines        1221     1228       +7     
==========================================
  Hits          181      181              
- Misses       1040     1047       +7
Impacted Files Coverage Δ
js/models/addressBook_model.js 4.34% <ø> (ø) ⬆️
js/services/contact_service.js 0.64% <0%> (-0.04%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42451b3...fdea685. Read the comment docs.

@skjnldsv
Cleanup
2481145 
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv skjnldsv force-pushed the check-if-move-to-ab-is-allowed branch 3 times, most recently from 60a7a56 to 69f8dbe Compare August 24, 2017 07:53
@skjnldsv
Readonly request and support
60cc140 
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv skjnldsv force-pushed the check-if-move-to-ab-is-allowed branch from 69f8dbe to 60cc140 Compare August 24, 2017 07:56
@skjnldsv
Permission denial
804c74a 
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Aug 24, 2017
jonatoni
jonatoni reviewed Aug 24, 2017
View reviewed changes
js/services/contact_service.js Outdated
@@ -251,6 +251,10 @@ angular.module('contactsApp')
if (contact.addressBookId === addressbook.displayName) {
return;
}
if(addressbook.readOnly) {
OC.Notification.showTemporary(t('contacts', 'You don\'t have permission to write to this addressbook.'));
Copy link
Member

Choose a reason for hiding this comment

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

It should be 'You don't have permission to write to this addressbook.'

Copy link
Member Author

Choose a reason for hiding this comment

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

That's exactly what it's written. 😁

Copy link
Member Author

Choose a reason for hiding this comment

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

If you're talking about the \' it's to escape the ' character since we're using it to define the string. :)

skjnldsv
skjnldsv commented Aug 24, 2017
View reviewed changes
js/dav/dav.js
@@ -1620,6 +1620,7 @@ var listAddressBooks = _co2['default'].wrap(regeneratorRuntime.mark(function cal
req = request.propfind({
props: [{ name: 'displayname', namespace: ns.DAV }, { name: 'owner', namespace: ns.DAV }, { name: 'getctag', namespace: ns.CALENDAR_SERVER }, { name: 'resourcetype', namespace: ns.DAV }, { name: 'sync-token', namespace: ns.DAV },
//{ name: 'groups', namespace: ns.OC },
{ name: 'read-only', namespace: ns.OC },
Copy link
Member Author

Choose a reason for hiding this comment

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

It's the only line I changed on this file, the rest is just Github failing to see I didn't changed anything else.... 😑

Copy link
Member

Choose a reason for hiding this comment

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

This is compiled code. I'm quite sure you shouldn't change it here, but in js/dav/lib/contacts.js and then run make in js/dav.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh you're right! :O

@skjnldsv
Notify on create too
d9e5ade 
In case if the default addressbook is in read only mode

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv
Fix variable declaration
87d4470 
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv
Break fix
fdea685 
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@irgendwie
Copy link
Member

Works and is ok for now - we need to push a new release 👍
For the future, I'd like to remove read-only addressbooks from the addressbook selection in the details and for contacts of such an addressbook a not editable field.

@irgendwie irgendwie merged commit a5e96ab into master Aug 25, 2017
@irgendwie irgendwie deleted the check-if-move-to-ab-is-allowed branch August 25, 2017 11:48
@skjnldsv
Copy link
Member Author

@irgendwie look #294 too :)

skjnldsv added a commit that referenced this pull request Aug 26, 2017
@skjnldsv
Fix unfinished edit for the dav lib, see #297
ec023ee 
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv skjnldsv mentioned this pull request Aug 26, 2017
Merged
skjnldsv added a commit that referenced this pull request Aug 26, 2017
@skjnldsv
Merge pull request #301 from nextcloud/fix-dav-lib-for-ab-permission
924aae1 
Fix unfinished edit for the dav lib, see #297
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonatoni jonatoni jonatoni left review comments

@irgendwie irgendwie irgendwie approved these changes

@jancborchardt jancborchardt Awaiting requested review from jancborchardt

@daniellandau daniellandau Awaiting requested review from daniellandau

@Henni Henni Awaiting requested review from Henni

@xh3n1 xh3n1 Awaiting requested review from xh3n1

@MorrisJobke MorrisJobke Awaiting requested review from MorrisJobke

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst

Assignees

@skjnldsv skjnldsv

Labels
3. to review Waiting for reviews bug Something isn't working high High priority
Projects
None yet
Milestone
2.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@skjnldsv @irgendwie @daniellandau @jonatoni