Page MenuHomePhabricator

Newsletter strange behaviour on an API create/edit
Closed, ResolvedPublic

Description

Review of 631882e3779 of Newsletter extension (Jan 29, 2017)

Anyone with newsletter-create rights can edit a Newsletter namespace page (via api). While these changes aren't reflected in the relavent tables, they can make things inconsistent and confusing (e.g. Have the page display a different list of publishers than the actual list). Newsletter-manage permissions should apply to editing Newsletter pages via the api like it does during normal editing. The most obvious way to do this is with a getUserPermissionsErrors (or similar) hook. However, it may also be better to make the content handler page canonical and use it instead of the separate db tables where possible to eliminate the possibility of inconsistency in the system.

Details

Related Gerrit Patches:
mediawiki/extensions/Newsletter : masterSync DB tables manually on a newsletter edit over API

Event Timeline

Can reproduce it following the procedure:

  • User with newslettter-create and newsletter-manage right editing a Newsletter over API (using Sandbox API).
  • The edited content is reflected on the Newsletter page, but not on the database.

Change 346055 had a related patch set uploaded (by 01tonythomas):
[mediawiki/extensions/Newsletter@master] Sync DB tables manually on a newsletter edit over API

https://gerrit.wikimedia.org/r/346055

Qgil added a comment.Apr 20 2017, 8:59 AM

If I understood correctly, this is the very last bug that needs to be fixed in order to pass T115095: Security review of Newsletter extension, right?

I see a lot of activity at https://gerrit.wikimedia.org/r/346055. Go @01tonythomas Go!

@Bawolff: Sorry for the delay, but I think https://gerrit.wikimedia.org/r/#/c/346055 should be a good to go as of now. Awaiting your review :)

Update from @Bawolff : The query in https://gerrit.wikimedia.org/r/#/c/346055/24/includes/content/NewsletterDataUpdate.php 44-50 needs to be fixed to make it better. Right now it just picks up everything from the db.

01tonythomas closed this task as Resolved.May 19 2017, 5:45 PM
01tonythomas claimed this task.

Yay. We got it merged finally - Thanks to @Bawolff!!!!!!!!!

Change 346055 merged by jenkins-bot:
[mediawiki/extensions/Newsletter@master] Sync DB tables manually on a newsletter edit over API

https://gerrit.wikimedia.org/r/346055

Qgil awarded a token.May 22 2017, 4:40 PM