Commit graph

127 commits

Author SHA1 Message Date
Daimona Eaytoy 0834f37e42 Fix some edge cases in ViewEdit
Follow-up Iabd0ae5b18571f8cad44ef2d86bcf2519e7f95ba.

This patch:
 - Moves some save-related code to a separate method
 - Reduces conditionals nesting
 - Fixes an edge case where the content of the form would be
 wiped in case the token didn't match.
 - Adds another (basic) selenium test
 - Standardizes return types
 - Moves data load outside of buildFilterEditor

Change-Id: I89444b59f04c495c9ab59244151c8ed5d38cf0fe
2020-02-08 15:35:46 +01:00
jenkins-bot 430058f2c0 Merge "Avoid keeping superfluous row properties" 2020-02-07 21:26:34 +00:00
jenkins-bot 02cd866f53 Merge "Refactor data load in ViewEdit" 2020-02-07 21:26:32 +00:00
libraryupgrader a14ec744f7 build: Updating composer dependencies
* mediawiki/minus-x: 0.3.2 → 1.0.0
* mediawiki/mediawiki-phan-config: 0.9.0 → 0.9.1

Change-Id: I119f4d56cce674302f34e938e598e6cc6bf28dc0
2020-01-28 17:51:38 +00:00
Daimona Eaytoy 102789f62a Avoid keeping superfluous row properties
Most of them are overwritten either in ViewEdit::loadRequest or
AbuseFilter::saveFilter. af_hit_count and af_throttled are actually
relevant for the old version, so list them explicitly. And also add
default af_group and af_global, which are later read, for import action.

Depends-On: Iabd0ae5b18571f8cad44ef2d86bcf2519e7f95ba
Change-Id: Ie9aae938cca06e38a7a834a3f74f3e8735ab01ee
2020-01-23 12:50:03 +00:00
Daimona Eaytoy 53b9f38888 Refactor data load in ViewEdit
Instead of having a single loadRequest method (which could end up
loading from the DB...), split it in a DB-only method and a request-only
one. Simplify the logic used to show the filter editor. Show the page
without changes or warnings if the user lost editing rights in the
meanwhile. Avoid two static properties, and pass them in when relevant
instead. Bonus: optimize a query to sort by afh_id instead of afh_timestamp to avoid filesort.

This will allow a subsequent patch to clean the $row object in
loadRequest.

Change-Id: Iabd0ae5b18571f8cad44ef2d86bcf2519e7f95ba
2020-01-21 14:15:41 +01:00
libraryupgrader 1d911b8187 build: Updating dependencies
composer:
* mediawiki/mediawiki-codesniffer: 28.0.0 → 29.0.0
  The following sniffs are failing and were disabled:
  * MediaWiki.Commenting.FunctionComment.MissingParamTag
  * MediaWiki.Commenting.FunctionComment.ParamNameNoMatch

npm:
* eslint-config-wikimedia: 0.13.1 → 0.15.0
* grunt-stylelint: 0.11.1 → 0.13.0
* stylelint-config-wikimedia: 0.6.0 → 0.8.0

Additional changes:
* Remove direct "stylelint" dependency in favor of "grunt-stylelint".
* Also sorted "composer fix" command to run phpcbf last.
* Removing manual reportUnusedDisableDirectives for eslint.

Change-Id: I8f73202db1333fbc36ccf556b3bb05b1e8c279cb
2020-01-21 07:38:54 +00:00
Daimona Eaytoy 87459ec679 build: Upgrade phan
Depends-On: I6d538ce3ca7fd2d495c2bafbab7cc279da69db1c
Change-Id: Ic8c3a01a5c37fdf461f4fd5598e597eb9c9073d3
2020-01-19 18:48:51 +00:00
Daimona Eaytoy b814c0827a i18n: Rename msg key for abusefilter-view-oldwarning
Thinking about it again, all messages on ViewEdit start with
abusefilter-edit. Also add a reference to the other message to
facilitate translations.

Follow-up: I3717d06d4a757684fe6622961391ae06b5bd3c38
Bug: T235590
Change-Id: I4cbaa2e92d22296f55a4b5ef0c633fe959fe9ea3
2019-12-16 10:56:30 +00:00
Martin Urbanec 5fd861365f SECURITY: Make sure provided filter id match provided history ID in history view
AbuseFilterViewEdit does privilege checks based on filter ID,
and displays what is hidden under given history ID, but doesn't
make sure those two IDs actually belong to one filter.

That means user can easily change filter ID to a public
filter and view old versions of nowadays private filters.

Bug: T237887
Change-Id: Ic12790bd33982473f77551bde9599ed083a3e1f1
2019-11-14 15:53:14 -06:00
DannyS712 338341d097 When viewing old filter revisions, show abusefilter-view-oldwarning to users who cannot edit the filter
Currently, `abusefilter-edit-oldwarning` is shown to all users, but not all users are able to edit the filters, and thus the warning about editing isn't applicable to them.

Bug: T235590
Change-Id: I3717d06d4a757684fe6622961391ae06b5bd3c38
2019-11-12 11:36:44 +00:00
Petr Pchelko 915b9a1538 Remove usages of deprecated User methods
Bug: T220191
Change-Id: I54e20870a32ff98b41a98495694ff563c4c4c5ca
2019-10-30 12:51:01 +00:00
Daimona Eaytoy 3a9eac9ad5 Unbreak filter edit form
In Ib7427e15f673a575738489476e604c387f449ddd, I thought that $parameters could've only been null if $action wasn't
enabled, but actually, they're null even if the action is just not set.
Which is true for all actions when creating a new filter, and all
non-set actions when editing an existing one.

Hence, revert the part that touched ViewEdit.

Also add a selenium test to ensure that warn parameters are visible.

Bug: T236286
Change-Id: I8150baa077208eb1fc54ebc1d8415a243d0f3bd3
2019-10-23 18:50:44 +02:00
Daimona Eaytoy b9e4475985 build: Upgrade mediawiki-phan-config to 0.8.0
This is to verify that our CI is able to handle the new version.

Bug: T235049
Change-Id: Ib7427e15f673a575738489476e604c387f449ddd
2019-10-09 19:12:51 +02:00
DannyS712 6699237b86 Show link to test filter to all users who can use it
Bug: T232962
Change-Id: I67357975a7064991e47c60b70c487d12bdf6b7b4
2019-09-15 22:03:56 +00:00
Daimona Eaytoy 15e9f34115 Fix filter validation in ViewEdit
Currently it's impossible to create new filters!

Bug: T231985
Change-Id: I92a7739fe9defae6b3d74381792340c7125d9086
2019-09-04 14:04:45 +02:00
jenkins-bot 2ed5be29e1 Merge "Use permissions accessors" 2019-09-02 10:55:46 +00:00
jenkins-bot fc3349df1f Merge "Fix param validation in ViewEdit" 2019-08-30 13:37:21 +00:00
Daimona Eaytoy 933b791ef3 Fix param validation in ViewEdit
We didn't check if the provided ID was valid. While editing an existing
filter (or creating a new one), we check the ID in SpecialAbuseFilter,
so it's guaranteed to get an integer in ViewEdit, and the case of a
non-existing filter is handled later, in buildFilterEditor.
But for links like Special:AbuseFilter/history/foobarbaz/item/1 (where
"foobarbaz" should be the filter ID), no validation was performed. This
caused a useless query to be carried out on the abuse_filter_history table (which would likely return false), then accessing properties of a non-object ('$row->afh_id'), and we ended up showing filter 1. This was spotted because we actually got notices in production.

Bug: T231632
Change-Id: I6436c7d2df8c1f0fc971f4a4079dac9118aa8209
2019-08-30 08:59:49 +00:00
Daimona Eaytoy 8e166f10d6 Refactor and speed up non-parser tests
Some of these are transformed into real unit tests, while the
AbuseFilterSaveTest class is refactored to avoid using the DB and to use
a lot more of mocks and DI.

Depends-On: I22743557e162fd23b3b4e52951a649d8c21109c8
Change-Id: Id8412e2b8a4e873fd4821ecc1a3c95710be9a870
2019-08-27 16:24:27 +00:00
Daimona Eaytoy 87713008d5 Use permissions accessors
There are lots of calls to $user->isAllowed which could be simplified
using available accessors like canEdit(). So simplify those calls and
avoid duplication.

Note that using canEdit also fixes a bug which affected blocked users:
we used to show e.g. the import link, and not to display as disabled
several text fields, while blocked users cannot actually edit filters.

Depends-On: I22743557e162fd23b3b4e52951a649d8c21109c8
Change-Id: I62779e940949ef49018a9c6d901bb6e10aa81da8
2019-08-27 13:21:55 +02:00
Daimona Eaytoy c469fb4b76 Mostly remove $wgUser
There are lots of cases where we can inject a User object without
additional efforts. Now $wgUser is only used inside AFComputedVariable,
which is a little bit harder to handle because some instances of that
class are serialized in the DB, and thus we cannot easily change the
constructor until T213006 is resolved.

This partly copies what Ia474f02dfeee8c7d067ee7e555c08cbfef08f6a6 tried
to do, but adopting a different approach for various can*() methods:
they're now static methods in the AbuseFilter class, so future callers
don't need to instantiate an AbuseFilterView class. This also allows to
re-use those methods in an API module for editing filters (T213037).

Bug: T213037
Bug: T159299
Change-Id: I22743557e162fd23b3b4e52951a649d8c21109c8
2019-08-27 13:20:37 +02:00
Daimona Eaytoy bf61414f88 Don't show empty "Tools:" section in ViewEdit
After having removed the export link in
I72f46247f4323fb5bfe7fa74f332076dbd346187, we don't have any tool to
show for new filters. So avoid outputting an empty section.

Change-Id: Ia07bccdbadb7b874397135bc3f7468d6e0b9eb13
2019-08-21 17:32:43 +02:00
jenkins-bot 8527a10774 Merge "Restyle edit box dimensions" 2019-08-20 16:33:16 +00:00
Daimona Eaytoy e4b1df1521 Fix object to array cast, typehint array params
This was broken in I34c040dbeb3ab01158fb3db22496def6ccaf72d9. I thought
the members of that object were always arrays, but I was wrong.
Plus typehint a few array parameters and make a couple of methods
private since they're only used in this class.

Bug: T230639
Change-Id: I0c51359769c4b3054f95755a96e7e0a2d8e5bf15
2019-08-17 17:04:34 +00:00
Daimona Eaytoy b235e1040a Restyle edit box dimensions
Now it's always wider, and so is the "notes" field. Moreover, the
fallback textarea has the exact same size. Plus removed a parameter
which only made it hard to write a CSS rule for the textarea. Since the
textarea is generated by the same code, and we're always using it for
the same thing (filter syntax, regardless of the final goal), make it
always use the same name.

Bug: T230591
Change-Id: Ibb308e80d954c0e81aa09249c38c39572f157948
2019-08-17 18:53:13 +02:00
Daimona Eaytoy c8b5b9321c Don't show export link for new filters
Bug: T230163
Change-Id: I72f46247f4323fb5bfe7fa74f332076dbd346187
2019-08-11 12:21:36 +02:00
Daimona Eaytoy c8ebb4956c Hide profiling for disabled filters
As data could be "old" and it may have no meaning.
Also remove a superfluous isset(), as $row->af_hidden is always set.

Depends-On: I2d8c8f8278073a9420e3eb373fb89a655925618a
Change-Id: I072363706c61f272c4c3691de4078e2a19148424
2019-08-03 23:28:42 +00:00
Daimona Eaytoy 0b7902fe6e Move per-filter matches profiling to per-filter data
They're currently stored separately, so move matches count together with
other per-filter data to keep it consistent. This also removes a
parameter from filterMatchesKey, as it's not needed anymore.
Split from child patch by Dragons flight.

Bug: T53294
Depends-On: I8f47beb73cfc1b63c4b3c809fc6d65a1e66ee334
Change-Id: I2d8c8f8278073a9420e3eb373fb89a655925618a
2019-08-03 23:22:20 +00:00
jenkins-bot 9650f11d16 Merge "Fix error display on ViewEdit" 2019-07-09 09:02:12 +00:00
Daimona Eaytoy 5eff8f73b0 Fix error display on ViewEdit
This changes the buildFilterEditor function to be protected and to
behave consistently: so, instead of adding stuff to OutputPage inside it
and also returning other stuff to be added by the caller, the function
now adds everything itself.
Also, the message "you're editing an old version of the filter" is now
shown only if the user can see the filter.

Change-Id: I1f40af41c5de0f63aa6210a261928892da0b3f69
2019-07-08 16:11:33 +02:00
Daimona Eaytoy d8d4750e6a Simplify action arrays
The current form is awkward. They're all like
[ actionname => [ 'action' => actionname, 'parameters' => params ] ]
This is greatly confusing since adds a nesting level, and just
duplicates the actionname information (also, we actually never retrieve
it from the internal array). Instead, change all of them to be
[ actionname => params ]
which is a lot shorter and clearer (and easier to handle).
A similar case is handled in I8134ecc41fbecdbed99faf406e9e3ca91b6123b9
(see PS 8..10).

Change-Id: I34c040dbeb3ab01158fb3db22496def6ccaf72d9
2019-07-05 10:00:48 +02:00
Daimona Eaytoy 6ea767f171 Tweak methods related to global filters
To make the switch to afl_filter_id and afl_global easier.

Bug: T227095
Depends-On: Ie550889495232b534c0f9aec31039cf21b2135b1
Change-Id: If557bad8f5c1a6d15e3556e4bfbd0330d7d49c59
2019-07-02 17:02:50 +02:00
jenkins-bot 99e821b125 Merge "Upgrade PHPCS and phan" 2019-05-15 19:42:42 +00:00
Daimona Eaytoy 311f68f6e8 Upgrade PHPCS and phan
Change-Id: Ibfb737e4552133d1ddd388e7410c6e1bc3a72e12
2019-05-15 19:06:04 +02:00
jenkins-bot 915bea466e Merge "Make use of PHP's list() feature where possible" 2019-05-15 15:06:20 +00:00
jenkins-bot c52850aae7 Merge "Add missing limits to explode() calls" 2019-05-15 15:06:18 +00:00
Thiemo Kreuz c6f20a64dd Add missing limits to explode() calls
This is fixing potential bugs where invalid strings with more than one
comma have silently been accepted.

Change-Id: Ib1e7d0c99973f243ef6faad6389bab688187c1cf
2019-05-15 16:14:12 +02:00
Thiemo Kreuz 3dece9ef8c Make use of PHP's list() feature where possible
Change-Id: If2b6c95a319800dd4944ecc0d7a8d3a819c846c1
2019-05-15 16:12:51 +02:00
Thiemo Kreuz fa3ce90851 Remove comments literally repeating what the code says
I find it obvious that a file called "AbuseFilterTokenizerTest" is a
"test for the AbuseFilterTokenizer class". A comment that is just
repeating this information is typicalls not helpful, but distracting
and a potential source of mistakes, e.g. when stuff is copy-pasted,
but the comment not adjusted.

Change-Id: I1d4cc06e9e5631955ff73bf675090cf9c33c9390
2019-05-15 16:04:32 +02:00
jenkins-bot 06cac3682e Merge "Replace deprecated cache-related functions" 2019-05-01 16:27:56 +00:00
jenkins-bot efe32b7c93 Merge "Add doc for every class member" 2019-04-06 14:37:19 +00:00
jenkins-bot e03488b66a Merge "Overhaul tag selector" 2019-04-06 12:35:20 +00:00
Daimona Eaytoy 5b4ea18045 Don't escape abusefilter-edit-status
This was sort of missed in Ia5c477edc8733bb1994cb6d01e1371ed496c8bcb.

Bug: T157235
Change-Id: Id952119a89df05a20c964eea8d4fe332c67f9086
2019-03-29 09:54:30 +01:00
Daimona Eaytoy 89520e2353 Remove $wgAbuseFilterProfiling
This variable was introduced to selectively enable profiling because
stats recording was bad for performance. Nowadays, stats are recorded in
a deferredupdate and don't harm performance anymore. Thus, this variable
can be removed and profiling be enabled by default.

Bug: T191039
Depends-On: Ib5fdeb75c1324f672b4ded39681f006fde34b4d1
Change-Id: Ia5c477edc8733bb1994cb6d01e1371ed496c8bcb
2019-03-23 11:31:11 +00:00
Daimona Eaytoy f2c1beec44 Replace double-equals with triple-equals
Since double-equals are evil. I left some of them in place where I
wasn't sure, but I may be changed some which were intended to be
doubles. It could be a good idea to delay merging this patch until we'll
have more code coverage.

Change-Id: I1721a3ba532d481e3ecf35f51099c1438b6b73b2
2019-03-22 16:12:13 +01:00
Daimona Eaytoy 553facee1e Move the throttle help tooltip to a message
Follow-up of I982d67aa62a899916a26452aceb9646df8c31232. The help text
was meant to be localized, and I probably forgot to do so in the
mentioned patch.

Change-Id: If394b02819911f9c97519b5c972977c38e6d83fa
2019-03-18 17:38:45 +01:00
Daimona Eaytoy d6c649bb0d Overhaul tag selector
If "tag" option is selected and the form is submitted without adding any
tag, just show it blank instead of adding an empty tag to the topbar.
Separately validate the empty tag case (and added a test for it).

Bug: T203353
Depends-On: I3b2e763bd8835207dc5df1db43d3e1881e6961c3
Change-Id: I8884b739fd17fa2eace5aac8775d3524aa606f1f
2019-03-17 14:04:50 +00:00
Daimona Eaytoy bedbe36744 Add doc for every class member
Adding PHPdocs to every class members, in every file. This patch only
touches comments, and moved properties on their own lines. Note that
some of these properties would need to be moved, somehow changed, or
just removed (either because they're old, unused leftovers, or just
because we can move them to local scope), but I wanted to keep this
patch doc-only.

Change-Id: I9fe701445bea8f09d82783789ff1ec537ac6704b
2019-03-17 11:40:24 +01:00
jenkins-bot 001a83272d Merge "Add help links for throttle groups" 2019-03-17 09:59:25 +00:00