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
Summary:
Ref T603. Several issues here:
1. Currently, `FileQuery` does not actually respect object attachment edges when doing policy checks. Everything else works fine, but this was missing an `array_keys()`.
2. Once that's fixed, we hit a bunch of recursion issues. For example, when loading a User we load the profile picture, and then that loads the User, and that loads the profile picture, etc.
3. Introduce a "Query Workspace", which holds objects we know we've loaded and know we can see but haven't finished filtering and/or attaching data to. This allows subqueries to look up objects instead of querying for them.
- We can probably generalize this a bit to make a few other queries more efficient. Pholio currently has a similar (but less general) "mock cache". However, it's keyed by ID instead of PHID so it's not easy to reuse this right now.
This is a bit complex for the problem being solved, but I think it's the cleanest approach and I believe the primitive will be useful in the future.
Test Plan: Looked at pastes, macros, mocks and projects as a logged-in and logged-out user.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7309
Summary: These end up a little weird with subclassing instead of `switch`, but some day we could alias them to one another or something I guess. If I'm feeling brave, I might get rid of the "user" variant when I migrate Maniphest custom field specs, and turn it into "users, limit = 1" or something like that.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7010
Summary:
Ref T2625. Ref T3794. Ref T418. Ref T1703.
This is a more general version of D5278. It expands CustomField support to include real integration with ApplicationSearch.
Broadly, custom fields may elect to:
- build indicies when objects are updated;
- populate ApplicationSearch forms with new controls;
- read inputs entered into those controls out of the request; and
- apply constraints to search queries.
Some utility/helper stuff is provided to make this easier. This part could be cleaner, but seems reasonable for a first cut. In particular, the Query and SearchEngine must manually call all the hooks right now instead of everything happening magically. I think that's fine for the moment; they're pretty easy to get right.
Test Plan:
I added a new searchable "Company" field to People:
{F58229}
This also cleaned up the disable/reorder view a little bit:
{F58230}
As it did before, this field appears on the edit screen:
{F58231}
However, because it has `search`, it also appears on the search screen:
{F58232}
When queried, it returns the expected results:
{F58233}
And the actually good bit of all this is that the query can take advantage of indexes:
mysql> explain SELECT * FROM `user` user JOIN `user_customfieldstringindex` `appsearch_0` ON `appsearch_0`.objectPHID = user.phid AND `appsearch_0`.indexKey = 'mk3Ndy476ge6' AND `appsearch_0`.indexValue IN ('phacility') ORDER BY user.id DESC LIMIT 101;
+----+-------------+-------------+--------+-------------------+----------+---------+------------------------------------------+------+----------------------------------------------+
| id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra |
+----+-------------+-------------+--------+-------------------+----------+---------+------------------------------------------+------+----------------------------------------------+
| 1 | SIMPLE | appsearch_0 | ref | key_join,key_find | key_find | 232 | const,const | 1 | Using where; Using temporary; Using filesort |
| 1 | SIMPLE | user | eq_ref | phid | phid | 194 | phabricator2_user.appsearch_0.objectPHID | 1 | |
+----+-------------+-------------+--------+-------------------+----------+---------+------------------------------------------+------+----------------------------------------------+
2 rows in set (0.00 sec)
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T418, T1703, T2625, T3794
Differential Revision: https://secure.phabricator.com/D6992
Summary:
Fixes T3810. In PhabricatorPeopleQuery, we issue an unnecessary query like this:
SELECT f.* FROM file f WHERE (f.phid IN ('')) ORDER BY f.id DESC
...if we're loading a user without a profile picture. Filter the file PHIDs before loading them to prevent this.
This doesn't change anything, but saves us a spurious/silly query.
Also makes `PhabricatorPeopleProfileController` use `needProfileImage()`, moving us closer to getting rid of `loadProfileImageURI()` eventually.
Test Plan: Looked at profiles of users with and without profile pictures. Checked query log in DarkConsole.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T3810
Differential Revision: https://secure.phabricator.com/D6913
Summary: Ref T2715. Had to start loading status information in the query class. Debated trying to clean up some of the attach / load stuff but decided to just add status under the new paradigm for now.
Test Plan: phid.query also made a status and checked that out. also played in conpherence.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2715
Differential Revision: https://secure.phabricator.com/D6585
Summary: We end up with both "user.id" and "email.id". Disambiguate for ORDER.
Test Plan: Ran Conduit user.query query with "email".
Reviewers: wez, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6234
Summary:
Ref T2625. Fixes T2812. Implement ApplicationSearch in People.
{F44788}
Test Plan: Made People queries. Used Conduit. Used `@mentions`.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2625, T2812
Differential Revision: https://secure.phabricator.com/D6092
Summary:
- `kill_init.php` said "Moving 1000 files" - I hope that this is not some limit in `FileFinder`.
- [src/infrastructure/celerity] `git mv utils.php map.php; git mv api/utils.php api.php`
- Comment `phutil_libraries` in `.arcconfig` and run `arc liberate`.
NOTE: `arc diff` timed out so I'm pushing it without review.
Test Plan:
/D1234
Browsed around, especially in `applications/repository/worker/commitchangeparser` and `applications/` in general.
Auditors: epriestley
Maniphest Tasks: T1103
Summary: This is an example of code simplification with D2557.
Test Plan: Display user list, verify the SQL queries.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2558
Summary:
Allow installs to require users to verify email addresses before they can use Phabricator. If a user logs in without a verified email address, they're given instructions to verify their address.
This isn't too useful on its own since we don't actually have arbitrary email registration, but the next step is to allow installs to restrict email to only some domains (e.g., @mycompany.com).
Test Plan:
- Verification
- Set verification requirement to `true`.
- Tried to use Phabricator with an unverified account, was told to verify.
- Tried to use Conduit, was given a verification error.
- Verified account, used Phabricator.
- Unverified account, reset password, verified implicit verification, used Phabricator.
- People Admin Interface
- Viewed as admin. Clicked "Administrate User".
- Viewed as non-admin
- Sanity Checks
- Used Conduit normally from web/CLI with a verified account.
- Logged in/out.
- Sent password reset email.
- Created a new user.
- Logged in with an unverified user but with the configuration set to off.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran, csilvers
Maniphest Tasks: T1184
Differential Revision: https://secure.phabricator.com/D2520
Summary: specify user table to make things not ambiguous
Test Plan: conduit console still works, including ID query...!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1075
Differential Revision: https://secure.phabricator.com/D2419
