Summary:
Ref T4398. This is still pretty rough and isn't exposed in the UI yet, but basically works. Some missing features / areas for improvement:
- Rate limiting attempts (see TODO).
- Marking tokens used after they're used once (see TODO), maybe. I can't think of ways an attacker could capture a token without also capturing a session, offhand.
- Actually turning this on (see TODO).
- This workflow is pretty wordy. It would be nice to calm it down a bit.
- But also add more help/context to help users figure out what's going on here, I think it's not very obvious if you don't already know what "TOTP" is.
- Add admin tool to strip auth factors off an account ("Help, I lost my phone and can't log in!").
- Add admin tool to show users who don't have multi-factor auth? (so you can pester them)
- Generate QR codes to make the transfer process easier (they're fairly complicated).
- Make the "entering hi-sec" workflow actually check for auth factors and use them correctly.
- Turn this on so users can use it.
- Adding SMS as an option would be nice eventually.
- Adding "password" as an option, maybe? TOTP feels fairly good to me.
I'll post a couple of screens...
Test Plan:
- Added TOTP token with Google Authenticator.
- Added TOTP token with Authy.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4398
Differential Revision: https://secure.phabricator.com/D8875
Summary:
This was really out of date and full of lies.
Mostly I've deleted sections, since the UI is way way more self-explanatory and much better at surfacing errors now.
Test Plan: L@@K
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8873
Summary: Ref T4715. We show this number on the homepage, provide an easy way to query matching commits.
Test Plan: Clicked "problem commits", saw them.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4715
Differential Revision: https://secure.phabricator.com/D8880
Summary:
Fixes T4911. See D8879. This gives us the correct query in cases where there are no audits.
This doesn't try to do the GROUP BY stuff yet.
Test Plan:
- Viewed a commit in Diffusion with no audits, got a commit detail page.
- Viewed "All Commits" in web UI, saw commits without any audits included in the list.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4911
Differential Revision: https://secure.phabricator.com/D8882
Summary:
Grab an audit we have authority over if possible, relying on how that's sorted by actor first. This gets us the best description possible of what the audit is about in the list. Also sort out highlighting; right now it looks silly on some views when everything is highlighted.
An open question in the diff - when to highlight audits?
Options I see -
- never
- don't do it on "needs attention" but other views
- calculate what percentage of shown audits user has authority over, if most ( > N% ) don't highlight, otherwise highlight
- something else
- some combo of the above
Test Plan: lists of audits looked better
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8876
Summary: Currently, clicking search on mobile, the background gradient is bleeding.
Test Plan: Click search icon on mobile, see black and not a pony color.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8878
Summary:
Ref T4398. This adds a settings panel for account activity so users can review activity on their own account. Some goals are:
- Make it easier for us to develop and support auth and credential information, see T4398. This is the primary driver.
- Make it easier for users to understand and review auth and credential information (see T4842 for an example -- this isn't there yet, but builds toward it).
- Improve user confidence in security by making logging more apparent and accessible.
Minor corresponding changes:
- Entering and exiting hisec mode is now logged.
- This, sessions, and OAuth authorizations have moved to a new "Sessions and Logs" area, since "Authentication" was getting huge.
Test Plan:
- Viewed new panel.
- Viewed old UI.
- Entered/exited hisec and got prompted.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4398
Differential Revision: https://secure.phabricator.com/D8871
Summary:
Ref T4398. Ref T4842. I want to let users review their own account activity, partly as a general security measure and partly to make some of the multi-factor stuff easier to build and debug.
To support this, implement modern policies and application search.
I also removed the "old" and "new" columns from this output, since they had limited utility and revealed email addresses to administrators for some actions. We don't let administrators access email addresses from other UIs, and the value of doing so here seems very small.
Test Plan: Used interface to issue a bunch of queries against user logs, got reasonable/expected results.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: keir, epriestley
Maniphest Tasks: T4842, T4398
Differential Revision: https://secure.phabricator.com/D8856
Summary:
Ref T4398. This is roughly a "sudo" mode, like GitHub has for accessing SSH keys, or Facebook has for managing credit cards. GitHub actually calls theirs "sudo" mode, but I think that's too technical for big parts of our audience. I've gone with "high security mode".
This doesn't actually get exposed in the UI yet (and we don't have any meaningful auth factors to prompt the user for) but the workflow works overall. I'll go through it in a comment, since I need to arrange some screenshots.
Test Plan: See guided walkthrough.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4398
Differential Revision: https://secure.phabricator.com/D8851
Summary: Removes many tables and uses PropertyLists and ObjectItemList when possible. Adds cleaner CSS, makes mobile editing more possible.
Test Plan: Test new UI on desktop and mobile. Verify all functionality still exists.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4272
Differential Revision: https://secure.phabricator.com/D8860
Summary: Android/Chrome seems to run Blink and not WebKit. Unclear if these bugs are temporary or "correct". Fixing in any case, can investigate if more come in.
Test Plan: Used Chrome Developer tether to a Nexus 7, fixed CSS by hand.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
Subscribers: epriestley, Korvin
Maniphest Tasks: T4907
Differential Revision: https://secure.phabricator.com/D8874
Summary: ...also kills off "PhabricatorAuditCommitQuery" and "PhabricatorAuditQuery", by moving the work to "DiffusionCommitQuery". Generally cleans up some code around the joint on this too. Also provides policies for audit requests, which is basically the policy for the underlying commit. Fixes T4715. (For the TODO I added about files, I just grabbed T4713.)
Test Plan:
Audit: verified the three default views all showed the correct things, including highligthing. did some custom queries and got the correct results.
Diffusion: verified "blame view" still worked. verified paths were highlighted for packages i owned.
Home: verified audit boxes showed up with proper commits w/ audits
bin/audit: played around with it via --dry-run and got the right audits back
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: chad, epriestley, Korvin
Maniphest Tasks: T4715
Differential Revision: https://secure.phabricator.com/D8805
Summary:
It's fairly common for people to show up and be interested in finding easy stuff to work on. This stuff basically doesn't exist and probably never will: it doesn't make much sense to deliberately leave easy bugs broken just because someone might show up and want to fix a couple of easy bugs.
Almost all of the work that's valuable to us requires a depth or bredth of context which can't be acquired in a few hours here and there, and probably always will. I think it also always //should//, in that as long as we continue refactoring and clearing technical debt aggressively and having solid static analysis support tools, we should never have a large backlog of human-intelligence codebase tasks. The closest we've ever come were probably `pht()` and `phutil_tag()`, which both have a lot of subtleties and we mostly automated `phutil_tag()` anyway. These tasks are also //incredibly boring// to write and review.
So, accept this as a reality and realign the contributor documentation to try to deal with this case:
- Set expectations about starter tasks not existing and throwing a couple of hours at the project writing code being a hard path.
- Suggest non-code contributions which anyone can do.
- Segue into code contributions with context and suggestions.
Test Plan: Generated and read documentation.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8872
Summary: The adds Workboard and Aphlict icons
Test Plan:
TERMS AND CONDITIONS
Acceptance of this Differential Revisions entitles the original author to pursue his dream of making Workboards a phull phledged applicaiton within Phabricator.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8870
Summary:
When showing contents of a file with the blame mode enabled, tooltips pops out
when the mouse hovers over previous commit linkes on left side. The last part of the
tooltips is the author's name. If an author is unregistered, the name becomes
<span>name</span>.
{F147724}
This doesn't happen if the author is registered.
Test Plan:
Check tooltips after making the change.
{F147725}
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8869
Summary:
This algorithm is tricky, and uses `phutil_safe_html()` directly, which makes it potentially unsafe.
In particular, D8859 fixes a bug with it which caused it to produce non-utf8 output. This doesn't guarantee it's a security problem, but does make it suspicious.
I don't actually see a way to break it, but rewrite it so that it's absolutely bulletproof and does not need to call `phutil_safe_html()`.
Test Plan:
{F147487}
@rugabarbo, if you have a chance, can you check if this still works for you?
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, rugabarbo
Differential Revision: https://secure.phabricator.com/D8862
Summary: Fixes T4899. Action strengths got lost somewhere along the way; actions like "Accepted" should be stronger than "Changed Subscribers".
Test Plan: Verified things sort as expected now, with major actions at the top.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4899
Differential Revision: https://secure.phabricator.com/D8857
Summary: We should always have some sort of menu on mobile for logging in.
Test Plan: Test mobile, tablet, and desktop breakpoints. Gate seearch icon by public_policy.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4731
Differential Revision: https://secure.phabricator.com/D8868
Summary: This provides a little more separation/clarify around the blockquote design.
Test Plan:
Find a verbose passage from @epriestley, apply style.
{F147692}
{F147693}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D8867
Summary: Fixes T4903. At some point maybe-soonish we should maybe go make `"device" => true` the default, and put `"device" => "hella-busted"` on the remaining bad pages.
Test Plan: L@@K @ W/ iOS Simulator
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley, k
Maniphest Tasks: T4903
Differential Revision: https://secure.phabricator.com/D8863
Summary: Fixes the spacing around "Send Message", which changed from "Pontificate"
Test Plan: Use Chrome explorer and iOS simulator to test new CSS
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8866
Summary:
I created this review to get an answer...
It should not be taken as a real fix.
I noticed that phabricator return corrupted search results for some russian queries (without this patch).
See screenshot:
{F147443}
But I can't reproduce this bug on https://secure.phabricator.com/
This search query causes problems only for my phabricator instance.
More than that, I didn't find any php.ini-settings that can resolve this problem.
It's look like your phabricator instance use /u-modifier by default.
But how is it possible?
Test Plan: NONE
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8859
Summary: Fixes T4878
Test Plan:
1. Go to paste
2. Add comment
3. Check that paste subsribers got paste link by email
See T4878 for more details.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T4878
Differential Revision: https://secure.phabricator.com/D8861
Summary: Some actions (notably, `!accept`) require more information than we currently load.
Test Plan: Piped in some `!accept` mail using `bin/mail receive-test`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8844
Summary: Fixes T3566 List of poll actions should include ability to close an open poll or reopen a closed poll.
Test Plan: Poll author should be able to close/reopen poll. Non-author should get policy screen when attempting to close/reopen poll.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T3566
Differential Revision: https://secure.phabricator.com/D8846
Summary: See IRC. We construct this a little bit wrong if there are multiple "open" statuses. Use a more modern construction.
Test Plan: Hit `?statuses=wontfix,invalid`, etc. Clicked "view all" from projects.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8854
Summary: Going to sit on this for a bit so we can fall back to it if needbe, but this table no longer has any reads or writes in the application.
Test Plan: Applied patch locally and poked around.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: aran
Differential Revision: https://secure.phabricator.com/D8190
Summary: Fixes T4606. Also shortens two unusual type names which are currently inconsistent.
Test Plan: Expanded advanced search.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4606
Differential Revision: https://secure.phabricator.com/D8853
Summary:
Our smtp server responded slowly sometimes and reached the timeout, but actually
it had sent the email successfully. The mta then retried and sent duplicated
emails. So changed to a bigger default value.
Test Plan: Tested in our deployed and it worked
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8852
Summary: I need my eyes checked. The previous specificity was incorrect.
Test Plan: Stare really hard and inspect both major and minor timeline events in Chrome.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8850
Summary:
In some applications, using `{V2}` syntax to embed a vote throws. The chain of causality looks like this:
- We try to render a `phabricator_form()`.
- This requires a CSRF token.
- We look for a CSRF token on the user.
- It's an omnipotent user with no token, so everything fails.
To resolve this, make sure we always pass the real user in.
Test Plan:
- Lots of `grep`.
- Made a Differential comment with `{V2}`.
- Made a Diffusion comment with `{V2}`.
- Made a Maniphest comment with `{V2}`.
- Replied to a Conpherence thread with `{V2}`.
- Created a Conpherence thread with `{V2}`.
- Used Conduit to update a Conpherence thread with `{V2}`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, lkassianik
Differential Revision: https://secure.phabricator.com/D8849
Summary: I did the math wrong here, plus better specificity just in case.
Test Plan: View comment box on timeline, verify icons are properly spaced now.
Reviewers: epriestley, btrahan
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8848
Summary:
If you create a diff with no hunks (e.g., it adds a single empty file), we never attachHunks() so we throw on getHunks().
Instead, make sure changesets get hunks attached if they expect it.
Test Plan: Created a new diff with a single empty file in it.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: zeeg, epriestley
Differential Revision: https://secure.phabricator.com/D8842
Summary: These are a little easier on the eyes.
Test Plan:
Reject an epriestley diff.
{F146851}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8841
Summary:
Ref T4866. I did a fancy version of this but it looks pretty bad/confusing so here's a simple version.
Fancy-but-whack version:
{F146847}
Test Plan: This version is like that, but just always uses `fa-user`.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4866
Differential Revision: https://secure.phabricator.com/D8840
Summary: center aligns the icons in the fill area, removes some of the positioning jank. Also set new icons for maniphest custom.
Test Plan: test desktop and mobile layouts, tested thin pins for proper centering.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4866
Differential Revision: https://secure.phabricator.com/D8839
Summary: Throwing this up for testing, swapped out all icons in timeline for their font equivelants. Used better icons where I could as well. We should feel free to use more / be fun with the icons when possible since there is no penalty anymore.
Test Plan: I browsed many, not all, timelines in my sandbox and in IE8. Some of these were just swagged, but I'm expecting we'll do more SB testing before landing.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8827
Summary: For the time being, no need to have these in the repository.
Test Plan: Reload UIExamples, only see FontAwesome
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8835
Summary: The token transactions can publish empty transaction feed stories.
Stop them from doing that, and make notifications fail more quietly.
Auditors: btrahan
Summary:
- Support file attachments in Mailgun, after D8831.
- Fix `bin/mail send-test --attach ...` flag.
- Make `bin/mail send-test` route mail through the daemons.
- Remove the `workerTaskID` on MetaMTAMail, which is only used (needlessly) by `bin/mail resend` and creates a huge mess elsewhere.
- Currently, when mail fails, the daemon exits with a very generic and useless message. Instead, make `sendNow()` throw when it fails, so the real reason is surfaced. This is OK now because mail is always sent via the daemons.
- Now that Mailgun supports attachments, document it.
- Update a bunch of mail docs.
Test Plan:
- Sent mail.
- Sent mail with attachments.
- Read documentation.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8832
Summary: Fixes T4833. I wish there was an elegant way to catch this exception but I think the stack is written such that we really should just do this one-off query here...
Test Plan: from the "create project" link under "edit task" I received a more detailed exception than the report in T4833 post patch. I also tested editing an existing project - yay - and editing an existing project to some other existing project's name - got a nice error dialogue.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4833
Differential Revision: https://secure.phabricator.com/D8834
Summary:
Ref T4830. A few methods, like `conduit.ping`, are callable without authentication, so this even has some use cases. Also:
- Make some Differential stuff a little more consistent.
- Use slightly more modern rendering.
- Deprecate the status-oriented `user` calls; these will be replaced by Calendar methods.
Test Plan: Browsed console as logged out / logged in users.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4830
Differential Revision: https://secure.phabricator.com/D8826
Summary:
Ref T4830.
- If the application policy is public, allow logged-out users to browse examples.
- Use standard elements instead of custom ones.
Test Plan: Browsed UIExamples.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4830
Differential Revision: https://secure.phabricator.com/D8825
Summary:
Ref T3551. Currently, there are many layers of indirection between pull requests and revisions. After D8822, revisions and other types of requested objects are recorded directly on the request. This allows us to simplify data access and querying.
A lot of stuff here is doing `instanceof` checks to keep APIs stable, but most of those can go away in the long run.
Test Plan:
- Browsed requests.
- Verified revision-dependent fields (like "Revision", "Size", "Churn") still render correctly.
- Called `releeph.queryrequests`.
- Called `releephwork.nextrequest`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3551
Differential Revision: https://secure.phabricator.com/D8824
Summary:
Ref T3662. Releeph blocks users from requsting unparsed commits, but there's no real technical reason for this.
The `releephwork.getorigcommitmessage` method assumes data exists, but should be replaced with `diffusion.querycommits` anyway.
Test Plan: Ran `diffusion.querycommits`. Requested a commit.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3662
Differential Revision: https://secure.phabricator.com/D8823
