Skip to content

Commit

Permalink
Make Consumer[Acceptance] and DAOAccessControl more type-safe
Browse files Browse the repository at this point in the history 
* subclass DAOAccessControl so IDEs can figure out what type will be
  returned by getDAO()
* add proper getters for the fields and for the most commons formatting
* switch between using the DAO and the access control in a few places
* use consistent variable naming ($cmr/$cmra for DAO, $cmrAc/$cmraAc for
  the access controls)
* along the way, fix a bunch of code style warnings, improve what
  exception classes are used to reduce IDE warnings, & remove some
  dead code
* also fix an apparently-unrelated test breakage, probably caused
  by I33fff561 (not sure how that went unnoticed until now)

Change-Id: Icc98b6e431edb102940fea34d8242a0e77ba5495
  • Loading branch information
@tgr
tgr committed Jan 3, 2019
1 parent a1de994 commit 98df9d7
Show file tree
Hide file tree
Showing 24 changed files with 1,074 additions and 384 deletions.
26 changes: 13 additions & 13 deletions api/MWOAuthSessionProvider.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,16 +96,16 @@ public function provideSessionInfo( WebRequest $request ) {
$dbr = MWOAuthUtils::getCentralDB( DB_REPLICA );

$access = MWOAuthConsumerAcceptance::newFromToken( $dbr, $accesstoken->key );
$logData['user'] = MWOAuthUtils::getCentralUserNameFromId( $access->get( 'userId' ), 'raw' );
$logData['user'] = MWOAuthUtils::getCentralUserNameFromId( $access->getUserId(), 'raw' );

// Access token is for this wiki
if ( $access->get( 'wiki' ) !== '*' && $access->get( 'wiki' ) !== $wiki ) {
if ( $access->getWiki() !== '*' && $access->getWiki() !== $wiki ) {
$this->logger->debug( 'OAuth request for wrong wiki from user {user}', $logData );
return $this->makeException( 'mwoauth-invalid-authorization-wrong-wiki', $wiki );
}

// There exists a local user
$localUser = MWOAuthUtils::getLocalUserFromCentralId( $access->get( 'userId' ) );
$localUser = MWOAuthUtils::getLocalUserFromCentralId( $access->getUserId() );
if ( !$localUser || !$localUser->isLoggedIn() ) {
$this->logger->debug( 'OAuth request for invalid or non-local user {user}', $logData );
return $this->makeException( 'mwoauth-invalid-authorization-invalid-user',
Expand All @@ -124,13 +124,13 @@ public function provideSessionInfo( WebRequest $request ) {
}

// The consumer is approved or owned by $localUser, and is for this wiki.
$consumer = MWOAuthConsumer::newFromId( $dbr, $access->get( 'consumerId' ) );
$consumer = MWOAuthConsumer::newFromId( $dbr, $access->getConsumerId() );
if ( !$consumer->isUsableBy( $localUser ) ) {
$this->logger->debug(
'OAuth request for consumer {consumer} not approved by user {user}', $logData
);
return $this->makeException( 'mwoauth-invalid-authorization-not-approved' );
} elseif ( $consumer->get( 'wiki' ) !== '*' && $consumer->get( 'wiki' ) !== $wiki ) {
} elseif ( $consumer->getWiki() !== '*' && $consumer->getWiki() !== $wiki ) {
$this->logger->debug( 'OAuth request for consumer {consumer} to incorrect wiki', $logData );
return $this->makeException( 'mwoauth-invalid-authorization-wrong-wiki', $wiki );
}
Expand All @@ -140,11 +140,11 @@ public function provideSessionInfo( WebRequest $request ) {
// We're not configured to use cookies, so concatenate some of the
// internal consumer-acceptance state to generate an ID.
$id = $this->hashToSessionId( implode( "\n", [
$access->get( 'id' ),
$access->get( 'wiki' ),
$access->get( 'userId' ),
$access->get( 'consumerId' ),
$access->get( 'accepted' ),
$access->getId(),
$access->getWiki(),
$access->getUserId(),
$access->getConsumerId(),
$access->getAccepted(),
$wiki,
] ) );
$persisted = false;
Expand All @@ -165,7 +165,7 @@ public function provideSessionInfo( WebRequest $request ) {
'forceUse' => $forceUse,
'metadata' => [
'key' => $accesstoken->key,
'rights' => \MWGrants::getGrantRights( $access->get( 'grants' ) ),
'rights' => \MWGrants::getGrantRights( $access->getGrants() ),
],
] );
}
Expand Down Expand Up @@ -288,9 +288,9 @@ public function onRecentChange_save( $rc ) {
if ( $data ) {
$dbr = MWOAuthUtils::getCentralDB( DB_REPLICA );
$access = MWOAuthConsumerAcceptance::newFromToken( $dbr, $data['key'] );
$consumerId = $access->get( 'consumerId' );
$consumerId = $access->getConsumerId();
$consumer = MWOAuthConsumer::newFromId( $dbr, $consumerId );
if ( !$consumer->get( 'ownerOnly' ) ) {
if ( !$consumer->getOwnerOnly() ) {
$rc->addTags( "OAuth CID: $consumerId" );
}
}
Expand Down
203 changes: 196 additions & 7 deletions backend/MWOAuthConsumer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
*/

/**
* Representation of an OAuth consumer request
* Representation of an OAuth consumer.
*/
class MWOAuthConsumer extends MWOAuthDAO {
/** @var array Backwards-compatibility grant mappings */
Expand Down Expand Up @@ -233,6 +233,188 @@ public static function getAllStages() {
];
}

/**
* Internal ID (DB primary key).
* @return int
*/
public function getId() {
return $this->get( 'id' );
}

/**
* Consumer key (32-character hexadecimal string that's used in the OAuth protocol
* and in URLs). This is used as the consumer ID for most external purposes.
* @return string
*/
public function getConsumerKey() {
return $this->get( 'consumerKey' );
}

/**
* Name of the consumer.
* @return string
*/
public function getName() {
return $this->get( 'name' );
}

/**
* Central ID of the owner.
* @return int
*/
public function getUserId() {
return $this->get( 'userId' );
}

/**
* Consumer version. This is mostly meant for humans: different versions of the same
* application have different keys and are handled as different consumers internally.
* @return string
*/
public function getVersion() {
return $this->get( 'version' );
}

/**
* Callback URL (or prefix). The browser will be redirected to this URL at the end of
* an OAuth handshake. See getCallbackIsPrefix() for the interpretation of this field.
* @return string
*/
public function getCallbackUrl() {
return $this->get( 'callbackUrl' );
}

/**
* When true, getCallbackUrl() returns a prefix; the callback URL can be provided by the caller
* as long as the prefix matches. When false, the callback URL will be determined by
* getCallbackUrl().
* @return bool
*/
public function getCallbackIsPrefix() {
return $this->get( 'callbackIsPrefix' );
}

/**
* Description of the consumer. Currently interpreted as plain text; might change to wikitext
* in the future.
* @return string
*/
public function getDescription() {
return $this->get( 'description' );
}

/**
* Email address of the owner.
* @return string
*/
public function getEmail() {
return $this->get( 'email' );
}

/**
* Date of verifying the email, in TS_MW format. In practice this will be the same as
* getRegistration().
* @return string
*/
public function getEmailAuthenticated() {
return $this->get( 'emailAuthenticated' );
}

/**
* Did the user accept the developer agreement (the terms of use checkbox at the bottom of the
* registration form)? Except for very old users, always true.
* @return bool
*/
public function getDeveloperAgreement() {
return $this->get( 'developerAgreement' );
}

/**
* Owner-only consumers will use one-legged flow instead of three-legged (see
* https://github.com/Mashape/mashape-oauth/blob/master/FLOWS.md#oauth-10a-one-legged ); there
* is only one user (who is the same as the owner) and they learn the access token at
* consumer registration time.
* @return bool
*/
public function getOwnerOnly() {
return $this->get( 'ownerOnly' );
}

/**
* The wiki on which the consumer is allowed to access user accounts. A wiki ID or '*' for all.
* @return string
*/
public function getWiki() {
return $this->get( 'wiki' );
}

/**
* The list of grants required by this application.
* @return string[]
*/
public function getGrants() {
return $this->get( 'grants' );
}

/**
* Consumer registration date in TS_MW format.
* @return string
*/
public function getRegistration() {
return $this->get( 'registration' );
}

/**
* Secret key used to derive the consumer secret for HMAC-SHA1 signed OAuth requests.
* The actual consumer secret will be calculated via MWOAuthUtils::hmacDBSecret() to mitigate
* DB leaks.
* @return string
*/
public function getSecretKey() {
return $this->get( 'secretKey' );
}

/**
* Public RSA key for RSA-SHA1 signerd OAuth requests.
* @return string
*/
public function getRsaKey() {
return $this->get( 'rsaKey' );
}

/**
* Application restrictions (such as allowed IPs).
* @return \MWRestrictions
*/
public function getRestrictions() {
return $this->get( 'restrictions' );
}

/**
* Stage at which the consumer is in the review workflow (proposed, approved etc).
* @return int One of the STAGE_* constants
*/
public function getStage() {
return $this->get( 'stage' );
}

/**
* Date at which the consumer was moved to the current stage, in TS_MW format.
* @return string
*/
public function getStageTimestamp() {
return $this->get( 'stageTimestamp' );
}

/**
* Is the consumer suppressed? (There is no plain deletion; the closest equivalent is the
* rejected/disabled stage.)
* @return bool
*/
public function getDeleted() {
return $this->get( 'deleted' );
}

/**
* @param MWOAuthDataStore $dataStore
* @param string $verifyCode verification code
Expand All @@ -243,7 +425,7 @@ public function generateCallbackUrl( $dataStore, $verifyCode, $requestKey ) {
$callback = $dataStore->getCallbackUrl( $this->key, $requestKey );

if ( $callback === 'oob' ) {
$callback = $this->get( 'callbackUrl' );
$callback = $this->getCallbackUrl();
}

return wfAppendQuery( $callback, [
Expand All @@ -264,7 +446,7 @@ public function generateCallbackUrl( $dataStore, $verifyCode, $requestKey ) {
* @return bool
*/
public function isUsableBy( \User $user ) {
if ( $this->stage === self::STAGE_APPROVED && !$this->get( 'ownerOnly' ) ) {
if ( $this->stage === self::STAGE_APPROVED && !$this->getOwnerOnly() ) {
return true;
} elseif ( $this->stage === self::STAGE_PROPOSED || $this->stage === self::STAGE_APPROVED ) {
$centralId = MWOAuthUtils::getCentralIdFromLocalUser( $user );
Expand All @@ -283,6 +465,10 @@ protected function normalizeValues() {
$this->emailAuthenticated = wfTimestamp( TS_MW, $this->emailAuthenticated );
$this->deleted = (int)$this->deleted;
$this->grants = (array)$this->grants; // sanity
$this->callbackIsPrefix = (bool)$this->callbackIsPrefix;
$this->ownerOnly = (bool)$this->ownerOnly;
$this->developerAgreement = (bool)$this->developerAgreement;
$this->deleted = (bool)$this->deleted;
}

protected function encodeRow( DBConnRef $db, $row ) {
Expand Down Expand Up @@ -321,7 +507,7 @@ protected function decodeRow( DBConnRef $db, $row ) {
}

/**
* Magic method so that $consumer->secret and $consumer->key work.
* Magic method so that fields like $consumer->secret and $consumer->key work.
* This allows MWOAuthConsumer to be a replacement for OAuthConsumer
* in lib/OAuth.php without inheriting.
* @param mixed $prop
Expand All @@ -332,14 +518,17 @@ public function __get( $prop ) {
return $this->consumerKey;
} elseif ( $prop === 'secret' ) {
return MWOAuthUtils::hmacDBSecret( $this->secretKey );
} elseif ( $prop === 'callback_url' ) {
return $this->callbackUrl;
} else {
return $this->$prop;
throw new \LogicException( 'Direct property access attempt: ' . $prop );
}
}

protected function userCanSee( $name, \RequestContext $context ) {
if ( $this->get( 'deleted' )
&& !$context->getUser()->isAllowed( 'mwoauthviewsuppressed' ) ) {
if ( $this->getDeleted()
&& !$context->getUser()->isAllowed( 'mwoauthviewsuppressed' )
) {
return $context->msg( 'mwoauth-field-hidden' );
} else {
return true;
Expand Down
Loading

0 comments on commit 98df9d7

Please sign in to comment.