Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(oauth): when requested, include ConsumerAcceptance in oauth data #446

Closed
wants to merge 1 commit into from
+60 −8
Closed

feat(oauth): when requested, include ConsumerAcceptance in oauth data #446

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions dist-persist/wbstack/src/Internal/ApiWbStackOauthGet.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ public function execute() {
// Try and get the required consumer
$consumerData = WbStackPlatformReservedUser::getOAuthConsumer(
$this->getParameter('consumerName'),
$this->getParameter('consumerVersion')
$this->getParameter('consumerVersion'),
$this->getParameter('includeAccess'),
);

// If it doesnt exist, make sure the user and consumer do
Expand All @@ -42,7 +43,8 @@ public function execute() {
);
$consumerData = WbStackPlatformReservedUser::getOAuthConsumer(
$this->getParameter('consumerName'),
$this->getParameter('consumerVersion')
$this->getParameter('consumerVersion'),
$this->getParameter('includeAccess'),
);
}

Expand Down Expand Up @@ -73,6 +75,10 @@ public function getAllowedParams() {
ParamValidator::PARAM_TYPE => 'string',
ParamValidator::PARAM_ISMULTI => true,
],
'includeAccess' => [
ParamValidator::PARAM_TYPE => 'boolean',
ParamValidator::PARAM_REQUIRED => false
],
'callbackUrlTail' => [
ParamValidator::PARAM_TYPE => 'string',
ParamValidator::PARAM_REQUIRED => true
Expand Down
24 changes: 22 additions & 2 deletions dist-persist/wbstack/src/Internal/WbStackPlatformReservedUser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ public static function createOauthConsumer($consumerName, $version, $grants, $ca
return true;
}

public static function getOAuthConsumer($consumerName, $version) {
public static function getOAuthConsumer($consumerName, $version, $includeAccess = false) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that it could maybe sense to refactor the name of this method to try and make it clear to future callers what the purpose is:

usecase 1 with $includeAccess = false is: I want an OAuth consumer that each individual user can authorise to edit on their behalf with their account

usecase 2 with $includeAccess = true is: I want credentials to edit the wiki as the PlatformReservedUser

I almost wonder if this would be better as a totally separate function. Really the common logic is more "incidental" than "conceptual"; it just happens to be that to get the credentials for usecase 2 you have to get a consumer but really you aren't asking for a consumer but instead editing credentials.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it just happens to be that to get the credentials for usecase 2 you have to get a consumer but really you aren't asking for a consumer but instead editing credentials.

So the way mw cli works is it expects all 4 tokens, not only the ConsumerAcceptance ones.

What we could do though is: always create the consumer with acceptance tokens, and then have a dedicated function that turns the consumer into a response based on the parameter given.

$user = self::getUser();
// TODO create the oauth consumer on the fly if it doesn't exist (needs grants and callbackurl)

Expand All @@ -131,10 +131,30 @@ public static function getOAuthConsumer($consumerName, $version) {
return false;
}

return [
$data = [
'agent' => $c->getName(),
'consumerKey' => $c->getConsumerKey(),
'consumerSecret' => \MediaWiki\Extension\OAuth\Backend\Utils::hmacDBSecret( $c->getSecretKey() ),
];

if ( $includeAccess ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My hacky way of testing this locally would be to short circuit this if statement and always execute the branch, then see if Magnustools Oauth consumers stil work and also receive the additional data.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And you'd be happiest if Magnustools does not get this additional data?

My understanding is that we wouldn't want Magnus tools to get these credentials since they would allow editing as the platform reserved user

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And you'd be happiest if Magnustools does not get this additional data?

Hence the additional parameter, yes

$a = \MediaWiki\Extension\OAuth\Backend\ConsumerAcceptance::newFromUserConsumerWiki(
$db,
$user->getId(),
$c,
$c->getWiki(),
\MediaWiki\Extension\OAuth\Backend\ConsumerAcceptance::READ_NORMAL,
$c->getOAuthVersion(),
);

if ( $a === false ) {
return false;
}

$data['accessKey'] = $a->getAccessToken();
$data['accessSecret'] = $a->getAccessSecret();
}

return $data;
}
}
10 changes: 8 additions & 2 deletions dist/wbstack/src/Internal/ApiWbStackOauthGet.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ public function execute() {
// Try and get the required consumer
$consumerData = WbStackPlatformReservedUser::getOAuthConsumer(
$this->getParameter('consumerName'),
$this->getParameter('consumerVersion')
$this->getParameter('consumerVersion'),
$this->getParameter('includeAccess'),
);

// If it doesnt exist, make sure the user and consumer do
Expand All @@ -42,7 +43,8 @@ public function execute() {
);
$consumerData = WbStackPlatformReservedUser::getOAuthConsumer(
$this->getParameter('consumerName'),
$this->getParameter('consumerVersion')
$this->getParameter('consumerVersion'),
$this->getParameter('includeAccess'),
);
}

Expand Down Expand Up @@ -73,6 +75,10 @@ public function getAllowedParams() {
ParamValidator::PARAM_TYPE => 'string',
ParamValidator::PARAM_ISMULTI => true,
],
'includeAccess' => [
ParamValidator::PARAM_TYPE => 'boolean',
ParamValidator::PARAM_REQUIRED => false
],
'callbackUrlTail' => [
ParamValidator::PARAM_TYPE => 'string',
ParamValidator::PARAM_REQUIRED => true
Expand Down
24 changes: 22 additions & 2 deletions dist/wbstack/src/Internal/WbStackPlatformReservedUser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ public static function createOauthConsumer($consumerName, $version, $grants, $ca
return true;
}

public static function getOAuthConsumer($consumerName, $version) {
public static function getOAuthConsumer($consumerName, $version, $includeAccess = false) {
$user = self::getUser();
// TODO create the oauth consumer on the fly if it doesn't exist (needs grants and callbackurl)

Expand All @@ -131,10 +131,30 @@ public static function getOAuthConsumer($consumerName, $version) {
return false;
}

return [
$data = [
'agent' => $c->getName(),
'consumerKey' => $c->getConsumerKey(),
'consumerSecret' => \MediaWiki\Extension\OAuth\Backend\Utils::hmacDBSecret( $c->getSecretKey() ),
];

if ( $includeAccess ) {
$a = \MediaWiki\Extension\OAuth\Backend\ConsumerAcceptance::newFromUserConsumerWiki(
$db,
$user->getId(),
$c,
$c->getWiki(),
\MediaWiki\Extension\OAuth\Backend\ConsumerAcceptance::READ_NORMAL,
$c->getOAuthVersion(),
);

if ( $a === false ) {
return false;
}

$data['accessKey'] = $a->getAccessToken();
$data['accessSecret'] = $a->getAccessSecret();
}

return $data;
}
}
Loading