A filter using a protected variable can be loaded via filter id
using testing tools even though the user might not have the right
to view protected variables. This can potentially leak PII and as
such, testing tools should check for the right before allowing
protected filters to be seen.
- Unload a filter asap if it uses protected variables and the
requestor doesn't have viewing rights. This:
+ disallows loading of existing protected filters on page load
+ disallows testing against rules that use protected variables
+ disallows subsequent requests for protected filters (via API)
There is a known bug (see T369620) where no user feedback is
provided if an API request for a filter returns no result (typically
when no filter matches the requested id). This commit adds another
pathway to that bug (the filter exists but is protected and not
returned by the API) but does not update this UI/UX.
Bug: T364834
Change-Id: I6a572790edd743596d70c9c4a2ee52b4561e25f3
Why:
- Ie13181b78b8e2903c6cc0f0f778689bcc8b8ce2e modifies the status message
returned
What:
- Loosen the check for the status error message such that 'captcha-edit'
and 'captcha-edit-fail' are both valid; we can revert this after
Ie13181b78b8e2903c6cc0f0f778689bcc8b8ce2e is merged
Change-Id: I5a0698d84932a474800a68dba9b76b3433b19290
Why:
- These checks are no longer needed, now that Idc47bda has been merged
What:
- Remove the `method_exists()` checks
Change-Id: I6e428df6b6e036146ae4cc57374cde8810d3f5f7
Search is restricted to users with the right to view private variables
but not necessarily the right to view protected variables. Users who
don't have the right to view protected variables shouldn't be able to
search against protected variables, as this might leak the PII.
- Filter out filters using protected variables in search results
if the user doesn't have the right to view protected variables
Bug: T367390
Change-Id: I7412112c9cc676f29d706b116b779bc17183a952
- Rename `$hidden` to `$privacyLevel` in Flags::__construct for
consistency with other places.
- Rename `shouldProtectFilter` and simplify its return value to always
be an array, since that's how it's currently used. Rename a variable
that is assigned the return value of this method.
- Add a missing message key to a list of dynamic message keys.
- Rename a property from 'hidden' to 'privacy' in FilterStoreTest for
consistency. Add a test for removing the protected flag.
- Update old comment referencing `filterHidden`; the method was removed
in I40b8c8452d9df.
- Use ISQLPlatform::bitAnd() instead of manual SQL in
AbuseFilterHistoryPager.
- Update mysterious reference to "formatRow" in SpecialAbuseLog.
- Update other references to the very same method in two other places,
this time credited as "SpecialAbuseLog".
- Add type hints to a few methods; this not only helps with type safety,
but it also allows PHPUnit to automatically use the proper type in
mocks.
Change-Id: Ib0167d993b761271c1e5311808435a616b6576fe
This test globally set `setForceShowCaptcha` to true, which caused
problems for following tests.
Bug: T368705
Change-Id: I5077e4b874c1bf1c6b68895349af0c9ecd4094ed
Why:
- The previous attempt to integrate AbuseFilter with ConfirmEdit set
a flag on the request object
(I110a5f5321649dcf85993a0c209ab70b9886057c) didn't work in WMF
production because in WMF, we load ConfirmEdit first, followed by
AbuseFilter. Therefore any flag set in an AbuseFilter hook is ignored
by ConfirmEdit
What:
- Remove implementation of ConfirmEditTriggersCaptchaHook, as this does
not work when AbuseFilter is loaded after ConfirmEdit.
- Repurpose onConfirmEditTriggersCaptcha to handle non-edit actions only
- Implement the EditFilterMergedContent hook and call SimpleCaptcha's
public confirmEditMerged method if CaptchaConsequence has specified
that a CAPTCHA should be displayed, and if the CAPTCHA has not already
been solved
Soft-Depends-On: Idc47bdae8007da938f31e1c0f33e9be4813f41d7
Bug: T20110
Change-Id: I7dd3a7c41606dcf5123518c2d3d0f4355f5edfd3
Changes to the use statements done automatically via script
Addition of missing use statement done manually
Change-Id: I48fcc02c61d423c9c5111ae545634fdc5c5cc710
First patch in a series of dropping the old columns.
Wikis now need to run the maintenance script (e.g., via
update.php) prior to serving this commit.
Wikimedia wikis are already on SCHEMA_COMPAT_NEW stage.
Bug: T188180
Change-Id: I86ec2b816eed17b62bf02bfd085570f132011b3e
- Add a basic checkbox on the filter edit page that must be checked if a
filter uses a protected variable to ensure that the user is aware that
their filter will also become protected
Bug: T364485
Change-Id: I7c7652f7d1a81223229b839ff7eee5da4af74c8a
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
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
InsertQueryBuilder does not ignore insert of no rows,
adding some conditions to avoid calling the query builder
Change-Id: I1752b90cc3a7ec3a7f9ee32a1873bf8c82b6e02e
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:
* 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
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
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
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
