[mds-compliance][mds-db] Fix/compliance handles timestamp #316

janedotx · 2020-05-13T02:40:12Z

Update compliance /count/ endpoint to return relevant policy and to accept a timestamp parameter. Update compliance /snapshot endpoint to search only for policies active at exactly the time specified by the timestamp parameter.

PR Checklist

simple searchable title - [mds-db] Add PG env var, [config] Fix eslint config
briefly describe the changes in this PR
mark as draft if should not be merged
write tests for all new functionality

Impacts

…nce-code-to-match-spec

avatarneil · 2020-05-13T16:34:34Z

packages/mds-compliance/api.ts

-      ? { end_date: query_end_date, start_date: query_end_date - days(365) }
-      : { end_date: now() + days(365), start_date: now() - days(365) }
+
+    const timestamp = query_end_date || now()


Proposal: Breaking change (but we have nothing querying for historical compliance and the spec is still in PR so should be okay

We should just change the query_end_date to a query param of timestamp, then we can just do a one-liner and assign a default value on line 81

I support renaming, but I'd rather not assign a default value. Easier to condition on timestamp being null when determining whether to get historical events or read from the cache that way.

avatarneil · 2020-05-13T16:35:33Z

packages/mds-compliance/api.ts

    if (!AllowedProviderIDs.includes(res.locals.provider_id)) {
      return res.status(401).send({ result: 'unauthorized access' })
    }

+    const timestamp = query_end_date || now()


Totally unnecessary to do this, just don't rename to query_end_date when destructuring on line 163 and instead supply a default value of now()

avatarneil · 2020-05-13T16:35:59Z

packages/mds-compliance/api.ts

+    const { timestamp: query_end_date } = {
+      ...parseRequest(req, { parser: Number }).query('timestamp')
+    }
+    //* /


Unsure what these comments are for

It renders like normal code in github, I can't tell if the block is commented out or not

avatarneil · 2020-05-13T16:40:28Z

packages/mds-db/policies.ts

@@ -56,6 +56,22 @@ export async function readPolicies(params?: {
  return res.rows.map(row => row.policy_json)
 }

+export async function readActivePolicies(timestamp: Timestamp = now()): Promise<Policy[]> {


I don't see why we can't just reuse the readPolicies method here, but if there's a need to create a new method then the naming seems a little off -- when I think of active policies I think of start_date <= now && end_date >= now && published roughly (with the requirement for some null checking as well). It seems that the start/end restrictions are being met, but it's not considering if the policies are published or not

It is checking for a non-null publish_date on line 79.

but publish_date should take into account the timestamp

I think that was not the case when I made this comment ;) I think the fundamental issue I have with this is that we're using a different method for determining "active policies" between Policy and Compliance. They really ought to be using the same methodology, which is tricky because Policy enables the user to get non-active policies depending on their permissions.

As much as I'd like to do all of the filtering in the DB query, I think what may make sense is to just use the same logic that we have for Policy

const policies = await db.readPolicies({ get_published, get_unpublished }) const prev_policies: UUID[] = policies.reduce((prev_policies_acc: UUID[], policy: Policy) => { if (policy.prev_policies) { prev_policies_acc.push(...policy.prev_policies) } return prev_policies_acc }, []) const active = policies.filter(p => { // overlapping segment logic const p_start_date = p.start_date const p_end_date = p.end_date || Number.MAX_SAFE_INTEGER return end_date >= p_start_date && p_end_date >= start_date && !prev_policies.includes(p.policy_id) })

(Indentation got substantially borked, but you get the idea)

avatarneil · 2020-05-13T16:41:03Z

packages/mds-db/policies.ts

+  return res.rows
+    .map(row => row.policy_json)
+    .filter(policy => {
+      return policy.end_date === null || policy.end_date >= timestamp
+    })


Seems super strange to me that we do the start_date check as part of the query, but then do a filter on the result for end_date

Noted. I'm keeping readActivePolicies though, since readPolicies is a more generalized method that allows you to set get_unpublished to true, and it feels weird to have that available as an option when active policies must be published.

avatarneil · 2020-05-13T16:45:24Z

packages/mds-compliance/api.ts

+      const deviceRecords = await db.readDeviceIds()
+      const deviceIDs = deviceRecords.map(recorded_device => recorded_device.device_id)


I don't think we should do these reads unless we're operating in a 'historical' context

avatarneil · 2020-05-13T16:49:33Z

packages/mds-compliance/api.ts

    async function fail(err: Error) {
      logger.error(err.stack || err)
-      if (err.message.includes('invalid rule_id')) {
+      if (err.message.includes('invalid rule_id') || err.message.includes('not found in any policy')) {


I think we should try to avoid adding more things into this error handling pattern if possible

+1, if we need to do this sort of error handling branching I'd rather be comparing status codes, string enums, or doing instanceof checks.

Fixing the error handling feels out of scope. I'd rather keep this PR focused on handling the timestampparam.

Sounds reasonable, maybe a good candidate for a follow up PR

I was going to split that out into a different PR so it'd be easier to read, but I think it's faster to just stick it all in this one.

avatarneil · 2020-05-13T16:51:35Z

packages/mds-compliance/api.ts

      const rule = await db.readRule(rule_id)
+      const policies = await db.findPoliciesByRule(rule_id)
+      if (policies.length === 0) {
+        throw new NotFoundError('Rule ID not found in any policy')
+      }


Should just get rid of the db.readRule method and extract the rule from whatever policy is returned

avatarneil · 2020-05-13T16:53:41Z

packages/mds-db/policies.ts

@@ -267,6 +283,19 @@ export async function readRule(rule_id: UUID): Promise<Rule> {
  }
 }

+export async function findPoliciesByRule(rule_id: UUID): Promise<Policy[]> {


We should make this only return a single policy instead of an array, especially if you're throwing an error upon the length != 1. That being said, I still think we could just use readPolicies and just add an extra param for rule_id and the conditional statement

The error is only thrown if zero policies are returned. It doesn't say in the spec that rules must have unique UUIDs and there's nothing preventing multiple policies from sharing a rule at the moment.

The error is thrown when res.rowCount !== 1 not res.rowCount === 0. The count endpoint is really only valid under the assumption that rule_ids are unique, even though it's not explicit in the spec (or enforced in the implementation)

macsj200

I'd like to learn a little bit more about what this is doing before I +1, maybe we can do a quick screenshare.

macsj200 · 2020-05-13T17:06:38Z

packages/mds-compliance/api.ts

    async function fail(err: Error) {
      logger.error(err.stack || err)
-      if (err.message.includes('invalid rule_id')) {
+      if (err.message.includes('invalid rule_id') || err.message.includes('not found in any policy')) {


+1, if we need to do this sort of error handling branching I'd rather be comparing status codes, string enums, or doing instanceof checks.

janedotx · 2020-05-13T18:23:27Z

packages/mds-compliance/api.ts

+      const events =
+        query_end_date && timestamp < now()
+          ? await db.readHistoricalEvents({ end_date: timestamp })
+          : await cache.readEvents(deviceIDs)


change to cache.readAllEvents

janedotx · 2020-05-13T18:25:00Z

packages/mds-compliance/api.ts

+      const deviceRecords = await db.readDeviceIds()
+      const deviceIDs = deviceRecords.map(recorded_device => recorded_device.device_id)
+      const events =
+        query_end_date && timestamp < now()


get rid of timestamp < now() in both endpoints

…flect that, and renaming `end_date` param to `timestamp`.

avatarneil · 2020-05-15T14:07:20Z

packages/mds-compliance/api.ts

    }

    const { rule_id } = req.params
    try {
-      const rule = await db.readRule(rule_id)
+      const policies = await db.readActivePolicies(query_date)
+      const filteredPolicies = policies.filter(policy => {


Instead of doing this filtering in TS, we could adapt the SQL query to look for a matching rule_id in the policy json.

Ought to also just be destructuring to pull off the head like const [policy] = db.readActivePolicies(...), I see that on line 209 the policy that we send in the response is not necessarily the policy we considered when computing compliance

Ought to also just be destructuring to pull off the head like const [policy] = db.readActivePolicies(...), I see that on line 209 the policy that we send in the response is not necessarily the policy we considered when computing compliance

I don't see how that's possible, now that rule uniqueness is actually enforced.

Because filteredPolicies !== policies. We consider the head of the filteredPolicies for the evaluation, but in the response body we are returning policies[0].

So the policy that we return in the payload may not be a policy that even has the rule_id in question

packages/mds-db/policies.ts

avatarneil · 2020-05-15T14:13:59Z

packages/mds-db/policies.ts

+      const other_policies = await readPolicies({ rule_id: rule.rule_id })
+      other_policies.map(other_policy => {
+        if (other_policy.policy_id !== policy.policy_id) {
+          throw new ConflictError(`Policy containing with a rule of id ${rule.rule_id} already exists`)
+        }


We typically use camelCase as opposed to snake_case for variables, unless they're based off of DB columns or query parameters.

packages/mds-compliance/api.ts

…iqueness checks

…' into fix/compliance-handles-timestamp

…dles-timestamp

janedotx added 7 commits May 8, 2020 13:09

Query for policy by rule_id.

4ce958c

Merge remote-tracking branch 'origin/develop' into fix/update-complia…

5d031d2

…nce-code-to-match-spec

Adding ability to see Policies active at a particular time.

0064541

Refactored snapshot logic.

4cb2a1c

Adding test to ensure timestamp paramenter works for count endpoint

85cba27

Merge remote-tracking branch 'origin/develop' into fix/update-complia…

479229d

…nce-code-to-match-spec

minor fixes

4cf2f49

janedotx requested review from avatarneil, mdurling, marie-x, macsj200 and wesleyfsmith May 13, 2020 02:40

avatarneil reviewed May 13, 2020

View reviewed changes

macsj200 reviewed May 13, 2020

View reviewed changes

macsj200 approved these changes May 13, 2020

View reviewed changes

janedotx commented May 13, 2020

View reviewed changes

janedotx added 3 commits May 13, 2020 22:05

Making policy rules unique and refactoring compliance api logic to re…

8713123

…flect that, and renaming `end_date` param to `timestamp`.

repairing test cases that broke because rules cannot be repeated

1e80d6e

Merge branch 'develop' into fix/compliance-handles-timestamp

40fb1d9

avatarneil requested changes May 15, 2020

View reviewed changes

Mark Maxham and others added 7 commits May 15, 2020 08:11

Merge branch 'develop' into fix/compliance-handles-timestamp

266a0f9

more consistent error handling and better active policies and rule un…

670ec40

…iqueness checks

Versioning and fixing the definition of an active policy

31456a0

Merge remote-tracking branch 'origin/fix/compliance-handles-timestamp…

347c02e

…' into fix/compliance-handles-timestamp

Merge remote-tracking branch 'origin/develop' into fix/compliance-han…

accdbb4

…dles-timestamp

minor fixes

c00bcdb

minor fixes

fdcd7ad

avatarneil approved these changes May 15, 2020

View reviewed changes

janedotx merged commit 86d157c into develop May 15, 2020

janedotx deleted the fix/compliance-handles-timestamp branch May 15, 2020 22:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mds-compliance][mds-db] Fix/compliance handles timestamp #316

[mds-compliance][mds-db] Fix/compliance handles timestamp #316

janedotx commented May 13, 2020

avatarneil May 13, 2020

janedotx May 13, 2020

avatarneil May 13, 2020

avatarneil May 13, 2020

macsj200 May 13, 2020

avatarneil May 13, 2020

janedotx May 15, 2020

janedotx May 15, 2020

avatarneil May 15, 2020

avatarneil May 15, 2020

avatarneil May 13, 2020

janedotx May 13, 2020

avatarneil May 13, 2020

avatarneil May 13, 2020

macsj200 May 13, 2020

janedotx May 13, 2020

macsj200 May 13, 2020

janedotx May 14, 2020

avatarneil May 13, 2020

avatarneil May 13, 2020

janedotx May 13, 2020

avatarneil May 13, 2020

macsj200 left a comment

macsj200 May 13, 2020

janedotx May 13, 2020

janedotx May 13, 2020

avatarneil May 15, 2020

avatarneil May 15, 2020

janedotx May 15, 2020

avatarneil May 15, 2020

avatarneil May 15, 2020

avatarneil May 15, 2020

		const deviceRecords = await db.readDeviceIds()
		const deviceIDs = deviceRecords.map(recorded_device => recorded_device.device_id)

[mds-compliance][mds-db] Fix/compliance handles timestamp #316

[mds-compliance][mds-db] Fix/compliance handles timestamp #316

Conversation

janedotx commented May 13, 2020

PR Checklist

Impacts

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macsj200 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment