Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion appinfo/app.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,4 @@
use OCA\OAuth2\AppInfo\Application;

$app = new Application();
$app->getContainer()->query('UserHooks')->register();
$app->boot();
3 changes: 3 additions & 0 deletions css/login.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
#body-login .icon-info-white {
display: inline;
}
28 changes: 28 additions & 0 deletions js/login.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/**
* @author Thomas Müller <[email protected]>
*
* @copyright Copyright (c) 2017, ownCloud GmbH
* @license AGPL-3.0
*
* This code is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License, version 3,
* as published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License, version 3,
* along with this program. If not, see <http://www.gnu.org/licenses/>
*
*/

$(document).ready(function(){
var $loginMessage = $('#body-login').find('#message');
if ($loginMessage.length) {
var client = $("data[key='oauth2']").attr('value');
var msg = t('oauth2', 'The application "{app}" is requesting access to your account. To authorize it, please log in first.', {app : client});

Choose a reason for hiding this comment

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

Forgive my ignorance, as i don't really know the frameworks involved... But is 'client' getting html-escaped somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

afaik t() is doing this properly .... will have a second look ....

Copy link
Member Author

Choose a reason for hiding this comment

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

Just tested - it is escaped - thx for the hint

$loginMessage.parent().append('<div class="warning"><div class="icon-info-white" />'+msg+'</div>');
}
});
32 changes: 32 additions & 0 deletions lib/AppInfo/Application.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,12 @@
namespace OCA\OAuth2\AppInfo;

use OCA\OAuth2\AuthModule;
use OCA\OAuth2\Db\Client;
use OCA\OAuth2\Db\ClientMapper;
use OCA\OAuth2\Hooks\UserHooks;
use OCA\OAuth2\Sabre\OAuth2;
use OCP\AppFramework\App;
use OCP\AppFramework\Db\DoesNotExistException;
use OCP\SabrePluginEvent;
use Sabre\DAV\Auth\Plugin;

Expand Down Expand Up @@ -78,4 +81,33 @@ public function __construct(array $urlParams = array()) {
});
}

public function boot() {

Choose a reason for hiding this comment

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

So if I understand correctly, this function is called for the login screen (Is it the login screen only or all the screens?) and injects the javascript that will change the login message.

Copy link
Member Author

Choose a reason for hiding this comment

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

it will be called for any request-

Choose a reason for hiding this comment

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

Should you not check that it is only for the login screen?

Copy link
Member Author

Choose a reason for hiding this comment

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

the js code is doing this - not perfect but enough

$this->getContainer()->query('UserHooks')->register();
$request = $this->getContainer()->getServer()->getRequest();
$redirectUrl = $request->getParam('redirect_url');
if ($redirectUrl === null) {
return;
}

$urlParts = parse_url(urldecode($redirectUrl));
if (strpos($urlParts['path'], 'apps/oauth2/authorize') === false) {
return;
}
$params = [];
parse_str($urlParts['query'], $params);
if (!isset($params['client_id'])) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You are checking for redirect_uri and client_id but in the controller function there are some more checks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Besides that: looks good. 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

basically 'just because' I don't need the other parameter at this point.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok. I just thought that a request with missing parameters at the login stage will eventually fail at the authorize stage...

return;
}
/** @var ClientMapper $mapper */
$mapper = \OC::$server->query(ClientMapper::class);
/** @var Client $client */
try {
$client = $mapper->findByIdentifier($params['client_id']);
\OCP\Util::addScript('oauth2', 'login');
\OCP\Util::addStyle('oauth2', 'login');
\OCP\Util::addHeader('data', ['key' => 'oauth2', 'value' => $client->getName()]);
} catch (DoesNotExistException $ex) {
// ignore - the given client id is not known
}
}
}