Details
- Reviewers
amckinley - Maniphest Tasks
- T13046: Surface repository pull logs in the web UI
- Commits
- rPe6a9db56a96a: Add a basic view for repository pull logs
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| src/applications/diffusion/controller/DiffusionPushLogListController.php | ||
|---|---|---|
| 5–7 | Are there any access control implications to getting this data? Could a user see the existence of a rSECRET_PLANS repo that they normally couldn't see? From the screenshot, it's not clear what the behavior is if the user doesn't select a repo. | |
| src/applications/diffusion/view/DiffusionPullLogListView.php | ||
| 26–27 | That's an interesting proxy for "should be able to see remote IPs". | |
| 34 | See above comment: we should add a CAN_VIEW check on the repos themselves (unless that's somehow handled by this code as-is). | |
| src/applications/repository/query/PhabricatorRepositoryPullEventQuery.php | ||
| 47–50 | Oh, this should result in $repositories = array() for repos that the viewer can't see. Nevermind! | |
| src/applications/repository/query/PhabricatorRepositoryPullEventQuery.php | ||
|---|---|---|
| 47–50 | Yeah, this is the substantive policy check: if you can't see a repository, you can't see the logs for it. Then we end up in a sort of grey area on logs which didn't make it as far as identifying a repository (for example, trying to pull a repository which does not exist). I just required ADMIN for those, later in this diff. None of this feels exactly like the most pristine or obvious approach to policies for these logs, although I think we end up somewhere mostly reasonable. | |
| src/applications/repository/storage/PhabricatorRepositoryPullEvent.php | ||
| 110 | This is the change which says "if the log has no repository, require admin". Another rule might be "if the log has no repository, don't show it at all". I'm not sure these logs are terribly useful from the web UI anyway, since you can't use them for any kind of auditing or reporting goal, I think, except "is someone trying to guess HTTP passwords through brute force", and access logs are probably better for that anyway. | |
Oh, and the remote address rule is just the rule currently used by push logs. I'm not sure it's a great rule in either case. For most installs I don't think remote addresses are very important -- no one cares if they're all corpnet addresses -- but they've been a bit contentious with WMF (since they have strict policies around retaining/disclosing remote addresses). Another possibility is something like "get rid of them completely, go dig through the database if you need the information", although that doesn't address the retention concern on the WMF side.