Some exposed variables (eg. `user_ip`) used in filters are sensitive
and need to only be available to restricted groups of users.
Back-end changes:
- Add `AbuseFilterProtectedVariables` which defines what variables are
protected by the new right `abusefilter-access-protected-vars`
- Add the concept of a `protected` variable, the use of which will
denote the entire filter as protected via a flag on `af_hidden`
New UX features:
- Display changes to the protected status of filters on history and diff
pages
- Check for protected variables and the right to see them in filter
validation and don't allow a filter to be saved if it uses a variable
that the user doesn't have access to
- Check for the right to view protected variables before allowing access
and edits to existing filters that use them
Bug: T364465
Bug: T363906
Change-Id: I828bbb4015e87040f69a8e10c7888273c4f24dd3
Protected variables will cause the filter using them to become
protected as well. `af_hidden` can be used to track this flag,
as it is a TINYINT and can be converted into a bitmask with no
schema changes.
This is not a backwards-compatible change, as now all checks must
check the `hidden` flag specifically or otherwise will be cast to
true if any flag is set.
To support this change:
- "hidden" is considered a flag set in the `af_hidden`. This is a
change in concept with no need for updates to the column values,
as there is currently only one flag in the bitmask.
- `Flag`s store the bitmask as well as the state of single flags
and can return either.
- Any checks against the `af_hidden` value no longer check a
boolean value and instead now check the `hidden` flag value.
Bug: T363906
Change-Id: I358205cb1119cf1e4004892c37e36e0c0a864f37
After temporary accounts are enabled, filters that rely on an ip
in the `user_name` will fail (eg. `ip_in_range` and `ip_in_ranges`).
To keep these filters working:
- Expose the IP through another variable, `user_unnamed_ip`, that can be
used instead of `user_name`.
- The variable is scoped to only reveal the IPs of temporary accounts
and un-logged in users.
- Wikis that don't have temporary accounts enabled will be able to see
this variable but it won't provide information that `user_name`
wasn't already providing
- Introduce the concept of transforming variable values before writing
to the blob store and after retrieval, as IPs need to be deleted from
the logs eventually and can't be stored as-is in the amend-only blob
store
Bug: T357772
Change-Id: I8c11e06ccb9e78b9a991e033fe43f5dded8f7bb2
Why:
- Temp accounts will be enabled by default in CI, eventually. For tests
that verify anonymous user editing behavior, we need to disable the
temp user feature
What:
- Check if the user identity isn't registered, and if so, disable auto
creating a temp user
Bug: T365645
Change-Id: I477fb6b44655e4190b5906c85390133e3e3a9feb
Why:
- We want AbuseFilter to able to require a CAPTCHA if an action
matches conditions in an AbuseFilter
What:
- Implement the ConfirmEditTriggersCaptcha hook, and check to see if
the CaptchaConsequence set a global flag that indicates if we
should show a CAPTCHA
Depends-On: Ie87e3d850541c7dc44aaeb6b30489a32a0c8cc60
Bug: T20110
Change-Id: I110a5f5321649dcf85993a0c209ab70b9886057c
Follows-up I5a5420df13893386.
> We lose useful coverage and waste valuable time on keeping tags
> accurate through refactors (or worse, forget to do so).
>
> Tracking tiny per-method details wastes time in realizing (and
> fixing) when people inevitably don't keep them in sync, and time
> lost in finding uncovered code to write tests to realize it was
> already covered but "not yet claimed".
>
> Given all used methods are de-facto and liberally claimed, and
> that we keep the coverage limited to the subject class, this
> maintains the spirit and intent. PHPUnit offers a more precise
> tool when you need it (i.e. when testing legacy monster classes),
> but for well-written code, the class-wide tag suffices.
Ref https://gerrit.wikimedia.org/r/q/owner:Krinkle+is:merged+message:Widen
Change-Id: If7304d8b5b43ab8a051fbcecced331a787bab960
> We lose useful coverage and waste valuable time on keeping tags
> accurate through refactors (or worse, forget to do so).
>
> Tracking tiny per-method details wastes time in realizing (and
> fixing) when people inevitably don't keep them in sync, and time
> lost in finding uncovered code to write tests to realize it was
> already covered but "not yet claimed".
>
> Given all used methods are de-facto and liberally claimed, and
> that we keep the coverage limited to the subject class, this
> maintains the spirit and intent. PHPUnit offers a more precise
> tool when you need it (i.e. when testing legacy monster classes),
> but for well-written code, the class-wide tag suffices.
Ref https://gerrit.wikimedia.org/r/q/owner:Krinkle+is:merged+message:Widen
Change-Id: I5a5420df1389338644a099ebfd072063653e6849
The static method has already been migrated.
Also rewrite the test cases to avoid non-static provider (T337144).
Change-Id: Ibf98539f442e1ba8a9e9eb510784d40778123f17
MessageLocalizer::msg( $key, ...$params ) accepts a variable number
of arguments, but getMockMessage does not. Passing only the first
argument as a string to getMockMessage is not supported.
Required for I86ff2d6.
Change-Id: I30ab799ceab36b80c330d1233f3009814d7f6c98
It does the same and is well supported in the PHP versions we
currently use, at least when the arrays contain integer keys.
Change-Id: Id038142621dab47bfc03db48ce676ed0b2cdc28b
InsertQueryBuilder does not ignore insert of no rows,
adding some conditions to avoid calling the query builder
Change-Id: I1752b90cc3a7ec3a7f9ee32a1873bf8c82b6e02e
Introduced in 2019 with 4c8dac4dc6. Redundant since 2020 with
commit c6c62e2c8f in MediaWiki core.
Bug: T139216
Change-Id: I51e9fc3899cf5505917d7899a395350dd86f5c0b
And more db clean ups:
- Use QueryBuilders
- Stop relying on actor migration to simplify query building
- Using expression builder in one case.
- Change the default actor migration stage to read new and write both.
Bug: T354194
Depends-On: I7c116cab0c748707d9a9fd17feeffe26e7d188ec
Depends-On: I74002911749335f4323a03fb430d02f936771b7e
Change-Id: Id84d1db7a2991f3cccc2f4f1502ba77643ddef24
The following sniffs are failing and were disabled:
* MediaWiki.Commenting.FunctionComment.MissingDocumentationPublic
Change-Id: I6075c76d53a899aac56af027f9a956a6b9e6a667
Why:
* An AbuseFilter variable is needed that allows filters to determine
what type the current user is. That is, whether the user is an
IP address, temporary account, named user or external user.
* Currently filters implement this by inspecting the value in
the 'user_name' variable, but this is likely to break when
temporary accounts are enabled as IPs would be hidden.
* Giving a dedicated variable that indicates the type of the user
allows filters to work out this information without having to
know the specific username of the user before performing the
check.
What:
* Add the 'user_type' variable which is lazily computed. It can have
the value 'named', 'temp', 'ip' or 'external' depending on the
type of the user. If the user does not match any of these, then
the value is 'unknown'.
* Replace call to deprecated User::newFromIdentity with a use of the
UserFactory service that is dependency injected.
* Add and update tests to ensure consistent test coverage.
Bug: T357615
Change-Id: Ifffa891879e7e49d2430a0330116b34c5a03049d
This patch solves a pending TODO which is to remove the ::factory()
method from the AutoPromoteGroupsHandler class. If the cache instance
is injected, we'll use it otherwise we'll default to a HashBagOStuff.
Bug: T358346
Change-Id: I2bc414da8733431d1d11025e954282fc7c73aa80
Why:
* In f3c87749b8, the sending of logs
to CheckUser when using a temporary account was fixed. As part
of that change, it was suggested to add a test to verify that
the call to `Hooks::updateCheckUserData` actually causes an
insert into the relevant CheckUser result table.
* This change adds this test as a follow-up.
What:
* Create a test for the AbuseLogger that verifies that an event is
sent to CheckUser and is saved into the DB for an abuse filter
hit. This test is only run if the CheckUser extension is installed.
Bug: T358632
Change-Id: I33ed0810db13e38eacf4e682eb54d4ffcd583084
LibUp was getting caught out by the false-positive hit to
es-x/no-resizable-and-growable-arraybuffers, so doing a
manual fix to make progress.
Otherwise just whitespace changes.
Change-Id: Ibf20dd2d88613b0a74813c33239e7d0bac682b17
Why:
* When CheckUser asks the AbuseFilter extension for modifications
to rows inserted into the CheckUser tables, the AbuseFilter
extension attempts to get the Filter user via User::newSystemUser
* User::newSystemUser can deadlock if multiple requests to create
the system user are being made at once.
* The CheckUserHander does not need to create the abuse filter system
and instead only needs to know if a given $user is the equal to
the FilterUser.
* As such the FilterUser service needs to provide a way to check if
a given $user is equal without creating the FilterUser.
What:
* Add FilterUser::isUserSameAs which returns a boolean value
indicating whether the Abuse Filter system user is the equal
to a given UserIdentity in the same way that UserIdentity::equals
is implemented.
* Refactor ::getUser to get the username for the filter user in
a separate method, so that the ::isUserSameAs method can also
use this method. Name this new method ::getFilterUserName.
* Add a test for the FilterUser service to ensure consistent test
coverage
* Convert the @covers and @coversDefaultClass annotations to be
a @covers for the class. This is because PHPUnit recommends this in
https://docs.phpunit.de/en/9.6/annotations.html#appendixes-annotations-covers-tables-annotations
Bug: T356275
Bug: T346967
Change-Id: I8a101781bb47612deabb0f2a06a398ac13e860e6
We are getting rid of the schema of implementing this interface and
calling self::READ_* constants, it's confusing, inconsistent, prone to
clashes and isn't really useful for non-ORM systems (which we are not)
Bug: T354194
Change-Id: I5d7a2c91a49311a6bdf6e56053c08610d4d6d110
Deprecated in T342301 in v1.41. This is now tracked
automatically. The variable can be safely deleted.
Change-Id: I7f42f3bfc58508421f4758089482fd1ed68c42c2
This requires 1.42 for some new names
Changes to the use statements done automatically via script
Addition of missing use statements and changes to docs done manually
Change-Id: Ic1e2c9a0c891382744e4792bba1effece48e53f3
The BadMethodCallException is documented as "thrown if a callback
refers to an undefined method or if some arguments are missing".
This is not what happens in these places.
Change-Id: Ic95b67acc2e17eea1dd0fa1d72f9ac94a86bcf17
Turns out the MediaWikiIntegrationTestCase base class comes with a
bunch of convenience methods for this. We can even remove the custom
messages because these methods already print a lot of debug info in
case of a failure.
Change-Id: I61fd86f1560c8e3bcf39a30b09fecdb063424613
Reverts part of Id9056528a433faf0, to switch to DeferredUpdates in
CirrusSearch back from DeferredUpdatesManager.
Bug: T265749
Change-Id: I8126cc76440724753c356c48ba4e0fcc9be5b41a
Most notably, make it not use additional DB tables to test global
filters. Instead, just pretend that the local database is not local (via
config) and "hide" local filters with a simple test-only flag in
FilterLookup.
Change-Id: Ib431dbf6c9d84978ee84e7f0671cfcbf8a54d7a2
Echo will at some point try to load the user with the given ID, and will
throw an exception if it doesn't exist. The test is currently passing
just because we're not properly cleaning DB tables, and the user with ID
1 happens to exist at that point, but it will fail with core change
Ie2f1809d.
Change-Id: Ie686f4d5c2842e45a6ed564b311bb5d9b0265091
tests\selenium\specs\importingFilters.js
81:1 warning This line has a length of 106. Maximum allowed is 100
max-len
Change-Id: Ic8bee37971afb6fe4ba7102d687f0fa212092f4c
These tests were accessing the Database, for mainly 3 reasons:
- User::newSystemUser
- Static methods in ChangeTags
- Echo's Event class
There isn't much we can do about them, so add tests to the Database
group where needed. In some cases, there are already comments that these
tests should be made unit tests once possible.
Bug: T155147
Change-Id: I8a0d52e0a4cae8a4059b62867853a73e60c878a1
TestUser requires a DB connection, so avoid using it in database-less
tests. Add to the Database group tests that are making DB writes (e.g.,
for log entries).
Change-Id: I211cb60296e5c2446128fcdf2caaadc728a8c272
I'm planning to add support for bypass and regex-based blocking which
means it'll grow a bit. So let's give it a dedicated class.
Bug: T337431
Change-Id: I5a6fe2fd2f1efdebd8cada0ba6c481341f830e27
The handler class uses hook interfaces from the CheckUser extension, so
it can't run if CheckUser is not installed.
Change-Id: I5f40366f27cc885e95e1bb93ec421b09c7caa9a6
This method actually consists of two: add derived vars, and initialize
content vars. The former part depends on no parameters of this method.
On the other hand, the latter part combines multiple implementations
for some of the content variables using branching.
The branching is a dirty workaround and inferior to the GRASP principle:
"When related alternatives or behaviors vary by type, assign
responsibility for the behavior to the types for which the behavior
varies."
In other words, the callers (extensions) should be responsible for
choosing the initialization strategy themselves, instead of letting
VariableGenerator figure it out.
As the first step, split the former part to a separate method.
For now, it will be implicitly called by ::addEditVars.
Change-Id: I5ff00dbdbf29ec54eabfd95c44a4fd7f713969f5
Service wiring should only depend on config, not on request state.
Creating a session object during service wiring causes issues with entry
points such as opensearch_desc.php that disable the session.
Bug: T340113
Change-Id: I2450b0b6821ff0b097e283ff660a0b8aeea9590a
Protected effectively means "public to subclasses" and should be
avoided for the same reasons as marking everything as public should
be avoided.
Change-Id: Iba674b486ce53fd1f94f70163d47824e969abb77
Unlike what the 20-year old source comments in UrlUtils.php would
have you believe, parse_url() works fine nowadays, including for
protocol-relative URLs and indeed lots of prod code uses it directly.
The class still has some convenience value for case where you need to
expand or manipulate URLs, but for the common case of extracting a part
of it, you really don't need it.
Test plan:
$ php phpunit.php ../../extensions/AbuseFilter/tests/phpunit/integration/FilteredActionsHandlerTest.php
Bug: T337431
Change-Id: I1e76d2f5aef65365743214530faba656325b965a
Or at least one step forward to better isolation.
Some providers were replaced with individual test cases.
Bug: T337144
Change-Id: I5cd8c5e79993260f18c3a17c40b8501a4da3c17f
And register AbuseFilterRunnerFactory as a service name that’s allowed
to not have a getRunnerFactory() method without the test complaining
(the service was renamed, getFilterRunnerFactory() exists).
Change-Id: Idedb87e64a6df02b0edae8d9e7dbf441752dc480
Needed-By: If5af88e7f70b83d53f66b9617a5ef37daf81830f
When forFilter is true and PreparedUpdate is available
(most save operations), retrieve all_links from
PreparedUpdate::getParserOutputForMetaData. Otherwise
do what was done before.
Note that this change probably leaves some dead code. It will be dealt
with later.
NOTE: this changes code potentially executed on every save operation.
Bug: T65632
Bug: T264104
Change-Id: I3628a56e5277846c1b90444fb55983870eb54c1e
The method for old_links retrieval depends on the "forFilter"
value, which we know in advance. If it's true, old_links should
be retrieved from the database. Make a case in the switch
that does nothing but retrieves links from the database,
and direct the evaluation to it.
This change was split from I3628a56e5 to make its review easier.
NOTE: this changes code potentially executed on every save operation.
Change-Id: I33b688f6be3c58beec403f7bf26407a42e7c18ab
Regarding array building: Instead of adding to array with
$array[] = 'foo' and then doing array_flip(), simply do
$array['foo'] = true;
Regarding tests: I originally wanted to create a unit test but I ended
up mocking so many things that it wasn't worth it and the config variable
is globaly which first we need to clean up after deployment is done.
Bug: T337431
Change-Id: Iac8dca7078668ee3441d19b6aafe499c1aa0d732
Treat temporary users the same as IP users. Neither has user groups,
so return early for both.
Bug: T335062
Change-Id: I20b48608cf6ba5f8e8e36a378d66c603d84b032f
It is behind a feature flag. Improvements on it can happen in follow
ups. The patch is already quite massive.
Bug: T337431
Bug: T279275
Change-Id: I3df949c4d41ce65bb4afa013da9c691ac05fc760
