Commit graph

1060 commits

Author SHA1 Message Date
Daimona Eaytoy b309c804fc Add dedicated classes for more hook handlers
The schema changes hook was chosen because the handler is very long. The
test ones were chosen to keep test things away from actual code.

Bug: T261067
Change-Id: Ie06bf62399f6353e3e268cccb3fe4b41bbf951c5
2020-10-22 18:23:09 +02:00
Daimona Eaytoy 4c06dd52c8 Replace $wgAbuseFilterRestrictions with more specific variables
So that sysadmins can further customize the extension. It was also wrong
to use the same variable for many different things.

Note that there's no associated patch in wmf-config because we use the
defaults. However, before merging this patch, please recheck that
AbuseFilterRestrictions and AbuseFilterDisallowGlobalLocalBlocks aren't
used there (https://codesearch.wmflabs.org/operations/?q=AbuseFilterDisallowGlobalLocalBlocks%7CAbuseFilterRestrictions&i=nope&files=&repos=)

Bug: T175221
Change-Id: I7581b3ee6d9d11a6cf1599b8ff874e8c3d54adf4
2020-10-22 13:38:59 +00:00
jenkins-bot 1c10edb80f Merge "Migrate change tags hooks to DI" 2020-10-21 18:04:20 +00:00
jenkins-bot 1c1b40f322 Merge "Inject ChangeTagsManager to ChangeTagger" 2020-10-21 17:21:23 +00:00
jenkins-bot c7e1d11c74 Merge "Add ChangeTagsManager service" 2020-10-21 17:15:40 +00:00
jenkins-bot c865e210de Merge "Simplify ViewEdit::loadRequest" 2020-10-21 16:39:18 +00:00
Matěj Suchánek 2ee3a0d247 Migrate change tags hooks to DI
Bug: T261067
Change-Id: I7b95cd19ab0ae04820e8dcb3481d29a2f9e7a0ca
2020-10-21 16:18:06 +00:00
Matěj Suchánek 93556284a0 Inject ChangeTagsManager to ChangeTagger
We decided to have the tag name provided by ChangeTagsManager,
so make ChangeTagger depend on it.

Change-Id: If3cbfd992f45651f47477031befffc0fd30f4a28
2020-10-21 16:30:43 +02:00
Matěj Suchánek 85e000c6ed Add ChangeTagsManager service
This service will be resposnsible for loading
and caching change tags used by abuse filters.

Change-Id: I9a710af1dd1ae58c47de1e8509246ed929d0a662
2020-10-21 16:24:32 +02:00
jenkins-bot f5950e638f Merge "Performance: don't check autopromotion if blockautopromote is disabled" 2020-10-21 13:20:13 +00:00
Daimona Eaytoy 7e44146781 Performance: don't check autopromotion if blockautopromote is disabled
This hook is called on every request, even for view actions, hence it's
a hot spot and a potential source of performance issues. We can slightly
optimize it by avoiding a cache lookup if blockautopromote is disabled.
Note: this won't really have an impact on WMF wikis since blockautopromote
is enabled almost everywhere.

Bug: T22487
Change-Id: I3743bfea9fe5865a3947cd23a07ae27e2dfa9301
2020-10-21 13:28:41 +02:00
Daimona Eaytoy 9bc885b6b3 Add a ChangeTagger class
The logic about action IDs and the persistent buffer is now encapsulated
inside a single service, which is a step towards getting rid of global
state in the AbuseFilter class, and reducing the responsibilities of the
Runner.

An important change made here is that we now require a LinkTarget rather
than a Title. This removes a dependency on the Title class (a monster
object), makes tests simpler, and denies the need to inject a
TitleFactory. This means living without some bits of context (e.g. we're
no longer using makeTitleSafe to ensure a valid title, and we have to
build a "prefixedtext" manually), but this shouldn't be a problem, given
that the titles are only used to create a cache key: invalid titles are
not a problem, and concatenating namespace + title should always be
sufficient.

Bug: T265370
Change-Id: Iff59cd3d889454a482a89c16691bfefcc5ec0a12
2020-10-21 13:19:30 +02:00
Daimona Eaytoy 215f16a177 Prevent uncaught warnings/exception on Special:AbuseFilter
This patch addresses two issues observed in WMF production:
 - Specifying a search mode without a search pattern would result in a
   call to mb_stripos (in AbuseFilterPager) with an empty delimiter,
   which triggers a PHP warning. Avoid this by checking that the search
   pattern is not the empty string, and unset the search mode if that's
   the case.
 - Trying to use an invalid search mode would result in an unhandled
   LogicException. We have some code in place to check the validity of
   the URL parameter, but the relevant code didn't reset the search mode
   to null, hence AbuseFilterPager would throw before we can show a
   pretty error to the user.

Bug: T265994
Change-Id: Ib19d36d6265981097bbb551783fdac8bdaa98854
2020-10-20 13:59:45 +02:00
jenkins-bot 3b59156b4c Merge "Minor updates related to var dumps" 2020-10-19 08:27:05 +00:00
jenkins-bot e002cbb4fa Merge "Exclude implicit groups when degrouping the user" 2020-10-19 07:56:41 +00:00
Daimona Eaytoy a330d0c454 Exclude implicit groups when degrouping the user
It doesn't make much sense to try to remove implicit groups like 'user'
and '*'. As a matter of fact, these groups are also excluded in
AbuseFilterViewRevert when undoing degroups.

Change-Id: I292499611ccfbd12df28b713d4244530db15c26d
2020-10-18 15:34:04 +02:00
Daimona Eaytoy 3a85e03c72 Simplify ViewEdit::loadRequest
This method was divided into multiple, shorter methods. We now have a
dedicated method for imports, and one for everything else, plus a method
for loading actions. Merged a conditional for when the token didn't
match. Avoid returning Status objects with data inside as it's too
difficult to properly infer types for those.

This is still not perfect, and another round of simplification might be
necessary before this class can be updated to use the upcoming Filter
value objects.

Change-Id: I2de1de1982105e5b9b817a893c357615ffb7db86
2020-10-18 11:06:30 +00:00
Daimona Eaytoy f589629b12 Avoid direct coupling between SpecialAbuseFilter and AbuseFilterView
While this might seem a small change, it removes the last remaining
coupling between SpecialAbuseFilter and the *View classes, that were
forming a huge tangle.

Change-Id: I5a9d6516e3fa2d3efc4bb2e19b05379dc33cd84d
2020-10-17 00:37:11 +02:00
jenkins-bot 94af753348 Merge "Use new services in AbuseFilterRunner" 2020-10-16 13:20:08 +00:00
jenkins-bot c094da9cec Merge "Simplify code for tagging the action on cache hit if the cond limit was hit" 2020-10-16 11:49:20 +00:00
Matěj Suchánek adbe9bcbce Improve display of log entries when global filters are not enabled
Don't create <a> tags without a href. Show a placeholder
message instead of nothing (alternatively, we could create
a new message for each existing one).

Bug: T174000
Change-Id: Id55b90881aacc620ff3c519ad6eedf212f36c4ed
2020-10-15 15:05:16 +02:00
Daimona Eaytoy 1efc324d97 Use new services in AbuseFilterRunner
The first one is UserGroupManager, used for the 'degroup' action. This
is a simple one-line replacement (repeated twice), and the current code
was already using this service under the hood.

The second one is BlockUser, which is not a one-line change (but still
quite simple). In particular, this allows us to avoid duplication with
core logic when constructing the log entry (this is now done by
BlockUser).

Bug: T248743
Change-Id: Ib7c1dc107a169b575f7021e64b6a8fee09529548
2020-10-14 23:08:32 +00:00
Daimona Eaytoy a7182acafd Simplify code for tagging the action on cache hit if the cond limit was hit
This code was simply caching the AbuseFilter::$tagsToSet property, but
this is not necessary. The only tag that can be buffered during edit
stashing is the conds limit tag. So we just save whether the conds limit
was hit, and apply the tag from a single point afterwards.

Also avoid checking whether 'tag' is enabled as an action, since this tag
should always be added when applicable.

Next step is creating some sort of Watcher service that will do
everything on its own: check whether the limit was hit, save this
information, and tag the action later.

Bug: T265370
Change-Id: I90319a658736fad7d564cb51152061709c230411
2020-10-13 16:05:18 +00:00
Daimona Eaytoy 45d80bc7e5 Clean up view classes
- Depend on a generic IContextSource rather than SpecialAbuseFilter
  (lower coupling);
- Inject a LinkRenderer (IContextSource doesn't have a ::getLinkRenderer
  method)
- Add a helper method in SpecialAbuseFilter to get the page title, that
  can also be used elsewhere (and the name constant can be made private
  now)
- Pull down the mFilter property (and rename it to just 'filter') to
  classes that actually need it. Some classes didn't need this at all
  and the types were different among subclasses

Now the only cause of coupling between the View classes and
SpecialAbuseFilter is the static call in getTitle.

Change-Id: I3df0c3a7621f0cc9a64a16b0a402a15aae2d5d73
2020-10-13 10:38:43 +02:00
jenkins-bot 95766762c4 Merge "Migrate a few hook handlers to DI" 2020-10-13 08:36:27 +00:00
jenkins-bot 3e61e886ba Merge "Add an AbuseFilterPermissionManager service" 2020-10-13 08:36:25 +00:00
jenkins-bot 51ce0bacf6 Merge "Delegate some switch cases to the parent in GlobalAFPager" 2020-10-12 10:13:25 +00:00
Matěj Suchánek 7ef2259228 Migrate a few hook handlers to DI
Bug: T261067
Change-Id: If699917c3d2e9e22525c7d0495554e25f6b45125
2020-10-10 17:23:04 +00:00
jenkins-bot 42525e4d5a Merge "Cleanup filter id handling on Special:AbuseFilter/history" 2020-10-10 12:28:05 +00:00
Daimona Eaytoy 2026e3ac3a Add an AbuseFilterPermissionManager service
This service should act as a mediator between the AF code and the
permission manager, and it should know what are the permissions required
by each action.

Change-Id: Ieb177d9992147b11fa7b8f05929da6c182cc2286
2020-10-10 14:03:29 +02:00
jenkins-bot f1de9145f5 Merge "Remove sorting by user from Special:AbuseFilter/history" 2020-10-10 11:58:54 +00:00
Matěj Suchánek d91ddd2169 Cleanup filter id handling on Special:AbuseFilter/history
In particular, the interface shouldn't generate links to
"Special:AbuseFilter/history/0" (AbuseFilterHistoryPager::getTitle,
can be seen when visiting "Special:AbuseFilter/history").

Change-Id: Id3dc1bb4fc3c5e853603bf0ec04a6b1751f7d862
2020-10-10 11:40:46 +00:00
Daimona Eaytoy f0539e0c1e Represent new filters with null instead of 'new'
PHP is not strongly typed, so it's not a good idea to use scalars of
different types (here it's an integer vs the string 'new') to represent
different possibilities. This can have bad effects when type juggling
occurs, and it's also harder to figure out what the type of the
parameter can be (because a numeric ID might have been passed as a
string). Using integer vs null avoids all of this, and also allows us to
use nullable typehints.

These changes were partly copied from
If981cb35bf19a8469aa6c43c907e107cf8c65bc2 and should help with the
migration to the Filter value objects.

Change-Id: I8837d46c3c33761fea53f67b530b721dc7bd49b0
2020-10-10 12:23:50 +02:00
jenkins-bot c0defc1055 Merge "Add a new FilterProfiler service" 2020-10-10 10:08:58 +00:00
jenkins-bot f4570e232d Merge "Hide filter group selector when filter selector is hidden" 2020-10-09 23:02:04 +00:00
jenkins-bot da3bfa3314 Merge "Reduce dependencies of AbuseFilter::saveFilter" 2020-10-09 14:58:30 +00:00
Matěj Suchánek 826f03d928 Remove sorting by user from Special:AbuseFilter/history
This feature didn't work and even if we fixed it as suggested
in the task, it would still be bogus. For deterministic paging,
the afh_user_text field should be in an index together with
another field(s). But currently it's indexed alone.

By the way, the indexes on abuse_filter_history should be fixed
anyway. Special:AbuseFilter/history also allows filtering by
filter/user which require index on the fields. They are present
but are not composite, so either the sorting is done
inefficiently without an index or there is a fullscan.

Also remove the getIndexField override. TablePager knows best
what value can be used there, we don't really have to override
it.

Bug: T204210
Change-Id: I7335f82c917a1d219fd7f0999da5b62433f14bd8
2020-10-09 15:41:19 +02:00
Matěj Suchánek 2c650f7710 Hide filter group selector when filter selector is hidden
This was a means to bypass the limitation to filter by
triggered filter (for example, when a group contains
a single filter).

Change-Id: Icd7b0b64ff16b4ce26f4d52ad9d9abce62972e60
2020-10-09 13:33:13 +00:00
Daimona Eaytoy 9f2906e34b Reduce dependencies of AbuseFilter::saveFilter
This patch removes the dependency of saveFilter on the ContextSource
kitchen sink. It also removes some unneded dependency, and adds
$originalRow/$originalActions as parameter, rather than hacky properties
in $newRow that are easy to forget. The related test can also be greatly
simplified.

This also introduces a behaviour change: checking $newRow instead of the Request allows us
to account for values normalization done in
AbuseFilterViewEdit::loadRequest, and to also work correctly for imports
(and generally speaking, it makes the method suitable for an
AbuseFilterEdit API module, too).

Next step is moving this method to a service. Some signatures,
indenting, name choices etc. are subpar, but this is just because these
methods are temporary anyway.

Bug: T213037
Change-Id: I235b928d7b9c2ef1c46ea0bf3e3ed212500b4161
2020-10-09 11:52:02 +00:00
jenkins-bot 73d6544bc6 Merge "Avoid array_filter on explode()" 2020-10-09 07:49:24 +00:00
jenkins-bot f2648afb15 Merge "Prevent cache pollution in fetchAllTags and clean up" 2020-10-08 18:02:21 +00:00
Daimona Eaytoy 7a1d6dbdbb Avoid array_filter on explode()
The array_filter is likely meant to empty the array if the empty string
was exploded ( `explode( "\n", '' ) === [ '' ]` ). However, it can also
remove other stuff, e.g. the string '0'. An explicit comparison is
easier to read & interpret, marginally faster, and avoids rare but not
impossible edge cases.

Change-Id: Ie77d65b56319664a2ac370f32341dc72b619a635
2020-10-08 18:56:27 +02:00
Matěj Suchánek 9e6bc2f4ee Move log formatters to a separate directory and namespace
This will clean up the includes/ directory a little.

Change-Id: I61adacf32257bb2402a272b60b52b69505d981c5
2020-10-07 16:25:38 +02:00
Matěj Suchánek b7cda4de4c Prevent cache pollution in fetchAllTags and clean up
Previously, the cached value would depend on the tags
parameter to be updated. The provided value may be
different for each call, so callers may receive
unexpected values.

For example, while core usually calls this with core-defined
hooks, our method AbuseFilter::isAllowedTag calls this
providing an empty array. If core's call happened shortly
after ours and hit cache, its array would be overwritten
with only AbuseFilter's tags, the rest would be lost.

Also do some clean up:

- only call array_filter on explode'd array
- call array_unique on the value, since it's usual that
  multiple filters share the same tag

Noticed when thinking about moving this to a service.

Change-Id: I4f4322e80ec89e48458a3bf46a1146863bec8237
2020-10-07 15:20:41 +02:00
Daimona Eaytoy 58538103c9 Delegate some switch cases to the parent in GlobalAFPager
af_actions and af_hidden are treated in the same way, so avoid
duplicating that code. Some of the remaining cases are also quite
similar (although not identical), so we might want to merge them in the
future.

Change-Id: I1b48502e077e58eb9ff459326bba18bb1d127242
2020-10-07 12:46:52 +02:00
jenkins-bot 1b0fa6bc6e Merge "Remove useless param to wfMessage" 2020-10-06 11:21:14 +00:00
jenkins-bot b0e68af5f5 Merge "ViewEdit: account for empty actions in imported data" 2020-10-06 09:34:30 +00:00
Daimona Eaytoy 11d922d1bd Remove useless param to wfMessage
Copying my investigation from I8c93e2ae7e7bd4fc561c5e8490ed2feb1ef0edc2:

This code was introduced in 2009, see rEABF0f1eb8db78bfa83ddb93427f39aad619523d8f25:

  $display = wfMsg( "abusefilter-action-$action" );
  $display = wfEmptyMsg( "abusefilter-action-$action", $display ) ? $action : $display;

And wfEmptyMsg looked like this:

  function wfEmptyMsg( $msg, $wfMsgOut ) {
    return $wfMsgOut === htmlspecialchars( "<$msg>" );
  }

so this made sense. But then, in 2010 (rMWae3ced88e535c7fd046f0ad6f0710cc87f0004ea) the function was changed:

  function wfEmptyMsg( $key ) {
    global $wgMessageCache;
    return $wgMessageCache->get( $key ) === false;
  }

without anyone removing the parameter from AbuseFilter.

Finally, in 2012 (rEABF176227e721c9475de2c2163d3b6e20ca4769c406) the usage of wfEmptyMsg was removed, and $display became a parameter to wfMessage().

Long story short, no need to pass that parameter.

Change-Id: Iad875f0c0ab5aaa06c795232638f52e9ca62786e
2020-10-05 23:39:23 +02:00
jenkins-bot 2cdec2473e Merge "Actually apply patch-afl_change_deleted_patrolled." 2020-10-05 08:15:48 +00:00
jenkins-bot 2e1afd5a51 Merge "Add test traits for uploads and account creation" 2020-10-05 07:49:38 +00:00