old_links and all_links are an array. Casting
them to string and then splitting by newlines
is a no-op.
Change-Id: I05c69f14e981ac2842032e7db888f4841d6b48b7
We should avoid using ObjectCache directly and use services instead. On
top of that, db-replicated writes to objectcache table that should be
avoided and it's now blocking schema changes.
This should go to MainStash.
Bug: T272512
Change-Id: I9c76399ec4c0ea6644f9ca1b4536428052e5ac38
These are not necessarily old links, the new links
can also be retrieved using this code path.
Also print debug messages before the code execution.
Change-Id: I1a85bb7b5a2af4fe514625d2236cf92f15daf304
The special page itself is a MessageLocalizer
SpecialPage::getContext on constructor results in global state
Change-Id: I6a3dd263dbe02111aea65a087ce69a5a191f63fc
The report is now generated in ParserOutput, not Parser, meaning we can
simply avoid passing the `enableLimitReport` option (off by default) if
we don't want the report to be there.
Depends-On: I154c0a77a5b0287b5572614d56339fb57ac56c33
Change-Id: I8cdab35c475f10433234ddb55b5e6a0cc8109498
Note that this doesn't have to be applied in WMF prod, since the indexes
are already correct there.
Bug: T291725
Change-Id: I7fcee0581f469e0d7ada43d1f30f31061c5bf5c0
This does the same as before, replacing \r\n as well as \r with \n.
Additionally the new method applies an rtrim() on both strings. I
believe this is even a good thing. It possibly removes irrelevant
noise from the diff.
Change-Id: I584740a24e6b25bbcbc928c2369f09b785a485c8
On second thought, no need to keep the migration script now, since it's
unusable anyway.
Also remove an usage in SpecialAbuseLog, likely a rebase artefact.
Change-Id: I938924b3617ef30046d8317e68a101ed2c1883d3
This replaces the previous pattern of callers having to use
RevisionLookup if the result was 'implicit'. Also, in some cases where
we were just hiding things if the visibility was !== true, properly
handle the implicit case by using the new method. Make the new method
return string constants rather than bool|string.
The new method also fixes some potential info leaks which happened when
the row was hidden, the user could view suppressed AbuseLog entries, but
the associated revision was also deleted and the user couldn't see it
(this shouldn't be relevant for WMF wikis since AF deletion is
oversight-level).
Also add a bunch of tests for the various cases to ensure we don't
regress again.
Bug: T261532
Change-Id: I929f865acf5d207b739cb3af043f70cb59243ee0
ParserStatus is now more lightweight, and doesn't know about "result"
and "from cache". Instead, it has an isValid() method which is merely a
shorthand for checking whether getException() is null.
Introduce a child class, RuleCheckerStatus, which knows about result and
cache and can be (un)serialized.
This removes the ambiguity of the $result field, and helps the
transition to a new RuleChecker class.
Change-Id: I0dac7ab4febbfdabe72596631db630411d967ab5
Something somewhere is leaving error_reporting in a dirty state
causing AbuseFilter's ConsequencesExecutorTest case to fail for
the core change Ic9fee6cdd88001025.
Per T253461, we're meant to eventually remove this anyway, so might
as well remove it in areas that are known to get it wrong somehow.
Change-Id: I2a665f09a357f2f2cc258d8c4011d49a7ab9c13b
The old parser now has the correct name "Evaluator", so the
ParserFactory name was outdated. Additionally, the plan is to create a
new RuleChecker class, acting as a facade for the different
parsing-related stages (lexer, parser, evaluator, etc.), which is what
most if not all callers should use. The RuleCheckerFactory still returns
a FilterEvaluator for now.
Also, "Parser" is a specific term defining *how* things happen
internally, whereas "RuleChecker" describes *what* callers should expect
from the new class.
Change-Id: I25b47a162d933c1e385175aae715ca38872b1442
Remove unnecessary setters, injecting everything in the constructor.
These were leftovers from before the introduction of ParserFactory.
Remove public access to the conds used, include the information inside
the returned ParserStatus instead, and consequently simplify callers.
Change-Id: I0a30e044877c6c858af3ff73f819d5ec7c4cc769
So that the method can be typehinted in core.
Also add phan-var to fix broken master build due to typehint additions
in core.
Change-Id: I4a072e00ffeeb437753fc3d3c1f15de9929df510
This commit adds a class AFPSyntaxChecker which can statically analyze
a filter code to detect the following errors:
- unbound variables (which comes in two modes: conservative and liberal,
default to conservative)
- unused variables (disabled by default for compatibilty)
- assignment on built-in identifiers
- function application's arity mismatch
- function application's invalid function name
- non-string literal in the first argument of set / set_var
The existing parser and evaluator are modified as follows:
- The new (caching) evaluator no longer needs to perform variable
hoisting at runtime.
- Note that for array assignment, this changes the semantics.
- The new parser is more lenient, reducing parsing errors.
The static analyzer will catch these errors instead, allowing us
to give a much better error message and reduces the complexity of
the parser.
* The parser now allows function name to be any identifier.
* The parser now allows arity mismatch to occur.
* The parser now allows the first argument of set to be any expression.
Concretely, obvious changes that users will see are:
1. a := [1]; false & (a[] := 2); a[0] === 1
would evaluate to true, while it used to evaluate to the undefined value
due to hoisting
2. f(1)
will now error with 'f is not a valid function' as opposed to
'Unexpected "T_BRACE"'
3. length
will now error with 'Illegal use of built-in identifier "length"'
as opposed to 'Expected a ('
Appendix: conservative and liberal mode
The conservative mode is completely compatible with the current evaluator.
That is,
false & (a := 1); a
will not deem `a` as unbound, though this is actually undesirable because
`a` would then be bound to the troublesome undefined value.
The liberal mode rejects the above pattern by deeming `a` as unbound.
However, it also rejects
true & (a := 1); a
even though (a := 1) is always executed. Since there are several filters
in Wikimedia projects that rely on this behavior, we default the mode
to conservative for now.
Note that even the liberal mode doesn't really respect lexical scope
appeared in some other programming languages (see also T234690).
For instance:
(if true then (a := 1) else (a := 2) end); a
would be accepted by the liberal checker, even though under lexical scope,
`a` would be unbound. However, it is unlikely that lexical scope
will be suitable for the filter language, as most filters in
Wikimedia projects that have user-defined variable do violate lexical scope.
Bug: T260903
Bug: T238709
Bug: T237610
Bug: T234690
Bug: T231536
Change-Id: Ic6d030503e554933f8d220c6f87b680505918ae2
Create a dedicated "Exception" sub-namespace and remove the "AFP"
prefix, a leftover from the pre-namespace era.
Change-Id: I7e5fded9316d8b7d1628bc1a6ba8b1879ac901e1
Previously, for non-newly-created pages, AbuseFilter would get the text
for filtering twice: once in AbuseFilterHooks::filterEdit(), and then
again in RunVariableGenerator::getEditTextForFiltering(). (Plus another
call for the text of the previous revision.) The first copy of the text
is only passed into RunVariableGenerator::getEditVars(), and there only
used if the title doesn’t exist, otherwise it’s overwritten with the
second copy. Instead, let’s make AbuseFilterHooks not get the text at
all, and only get the text from the content when we actually need it
(the content is new).
Change-Id: Id12430fa6ba4643113b945e0d0c01b9c0ee1742f
This reverts commit 15fc159cb1.
Reason for revert: this is breaking the addition of rev ids to filter
hits after edits are saved. I suspect this is because the context wikipage
is for a different title than the one being edited, though I'm not sure
way - regardless, testing on patchdemo shows that with this revert
is applied, rev ids are once again added to filter hits.
Bug: T286140
Change-Id: I3ab6324a73050154cef1c20a2bf8307eb11eea2d
If the content language is English and the message is invalid as
a username, or the content language is not English and both the
content language version and the English version are invalid, the
user in FilterUser would not be created - now, avoid the onwiki
version of the English message in the fallback, so it could only
be invalid if the default in the i18n files was invalid.
Bug: T284364
Change-Id: I9e9f44b7663e810de70fb9ac7f6760f83dd4895b
The master version of the extension is only meant to support the most
recent version of MediaWiki.
Change-Id: I33612e69fc37bf5eb70133c8f0e95199dd7fcb65
UserEditTracker::getUserEditCount now allows anonymous users,
but it returns null and phan is aware of this. Suppress this
warning until at least 1.37 is required.
Change-Id: I9962abe08fa31d55421d8bdda23ea0a1c0471a86
Sharing a handler class with UserRenameHandler means that attempting to
merge users fails due to a missing interface if AbuseFilter and MergeUser
are installed but Renameuser is not installed.
Change-Id: I1244ab1c446840ff2648248f943d7fc784b889a7
These are part of legacy styles and aren't provided by all skins.
Using Html::successbox abstracts the classes away.
Internally that uses div class="successbox" instead.
Bug: T280766
Change-Id: I0cca59e2f391510095c2c6fb187ace5e91fdde8b
Follow-up I574bda15f0f5c92a7d97a6e3150981b8f97ee7fc
Apologies for not noticing before:
If somebody hadn't already added the afl_filter_id column, the
rename-indexes patch would try to rename a non-existing index
(filter_timestamp_full and fail). So put rename-indexes after the other
patch.
Then, for the afl_filter_id patch, check the column and not the index.
We were checking the index because it's the last thing that the DB patch
does (so if the index is found, we can be certain that the patch was
fully applied). However, now that renaming the index happens afterwards,
if somebody had already added afl_filter_id (with the old index name),
running the updater would try adding it again, because the new index
name isn't found (as it's renamed later).
Change-Id: I0250a7c187202facd932c160ace57930db510f64
Extensions are supposed to return false to break hook chains when failed, which can avoid unnecessary call of later handlers in other extensions and work around with problems caused by difference betwen multiple triggers.
On mediawiki version 1.36 and before, just returning false in this hook can't display error message by default.
Set $status->value manually still to provide backward compatibility.
Bug: T280312
Change-Id: I78888247063c726ebcd18ba54a21d6c7891481fc
Migration was completed in MW 1.34, so it's no longer necessary to
call ActorMigration.
Bug: T278917
Change-Id: I26ad45b6d26756c3074c44f0192ceb04fb2698ae
- Inject dependencies.
- Make class variables private or protected and rename them.
Untangling the circular dependency is left for a future patch.
Change-Id: I5d625e30171bfbf60d9f5a94fa50475fdfe853dd
The block log entry will be automatically suppressed, until we can
implement a better solution.
Bug: T152394
Change-Id: I8bae477ad7e4d0190335363ac2decf28e4313da1
The only user we were worried about has now migrated to this; it auto-
ran in other installs just fine, so let's proceed.
Bug: T269712
Depends-On: I2b905f1e13ec13ec94d33527803c91c04b491eb2
Change-Id: Ie7d6bc95ebc871b0effee069e2146f2750d5f408
All methods were moved to the new parser. Tests and other pieces were
adjusted to expect just a single parser. There are still some TODOs
(remove AFPTransitionBase, remove $this->mCur), but these are left for
another commit.
Note that the new parser was not renamed: this is because the names are
wrong anyway (CachingParser is more of an Evaluator than a Parser, and
AFPTreeParser is the real parser, and should be renamed as well).
NOTE to reviewers: this patch looks quite big, but if you diff the old
parser with the new version of the CachingParser, you'll notice that the
diff is actually small, since everything was basically copied verbatim.
Bug: T239990
Change-Id: Ie914ef64c70503a201b4d2dec698ca2fa8e69b10
- mark properties as private (unused outside)
and rename them to avoid legacy naming
- do result filtering server-side
- order query by timestamp
Change-Id: If2d714753a2b040c5cefa8f8126f82a3c08dab44
Some of these api modules still retrieve other services
statically, this patch is focused just on injecting the
permission manager and setting up DI
Bug: T259960
Change-Id: Ic5196f230d68604fdf321f705377a1e6e1e2efca
Article::prepareContentForEdit is deprecated and being removed,
refer to WikiPage::prepareContentForEdit instead
Plus remove an extra line
Change-Id: Ie4438c710639a16557816b53510ce230d15d641c
UserEditTracker checks that the user is not anonymous, whereas
User::getEditCount() would just return null. This was not spotted by
tests because UserEditTracker is mocked.
Bug: T277859
Follow-up: I8a55bd5cb17bbc259ec36c40261058e0b46ee4a6
Change-Id: I05fb6cc780c80b72b3278e6dc670ed2025628ffb
This is kind of a nuclear option, if anything in a row is hidden, we
hide the whole row. This is just to keep this patch slim. A public
follow-up will adjust the visibility
Bug: T274152
Change-Id: I6063c02fa261c4cc0e6dbbb2db4e111eb85912c2
Otherwise we'd be telling whether the filter matches or not the edit. If
we're showing all edits regardless of whether they match the filter, we
can keep showing the row: it will be redacted (and the filter result
hidden) by AbuseFilterChangesList.
Bug: T223654
Change-Id: I3f7dbd8b873d411e37c8c3aac2339bf5ec36907d
There are various info leaks for both deleted rc rows, and suppressed
AbuseLog entries.
Bug: T223654
Change-Id: I4900b1be73323599d74e3164447f81eded094d75
The hook names contain a dash, which is mapped to an underscore by the
hook runner (see Ie8c8fb603b33ff95c8f8d52f392227f147c528d8), and the
previous method names weren't matching this.
Follow-up: Ic5c82a367e34135bbc0f00ece5aeef4f2d92881b
Change-Id: Ie80b62c49b2f4aaea49d5a1883f513348689d16a
Dashes are mapped to underscores, but following the "modern" convention,
the hook name should be pure PascalCase.
Bug: T275798
Change-Id: I77909b3ee772b983c7933f3b82230476772bd3b5
Requires injecting a temporary block factory, and excluding
ManualLogEntry::insert from the test, but it's now much cleaner and
quicker.
It still cannot be a unit test due to the usage of User.
Change-Id: Iba9732d6d79733b31b45eb4d0187b1c8a82499dc
It is currently possible to save a filter with an invalid group, if you
manually change the form data. So prevent this by validating the group
before saving.
Change-Id: I03f80b8c6ab583a357273f7b2679a424ac784db7
Remove using of User::getCanonicalName since this method will be hard-deprecated. Now it is soft-deprecated
Bug: T275030
Change-Id: I3ce1199f18276096279ce3c80f63e53d023a0f5a
Everyone can examine generated variables but not everyone
can test filters. Concerns Special:AbuseFilter/examine.
Change-Id: I9c205a0f1d9a7fdf15c4998d43983b9fa37f4694
This commit doesn't change any permissions for anybody.
It's the first step to achieve what the task asks for.
Bug: T242821
Change-Id: I8060ca926e6769b11d470fe4037854cda496000d
1 - Change the structure of if/elseif for readability
2 - In the old parser, if there's an empty argument, never add it (the
new parser was already doing that).
Bug: T156095
Bug: T156096
Change-Id: I4237b1a0ba01e7ce04dcc945f7daf34612fcf07d
Introduce a clear distinction between internal exceptions and
user-visible exceptions, leaving AFPException as base abstract class.
Later, it should be possible to narrow some types around, e.g. in
ParserStatus (that might work with user-visible exceptions only).
Also a future TODO is putting all the exceptions in their own namespace
(probably ...\Parser\Exception).
Change-Id: I4e33a45117f0a3e73af03cc1e3f2734beaf2b5e1
Thanks to this, we will be able to provide more information
to consequences and watchers, which will open door for new
features and possibly cleaner code.
Change-Id: I7135509823ea84b2a2923d2c1831ce293b98a9f9
Small refactoring. Create checkAllFiltersInternal and process
its return value in checkAllFilters to ensure compatibility.
Also fix some annotations.
Change-Id: If9d296de48f08d719f1700f88870002b814c5b31
This is a small refactoring. The method is protected,
so we only take care of compatibility of ::checkAllFilters.
This might be also be useful if we decide to work on T174554.
Change-Id: I83cd58ec325972264e86d7a73366c0affed0a37e
It was changed to use AFPData::toNative, so it no longer returns a
string. Instead, it can return any PHP native type.
Change-Id: I92eba03a5fa1149860634a97318b5b15807eb5a5
Every hook that is not directly responsible for filtering an action is
now moved to its own handler class. Some of these are still static
methods because the respective hooks still use the old system.
Bug: T261067
Change-Id: I157169f968a7d6a4d1bcfde09358e5a66a3353bf
This patch adds a transparent HTMLForm field that can be used to insert
the edit box inside an HTMLForm, and updates /test and /tools to use
that. The field class, together with the other editbox-related classes,
is now in a dedicated namespace. A future TODO is making it a real
HTMLForm field.
Also improve a bit the form in /test: add section labels and
avoid reusing the same label message used on Special:AbuseFilter.
Bug: T261584
Change-Id: Ib74bb5fdba4f8476169b754030fce6d4f72ce65a
- Clarify the label of the search form on Special:AbuseFilter
- Move introductory paragraphs to the very beginning of the page:
-- Before the profiling data on Special:AbuseFilter
-- Before the search form on Special:AbuseLog
- Make the search form on Special:AbuseFilter collapsible, and collapsed
by default
- Make a few buttons primary+progressive, specifically those that take
the user to a different page or act as submit-like buttons
Bug: T261584
Change-Id: I54517b01a9ea81d276283140e5cfafef575c3e2b
This service allows linking the EditFilterMergedContent and
PageSaveComplete hooks for the same edit, so we can update rev IDs in
the abuse_filter_log table. Having such a services also avoids two hacky
static props, and should allow separating the hook handlers easily.
Change-Id: I622d15225ee3af202cb5730a7112652aef8ca71a
Also add a bunch of tests for this function.
REMINDER: Change the docs on mw.org when this will be merged.
Bug: T218074
Depends-On: I155024341e8e6b13240e37b30c31b95dc83a47e0
Change-Id: I979e45110bc0e76b499679184993085062ffcac5
And report an invalid ID in this case. Also, assume that the filter is
hidden if the global DB is not available, for consistency with the UI.
Bug: T272593
Change-Id: Ic08023161d95be5cadc8837d3aaaf941cacd89bd
Use null if no version can be found, like the previous code.
Follow-up: I747216df65c2f34f7167612e90506890bc61880a
Bug: T272505
Change-Id: Ie574523fb8a779dda495b05ed6d56fd3f4086f1d
This will not be correct if the target already has a partial block
applied (which is very rare BTW). Leaving a TODO because this is low
priority.
Also keep returning the status in tests, because it makes tests easier
to write.
Change-Id: Ifac795125927d584a31d95e1b4c4241eef860fa1
The DB lookup was changed to return ExistingFilter objects, not Filter,
and FilterRunner also requires ExistingFilter's. So update the version
to avoid fatals due to cached data.
Bug: T272248
Change-Id: I1076f65df5b6d030cea40beb2266c9ec54fa675f
In particular, this brings stronger typing for getID(), and we can get
rid of many phan suppressions.
Change-Id: Icbf3a6f7db8105082646ec227f62c09449fb165d
With explicit calls it's easier to see what method is being used,
whether it's deprecated, etc. Some methods here are in fact deprecated
or already have a proper replacement, but this is left for a follow-up.
Change-Id: Iee3154855f86c76aab98e7c14250c14e8b9ee939
- Exclude a couple of classes from coverage reports
- Add tests for all handlers
- Add tests for the runner, copied from core
- Make AbuseFilterRunner a real service
Change-Id: I7a0fe3cd8300faef5ef72d7f986b1734c324d8d1
This is using core methods, so it can be unit tested. The same isn't
true for load-recent-authors, which performs a custom DB query and whose
test is probably the slowest AbuseFilter test. Simplify it for now,
until the method is moved to MW core.
Change-Id: Ifbdae1a06aabca996eeac151a6d029fd991ad64d
Additionally, avoid building Title objects in LazyVariableComputer, it
just adds a dependency on TitleFactory and creating mocks is more
complicated, but it's pointless because the caller already has a Title
object.
And also stop using Title::getEarliestRevTime(), since the replacement
is easy (we already have a RevisionLookup).
Note for reviewers about renames:
- Code VariableGeneratorDBTest was moved to LazyVariableComputerDBTest,
RCVariableGeneratorTest, and AbuseFilterVariableGeneratorTest
- AbuseFilterVariableGenerator test was moved into a dedicated
directory, methods were changed not to test the var values
Change-Id: I3dff8739a9b79f33321d836449b082c3ce63f277
The checkbox should only appear on Special:AbuseLog, not when deleting
items (checked with $this->hideEntries), AND not when viewing details of
a single entry, which is check with $isListItem.
Change-Id: Id2db07641bf98992b4838e4e7439ac3ee4b1ad8e
Mostly uncaught exceptions, that appeared in places where the previous
code was silently using DWIM-style booleans.
Also a TypeError due to ViewDiff not using filter objects.
Copy the fix from Ic8032592799756521a59ee23c0e76cb03a510b94 to another
place as well.
Bug: T271430
Bug: T271431
Bug: T271432
Bug: T271433
Change-Id: Ica4b82024c57482656cf6bca95bf37641c09cb9a
Mainly constructor and conditions limit, which can be removed from
ConsequencesTest (where it was very slow).
Additionally, inject globals into FilterRunner.
Change-Id: I56ca67de6878dbc2185038faae3eb2b04fb56be9
Additionally:
- Add typehints for stronger typing, and use strict comparison in the
callers
- Use MIN instead of sorting, as the former is optimized by the DBMS;
sorting was also happening on the wrong key, i.e. afh_timestamp, as
opposed to afh_id
Change-Id: I631772fdfeb510b0bc8b582b84bcf2533d7bc097
Code change: in buildVarDumpTable remove special-cased null value. This
was used to avoid passing null to Html::element, but is no longer
necessary, since we now pretty-print the value.
Change-Id: I6180f6c53448d2a8c8c6066f222e9fd9df577554
So everything can be loaded using PSR-4. These classes weren't renamed,
nor the alias for the AbuseFilter class was deprecated, because they
should be refactored first.
Change-Id: Ia328db58eb326968edf5591daac9bacf8c2f75da
So we can use DI in all generators. Some improvements were deliberately
omitted, e.g. injecting more services and relaxing User/Title to
UserIdentity/LinkTarget, and they'll be included in a subsequent commit.
Depends-On: I1f351071ef2b0b7c80e91407a9c3bb17be293044
Depends-On: Ie71740fac35a86f8fe03023080ae8ca08671243d
Depends-On: I589a0e1c2c5891070ab82cd5adfd9cedec19e67d
Change-Id: I92ef0abd5e45b672e6f297a71b3c2c345d56f136
This makes VariableHolder a true value object, and introduces a
stateless service, VariableManager, to operate on it.
Note, in theory, this new service is still cyclically coupled with
LazyVariableComputed. However, it's now two stateless service being
coupled, not two smart/god value objects, so we've still earned
something. For now, the dependency is hidden by using a callback. Some
alternatives for that are mentioned in a code comment.
Bug: T261069
Change-Id: I2f2c84c8e91472ba36084a8bbb4a923f6e04354b
Documentation is already in hooks.txt and in every hook interface, let's
not have to maintain it in a third place.
Change-Id: I8cc5e52b6bc164d9512d22283700966d4c51b943
I think either all or none should consider global filters.
Are there any backwards compatibility concerns?
Change-Id: I22b664e9752588edc195dc4e4f5369392f91ad23
This is an important step towards removing the AbuseFilter class. Note:
proposals for the name of the new service are welcome.
Change-Id: Ib4632173f728b1bdafadef96e01645a833bfceaa
Moves more methods away from the AbuseFilter class. Testing
buildVarDumpTable is not easy because we'd have to parse the generated HTML.
Change-Id: I073a537201de150ba9dd7bf15a99f3a009dc6ba1
Skip a test that fails with
Wikimedia\Rdbms\DBQueryError: Error 5: database is locked
Function: Wikimedia\Rdbms\Database::beginIfImplied (MediaWiki\Extension\AbuseFilter\FilterLookup::getAllActiveFiltersInGroupFromDB)
Probably due to some concurrency issue caused by the duplicate connection, and also with
Wikimedia\Rdbms\DBQueryError: Error 1: no such table: unittest_external_abuse_filter
Function: MediaWiki\Extension\AbuseFilter\FilterLookup::getAllActiveFiltersInGroupFromDB
for unknown reasons.
Move the mwGlobals override inside the test to avoid the same "database is locked" error
on every other test in that class.
Bug: T251967
Change-Id: I552a8d1fa532941f630fd734e590993e7462aeb0
Introduce ReversibleConsequence interface for Consequence classes
whose potentially destructive actions can be reverted using
Special:AbuseFilter/revert. This allows moving reverting logic from
AbuseFilterViewRevert to individual Consequence classes and testing.
Unfortunately, the code is definitely not very clean now.
Change-Id: I558da711f1645ccf64792c6102cf743827171320
See task for a description of the plan. Also note that
AFComputedVariable should be renamed and its properties made private.
This commit includes some adjustments for taint-check in
AbuseFilter::buildVarDumpTable and ::revisionToString.
There's some space for improvement in the new LazyVariableComputer, but
that's left for another commit.
Bug: T261069
Change-Id: Ia44f6e079d39f44cf0122dec5ddb5513ab54f0c6
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
