feat: add UniFi connector support - #1690
Conversation
- Implemented UniFi registration in the built-in registry. - Added buildUniFiProbeURL function to construct probe URLs for UniFi consoles. - Created unifi.go and unifi_test.go files to define the UniFi connector logic and tests. - Updated coredata to include UniFi connector settings and provider. - Added migration for UniFi connector provider to the database. - Enhanced API to handle UniFi console ID in connector settings. - Updated GraphQL schema to include UniFi connector input fields. Signed-off-by: Steven4Hooisma <112615049+Steven4Hooisma@users.noreply.github.com>
45d0207 to
933d6c1
Compare
There was a problem hiding this comment.
5 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="pkg/connector/provider/probe.go">
<violation number="1" location="pkg/connector/provider/probe.go:399">
P2: The probe treats everything except 401/403 as 'connected', so a globally-valid X-API-KEY aimed at a wrong/nonexistent console ID returns a 404 from api.ui.com that this generic probe swallows — the connection check shows green while every later access review fails on the misconfigured console. This is the exact case probeCrisp in this file solves by adding 404 to the reject set ("a valid token whose website_id is wrong or unbound returns 404 ... would otherwise pass the probe"). Consider converting UniFi to a probe closure that treats 404 (and any non-200) as a hard rejection so a bad console ID surfaces at connection time rather than silently passing.</violation>
</file>
<file name="pkg/connector/provider/probe_test.go">
<violation number="1" location="pkg/connector/provider/probe_test.go:459">
P3: The header comment for TestBuildProbeURLFromAPIBase still says 'three probe builders', but adding the unifi case makes it four. Update the wording so the documentation matches the actual case count.</violation>
</file>
<file name="pkg/accessreview/drivers/unifi.go">
<violation number="1" location="pkg/accessreview/drivers/unifi.go:568">
P2: The ExternalID fallback for a RADIUS user without an `_id` reuses unifiMACKey, which is a MAC normalizer that strips ':', '-', '.', and space. Applied to a username, distinct names (e.g. "alice.smith" vs "alicesmith", or "a-b-c" vs "abc") collapse to the same key. Since review entries key on ExternalID — and the driver already goes to lengths elsewhere to guarantee unique ExternalIDs — two real accounts could merge into one review entry, letting a reviewer approve one while the other remains live. Consider keying the fallback off the raw username (optionally lowercased) rather than a separator-stripping MAC normalizer, so a username keeps its own identity unless it is genuinely a MAC address.</violation>
<violation number="2" location="pkg/accessreview/drivers/unifi.go:746">
P1: A deleted or inaccessible configured console returns 404 for the required site list, but this branch treats it as an empty feature and marks every prior UniFi entry REMOVED; return an error for `sites` while retaining best-effort 404 handling for optional collections.</violation>
</file>
<file name="pkg/server/api/console/v1/connector_settings.go">
<violation number="1" location="pkg/server/api/console/v1/connector_settings.go:382">
P2: The console ID validation is meant to reject any value that would silently retarget the driver's requests, but it does not account for dot path segments. Because the ID is fed through `url.PathEscape` (which leaves '.' unescaped) and then `url.JoinPath` (which cleans `..`/`.` segments), a value like `..` or `foo/..` passes the `ContainsAny` check yet gets resolved by JoinPath to a different path on api.ui.com — exactly the 'silently retarget' behavior the comment says it exists to prevent. The real console ID format (`<hex>:<digits>`) never contains a dot, so rejecting dots is safe and closes this hole. Consider adding '.' to the rejected character set (or validating the format more strictly) and adding a `..` case to the test.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| return nil, err | ||
| } | ||
|
|
||
| if status == http.StatusNotFound { |
There was a problem hiding this comment.
P1: A deleted or inaccessible configured console returns 404 for the required site list, but this branch treats it as an empty feature and marks every prior UniFi entry REMOVED; return an error for sites while retaining best-effort 404 handling for optional collections.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pkg/accessreview/drivers/unifi.go, line 746:
<comment>A deleted or inaccessible configured console returns 404 for the required site list, but this branch treats it as an empty feature and marks every prior UniFi entry REMOVED; return an error for `sites` while retaining best-effort 404 handling for optional collections.</comment>
<file context>
@@ -0,0 +1,981 @@
+ return nil, err
+ }
+
+ if status == http.StatusNotFound {
+ return nil, nil
+ }
</file context>
| // site listing — the same call the driver opens with, so a check that passes | ||
| // proves the key reaches the console the review will read, not merely that | ||
| // api.ui.com accepted it. | ||
| func buildUniFiProbeURL(conn *coredata.Connector, ep Endpoints) (string, error) { |
There was a problem hiding this comment.
P2: The probe treats everything except 401/403 as 'connected', so a globally-valid X-API-KEY aimed at a wrong/nonexistent console ID returns a 404 from api.ui.com that this generic probe swallows — the connection check shows green while every later access review fails on the misconfigured console. This is the exact case probeCrisp in this file solves by adding 404 to the reject set ("a valid token whose website_id is wrong or unbound returns 404 ... would otherwise pass the probe"). Consider converting UniFi to a probe closure that treats 404 (and any non-200) as a hard rejection so a bad console ID surfaces at connection time rather than silently passing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pkg/connector/provider/probe.go, line 399:
<comment>The probe treats everything except 401/403 as 'connected', so a globally-valid X-API-KEY aimed at a wrong/nonexistent console ID returns a 404 from api.ui.com that this generic probe swallows — the connection check shows green while every later access review fails on the misconfigured console. This is the exact case probeCrisp in this file solves by adding 404 to the reject set ("a valid token whose website_id is wrong or unbound returns 404 ... would otherwise pass the probe"). Consider converting UniFi to a probe closure that treats 404 (and any non-200) as a hard rejection so a bad console ID surfaces at connection time rather than silently passing.</comment>
<file context>
@@ -392,6 +392,19 @@ func buildSigNozProbeURL(conn *coredata.Connector, _ Endpoints) (string, error)
+// site listing — the same call the driver opens with, so a check that passes
+// proves the key reaches the console the review will read, not merely that
+// api.ui.com accepted it.
+func buildUniFiProbeURL(conn *coredata.Connector, ep Endpoints) (string, error) {
+ baseURL, err := unifiConsoleBaseURL(conn, ep)
+ if err != nil {
</file context>
| // request. Reject rather than sanitize: the ID is copied out of the | ||
| // Site Manager UI, so anything decorated is a paste error the operator | ||
| // should see. The message names only the field — never the value. | ||
| if strings.ContainsAny(consoleID, "/\\?#% \t") { |
There was a problem hiding this comment.
P2: The console ID validation is meant to reject any value that would silently retarget the driver's requests, but it does not account for dot path segments. Because the ID is fed through url.PathEscape (which leaves '.' unescaped) and then url.JoinPath (which cleans ../. segments), a value like .. or foo/.. passes the ContainsAny check yet gets resolved by JoinPath to a different path on api.ui.com — exactly the 'silently retarget' behavior the comment says it exists to prevent. The real console ID format (<hex>:<digits>) never contains a dot, so rejecting dots is safe and closes this hole. Consider adding '.' to the rejected character set (or validating the format more strictly) and adding a .. case to the test.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pkg/server/api/console/v1/connector_settings.go, line 382:
<comment>The console ID validation is meant to reject any value that would silently retarget the driver's requests, but it does not account for dot path segments. Because the ID is fed through `url.PathEscape` (which leaves '.' unescaped) and then `url.JoinPath` (which cleans `..`/`.` segments), a value like `..` or `foo/..` passes the `ContainsAny` check yet gets resolved by JoinPath to a different path on api.ui.com — exactly the 'silently retarget' behavior the comment says it exists to prevent. The real console ID format (`<hex>:<digits>`) never contains a dot, so rejecting dots is safe and closes this hole. Consider adding '.' to the rejected character set (or validating the format more strictly) and adding a `..` case to the test.</comment>
<file context>
@@ -364,6 +364,26 @@ func apiKeyConnectorSettings(input types.CreateAPIKeyConnectorInput) (json.RawMe
+ // request. Reject rather than sanitize: the ID is copied out of the
+ // Site Manager UI, so anything decorated is a paste error the operator
+ // should see. The message names only the field — never the value.
+ if strings.ContainsAny(consoleID, "/\\?#% \t") {
+ return nil, fmt.Errorf("cannot create unifi connector: unifiConsoleId must be a bare console identifier")
+ }
</file context>
| // recognises; the opaque _id is the fallback. | ||
| external := id | ||
| if external == "" { | ||
| external = unifiMACKey(name) |
There was a problem hiding this comment.
P2: The ExternalID fallback for a RADIUS user without an _id reuses unifiMACKey, which is a MAC normalizer that strips ':', '-', '.', and space. Applied to a username, distinct names (e.g. "alice.smith" vs "alicesmith", or "a-b-c" vs "abc") collapse to the same key. Since review entries key on ExternalID — and the driver already goes to lengths elsewhere to guarantee unique ExternalIDs — two real accounts could merge into one review entry, letting a reviewer approve one while the other remains live. Consider keying the fallback off the raw username (optionally lowercased) rather than a separator-stripping MAC normalizer, so a username keeps its own identity unless it is genuinely a MAC address.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pkg/accessreview/drivers/unifi.go, line 568:
<comment>The ExternalID fallback for a RADIUS user without an `_id` reuses unifiMACKey, which is a MAC normalizer that strips ':', '-', '.', and space. Applied to a username, distinct names (e.g. "alice.smith" vs "alicesmith", or "a-b-c" vs "abc") collapse to the same key. Since review entries key on ExternalID — and the driver already goes to lengths elsewhere to guarantee unique ExternalIDs — two real accounts could merge into one review entry, letting a reviewer approve one while the other remains live. Consider keying the fallback off the raw username (optionally lowercased) rather than a separator-stripping MAC normalizer, so a username keeps its own identity unless it is genuinely a MAC address.</comment>
<file context>
@@ -0,0 +1,981 @@
+ // recognises; the opaque _id is the fallback.
+ external := id
+ if external == "" {
+ external = unifiMACKey(name)
+ }
+
</file context>
| external = unifiMACKey(name) | |
| external = strings.ToLower(name) |
| // The probe lands on the console's own site listing — the call the | ||
| // driver opens with — so a passing check proves the key reaches the | ||
| // configured console, not merely that api.ui.com accepted it. | ||
| name: "unifi", |
There was a problem hiding this comment.
P3: The header comment for TestBuildProbeURLFromAPIBase still says 'three probe builders', but adding the unifi case makes it four. Update the wording so the documentation matches the actual case count.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pkg/connector/provider/probe_test.go, line 459:
<comment>The header comment for TestBuildProbeURLFromAPIBase still says 'three probe builders', but adding the unifi case makes it four. Update the wording so the documentation matches the actual case count.</comment>
<file context>
@@ -452,6 +452,16 @@ func TestBuildProbeURLFromAPIBase(t *testing.T) {
+ // The probe lands on the console's own site listing — the call the
+ // driver opens with — so a passing check proves the key reaches the
+ // configured console, not merely that api.ui.com accepted it.
+ name: "unifi",
+ reg: unifiRegistration(),
+ settings: &coredata.UniFiConnectorSettings{ConsoleID: "ABCDEF0123456789:1234567890"},
</file context>
Access review driver for Unifi, a network platform.
The access review will cover the radius server. There is no email or mfa available for the radius service, only username.
Unifi has no oAuth posibility, which is the reason that this drive will use an API Key instead.
Summary by cubic
Add UniFi connector with API key auth and a new access review driver for UniFi RADIUS users, MAC filters, and vouchers. Updates API, DB, console app, and UI to support the
UNIFIprovider with a console ID setting, connection probe, and logo.Service
+1376-0UNIFIprovider (API key auth) and implemented console base URL helpers and a probe that hits the console’s site listing.GraphQL API
+22-0UNIFIin the schema and addedunifiConsoleIdtoCreateAPIKeyConnectorInput.unifiConsoleIdand rejects values with path/query separators to prevent retargeting.Coredata
+39-1UNIFIprovider enum andUniFiConnectorSettingswithconsole_id.App: console
+3-0consoleIdtounifiConsoleIdin connector settings UI.Package: ui
+15-0ThirdPartyLogo.Tests
+1166-0Written for commit 933d6c1. Summary will update on new commits.