This requires a MessageLocalizer, which currently means providing the
main RequestContext. This is the only alternative right now, until core
provides a proper MessageLocalizer service (see T247127).
Change-Id: I8c93e2ae7e7bd4fc561c5e8490ed2feb1ef0edc2
Use Echo for delivering the notification to the last
user who edited the filter.
Much boilerplate.
Change-Id: I7a46a03b4f15de20902ec70c62fb4fe750096842
Depends-On: If585b14a6dd6fb8c7d2c3bee1f20d9d08eaac706
This commit introduces some boilerplate for emitting warnings from the
AbuseFilter parser, and also code for showing these warnings in the ace
editor. Adding new warnings should be as simple as appending to
AbuseFilterParser::warnings (and adding the relevant i18n).
Bug: T264768
Bug: T269770
Change-Id: Ic11021b379f997a89f59c8c0572338d957e089a6
This is the last big step towards moving Consequences-related things away from
AbuseFilterRunner. There's still some cleanup to do (+ write proper tests), but
this should really be the last important code change.
Change-Id: I347795fe93ba496c43b1d5cfc9ba6e1326842c06
AbuseFilter emulates the storage mechanism also used for page content.
Instead of duplicating the relevant code, AbuseFilter should use the
same BlobStore service also used by RevisionStore.
Note that this change is not strictly needed to resolve T198341, but is
needed to unblock T183490
Bug: T261889
Bug: T198341
Bug: T183490
Change-Id: I3fc8475dd8d50d73d705b706ff597a130267e990
This is just a temporary location for these two methods. Since they're
used a lot, having them in the AbuseFilter class means that the
dependency graph is unnecessarily complicated. Thus, since these methods
aren't doing much, they were moved to a dedicated class. Future todo is
finding an appropriate location, that might be either as part of another
service, or keep them in a Utilities class, perhaps a single class with
all util methods, rather than a specific class.
Change-Id: I52cc47a6b9a387cd1e68c5127f6598a4c43ca428
The main change is the addition of checkboxes to hide/show multiple
entries at the same time. Also, tweaked some i18n and made the process
return more useful success/error messages.
This patch introduces some technical debt, caused by SpecialAbuseLog and
AbuseLogPager being tightly coupled (which is a pre-existing problem,
but it got worse here).
Bug: T260904
Bug: T144096
Bug: T206945
Bug: T206938
Change-Id: I13f476d8126f81b0417e7509784c83d4f21cf348
Move to the latter some methods that make more sense in there. Inject
some more services, don't require a SpecialAbuseLog to be passed in the
constructor.
There are still a couple of static calls, but fixing those would require
factoring more classes out of SpecialAbuseLog (e.g. a service to
determine visibility of AbuseLog entries).
Change-Id: I1b3012ca85bf049a07e0433fc0b357f502c355ad
This is moving code away from SpecialAbuseLog, which is already too big
and has too many purposes. As such, the behaviour is not changed,
including for now bugs that were already present in the old version.
Change-Id: Idc13f7f746ada2e425662c6948c32aa744edac61
This is achieved by creating a new ParserStatus class. Aside from the
result of parse(), it contains whether the cache was warm. This can be
used to differentiate profiling data as part of T231112.
Another use case is returning non-fatal warnings (T269770).
Change-Id: Ifcbda861ce1a44bbe9bffba5b83cd9ef338a8dba
This is the last use, and it was a bit harder to remove because it was
buried inside AFComputedVariable. Starting with
I4444cada720ab62d187f2dd0c4760697e465f2ff, we can freely change the
parameters to AFComputedVariable without breaking old log entries.
Note, we still need a fallback for other extensions calling this
method...
Bug: T246733
Depends-On: I4444cada720ab62d187f2dd0c4760697e465f2ff
Change-Id: I5d786a518ef88fad9c8d9c25ef4553a0bf30b2b2
The schema was introduced in 1.34, so there should be no issue in
starting off with writing the new columns.
Bug: T220791
Change-Id: I8f956d4a27692a33368a413fbf4a8eb5da20afe1
Add a script to migrate the columns (which can also
be executed in dry run), and a config option with the migration stage
(defaults to SCHEMA_COMPAT_OLD).
Some of the script-related code is stolen from
Ic755526d5f989c4a66b1d37527cda235f61cb437.
Bug: T220791
Change-Id: I7460a2d63f60c2933b36f8383a8abdbba8649e12
There is a try-catch block but the same call was also done
unconditionally after it, making it throw when global filters
are disabled.
Change-Id: Ic8032592799756521a59ee23c0e76cb03a510b94
$wgAbuseFilterActions shouldn't be used normally, as it excludes actions
registered by other extensions.
Note: mw:Extension:AbuseFilter#Integration_with_other_extensions should
be updated after merging.
Bug: T239348
Change-Id: I89b3f0228eacdf145e8f2dd2a5602d0c7ce75a86
This was NULL for old entries, because no default was added
in I758795f01eaf3ff56c5720d660cd989ef95764a7 (see T263324)
Bug: T269314
Change-Id: I5af8b0d3a9d7b6d2570cf79bbbe8b5b170ba1230
Also fix a bug in FilterProfiler. It would attempt to reset
stats for global filters but we do not record them (yet?).
Change-Id: I0228d8c85dab146deb877dfce506f1e8e7711a9f
* Move all SQL files into db_patches (or below)
** Remove db type from filename
* Remove a lot of duplicated code and simplify
Change-Id: If22f2a2c46a59ac24c89ce612c74d169f053ab26
Just moving code around. Without a unit test because DI
coverage of change tags in core isn't available yet.
Change-Id: Iac861e1e24dae13581b8d9173357a1d6c94be88a
It makes sense to look at this and Iedd7a5dca24 together,
as this patch itself doesn't really fix anything.
Change-Id: Ifef5266b1803d1a96489789b08d9beed044d908f
The consequence-taking logic is moved away from AbuseFilterRunner, to
dedicated classes. There's now one class per consequence, encapsulating
everything it needs to take the consequence.
Several interfaces allow customizing different types of consequences.
Every "special check" in AbuseFilter was generalized to use these
interfaces, rather than knowing how to handle each consequence.
Adding more consequences from other extensions will also be easier, and
it should happen via a hook (not a global), returning a class that
implements Consequence. The BCConsequence class was temporarily added
for legacy custom consequences.
A ConsequenceFactory class is added to instantiate consequences; this
would possibly benefit from using ObjectFactory, but it doesn't because
it would also reduce readability (although we might do that in the
future).
These classes are still not covered by unit tests, and this is left to
do for later. The new unit tests should mostly replace
AbuseFilterConsequencesTest. @covers tag were added to keep the status
quo (i.e. code that was considered covered while in AbuseFilterRunner
will still be considered covered), although we'll have to adjust them.
Change-Id: Ia1a9a8bbf55ddd875dfd5bbc55fcd612cff568ef
This will ease adding new watchers, for instance to send Echo
notifications (see T179495 and T100892).
For now, this is just boilerplate, and converting EmergencyWatcher to
the new interface.
Change-Id: I18d62aba53471202b709cdb19033b1729c5c25b4
This will avoid unneeded queries, in theory. In practice, it will
almost never happen to have more than one filter to throttle.
Change-Id: I5b8df51215463ce4464f6a2d0390f58612a5a213
-Exclude methods and classes that cannot be meaningfully covered
-Add a simple test for AbuseFilterServices
-Exclude ServiceWiring because there's no way to tell PHPUnit it's
covered
Change-Id: I4c67b0d3fea68c7a3b3cbe01b5608f87e1b492db
Needs the patch in ContentTranslation first.
Depends-On: I0b74db70ad4e9768e4dcb84b9decb9c737e942e5
Change-Id: Id186ea99fcf69aa4348e404677ce5da998d83170
The slow filter hits are logged for the target wiki, but
the fix would be on meta, so make it possible to filter
for those
Change-Id: I6e02866479e77d707f4fa951ec909c325b944158
The behaviour is:
- When assigning to an undefined offset, delete the whole array and turn
it into another DUNDEFINED
- When retrieving from an undefined offset, just return DUNDEFINED.
Bug: T237214
Change-Id: I621ee7a16c90bb86a57be04e7ce0a748ecdbfcc7
The main benefit of having a dedicated interface is that we can easily
change the output format. So we're now using a custom array without
references to the DB schema, thus making the import/export process
completely independent from the schema.
Change-Id: I4c0de41d914baf1e9a0e588bd31f95b3524a424b
Add a radio to select between "hide" and "show" instead of a single,
cryptic checkbox which doesn't really explain what it does.
Also wrap the list in a form which will later be used to mass-delete
entries.
Depends-On: I1bb45e47c3b42c01388b99778ce833e4e44419e1
Change-Id: Ie2d019fad5af7c626d722dc348f40eb0db21e527
Remove $title->exists() from the check, so we have the following
changes:
- The AbuseLog will add a diff link for page creations
- Searching the AbuseLog for impact:saved will include page creations
- We don't have to recreate the WikiPage again in RunVariableGenerator
Also remove an old reference to "bug 31656": that comment was added in
rEABFefecf8b2441ae2f31f924ff33103f5affe5d1d62, which changed
Article::getContent() to Article::getRevision()->getRawText(). Nowadays
we don't even use Article anymore, and that conditional isn't even for
retrieving the page content, so the comment is wrong.
Add logging for when the Title object cannot exist, as this should never
happen in the context of the EditFilterMergedContent hook, and always
create a WikiPage. Some signatures were changed to require a WikiPage
object now, and every caller updated to provide it.
Bug: T263104
Bug: T62179
Depends-On: Ic238eaa529ef6bfba06b4dd03924a8e0111d8259
Change-Id: Ibf3bf4f68328ba4a5616ab8f26a8b44d27a25cd7
This is a no-op, moving code around, introducing another distinction re
"filtering actions", which now happens in 2 steps:
- The first step only uses "generic" information available by looking
at enabled actions as a "group". This includes keeping only the
longest block, and removing 'disallow' if other blocking actions are
enabled.
- The second step uses information that is only available after having
"partly executed" (named "pre-checked") a consequence. For instance,
we need to pre-check 'throttle' to see if the throttle was hit, and
remove any other actions if not.
Change-Id: I7be5cfaa61e942a06f97ed52f50e9c8c70a120e8
This way we don't have special cases in executeFilterActions, and instead, we execute
all actions in the same place. In turn, this is going to ease the
transition to a new consequences system: next step is refactoring this
code into a service with proper DI etc.
Bug: T204447
Change-Id: I8134ecc41fbecdbed99faf406e9e3ca91b6123b9
The scope is still quite limited, but as noted in a todo, we might want
to make this completely independent from the database, and add the use
case of ViewDiff.
Change-Id: Ie980fff0983b3e86037265e85da04444c809a6e8
They've been replaced by getters in the Filter class.
Note, the Lookup is not injected in this patch because some places would
need careful thought, so it's left to do later.
Change-Id: I40b8c8452d9df741217d7fa090a5e746a2f46994
This moves a lot of things away from the AbuseFilter class. There's a
nasty static dependency on ChangeTags, but it's very limited anyway, and
it's going to be fixed once T245964 is resolved.
Change-Id: Ia7df4b4d3289c2722323f59ceecf3fdd38277785
Some pieces of code were updated to use Filter objects, while other
places are still to be updated. We also need to change the history part
to exclude actions somehow, cleanup the ViewEdit, reduce direct DB
access or anything mentioning DB fields outside of FilterLookup, etc.
Change-Id: I42b7ded685db76eddd45e4b1336f9828cba811ce
This requires adjusting some methods to work with Filter objects. Some
methods and tests are left in an inconsistent/suboptimal state, plus some todos
were added, but all of this is going to be remediated in another commit.
Change-Id: Id063ee73d97c7aef56323e1457d99704f77ab943
Phan is failing on master with
includes/Views/AbuseFilterViewEdit.php:506 PhanTypeMismatchArgument Argument 1 ($salt) is ['abusefilter',$filter] of type array{0:'abusefilter',1:?int} but \User::getEditToken() takes string|string[] defined at ../../includes/user/User.php:3735
due to a documentation change in core.
Change-Id: Ibc01332c67224e3efc7922d1be882615c2de5d9a
The only usage outside of AbuseFilter (in ContentTranslation) was fixed with
Ifc9ede277791398290786cdb6743137004b5c713.
Change-Id: I22cf9c76ef3b007502045a02c82255ba6c9fd0f2
This is just a start; next step is adding a factory/store method to
get/store these objects. And then use these value objects whenever
applicable.
Note: the actions-related code is still not fully implemented. This is
going to happen as part of the FilterLookup.
Change-Id: I5f33227887c035e301313bbe24d1c1fefb75bc6a
Rely on modern HTMLForm features instead of using a dedicated class
property. The form identifiers are necessary, because these forms are
GET forms, and HTMLForm will always think that the form was submitted,
if it doesn't have an identifier (see T238467 and related
tasks/patches).
Additionally, make the first form on ViewRevert a GET form, like on
Special:AbuseLog.
Bug: T263627
Change-Id: Ia6ca45896732742ef73e401b09663728b9e7dda2
The publish() method that it resembles is not a method defined in
the LogEntry class, and not even in LogEntryBase class. It is
defined in the ManualLogEntry class. Let's reference it correctly.
Change-Id: I60cfceac7c19047e299cf9f704dda8d8ef2f2ba6
TODO For the future: the final directory for Parser-related classes
should be "Parser", not "ParserNS". However, moving all classes now
would make it harder to rebase changes etc.
Change-Id: Ice335f4723e74f4e5fbe8dcc76ff8ea16310962c
Ordering is done by in IndexPager::buildQueryInfo. In fact,
this key is unconditionally overridden there and the query
is sorted by rc_id (specified in ::getIndexField). It would
probably deserve some performance analysis because
the ordering and filtering don't seem to use matching indices.
Change-Id: I9e73d44d868ddf5beba6dc6e4550e851a6df5119
This is a thin wrapper around LBFactory and the global variable, that
can be injected in classes requiring it (no real class right now, but
that's going to change soon).
Also, remove some DWIM-style returns which made the code harder to
understand.
Change-Id: I1d28ad4a67f914103f3a17cda5f61b28070c7f1c
Remove outdated/pointless comments, use already defined variables, etc.
Additionally, make it possible to disable throttling locally.
Change-Id: I98fd5f3eb47b32fc1013360e462a57d932174a95
This is still not very useful, but it's going to come up handy when
we'll be refactoring this code.
Additionally, fix a shortcircuit issue which caused additional throttle
types to not be processed if a type was already triggered.
Change-Id: Ied44d9300b3fa2ad00fe95c9c3da3c3f8faa650b
Make FilterProfiler::getFilterProfile return stats unchanged,
in a structured way. Move computations to AbuseFilterViewEdit,
as they are only useful there. Don't return false on cache
misses, return arrays with zero values instead.
Bug: T266531
Change-Id: I8718cc31a5004340bf742315c7075e10a61fcbfd
This commit splits this method into a version that doesn't need a
filter, and another version which requires one. This latter version has
a single mandatory parameter, $filterHidden, and it's up to the callers
to retrieve the value to pass in.
As mentioned in a TODO, this should eventually be changed to take a
Filter object (still under review as
I5f33227887c035e301313bbe24d1c1fefb75bc6a), which is also why
AbuseFilter::filterHidden is not being used here.
Change-Id: Id47a80131e12a5f7e1e93676299641dbf1e2b0ad
FilterProfiler::getFilterProfile returns data in a different
format than the data is really stored.
Bug: T266531
Change-Id: I0d961a1ae67769da61f841df2462d47f81849972
This deals with data inconsistencies in buildFilterEditor. Every
property of $row was tested in all 5 scenarios (also using Selenium) to
check when it's set. The result is in the normalizeRow method, which
aims to remove any inconsistencies, so that buildFilterEditor always
receives a "complete" row with all defaults set.
The code in buildFilterEditor is now cleaner (because there are no
isset() checks), and it gives us a unique place where we can set
defaults (rather than partly doing that in
loadRequest/loadFilterData/loadImport, and partly relying on isset).
This will be especially useful when introducing value objects to
represent filters, because now you just have to look at normalizeRow()
to tell which properties are allowed to be missing, and thus what "kind"
of filter object you need (see
I5f33227887c035e301313bbe24d1c1fefb75bc6a).
Additionally, reduce the properties that get passed around during
export/import, and make the selenium test try a roundtrip, rather than
relying on hardcoded data that may get outdated. A future patch will
refactor the import/export code.
Change-Id: Id52c466baaf6da18e2981f27a81ffdad3a509e78
Unfortunately, this isn't using DI completely, because of the
User::newSystemUser call. I'm not even sure if we really need to call it
or we can just stick to new UserIdentityValue, but leaving like this for
now.
Also, the types were weakened to UserIdentity, so the transition is
going to be easy anyway.
Change-Id: I08f8fae0fcc622ff0ac3f86771476d06d1c18549
The previous code would call getUserGroups again once creating the log
entry, but this was slightly flawed: we're updating groups on master,
but the read happens on a replica that might be outdated, hence
resulting in broken logging. Instead of reading from master, we can just
keep a list of the groups that were actually added, and use that
afterwards.
Change-Id: I7cc282e15561de3a3d3e183808a65991aa27d2bb
This service is responsible for the blockautopromote feature:
(un)block autopromotion and check status.
The patch mostly moves code from static methods to the new class
and relaxes type hints (e.g. from User to UserIdentity).
Change-Id: I79a72377881cf06717931cd09af12f3b8e5f3e3f
Previously, AbuseFilterHooks would proxy the data from a slower backend
(db-replicated) to a faster one (hash) reusing the same key. This change
makes it use a dedicated key, so that the "main" key can be kept
internal inside the upcoming BlockAutopromoteStore.
Change-Id: Id46a66991d0e994ee0a83b83b9c95e8951f3041c
- Add a helper method to output an unrecoverable error, comprising a
button to go back to the filters list;
- Move the token check to attemptSave, so to make the conditionals
easier to read, and group errors together
- Make buildFilterEditor take an HTML parameter for the error, so the
caller can specify whether it's error or warning
- Move the check for non-existing filters out of buildFilterEditor
- Add a bunch of typehints
- Don't set af_throttled and af_hit_count in the empty row template, but
set af_deleted (these are only used in buildFilterEditor)
- Make AbuseFilter::translateFromHistory consistently include the af_global
property (previously it would only be set for global filters; this error
was introduced when first implementing global filters)
- The only user-facing change is that, when trying to use a custom
warning/disallow message on a global filter, this is now considered a
non-fatal error, so we now show the editing interface (and not just an
unrecoverable error).
The next step is resolving the @todo in buildFilterEditor about null
checks.
Change-Id: I9d217dcac3f4cc0b26e53eca735cc327d5efc76d
This commit avoids direct queries on the DB, which is already an
improvement. It also adds some TODO comments for future improvements,
mostly things that depend on core changes.
Bug: T265224
Change-Id: I8eb76a0c463751976c2c5deedb3570305f1ab4f0
There's no point in repeating the action name, because it's already used
as key. We can then flatten the array and just keep the parameters in
the third nesting level.
Change-Id: I54abcc49322f432cedd361abeedb72e067d3de41
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
Follows up Ib66c42ac220731f4e1da9ee6cfb5290759dd6494.
Apply DannyS712's suggestions from that patch.
Change-Id: Ib9f19969a888bd29f9f46e90fb52b49ce883c667
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
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
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
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
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
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
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
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
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
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
- 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
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
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
