Some minor bugfixes for newinternal

[stwalkerster]

• Use constants instead of magic strings for user status
• Add a provision to allow extra CSS and JS resources to be loaded on a page-by-page basis
• Fix unhandled exception in IndentificationVerifier
• Refactor and expand capabilities of SearchHelper

[stwalkerster]

(depends on #284 )

[dqwiki]

Since I don't know enough about some of the testing stuff, I am going to add this checklist so FL4 knows which part still need review.

• includes/ConsoleTasks/ClearOldDataTask.php
• includes/DataObjects/User.php
• includes/Fragments/TemplateOutput.php
• includes/Helpers/SearchHelpers/LogSearchHelper.php
• includes/Helpers/SearchHelpers/RequestSearchHelper.php
• includes/Helpers/SearchHelpers/SearchHelperBase.php
• includes/IdentificationVerifier.php
• includes/Pages/PageUserManagement.php
• includes/Tasks/PageBase.php - Question --DQ
• redir.php
• sql/patches/patch17-preferences-nullable.sql → sql/patches/patch17-idx-pend-reserved.sql
• sql/patches/patch18-automatic-id-system.sql → sql/patches/patch18-newinternal.sql
• templates/base.tpl - Question --DQ
• templates/header-internal.tpl
• templates/navigation-menu.tpl
• templates/view-request/ip-links.tpl
• templates/view-request/main.tpl
• tests/includes/DataObjectTest.php
• tests/includes/Helpers/DebugHelperTest.php
• tests/includes/PdoDatabaseTest.php
• tests/includes/WebStartTest.php

[dqwiki]

IdentificationVerifier.php#L37 specifies API params. Throwing them in the sandbox at meta, throws me back this:

{ "batchcomplete": "", "query": { "normalized": [ { "from": "Access_to_nonpublic_information_policy/Noticeboard", "to": "Access to nonpublic information policy/Noticeboard" } ], "pages": { "-1": { "ns": 0, "title": "Access to nonpublic information policy/Noticeboard", "missing": "" } } } }

That doesn't seem to be the expected output.

[dqwiki]

Is there any way in which the changes in includes/Tasks/PageBase.php could have something injected by cookie, or user set setting? I'm asking more than looking because I don't know all the pages to look at.

Update: I do see templates/base.tpl which requires the js/css to come from our tool, I'm just worried there is still a way through dns/request modification that it could be screwed with.

[dqwiki]

I have finished reviewing what I am comfortable reviewing.
@FastLizard4 the rest is moreso up to you unless @stwalkerster can provide some docs for me to look at or training.

[stwalkerster]

IdentificationVerifier.php#L37 specifies API params. Throwing them in the sandbox at meta, throws me back this:

{ "batchcomplete": "", "query": { "normalized": [ { "from": "Access_to_nonpublic_information_policy/Noticeboard", "to": "Access to nonpublic information policy/Noticeboard" } ], "pages": { "-1": { "ns": 0, "title": "Access to nonpublic information policy/Noticeboard", "missing": "" } } } }

That doesn't seem to be the expected output.

Yeah, this needs to be updated to the new location of the noticeboard, and to account for the identification versioning change I put into master three months ago. If that's the only issue you can see with it, I'm tempted to ignore it just now and fix it as part of the merge of changes from master.

[stwalkerster]

Is there any way in which the changes in includes/Tasks/PageBase.php could have something injected by cookie, or user set setting? I'm asking more than looking because I don't know all the pages to look at.

Update: I do see templates/base.tpl which requires the js/css to come from our tool, I'm just worried there is still a way through dns/request modification that it could be screwed with.

Yeah, DNS poisoning is possible, for which we could start using nonces and SRI (both of which have more widespread implementations now), but this is a more global issue than just with this bit of code, which I think I'll want to address in a new PR.

I think there probably is some tidy-up needed around both the addCss and addJs calls, and also the tailscript functionality. Reviewing it all myself, I'm happy my current usage of it is fine (with the exception of the U2F code, which I will poke a bit), but I'm not happy with it's encouragement of good practice.

In truth, those functions shouldn't be in this PR; they're not used yet; and neither is the tailscript functionality, so I'm happy that it's not possible to exploit it here, and it's also not used in the rbac branch. Both are used in the OTP/OAuth creation code though, neither of which are up for review yet.

[stwalkerster]

I've raised #506 and #507 for these - I've put them in the blocking newinternal golive column, and I'll work on them in a later PR on something that's a bit closer to the code I'm wanting to go live.

[dqwiki]

IdentificationVerifier.php#L37 specifies API params. Throwing them in the sandbox at meta, throws me back this:
{ "batchcomplete": "", "query": { "normalized": [ { "from": "Access_to_nonpublic_information_policy/Noticeboard", "to": "Access to nonpublic information policy/Noticeboard" } ], "pages": { "-1": { "ns": 0, "title": "Access to nonpublic information policy/Noticeboard", "missing": "" } } } }
That doesn't seem to be the expected output.

Yeah, this needs to be updated to the new location of the noticeboard, and to account for the identification versioning change I put into master three months ago. If that's the only issue you can see with it, I'm tempted to ignore it just now and fix it as part of the merge of changes from master.

Well oops, forgot there was a new one.

[dqwiki]

While I have approved, this should get @FastLizard4 to sign off on the rest or @stwalkerster can repoke me to help me understand the other files

[stwalkerster]

@dqwiki, here's a but of a rundown of what the changes here are. Some of the ones you've checked off in your list above I've not mentioned, as I assume you're happy with what they're doing.

.travis.phplint.sh, .travis.yml, and codecov.yml

These are configuration files and a shell script used as part of the Travis CI builds. It's not part of the software itself, but part of the tooling surrounding the codereview process.

includes/ConsoleTasks/ClearOldDataTask.php

This is the reimplementation of the datapurge script; specifically this change is the newinternal implementation of 27d56cb from the master branch

includes/IdentificationVerifier.php

Requires a fix, but not in this PR. I've opened #509 for this in the "blocking newinternal" column in the project.

Search helpers: LogSearchHelper, RequestSearchHelper, and SearchHelperBase

These are helper classes for building up search queries for the objects named. The changes here move some duplicated code into the base class, and add support for a few more query types.

Essentially, to use them, you call several methods chained onto each other to further restrict the returned results to what you want. It's an abstraction layer away from building raw SQL in code directly. For example, to get a set of 25 checkuser requests, you might use this:

/**
 * @var RequestSearchHelper $search
 */
$search = RequestSearchHelper::get($database)->limit(25)->byStatus('checkuser');

This returns an object of type RequestSearchHelper into $search, which is equivalent to SELECT * FROM request WHERE status = 'checkuser' LIMIT 25. We can extend this further to apply common filter conditions, such as only returning those requests with a confirmed email address:

$search->withConfirmedEmail();

Finally, we can pull back the actual data with a fetch() call:

/**
 * @var Request[] $results
 */
$results = $search->fetch();

The specific changes that have been made to these classes are to support fetching a single column of data, to filter by a list (a SQL IN (...) clause), and to support some basic JOIN conditions.

Database patches

This was a merge of two newinternal patches into one, because the master branch added another database patch with the ID 17, so newinternal had to move up a bit. Instead of renumbering the newinternal patches, I simply merged the two into one.

It follows the standard database patching system we have already - changes are wrapped in a block of SQL code which validates and ensures patches are run in the correct order, and that the version of the database matches the version of the application (managed by the schemaversion table). There's a lot of boilerplate code wrapping these changes - you can see the example in patch00-example.sql

DataObjectTest, PdoDatabaseTest, DebugHelperTest

These are basic unit test files; I'm not sure what there is to explain here. They are not part of the main code.

base.tpl, PageBase, and TemplateOutput

The change here is to add the framework needed to allow individual pages to augment the loaded CSS and JS with additional resources dynamically. While this is not used by anything in this PR, it is used by some pages in future CRs I have lined up.

I've raised #506 and #507 to resolve some cleanup and safety changes here, but again I'm not really wanting to deal with that in this PR. Both of these are on the "blocking newinternal" column in the project.

By "page", I do mean any subclass of PageBase, which is the "parent" of every single page in the new tool. Each page is implemented as a subclass of that class. Methods implement the functionality exposed on that page. For example, there is a page called PageEmailManagement which represents the page which allows you to see, edit, and create email responses to requests. Actions, such as editing a template or showing the list, are represented as methods in that class. The routing of web requests to those methods is handled by a huge array in RequestRouter which ensures you can't call random methods on classes from the web.

[dqwiki]

Ok, everything has been signed off now.