empty() only makes sense when the expression it checks is possibly
undefined, otherwise it's equivalent to a truthiness check with the
additional downside of suppressing errors when it's not wanted.
Replace it with simple truthiness checks, using strict comparison when
that seems to help with polymorphic variables.
These were caught by a bespoke phan plugin.
Change-Id: Ide262162553d2da7e5388d05e8731529c44591c1
The design principle for SelectQueryBuilder was to make the chained
builder calls look as much like SQL as possible, so that developers
could leverage their knowledge of SQL to understand what the query
builder is doing.
That's why SelectQueryBuilder::select() takes a list of fields, and by
the same principle, it makes sense for UpdateQueryBuilder::update() to
take a table. However with "insert" and "delete", the SQL designers
chose to add prepositions "into" and "from", and I think it makes sense
to follow that here.
In terms of natural language, we update a table, but we don't delete a
table, or insert a table. We delete rows from a table, or insert rows
into a table. The table is not the object of the verb.
So, add insertInto() as an alias for insert(), and add deleteFrom() as
an alias for delete(). Use the new methods in MW core callers where
PHPStorm knows the type.
Change-Id: Idb327a54a57a0fb2288ea067472c1e9727016000
This class is used heavily basically everywhere, moving it to Utils
wouldn't make much sense. Also with this change, we can move
StatusValue to MediaWiki\Status as well.
Bug: T321882
Depends-On: I5f89ecf27ce1471a74f31c6018806461781213c3
Change-Id: I04c1dcf5129df437589149f0f3e284974d7c98fa
Using a php parser written on top of ANTLR4, done semi-automatically.
I checked everything and made adjustments.
Bug: T311866
Change-Id: I6150c6909bce8f3dbd745a26380cc0af9d9c547f
I did this using a script written on top of antlr4 parser so it doesn't
have some clean ups a human would do but it's pretty nice already.
Bug: T330640
Change-Id: I608566700c6d737ee986bf47dda87effc69614d6
The Hooks class contains deprecated functions and the whole class is
going to get removed, so remove the convenience function and inline the
code.
Bug: T335536
Change-Id: I8ef3468a64a0199996f26ef293543fcacdf2797f
This covers all occurrences of /onfig->.*get( '/ in includes/.
Undoubtedly there are still plenty more to go.
Change-Id: I33196c4153437778496f40436bcde399638ac361
Now that CentralIdLookup doesn't require full User objects
Bug: T265769
Follow-up: I67bf4f2442269b8aa1002868ede9a71a5ca43a84
Change-Id: I953333116f6cd6dc095914929914a970cc190292
I was considering adding a separate service for the creation/fetching
of BotPassword instances (probably BotPasswordLookup or
BotPasswordFactory) but since it would need to access the same
bot password database that BotPasswordStore uses, it would either
need to duplicate the logic, or have a store injected, which would
reduce the benefits of having separate services.
The following methods were moved:
* newFromUser (also renamed to getByUser)
* newFromCentralId (also renamed to getByCentralId)
* newUnsaved
Additionally, BotPassword::__construct() is made public, though
marked as @internal
Bug: T265769
Change-Id: I2a5f46a8fbdf3867517eeb006d886b943fa85af1
BotPassword::invalidateAllPasswordsForCentralId() and
BotPassword::removeAllPasswordsForCentralId() are no
longer used following introduction of BotPasswordStore.
Bug: T265769
Change-Id: I77589032a8b771d1e111e8adbee3600f181fedbc
Does not (yet) handle looking up bot passwords
in the database, just inserting/updating/removing
them.
Bug: T265769
Change-Id: I20e3aaee63ec182483489681282dae52fe41ec90
In all these cases the property is unconditionally set in
the constructor. The extra initialisation is effectively
dead code and an extra source of errors and confusion.
Change-Id: Icae13390d5ca5c14e2754f3be4eb956dd7f54ac4
This includes fixing some mistakes, as well as removing
redundant text that doesn't add new information, either because
it literally repeats what the code already says, or is actually
duplicated.
Change-Id: I3a8dd8ce57192deda8916cc444c87d7ab1a36515
For example, documenting the method getUser() with "get the User
object" does not add any information that's not already there.
But I have to read the text first to understand that it doesn't
document anything that's not already obvious from the code.
Some of this is from a time when we had a PHPCS sniff that was
complaining when a line like `@param User $user` doesn't end
with some descriptive text. Some users started adding text like
`@param User $user The User` back then. Let's please remove
this.
Change-Id: I0ea8d051bc732466c73940de9259f87ffb86ce7a
The class is not stable to extend, so convert things
from protected to private.
Will be followed by a patch introducing a new
BotPasswordStore service.
Bug: T265769
Change-Id: I318131685fcec80816bd7b59ed27baa6511da3c6
This is useful in testing environments where we want to spin up new mediawiki
instances and configure them programatically.
Change-Id: Ice94b67cb8775786a754de51010e78211954b2b0
Migrate all callers of Hooks::run() to use the new
HookContainer/HookRunner system.
General principles:
* Use DI if it is already used. We're not changing the way state is
managed in this patch.
* HookContainer is always injected, not HookRunner. HookContainer
is a service, it's a more generic interface, it is the only
thing that provides isRegistered() which is needed in some cases,
and a HookRunner can be efficiently constructed from it
(confirmed by benchmark). Because HookContainer is needed
for object construction, it is also needed by all factories.
* "Ask your friendly local base class". Big hierarchies like
SpecialPage and ApiBase have getHookContainer() and getHookRunner()
methods in the base class, and classes that extend that base class
are not expected to know or care where the base class gets its
HookContainer from.
* ProtectedHookAccessorTrait provides protected getHookContainer() and
getHookRunner() methods, getting them from the global service
container. The point of this is to ease migration to DI by ensuring
that call sites ask their local friendly base class rather than
getting a HookRunner from the service container directly.
* Private $this->hookRunner. In some smaller classes where accessor
methods did not seem warranted, there is a private HookRunner property
which is accessed directly. Very rarely (two cases), there is a
protected property, for consistency with code that conventionally
assumes protected=private, but in cases where the class might actually
be overridden, a protected accessor is preferred over a protected
property.
* The last resort: Hooks::runner(). Mostly for static, file-scope and
global code. In a few cases it was used for objects with broken
construction schemes, out of horror or laziness.
Constructors with new required arguments:
* AuthManager
* BadFileLookup
* BlockManager
* ClassicInterwikiLookup
* ContentHandlerFactory
* ContentSecurityPolicy
* DefaultOptionsManager
* DerivedPageDataUpdater
* FullSearchResultWidget
* HtmlCacheUpdater
* LanguageFactory
* LanguageNameUtils
* LinkRenderer
* LinkRendererFactory
* LocalisationCache
* MagicWordFactory
* MessageCache
* NamespaceInfo
* PageEditStash
* PageHandlerFactory
* PageUpdater
* ParserFactory
* PermissionManager
* RevisionStore
* RevisionStoreFactory
* SearchEngineConfig
* SearchEngineFactory
* SearchFormWidget
* SearchNearMatcher
* SessionBackend
* SpecialPageFactory
* UserNameUtils
* UserOptionsManager
* WatchedItemQueryService
* WatchedItemStore
Constructors with new optional arguments:
* DefaultPreferencesFactory
* Language
* LinkHolderArray
* MovePage
* Parser
* ParserCache
* PasswordReset
* Router
setHookContainer() now required after construction:
* AuthenticationProvider
* ResourceLoaderModule
* SearchEngine
Change-Id: Id442b0dbe43aba84bd5cf801d86dedc768b082c7
There is no guarantee that this is a maintenance able connection,
but drop, create or alter is also not needed in this case
Change-Id: I7dd294b3d28ef8c4e416e826f38ed39f824b2f8f
I would argue that these comments do not add any information that
would not be there already. Having them adds mental overhead, because
one needs to read both the comment and the next line of code first to
understand they say the exact same. I don't find this helpful, but
more distracting.
Change-Id: I39c98f25225947ebffdcc2fd8f0243e7a6c070d7
So far, our key derivation code assumed that it has control over
the salt used by the derivation routines, however I want to add Argon2
support and it doesn't work this way: password_hash() generates the
salt itself, and the only way to verify a password is by using
password_verify(). Current way the things are done doesn't support it
because it relies on the result of password hashing with parameters we
provide to be deterministic.
Therefore, I'm deprecating Password::equals(), as well as whole concept
of comparing Password objects - it's used only in tests anyway. It's
getting replaced with verify() that only accepts password strings.
Uses of old function are fixed with exception of a few calls in tests
that will be addressed in my Argon2 patch.
Change-Id: I2b2be9a422ee0f773490eac316ad81505c3f8571
Authentication audit logs should indicate whether a login is via the
normal password or a bot password (and which one). For failed logins
it could be included in the error message, and it usually is, but
for successful ones there is no message, so we'll send the app ID as
a new AuthManagerLoginAuthenticateAudit parameter.
Bug: T194338
Change-Id: I8aab48177b81a8e80dae118e6476a8f6a32089f1
Depends-On: Id482d2e2205960a0facd334e456d3a23bcad0ece
To facilitate logging all authentications, even ones not via
AuthManager, BotPassword::login() should call the
AuthManagerLoginAuthenticateAudit hook.
Bug: T194338
Change-Id: I27b75c56e060fcb8f71d84ddc2eb8e1533fef33a
Coverage is 100% except for one session-related bit that seems a bit
involved to test right now. It looks like it will be easier once
SessionManager becomes a service.
I removed the third parameter from the return value of
canonicalizeLoginData, since af37a4c7 made it always return true.
I also removed three lines of dead code from ApiLogin.php.
Change-Id: Ia0073eddd27c82827518e0031e3c313f83cfd7cc
Instead of having basically every caller do:
$pf = new PasswordFactory();
$pf->init( RequestContext::getMain()->getConfig() );
Just create a single PasswordFactory via MediaWikiServices and pass that
around. Things that want to use their own config can still pass settings
via the new constructor.
This will eventually let us remove the init() function, removing the
only hard dependency upon MediaWiki, to make it easier to librarize
(T89742).
Change-Id: I0fc7520dc023b11a7fa66083eff7b88ebfe49c7b
Find: /isset\(\s*([^()]+?)\s*\)\s*\?\s*\1\s*:\s*/
Replace with: '\1 ?? '
(Everywhere except includes/PHPVersionCheck.php)
(Then, manually fix some line length and indentation issues)
Then manually reviewed the replacements for cases where confusing
operator precedence would result in incorrect results
(fixing those in I478db046a1cc162c6767003ce45c9b56270f3372).
Change-Id: I33b421c8cb11cdd4ce896488c9ff5313f03a38cf
Certain things, such as changing the account's main login credentials,
causes all bot passwords to be invalidated. This state should be
indicated in Special:BotPasswords, and the API when login fails.
Bug: T193829
Change-Id: Ib12929fed861742c9f2f76702c9ac3254e8a5d97
ApiLogin which will currently always try an AuthManager login which will
by default throttle via ThrottlePreAuthenticationProvider, but this only
happens after the BotPassword is checked so it's still possible to keep
trying to break the bot password.
There's a potential odd-behavior mode here: if the main account username
and password looks like a BotPasswords username and password, a
successful main account login will increment the BotPasswords throttle
for the user and not reset it after the successful main account login.
That seems such an odd edge case I say let's not worry about it.
Bug: T165846
Change-Id: Ie60f0e05c2a94722b91bc3a80c80346e28b443f4
And auto-fix all errors.
The `<exclude-pattern>` stanzas are now included in the default ruleset
and don't need to be repeated.
Change-Id: I928af549dc88ac2c6cb82058f64c7c7f3111598a
