There's no validation (database level or in code) to check that the authname being saved for a user is not already taken by some other user within the same provider. This situation should not be allowed to happen and makes it easier for depending modules to create a security vulnerability.

I suggest that the Authmap service check if there's already some other user account using that authname, and if so, throw an exception.

CommentFileSizeAuthor
#2 externalauth-prevent_same_auth_name-2804797-2.patch2.44 KBroderik

Comments

bkosborne created an issue. See original summary.

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Status: Active » Needs review
StatusFileSize
new externalauth-prevent_same_auth_name-2804797-2.patch2.44 KB

Agreed.

I don't think an extra explicit check is needed; if we change the database index, the save() will throw an exception, so that should be enough.

And the exception is pretty easy to prevent by a depending module; just call authmap->getUid($authname, $provider) or externalauth->load($authname, $provider). (I implemented this patch when I thought I really needed it to harden a module's code... but that turns out to be a mistake.)

I don't know if this exception should be documented in the interface; I just added comments (in a related and an only vaguely related place, to un-confuse myself).

snufkin’s picture

snufkin commented
Status: Needs review » Reviewed & tested by the community

Yeah this makes sense. The change itself doesn't mean that the module will do extra validation, simply by making the authname and provider fields together a unique index prevents other modules and extensions from saving duplicate information, helping modules that depend on this one from making a mistake.

svendecabooter’s picture

svendecabooter
he/him
Primary language Dutch
Location Gent
commented
Status: Reviewed & tested by the community » Fixed

Thank you for the patch and review.
It didn't apply to current HEAD, but I rerolled and committed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.