liuch / dmarc-srg

A php parser, viewer and summary report generator for incoming DMARC reports.
GNU General Public License v3.0
213 stars 31 forks source link

Add actual fail count to Result buttons #96

Open KlaasBonnema opened 10 months ago

KlaasBonnema commented 10 months ago

The results column on the Report list shows a DKIM and SPF button with a red or green text. A red text does not give any indication about how many failures are actually reported. If the Messages column has a count of 100 then there could be anywhere between 1 or 100 failures. Suggest to report an actual DKIM failure count of 5 like "DKIM: 5". This way you know how urgently you need to dig deeper.

williamdes commented 10 months ago

I agree, I have some people that receive the emails that must be doing forwarding and always report broken dkim and spf So basically all reports are red because one entry in the report is always failing out of my control

KlaasBonnema commented 10 months ago

I made the following changes to make this happening: ReportMapper.php

    public function list(array &$filter, array &$order, array &$limit): array
    {
        $db = $this->connector->dbh();
        $list = [];
        $f_data = $this->prepareFilterData($filter);
        $order_str = $this->sqlOrderList($order, '`rp`.`id`');
        $cond_str0 = $this->sqlConditionList($f_data, ' AND ', 0);
        $cond_str1 = $this->sqlConditionList($f_data, ' HAVING ', 1);
        $limit_str = $this->sqlLimit($limit);
        try
        {
            $st = $db->prepare('SELECT `org`, `begin_time`, `end_time`, `fqdn`, `external_id`, `seen`, SUM(`rcount`) AS `rcount`,' . ' MIN(`dkim_align`) AS `dkim_align`, MIN(`spf_align`) AS `spf_align`,' . ' MIN(`disposition`) AS `disposition` 
, (SELECT IFNULL(SUM(f2.rcount),"") FROM `' . $this->connector->tablePrefix('rptrecords') . '` AS `f2` WHERE f2.report_id=rr.report_id AND f2.dkim_align<2) AS dkim_fail_count
, (SELECT IFNULL(SUM(f3.rcount),"") FROM `' . $this->connector->tablePrefix('rptrecords') . '` AS `f3` WHERE f3.report_id=rr.report_id AND f3.spf_align<2) AS spf_fail_count
FROM `' . $this->connector->tablePrefix('rptrecords') . '` AS `rr` 
RIGHT JOIN (SELECT `rp`.`id`, `org`, `begin_time`, `end_time`, `external_id`,' . ' `fqdn`, `seen` FROM `' . $this->connector->tablePrefix('reports') . '` AS `rp` 
INNER JOIN `' . $this->connector->tablePrefix('domains') . '` AS `d` ON `d`.`id` = `rp`.`domain_id`' . $cond_str0 . $order_str . ') AS `rp` ON `rp`.`id` = `rr`.`report_id` GROUP BY `rp`.`id`' . $cond_str1 . $order_str . $limit_str);
            $this->sqlBindValues($st, $f_data, $limit);
            $st->execute();
            while ($row = $st->fetch(\PDO::FETCH_NUM))
            {
                $list[] = [
                    'org_name' => $row[0],
                    'date' => [
                        'begin' => new DateTime($row[1]),
                        'end' => new DateTime($row[2])
                    ],
                    'domain' => $row[3],
                    'report_id' => $row[4],
                    'seen' => (bool) $row[5],
                    'messages' => $row[6],
                    'dkim_align' => Common::$align_res[$row[7]],
                    'spf_align' => Common::$align_res[$row[8]],
                    'disposition' => Common::$disposition[$row[9]],
                    'dkim_fail_count' => $row[10],
                    'spf_fail_count' => $row[11]
                ];
            }
            $st->closeCursor();
        } catch (\PDOException $e)
        {
            throw new DatabaseFatalException('Failed to get the report list', - 1, $e);
        }
        return $list;
    }

list.js:

    _make_row_data(d) {
        let rd = {
            cells: [],
            userdata: { domain: d.domain, time: d.date.begin, org: d.org_name, report_id: d.report_id },
            seen: d.seen && true || false
        };
        rd.cells.push({ content: d.domain, label: "Domain" });
        let d1 = new Date(d.date.begin);
        let d2 = new Date(d.date.end);
        rd.cells.push({ content: date_range_to_string(d1, d2), title: d1.toUIString(true) + " - " + d2.toUIString(true), label: "Date" });
        rd.cells.push({ content: d.org_name, label: "Reporting Organization" });
        rd.cells.push({ content: d.report_id, class: "report-id" });
        rd.cells.push({ content: d.messages, label: "Messages" });
        rd.cells.push(new StatusColumn({ dkim_align: d.dkim_align, spf_align: d.spf_align,dkim_fail_count: d.dkim_fail_count, spf_fail_count:d.spf_fail_count }));
        return rd;
    }
class StatusColumn extends ITableCell {
    element() {
        if (!this._element) {
            super.element().setAttribute("data-label", "Result");
        }
        return this._element;
    }

    value(target) {
        if (target === "dom") {
            let d = this._content;
            let fr = document.createDocumentFragment();
            if (d.dkim_align) {
                fr.appendChild(create_report_result_element("DKIM", d.dkim_fail_count,d.dkim_fail_count,d.dkim_align));
            }
            if (d.spf_align) {
                fr.appendChild(create_report_result_element("SPF", d.spf_fail_count,d.spf_fail_count,d.spf_align));
            }
            return fr;
        }
        return super.value(target);
    }
}
williamdes commented 10 months ago

Can you edit your post and use fences?


```php
// code
KlaasBonnema commented 10 months ago

I don't understand what you mean with fences.

Following is the actual lines that were changed. I copied complete functions before:

$st = $db->prepare('SELECT org, begin_time, end_time, fqdn, external_id, seen, SUM(rcount) AS rcount,' . ' MIN(dkim_align) AS dkim_align, MIN(spf_align) AS spf_align,' . ' MIN(disposition) AS disposition
, (SELECT IFNULL(SUM(f2.rcount),"") FROM ' . $this->connector->tablePrefix('rptrecords') . ' AS f2 WHERE f2.report_id=rr.report_id AND f2.dkim_align<2) AS dkim_fail_count
, (SELECT IFNULL(SUM(f3.rcount),"") FROM ' . $this->connector->tablePrefix('rptrecords') . ' AS f3 WHERE f3.report_id=rr.report_id AND f3.spf_align<2) AS spf_fail_count
'dkim_fail_count' => $row[10],
'spf_fail_count' => $row[11]

fr.appendChild(create_report_result_element("DKIM", d.dkim_fail_count,d.dkim_fail_count,d.dkim_align));

fr.appendChild(create_report_result_element("SPF", d.spf_fail_count,d.spf_fail_count,d.spf_align));
williamdes commented 10 months ago

I don't understand what you mean with fences.

You can edit your comments and use my example

```php
$st = $db->prepare('SELECT org, begin_time, end_time, fqdn, external_id, seen, S...

will look like 
```php
$st = $db->prepare('SELECT org, begin_time, end_time, fqdn, external_id, seen, S...
KlaasBonnema commented 10 months ago

@williamdes, Thanks - I learned something new :-)

williamdes commented 10 months ago

Thank you! I looks 200% better It works on many Markdown based platforms, like Stackoverflow

You have one last message to fence just before my example :)

Also, you can propose diffs using diff

Like

- $foo = 1;
+ $foo = 2;
liuch commented 10 months ago

I think this idea is worth implementing. But I need to think about a more optimal SQL query, as nested queries don't seem to be the optimal solution to me.