diff --git a/CHANGELOG.md b/CHANGELOG.md index 788b308..aaebe51 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ All notable changes to this project are documented here. Format: [Keep a Changel ## [Unreleased] +- Fix: a degraded `authentik.mapper` health check now says what to fix. The message and details (`flaggedGroups`, `summary`) name each flagged group with a safe reason: `grants nothing: `, `malformed ` or `superuser group carries qqq.io/permissions`. It used to say only "Mapper drift detected". Typed attribute values and service-account activity subjects (people) stay out of the outcome. A group with no `qqq.io/*` attributes is not drift and never was. - Fix: the `token_broker` health check probes with the grant the broker uses (token exchange) and sends no subject token, so nothing is issued. `invalid_request` (credentials passed) is UP, `invalid_client` is DOWN (bad secret), and `invalid_grant` is DOWN (provider does not allow token exchange). It used to probe `client_credentials`, which the broker provider does not allow, so Authentik logged a 400 every 5 minutes ("Invalid grant_type for provider"). - Replace expired token-delivery and credential-scope reflection bridges with the RC.7 APIs; retain conservative permission narrowing and explicit unavailable-delivery tests. - Ship the expanded, host-neutral QBIT help bundle using QQQ 4.1.0-RC.7 registration and validation, with coverage for optional features and live references. Preserve inline app/table help for hosts without help storage; document HOST bundle overrides. diff --git a/src/main/java/com/kingsrook/qbits/authentik/drift/AuthentikDriftReport.java b/src/main/java/com/kingsrook/qbits/authentik/drift/AuthentikDriftReport.java index eecfd8a..8bf362a 100644 --- a/src/main/java/com/kingsrook/qbits/authentik/drift/AuthentikDriftReport.java +++ b/src/main/java/com/kingsrook/qbits/authentik/drift/AuthentikDriftReport.java @@ -22,6 +22,7 @@ import java.util.Comparator; import java.util.HashSet; import java.util.List; import java.util.Set; +import java.util.regex.Pattern; import com.kingsrook.qbits.authentik.AuthentikQBitConfig; import com.kingsrook.qbits.authentik.client.AuthentikActionUtil; import com.kingsrook.qbits.authentik.client.AuthentikJson; @@ -61,6 +62,9 @@ public class AuthentikDriftReport { private static final QLogger LOG = QLogger.getLogger(AuthentikDriftReport.class); + private static final Pattern SAFE_PERMISSION = Pattern.compile("[A-Za-z0-9_.*-]{1,120}"); + private static final Pattern SAFE_ATTRIBUTE = Pattern.compile("[A-Za-z0-9_./-]{1,60}"); + private final AuthentikActionUtil client; private final AuthentikQBitConfig config; private final AuthentikGuard guard; @@ -373,6 +377,19 @@ public class AuthentikDriftReport + /******************************************************************************* + ** The attribute name of a GroupGraph malformed entry ("key: detail"), never + ** the detail, which can echo whatever a person typed. + *******************************************************************************/ + static String attributeOf(String malformedEntry) + { + int colon = malformedEntry == null ? -1 : malformedEntry.indexOf(": "); + String key = colon < 0 ? "" : malformedEntry.substring(0, colon); + return (SAFE_ATTRIBUTE.matcher(key).matches() ? key : "attribute"); + } + + + /******************************************************************************* ** 6 *******************************************************************************/ @@ -388,14 +405,16 @@ public class AuthentikDriftReport for(String bad : node.malformed()) { issues++; - findings.add(DriftFinding.warning("group attributes", node.name(), "Malformed entry ignored (" + bad + "); qqq.io/permissions and qqq.io/app-access must be lists of strings.")); + findings.add(DriftFinding.warning("group attributes", node.name(), "Malformed entry ignored (" + bad + "); qqq.io/permissions and qqq.io/app-access must be lists of strings.") + .withReason("malformed " + attributeOf(bad))); } for(String permission : node.permissions()) { if(resolver.expand(permission).isEmpty()) { issues++; - findings.add(DriftFinding.warning("group attributes", node.name(), "qqq.io/permissions entry \"" + permission + "\" grants nothing in this app (unknown permission or empty wildcard).")); + findings.add(DriftFinding.warning("group attributes", node.name(), "qqq.io/permissions entry \"" + permission + "\" grants nothing in this app (unknown permission or empty wildcard; a name the app checks only in code, not in its metadata, is not in the catalog).") + .withReason("grants nothing: " + (SAFE_PERMISSION.matcher(permission).matches() ? permission : "(unprintable name)"))); } } for(String app : node.appAccess()) @@ -409,7 +428,8 @@ public class AuthentikDriftReport if(graph.isSuperuserInherited(node.pk()) && !node.permissions().isEmpty()) { issues++; - findings.add(DriftFinding.warning("group attributes", node.name(), "Superuser group (or under one) carries qqq.io/permissions. QQQ cannot manage it; keep QQQ grants on ordinary groups.")); + findings.add(DriftFinding.warning("group attributes", node.name(), "Superuser group (or under one) carries qqq.io/permissions. QQQ cannot manage it; keep QQQ grants on ordinary groups.") + .withReason("superuser group carries qqq.io/permissions")); } } if(issues == 0) diff --git a/src/main/java/com/kingsrook/qbits/authentik/drift/DriftFinding.java b/src/main/java/com/kingsrook/qbits/authentik/drift/DriftFinding.java index 1820f4b..4c770d9 100644 --- a/src/main/java/com/kingsrook/qbits/authentik/drift/DriftFinding.java +++ b/src/main/java/com/kingsrook/qbits/authentik/drift/DriftFinding.java @@ -25,10 +25,33 @@ package com.kingsrook.qbits.authentik.drift; ** @param check the check's short name, e.g. "blueprints" ** @param subject what the finding is about, e.g. a group name ** @param message plain-language explanation and what to do + ** @param reason a short, safe summary for health outcomes (no attribute + ** values a person typed beyond validated permission names); + ** null when the finding has none *******************************************************************************/ -public record DriftFinding(Severity severity, String check, String subject, String message) +public record DriftFinding(Severity severity, String check, String subject, String message, String reason) { + /******************************************************************************* + ** A finding without a health reason. + *******************************************************************************/ + public DriftFinding(Severity severity, String check, String subject, String message) + { + this(severity, check, subject, message, null); + } + + + + /******************************************************************************* + ** This finding with a short, safe reason for health outcomes. + *******************************************************************************/ + public DriftFinding withReason(String reason) + { + return (new DriftFinding(severity, check, subject, message, reason)); + } + + + /******************************************************************************* ** Severity, most severe first. *******************************************************************************/ diff --git a/src/main/java/com/kingsrook/qbits/authentik/health/AuthentikHealthChecks.java b/src/main/java/com/kingsrook/qbits/authentik/health/AuthentikHealthChecks.java index 20046d7..55aec00 100644 --- a/src/main/java/com/kingsrook/qbits/authentik/health/AuthentikHealthChecks.java +++ b/src/main/java/com/kingsrook/qbits/authentik/health/AuthentikHealthChecks.java @@ -19,7 +19,10 @@ package com.kingsrook.qbits.authentik.health; import java.time.Instant; import java.time.temporal.ChronoUnit; +import java.util.ArrayList; import java.util.List; +import java.util.Locale; +import java.util.TreeMap; import java.util.TreeSet; import com.kingsrook.qbits.authentik.AuthentikQBitConfig; import com.kingsrook.qbits.authentik.client.AuthentikActionUtil; @@ -45,6 +48,8 @@ import org.json.JSONObject; *******************************************************************************/ public final class AuthentikHealthChecks { + private static final String GROUP_ATTRIBUTES = "group attributes"; + /******************************************************************************* ** Utility class. *******************************************************************************/ @@ -151,30 +156,91 @@ public final class AuthentikHealthChecks /******************************************************************************* - ** Findings carry fixed codes only, never subjects or diagnostic messages. + ** Findings carry fixed codes, plus the flagged group names and each group + ** finding's safe reason (DriftFinding.reason), so a degraded mapper says + ** what to fix. Service-account activity subjects (people) stay private. *******************************************************************************/ - private static HealthCheckOutcome mapper(AuthentikActionUtil util, AuthentikQBitConfig config) + static HealthCheckOutcome mapper(AuthentikActionUtil util, AuthentikQBitConfig config) { - HealthStatus status = HealthStatus.UP; - TreeSet codes = new TreeSet<>(); - for(DriftFinding finding : new AuthentikDriftReport(util, config).runMapperChecks()) + return (mapperOutcome(new AuthentikDriftReport(util, config).runMapperChecks())); + } + + + + /******************************************************************************* + ** The mapper outcome for a set of drift findings. + *******************************************************************************/ + static HealthCheckOutcome mapperOutcome(List findings) + { + HealthStatus status = HealthStatus.UP; + TreeSet codes = new TreeSet<>(); + TreeMap>> byGroup = new TreeMap<>(); + TreeSet other = new TreeSet<>(); + for(DriftFinding finding : findings) { - if(finding.severity() == DriftFinding.Severity.ERROR || finding.severity() == DriftFinding.Severity.WARNING) + if(finding.severity() != DriftFinding.Severity.ERROR && finding.severity() != DriftFinding.Severity.WARNING) { - String code = "group attributes".equals(finding.check()) ? "group_attributes" : "service_account_activity"; - codes.add(code + "." + finding.severity().name().toLowerCase(java.util.Locale.ROOT)); - if(finding.severity() == DriftFinding.Severity.ERROR) + continue; + } + boolean groupCheck = GROUP_ATTRIBUTES.equals(finding.check()); + codes.add((groupCheck ? "group_attributes" : "service_account_activity") + "." + finding.severity().name().toLowerCase(Locale.ROOT)); + status = finding.severity() == DriftFinding.Severity.ERROR || status == HealthStatus.DOWN ? HealthStatus.DOWN : HealthStatus.DEGRADED; + if(groupCheck && finding.reason() != null) + { + String reason = finding.reason(); + int colon = reason.indexOf(": "); + TreeSet values = byGroup.computeIfAbsent(safeName(finding.subject()), k -> new TreeMap<>()) + .computeIfAbsent(colon < 0 ? reason : reason.substring(0, colon), k -> new TreeSet<>()); + if(colon >= 0) { - status = HealthStatus.DOWN; - } - else if(status != HealthStatus.DOWN) - { - status = HealthStatus.DEGRADED; + values.add(reason.substring(colon + 2)); } } + else + { + other.add(groupCheck ? "group attributes check could not run" : "service-account activity (see the identity drift report)"); + } } - return (new HealthCheckOutcome().withStatus(status).withMessage(status == HealthStatus.UP ? "Mapper checks passed" : "Mapper drift detected") - .withDetails(new HealthDetails().withValue("findingCodes", String.join(",", codes)))); + if(status == HealthStatus.UP) + { + return (HealthCheckOutcome.up("Mapper checks passed").withDetails(new HealthDetails().withValue("findingCodes", ""))); + } + + List parts = new ArrayList<>(); + byGroup.forEach((group, reasons) -> + { + List texts = new ArrayList<>(); + reasons.forEach((label, values) -> texts.add(values.isEmpty() ? label : label + ": " + String.join(", ", values))); + parts.add(group + " (" + String.join("; ", texts) + ")"); + }); + parts.addAll(other); + String summary = String.join("; ", parts); + return (new HealthCheckOutcome().withStatus(status).withMessage(truncate("Mapper drift: " + summary, 500)) + .withDetails(new HealthDetails() + .withValue("findingCodes", String.join(",", codes)) + .withValue("flaggedGroups", truncate(String.join(", ", byGroup.keySet()), 500)) + .withValue("summary", truncate(summary, 500)))); + } + + + + /******************************************************************************* + ** A group name fit for a health message: printable and bounded. + *******************************************************************************/ + private static String safeName(String name) + { + String printable = name == null ? "" : name.replaceAll("\\p{Cntrl}", "").trim(); + return (printable.isEmpty() ? "(unnamed group)" : truncate(printable, 100)); + } + + + + /******************************************************************************* + ** Cut a text to a length, marking the cut. + *******************************************************************************/ + private static String truncate(String text, int max) + { + return (text.length() <= max ? text : text.substring(0, max - 3) + "..."); } diff --git a/src/test/java/com/kingsrook/qbits/authentik/drift/AuthentikDriftReportTest.java b/src/test/java/com/kingsrook/qbits/authentik/drift/AuthentikDriftReportTest.java index 89159c2..98867a5 100644 --- a/src/test/java/com/kingsrook/qbits/authentik/drift/AuthentikDriftReportTest.java +++ b/src/test/java/com/kingsrook/qbits/authentik/drift/AuthentikDriftReportTest.java @@ -186,4 +186,20 @@ class AuthentikDriftReportTest assertThatThrownBy(() -> new AuthentikDriftReportWidgetRenderer().render(input)).isInstanceOf(QPermissionDeniedException.class); } + + + + /******************************************************************************* + ** A malformed entry's health reason keeps the attribute name, never the + ** value a person typed. + *******************************************************************************/ + @Test + void malformedReasonKeepsOnlyTheAttribute() + { + assertThat(AuthentikDriftReport.attributeOf("qqq.io/permissions: {\"x\":\"typed value\"}")).isEqualTo("qqq.io/permissions"); + assertThat(AuthentikDriftReport.attributeOf("qqq.io/policy: line 1: bad")).isEqualTo("qqq.io/policy"); + assertThat(AuthentikDriftReport.attributeOf("no separator")).isEqualTo("attribute"); + assertThat(AuthentikDriftReport.attributeOf("bad key with spaces: x")).isEqualTo("attribute"); + assertThat(AuthentikDriftReport.attributeOf(null)).isEqualTo("attribute"); + } } diff --git a/src/test/java/com/kingsrook/qbits/authentik/health/AuthentikHealthChecksTest.java b/src/test/java/com/kingsrook/qbits/authentik/health/AuthentikHealthChecksTest.java index 0c7b82d..9b43269 100644 --- a/src/test/java/com/kingsrook/qbits/authentik/health/AuthentikHealthChecksTest.java +++ b/src/test/java/com/kingsrook/qbits/authentik/health/AuthentikHealthChecksTest.java @@ -26,6 +26,7 @@ import com.kingsrook.qbits.authentik.AuthentikTestSupport; import com.kingsrook.qbits.authentik.client.AuthentikActionUtil; import com.kingsrook.qbits.authentik.client.FixtureActionUtil; import com.kingsrook.qbits.authentik.client.TokenBucketThrottle; +import com.kingsrook.qbits.authentik.drift.DriftFinding; import com.kingsrook.qqq.backend.core.context.QContext; import com.kingsrook.qqq.backend.core.exceptions.QException; import com.kingsrook.qqq.backend.core.health.HealthCategory; @@ -176,22 +177,49 @@ class AuthentikHealthChecksTest /******************************************************************************* - ** Only the two relevant drift checks run; their subjects stay private. + ** Only the two relevant drift checks run. Group findings name the group and + ** a safe reason; typed attribute values and people (service-account + ** activity subjects) stay private. *******************************************************************************/ @Test void mapperStates() throws QException { assertThat(run("mapper").getStatus()).isEqualTo(HealthStatus.UP); - JSONObject group = new JSONObject().put("pk", "g1").put("name", SECRET) - .put("attributes", new JSONObject().put("qqq.io/permissions", new JSONArray().put(SECRET))); - FixtureActionUtil.groupGraphBody = new JSONObject(FixtureActionUtil.EMPTY_PAGE).put("results", new JSONArray().put(group)).toString(); + + ////////////////////////////////////////////////////////////////////////////// + // a group with no qqq.io/* attributes (kof22-admins on its static floor) is // + // not drift // + ////////////////////////////////////////////////////////////////////////////// + JSONObject floorOnly = new JSONObject().put("pk", "g0").put("name", "kof22-admins").put("attributes", new JSONObject()); + groups(floorOnly); var outcome = run("mapper"); + assertThat(outcome.getStatus()).isEqualTo(HealthStatus.UP); + assertThat(outcome.getMessage()).isEqualTo("Mapper checks passed"); + + JSONObject unknown = new JSONObject().put("pk", "g1").put("name", "file-gateway-admins") + .put("attributes", new JSONObject().put("qqq.io/permissions", new JSONArray().put("file_gateway.view").put("file_gateway.admin"))); + JSONObject malformed = new JSONObject().put("pk", "g2").put("name", "editors") + .put("attributes", new JSONObject().put("qqq.io/permissions", new JSONArray().put(new JSONObject().put("x", SECRET)))); + JSONObject superuser = new JSONObject().put("pk", "g3").put("name", "authentik Admins").put("is_superuser", true) + .put("attributes", new JSONObject().put("qqq.io/permissions", new JSONArray().put("authentik.users.read"))); + groups(floorOnly, unknown, malformed, superuser); + outcome = run("mapper"); assertThat(outcome.getStatus()).isEqualTo(HealthStatus.DEGRADED); - assertThat(outcome.getDetails().getValues()).containsEntry("findingCodes", "group_attributes.warning"); + assertThat(outcome.getMessage()).isEqualTo("Mapper drift: authentik Admins (superuser group carries qqq.io/permissions); " + + "editors (malformed qqq.io/permissions); file-gateway-admins (grants nothing: file_gateway.admin, file_gateway.view)"); + assertThat(outcome.getDetails().getValues()) + .containsEntry("findingCodes", "group_attributes.warning") + .containsEntry("flaggedGroups", "authentik Admins, editors, file-gateway-admins") + .containsEntry("summary", outcome.getMessage().substring("Mapper drift: ".length())); + assertClean(outcome); + + groups(unknown); page(new JSONObject().put("action", "model_deleted").put("context", new JSONObject() .put("model", new JSONObject().put("model_name", "user").put("pk", "1").put("name", SECRET)))); outcome = run("mapper"); assertThat(outcome.getStatus()).isEqualTo(HealthStatus.DOWN); + assertThat(outcome.getMessage()).isEqualTo("Mapper drift: file-gateway-admins (grants nothing: file_gateway.admin, file_gateway.view); " + + "service-account activity (see the identity drift report)"); assertClean(outcome); assertThat(FixtureActionUtil.REQUESTS).allSatisfy(request -> assertThat(request.getURI().getPath()) .doesNotContain("/admin/version/", "/core/tokens/", "/managed/blueprints/")); @@ -199,6 +227,52 @@ class AuthentikHealthChecksTest + /******************************************************************************* + ** A group check that cannot run, unprintable names and long summaries stay + ** readable and within the health limits. + *******************************************************************************/ + @Test + void mapperOutcomeEdges() + { + var outcome = AuthentikHealthChecks.mapperOutcome(List.of( + DriftFinding.error("group attributes", "-", "Check could not run: " + SECRET), + DriftFinding.info("group attributes", "x", "info only").withReason("ignored"), + DriftFinding.warning("group attributes", "bad\u0007name", "m").withReason("grants nothing: (unprintable name)"), + DriftFinding.warning("group attributes", "", "m").withReason("malformed attribute"))); + assertThat(outcome.getStatus()).isEqualTo(HealthStatus.DOWN); + assertThat(outcome.getMessage()).isEqualTo("Mapper drift: (unnamed group) (malformed attribute); badname (grants nothing: (unprintable name)); " + + "group attributes check could not run"); + assertThat(outcome.getDetails().getValues()).containsEntry("findingCodes", "group_attributes.error,group_attributes.warning"); + assertClean(outcome); + + List many = new java.util.ArrayList<>(); + for(int i = 0; i < 60; i++) + { + many.add(DriftFinding.warning("group attributes", "group-number-" + i, "m").withReason("grants nothing: some.long.permission.name." + i)); + } + outcome = AuthentikHealthChecks.mapperOutcome(many); + assertThat(outcome.getMessage()).hasSize(500).endsWith("..."); + assertThat(outcome.getDetails().toJson().getBytes(java.nio.charset.StandardCharsets.UTF_8).length).isLessThanOrEqualTo(4096); + assertThat(outcome.getDetails().getValues()).containsKeys("findingCodes", "flaggedGroups", "summary"); + } + + + + /******************************************************************************* + ** Serve these groups as the group graph. + *******************************************************************************/ + private static void groups(JSONObject... groups) + { + JSONArray results = new JSONArray(); + for(JSONObject group : groups) + { + results.put(group); + } + FixtureActionUtil.groupGraphBody = new JSONObject(FixtureActionUtil.EMPTY_PAGE).put("results", results).toString(); + } + + + /******************************************************************************* ** The broker check uses the grant the broker uses (token exchange) with no ** subject: invalid_request proves the credentials; nothing is ever issued.