2013-06-17 21:14:00 +02:00
|
|
|
<?php
|
|
|
|
|
Introduce CAN_EDIT for ExternalAccount, and make CAN_VIEW more liberal
Summary:
Fixes T3732. Ref T1205. Ref T3116.
External accounts (like emails used as identities, Facebook accounts, LDAP accounts, etc.) are stored in "ExternalAccount" objects.
Currently, we have a very restrictive `CAN_VIEW` policy for ExternalAccounts, to add an extra layer of protection to make sure users can't use them in unintended ways. For example, it would be bad if a user could link their Phabricator account to a Facebook account without proper authentication. All of the controllers which do sensitive things have checks anyway, but a restrictive CAN_VIEW provided an extra layer of protection. Se T3116 for some discussion.
However, this means that when grey/external users take actions (via email, or via applications like Legalpad) other users can't load the account handles and can't see anything about the actor (they just see "Restricted External Account" or similar).
Balancing these concerns is mostly about not making a huge mess while doing it. This seems like a reasonable approach:
- Add `CAN_EDIT` on these objects.
- Make that very restricted, but open up `CAN_VIEW`.
- Require `CAN_EDIT` any time we're going to do something authentication/identity related.
This is slightly easier to get wrong (forget CAN_EDIT) than other approaches, but pretty simple, and we always have extra checks in place anyway -- this is just a safety net.
I'm not quite sure how we should identify external accounts, so for now we're just rendering "Email User" or similar -- clearly not a bug, but not identifying. We can figure out what to render in the long term elsewhere.
Test Plan:
- Viewed external accounts.
- Linked an external account.
- Refreshed an external account.
- Edited profile picture.
- Viewed sessions panel.
- Published a bunch of stuff to Asana/JIRA.
- Legalpad signature page now shows external accounts.
{F171595}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3732, T1205, T3116
Differential Revision: https://secure.phabricator.com/D9767
2014-07-10 19:18:10 +02:00
|
|
|
/**
|
2014-07-23 02:03:09 +02:00
|
|
|
* NOTE: When loading ExternalAccounts for use in an authentication context
|
|
|
|
* (that is, you're going to act as the account or link identities or anything
|
|
|
|
* like that) you should require CAN_EDIT capability even if you aren't actually
|
Introduce CAN_EDIT for ExternalAccount, and make CAN_VIEW more liberal
Summary:
Fixes T3732. Ref T1205. Ref T3116.
External accounts (like emails used as identities, Facebook accounts, LDAP accounts, etc.) are stored in "ExternalAccount" objects.
Currently, we have a very restrictive `CAN_VIEW` policy for ExternalAccounts, to add an extra layer of protection to make sure users can't use them in unintended ways. For example, it would be bad if a user could link their Phabricator account to a Facebook account without proper authentication. All of the controllers which do sensitive things have checks anyway, but a restrictive CAN_VIEW provided an extra layer of protection. Se T3116 for some discussion.
However, this means that when grey/external users take actions (via email, or via applications like Legalpad) other users can't load the account handles and can't see anything about the actor (they just see "Restricted External Account" or similar).
Balancing these concerns is mostly about not making a huge mess while doing it. This seems like a reasonable approach:
- Add `CAN_EDIT` on these objects.
- Make that very restricted, but open up `CAN_VIEW`.
- Require `CAN_EDIT` any time we're going to do something authentication/identity related.
This is slightly easier to get wrong (forget CAN_EDIT) than other approaches, but pretty simple, and we always have extra checks in place anyway -- this is just a safety net.
I'm not quite sure how we should identify external accounts, so for now we're just rendering "Email User" or similar -- clearly not a bug, but not identifying. We can figure out what to render in the long term elsewhere.
Test Plan:
- Viewed external accounts.
- Linked an external account.
- Refreshed an external account.
- Edited profile picture.
- Viewed sessions panel.
- Published a bunch of stuff to Asana/JIRA.
- Legalpad signature page now shows external accounts.
{F171595}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3732, T1205, T3116
Differential Revision: https://secure.phabricator.com/D9767
2014-07-10 19:18:10 +02:00
|
|
|
* editing the ExternalAccount.
|
|
|
|
*
|
|
|
|
* ExternalAccounts have a permissive CAN_VIEW policy (like users) because they
|
|
|
|
* interact directly with objects and can leave comments, sign documents, etc.
|
|
|
|
* However, CAN_EDIT is restricted to users who own the accounts.
|
|
|
|
*/
|
2013-06-17 21:14:00 +02:00
|
|
|
final class PhabricatorExternalAccountQuery
|
|
|
|
extends PhabricatorCursorPagedPolicyAwareQuery {
|
|
|
|
|
|
|
|
private $ids;
|
|
|
|
private $phids;
|
|
|
|
private $accountTypes;
|
|
|
|
private $accountDomains;
|
|
|
|
private $accountIDs;
|
|
|
|
private $userPHIDs;
|
|
|
|
private $needImages;
|
|
|
|
private $accountSecrets;
|
|
|
|
|
|
|
|
public function withUserPHIDs(array $user_phids) {
|
|
|
|
$this->userPHIDs = $user_phids;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
|
|
|
public function withAccountIDs(array $account_ids) {
|
|
|
|
$this->accountIDs = $account_ids;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
|
|
|
public function withAccountDomains(array $account_domains) {
|
|
|
|
$this->accountDomains = $account_domains;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
|
|
|
public function withAccountTypes(array $account_types) {
|
|
|
|
$this->accountTypes = $account_types;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
|
|
|
public function withPHIDs(array $phids) {
|
|
|
|
$this->phids = $phids;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
|
|
|
public function withIDs($ids) {
|
|
|
|
$this->ids = $ids;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
|
|
|
public function withAccountSecrets(array $secrets) {
|
|
|
|
$this->accountSecrets = $secrets;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
|
|
|
public function needImages($need) {
|
|
|
|
$this->needImages = $need;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
2016-04-03 19:04:10 +02:00
|
|
|
public function newResultObject() {
|
|
|
|
return new PhabricatorExternalAccount();
|
|
|
|
}
|
|
|
|
|
2015-01-13 20:56:07 +01:00
|
|
|
protected function loadPage() {
|
2016-04-03 19:04:10 +02:00
|
|
|
return $this->loadStandardPage($this->newResultObject());
|
2013-06-17 21:14:00 +02:00
|
|
|
}
|
|
|
|
|
2015-01-13 20:56:07 +01:00
|
|
|
protected function willFilterPage(array $accounts) {
|
Give ExternalAccount a providerConfigPHID, tying it to a particular provider
Summary:
Depends on D20111. Ref T6703. Currently, each ExternalAccount row is tied to a provider by `providerType` + `providerDomain`. This effectively prevents multiple providers of the same type, since, e.g., two LDAP providers may be on different ports on the same domain. The `domain` also isn't really a useful idea anyway because you can move which hostname an LDAP server is on, and LDAP actually uses the value `self` in all cases. Yeah, yikes.
Instead, just bind each account to a particular provider. Then we can have an LDAP "alice" on seven different servers on different ports on the same machine and they can all move around and we'll still have a consistent, cohesive view of the world.
(On its own, this creates some issues with the link/unlink/refresh flows. Those will be updated in followups, and doing this change in a way with no intermediate breaks would require fixing them to use IDs to reference providerType/providerDomain, then fixing this, then undoing the first fix most of the way.)
Test Plan: Ran migrations, sanity-checked database. See followup changes for more comprehensive testing.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T6703
Differential Revision: https://secure.phabricator.com/D20112
2019-02-06 22:11:34 +01:00
|
|
|
$viewer = $this->getViewer();
|
|
|
|
|
|
|
|
$configs = id(new PhabricatorAuthProviderConfigQuery())
|
|
|
|
->setViewer($viewer)
|
|
|
|
->withPHIDs(mpull($accounts, 'getProviderConfigPHID'))
|
|
|
|
->execute();
|
|
|
|
$configs = mpull($configs, null, 'getPHID');
|
|
|
|
|
|
|
|
foreach ($accounts as $key => $account) {
|
|
|
|
$config_phid = $account->getProviderConfigPHID();
|
|
|
|
$config = idx($configs, $config_phid);
|
|
|
|
|
|
|
|
if (!$config) {
|
|
|
|
unset($accounts[$key]);
|
|
|
|
continue;
|
|
|
|
}
|
|
|
|
|
|
|
|
$account->attachProviderConfig($config);
|
|
|
|
}
|
|
|
|
|
2013-06-17 21:14:00 +02:00
|
|
|
if ($this->needImages) {
|
|
|
|
$file_phids = mpull($accounts, 'getProfileImagePHID');
|
|
|
|
$file_phids = array_filter($file_phids);
|
|
|
|
|
|
|
|
if ($file_phids) {
|
|
|
|
// NOTE: We use the omnipotent viewer here because these files are
|
|
|
|
// usually created during registration and can't be associated with
|
|
|
|
// the correct policies, since the relevant user account does not exist
|
|
|
|
// yet. In effect, if you can see an ExternalAccount, you can see its
|
|
|
|
// profile image.
|
|
|
|
$files = id(new PhabricatorFileQuery())
|
|
|
|
->setViewer(PhabricatorUser::getOmnipotentUser())
|
|
|
|
->withPHIDs($file_phids)
|
|
|
|
->execute();
|
|
|
|
$files = mpull($files, null, 'getPHID');
|
|
|
|
} else {
|
|
|
|
$files = array();
|
|
|
|
}
|
|
|
|
|
|
|
|
$default_file = null;
|
|
|
|
foreach ($accounts as $account) {
|
|
|
|
$image_phid = $account->getProfileImagePHID();
|
|
|
|
if ($image_phid && isset($files[$image_phid])) {
|
|
|
|
$account->attachProfileImageFile($files[$image_phid]);
|
|
|
|
} else {
|
|
|
|
if ($default_file === null) {
|
|
|
|
$default_file = PhabricatorFile::loadBuiltin(
|
|
|
|
$this->getViewer(),
|
|
|
|
'profile.png');
|
|
|
|
}
|
|
|
|
$account->attachProfileImageFile($default_file);
|
|
|
|
}
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
|
|
|
return $accounts;
|
|
|
|
}
|
|
|
|
|
2016-04-03 19:04:10 +02:00
|
|
|
protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
|
|
|
|
$where = parent::buildWhereClauseParts($conn);
|
2013-06-17 21:14:00 +02:00
|
|
|
|
2016-04-03 19:04:10 +02:00
|
|
|
if ($this->ids !== null) {
|
2013-06-17 21:14:00 +02:00
|
|
|
$where[] = qsprintf(
|
2016-04-03 19:04:10 +02:00
|
|
|
$conn,
|
2013-06-17 21:14:00 +02:00
|
|
|
'id IN (%Ld)',
|
|
|
|
$this->ids);
|
|
|
|
}
|
|
|
|
|
2016-04-03 19:04:10 +02:00
|
|
|
if ($this->phids !== null) {
|
2013-06-17 21:14:00 +02:00
|
|
|
$where[] = qsprintf(
|
2016-04-03 19:04:10 +02:00
|
|
|
$conn,
|
2013-06-17 21:14:00 +02:00
|
|
|
'phid IN (%Ls)',
|
|
|
|
$this->phids);
|
|
|
|
}
|
|
|
|
|
2016-04-03 19:04:10 +02:00
|
|
|
if ($this->accountTypes !== null) {
|
2013-06-17 21:14:00 +02:00
|
|
|
$where[] = qsprintf(
|
2016-04-03 19:04:10 +02:00
|
|
|
$conn,
|
2013-06-17 21:14:00 +02:00
|
|
|
'accountType IN (%Ls)',
|
|
|
|
$this->accountTypes);
|
|
|
|
}
|
|
|
|
|
2016-04-03 19:04:10 +02:00
|
|
|
if ($this->accountDomains !== null) {
|
2013-06-17 21:14:00 +02:00
|
|
|
$where[] = qsprintf(
|
2016-04-03 19:04:10 +02:00
|
|
|
$conn,
|
2013-06-17 21:14:00 +02:00
|
|
|
'accountDomain IN (%Ls)',
|
|
|
|
$this->accountDomains);
|
|
|
|
}
|
|
|
|
|
2016-04-03 19:04:10 +02:00
|
|
|
if ($this->accountIDs !== null) {
|
2013-06-17 21:14:00 +02:00
|
|
|
$where[] = qsprintf(
|
2016-04-03 19:04:10 +02:00
|
|
|
$conn,
|
2013-06-17 21:14:00 +02:00
|
|
|
'accountID IN (%Ls)',
|
|
|
|
$this->accountIDs);
|
|
|
|
}
|
|
|
|
|
2016-04-03 19:04:10 +02:00
|
|
|
if ($this->userPHIDs !== null) {
|
2013-06-17 21:14:00 +02:00
|
|
|
$where[] = qsprintf(
|
2016-04-03 19:04:10 +02:00
|
|
|
$conn,
|
2013-06-17 21:14:00 +02:00
|
|
|
'userPHID IN (%Ls)',
|
|
|
|
$this->userPHIDs);
|
|
|
|
}
|
|
|
|
|
2016-04-03 19:04:10 +02:00
|
|
|
if ($this->accountSecrets !== null) {
|
2013-06-17 21:14:00 +02:00
|
|
|
$where[] = qsprintf(
|
2016-04-03 19:04:10 +02:00
|
|
|
$conn,
|
2013-06-17 21:14:00 +02:00
|
|
|
'accountSecret IN (%Ls)',
|
|
|
|
$this->accountSecrets);
|
|
|
|
}
|
|
|
|
|
2016-04-03 19:04:10 +02:00
|
|
|
return $where;
|
2013-06-17 21:14:00 +02:00
|
|
|
}
|
|
|
|
|
Lock policy queries to their applications
Summary:
While we mostly have reasonable effective object accessibility when you lock a user out of an application, it's primarily enforced at the controller level. Users can still, e.g., load the handles of objects they can't actually see. Instead, lock the queries to the applications so that you can, e.g., never load a revision if you don't have access to Differential.
This has several parts:
- For PolicyAware queries, provide an application class name method.
- If the query specifies a class name and the user doesn't have permission to use it, fail the entire query unconditionally.
- For handles, simplify query construction and count all the PHIDs as "restricted" so we get a UI full of "restricted" instead of "unknown" handles.
Test Plan:
- Added a unit test to verify I got all the class names right.
- Browsed around, logged in/out as a normal user with public policies on and off.
- Browsed around, logged in/out as a restricted user with public policies on and off. With restrictions, saw all traces of restricted apps removed or restricted.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7367
2013-10-22 02:20:27 +02:00
|
|
|
public function getQueryApplicationClass() {
|
2014-07-23 02:03:09 +02:00
|
|
|
return 'PhabricatorPeopleApplication';
|
Lock policy queries to their applications
Summary:
While we mostly have reasonable effective object accessibility when you lock a user out of an application, it's primarily enforced at the controller level. Users can still, e.g., load the handles of objects they can't actually see. Instead, lock the queries to the applications so that you can, e.g., never load a revision if you don't have access to Differential.
This has several parts:
- For PolicyAware queries, provide an application class name method.
- If the query specifies a class name and the user doesn't have permission to use it, fail the entire query unconditionally.
- For handles, simplify query construction and count all the PHIDs as "restricted" so we get a UI full of "restricted" instead of "unknown" handles.
Test Plan:
- Added a unit test to verify I got all the class names right.
- Browsed around, logged in/out as a normal user with public policies on and off.
- Browsed around, logged in/out as a restricted user with public policies on and off. With restrictions, saw all traces of restricted apps removed or restricted.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7367
2013-10-22 02:20:27 +02:00
|
|
|
}
|
|
|
|
|
2013-06-17 21:14:00 +02:00
|
|
|
}
|