From d2cabccc556d82d6c269949c45a77fa524dbda2c Mon Sep 17 00:00:00 2001 From: "Pola, Sudhir" Date: Fri, 28 Aug 2026 16:32:41 +0530 Subject: [PATCH 1/3] feat(api): add multi-tenancy support --- .golangci.yml | 3 +- ...nsole_environment.postman_environment.json | 18 + .../console_mps_apis.postman_collection.json | 371 +++++++++++- .../console_rps_apis.postman_collection.json | 549 +++++++++++++++++- .../controller/httpapi/middleware/tenant.go | 38 ++ .../httpapi/middleware/tenant_test.go | 63 ++ internal/controller/httpapi/router.go | 3 + internal/controller/httpapi/v1/ciraconfigs.go | 24 +- internal/controller/httpapi/v1/devices.go | 64 +- .../controller/httpapi/v1/devices_test.go | 21 +- internal/controller/httpapi/v1/domains.go | 24 +- internal/controller/httpapi/v1/error.go | 6 + internal/controller/httpapi/v1/explorer.go | 2 +- .../controller/httpapi/v1/explorer_test.go | 66 +++ .../controller/httpapi/v1/ieee8021xconfigs.go | 24 +- internal/controller/httpapi/v1/profiles.go | 27 +- .../controller/httpapi/v1/profiles_test.go | 49 ++ internal/controller/httpapi/v1/tenant.go | 45 ++ internal/controller/httpapi/v1/tenant_test.go | 79 +++ internal/controller/httpapi/v1/wificonfigs.go | 24 +- internal/controller/openapi/route_options.go | 16 + internal/controller/openapi/tenant_test.go | 88 +++ internal/controller/tcp/cira/handler.go | 22 +- internal/controller/tcp/cira/handler_test.go | 108 ++++ internal/controller/tcp/cira/tunnel.go | 10 +- internal/controller/ws/v1/redirect.go | 34 +- internal/controller/ws/v1/redirect_test.go | 110 +++- internal/mocks/devicemanagement_mocks.go | 30 + internal/tenant/tenant.go | 46 ++ internal/tenant/tenant_test.go | 98 ++++ internal/usecase/devices/alarms.go | 6 +- internal/usecase/devices/boot.go | 4 +- internal/usecase/devices/certificates.go | 8 +- internal/usecase/devices/connections.go | 2 +- internal/usecase/devices/consent.go | 6 +- internal/usecase/devices/features.go | 4 +- internal/usecase/devices/features_test.go | 13 + internal/usecase/devices/info.go | 12 +- internal/usecase/devices/interceptor.go | 17 +- .../devices/interceptor_private_test.go | 27 + internal/usecase/devices/interfaces.go | 4 + internal/usecase/devices/kvm.go | 4 +- internal/usecase/devices/linkpreference.go | 2 +- internal/usecase/devices/network.go | 4 +- internal/usecase/devices/power.go | 10 +- internal/usecase/devices/redirection.go | 9 +- internal/usecase/devices/redirection_test.go | 22 + internal/usecase/devices/repo.go | 26 + internal/usecase/devices/tenant.go | 14 + internal/usecase/devices/tenant_test.go | 38 ++ internal/usecase/devices/wifiprofile.go | 2 +- internal/usecase/devices/wifistate.go | 4 +- internal/usecase/devices/wsman/message.go | 17 +- .../usecase/devices/wsman/message_test.go | 60 ++ internal/usecase/nosqldb/mongo/device.go | 34 +- internal/usecase/nosqldb/mongo/device_test.go | 68 +++ internal/usecase/sqldb/device.go | 79 ++- internal/usecase/sqldb/device_test.go | 147 ++++- 58 files changed, 2586 insertions(+), 119 deletions(-) create mode 100644 internal/controller/httpapi/middleware/tenant.go create mode 100644 internal/controller/httpapi/middleware/tenant_test.go create mode 100644 internal/controller/httpapi/v1/explorer_test.go create mode 100644 internal/controller/httpapi/v1/tenant.go create mode 100644 internal/controller/httpapi/v1/tenant_test.go create mode 100644 internal/controller/openapi/tenant_test.go create mode 100644 internal/controller/tcp/cira/handler_test.go create mode 100644 internal/tenant/tenant.go create mode 100644 internal/tenant/tenant_test.go create mode 100644 internal/usecase/devices/tenant.go create mode 100644 internal/usecase/devices/tenant_test.go diff --git a/.golangci.yml b/.golangci.yml index ed2ecd93f..f4f974826 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -136,7 +136,8 @@ formatters: - Prefix(github.com/device-management-toolkit) - localmodule gofumpt: - extra-rules: true + extra: + group-params: true exclusions: generated: lax paths: diff --git a/integration-test/collections/console_environment.postman_environment.json b/integration-test/collections/console_environment.postman_environment.json index 5247c1e5a..5387f2844 100644 --- a/integration-test/collections/console_environment.postman_environment.json +++ b/integration-test/collections/console_environment.postman_environment.json @@ -69,6 +69,24 @@ "key": "token", "value": "", "enabled": true + }, + { + "key": "tenantId", + "value": "", + "type": "default", + "enabled": true + }, + { + "key": "tenantA", + "value": "tenant-a", + "type": "default", + "enabled": true + }, + { + "key": "tenantB", + "value": "tenant-b", + "type": "default", + "enabled": true } ], "_postman_variable_scope": "environment", diff --git a/integration-test/collections/console_mps_apis.postman_collection.json b/integration-test/collections/console_mps_apis.postman_collection.json index 5943679a0..f8ae0f7ac 100644 --- a/integration-test/collections/console_mps_apis.postman_collection.json +++ b/integration-test/collections/console_mps_apis.postman_collection.json @@ -2667,6 +2667,369 @@ "response": [] } ] + }, + { + "name": "Tenancy", + "item": [ + { + "name": "Tenant A device list is scoped", + "event": [ + { + "listen": "test", + "script": { + "exec": [ + "pm.test(\"Status code is 200 OK\", function () {", + " pm.response.to.have.status(200);", + "});", + "", + "pm.test(\"Every device belongs to tenant A\", function () {", + " const body = pm.response.json();", + " const items = Array.isArray(body) ? body : (body.data || []);", + " items.forEach(function (device) {", + " pm.expect(device.tenantId).to.eql(pm.variables.get('tenantA'));", + " });", + "});" + ], + "type": "text/javascript" + } + } + ], + "request": { + "method": "GET", + "header": [ + { + "key": "Content-Type", + "value": "application/json" + }, + { + "key": "x-tenant-id", + "value": "{{tenantA}}" + } + ], + "url": { + "raw": "{{protocol}}://{{host}}/api/v1/devices", + "protocol": "{{protocol}}", + "host": [ + "{{host}}" + ], + "path": [ + "api", + "v1", + "devices" + ] + } + }, + "response": [] + }, + { + "name": "Tenant B cannot read a tenant A device", + "event": [ + { + "listen": "test", + "script": { + "exec": [ + "pm.test(\"Status code is 404 Not Found\", function () {", + " pm.response.to.have.status(404);", + "});" + ], + "type": "text/javascript" + } + } + ], + "request": { + "method": "GET", + "header": [ + { + "key": "Content-Type", + "value": "application/json" + }, + { + "key": "x-tenant-id", + "value": "{{tenantB}}" + } + ], + "url": { + "raw": "{{protocol}}://{{host}}/api/v1/devices/{{deviceId}}", + "protocol": "{{protocol}}", + "host": [ + "{{host}}" + ], + "path": [ + "api", + "v1", + "devices", + "{{deviceId}}" + ] + } + }, + "response": [] + }, + { + "name": "Tenant A tag list is scoped", + "event": [ + { + "listen": "test", + "script": { + "exec": [ + "pm.test(\"Status code is 200 OK\", function () {", + " pm.response.to.have.status(200);", + "});" + ], + "type": "text/javascript" + } + } + ], + "request": { + "method": "GET", + "header": [ + { + "key": "Content-Type", + "value": "application/json" + }, + { + "key": "x-tenant-id", + "value": "{{tenantA}}" + } + ], + "url": { + "raw": "{{protocol}}://{{host}}/api/v1/devices/tags", + "protocol": "{{protocol}}", + "host": [ + "{{host}}" + ], + "path": [ + "api", + "v1", + "devices", + "tags" + ] + } + }, + "response": [] + }, + { + "name": "Tenant B cannot delete a tenant A device", + "event": [{"listen": "test", "script": {"exec": ["pm.test(\"Status code is 404 Not Found\", function () {", " pm.response.to.have.status(404);", "});"], "type": "text/javascript"}}], + "request": {"method": "DELETE", "header": [{"key": "x-tenant-id", "value": "{{tenantB}}"}], "url": {"raw": "{{protocol}}://{{host}}/api/v1/devices/{{deviceId}}", "protocol": "{{protocol}}", "host": ["{{host}}"], "path": ["api", "v1", "devices", "{{deviceId}}"]}}, + "response": [] + }, + { + "name": "Tenant B cannot modify tenant A AMT features", + "event": [{"listen": "test", "script": {"exec": ["pm.test(\"Status code is 404 Not Found\", function () {", " pm.response.to.have.status(404);", "});"], "type": "text/javascript"}}], + "request": {"method": "POST", "header": [{"key": "Content-Type", "value": "application/json"}, {"key": "x-tenant-id", "value": "{{tenantB}}"}], "body": {"mode": "raw", "raw": "{\n \"userConsent\": \"none\",\n \"enableSOL\": false,\n \"enableIDER\": false,\n \"enableKVM\": false,\n \"ocr\": false,\n \"rpe\": true\n}", "options": {"raw": {"language": "json"}}}, "url": {"raw": "{{protocol}}://{{host}}/api/v1/amt/features/{{deviceId}}", "protocol": "{{protocol}}", "host": ["{{host}}"], "path": ["api", "v1", "amt", "features", "{{deviceId}}"]}}, + "response": [] + }, + { + "name": "Tenant B cannot add a certificate to tenant A device", + "event": [{"listen": "test", "script": {"exec": ["pm.test(\"Status code is 404 Not Found\", function () {", " pm.response.to.have.status(404);", "});"], "type": "text/javascript"}}], + "request": {"method": "POST", "header": [{"key": "Content-Type", "value": "application/json"}, {"key": "x-tenant-id", "value": "{{tenantB}}"}], "body": {"mode": "raw", "raw": "{\n \"cert\": \"-----BEGIN CERTIFICATE-----\\nMIIDtTM=\\n-----END CERTIFICATE-----\",\n \"isTrusted\": true\n}", "options": {"raw": {"language": "json"}}}, "url": {"raw": "{{protocol}}://{{host}}/api/v1/amt/certificates/{{deviceId}}", "protocol": "{{protocol}}", "host": ["{{host}}"], "path": ["api", "v1", "amt", "certificates", "{{deviceId}}"]}}, + "response": [] + }, + { + "name": "Tenant B cannot update tenant A wired network", + "event": [{"listen": "test", "script": {"exec": ["pm.test(\"Status code is 404 Not Found\", function () {", " pm.response.to.have.status(404);", "});"], "type": "text/javascript"}}], + "request": {"method": "PATCH", "header": [{"key": "Content-Type", "value": "application/json"}, {"key": "x-tenant-id", "value": "{{tenantB}}"}], "body": {"mode": "raw", "raw": "{\n \"dhcpEnabled\": false,\n \"ipAddress\": \"192.168.1.50\",\n \"subnetMask\": \"255.255.255.0\",\n \"defaultGateway\": \"192.168.1.1\",\n \"primaryDNS\": \"192.168.1.1\"\n}", "options": {"raw": {"language": "json"}}}, "url": {"raw": "{{protocol}}://{{host}}/api/v1/amt/networkSettings/wired/{{deviceId}}", "protocol": "{{protocol}}", "host": ["{{host}}"], "path": ["api", "v1", "amt", "networkSettings", "wired", "{{deviceId}}"]}}, + "response": [] + }, + { + "name": "Tenant B cannot use AMT Explorer on tenant A device", + "event": [{"listen": "test", "script": {"exec": ["pm.test(\"Status code is 404 Not Found\", function () {", " pm.response.to.have.status(404);", "});"], "type": "text/javascript"}}], + "request": {"method": "GET", "header": [{"key": "x-tenant-id", "value": "{{tenantB}}"}], "url": {"raw": "{{protocol}}://{{host}}/api/v1/amt/version/{{deviceId}}", "protocol": "{{protocol}}", "host": ["{{host}}"], "path": ["api", "v1", "amt", "version", "{{deviceId}}"]}}, + "response": [] + }, + { + "name": "Malformed tenant rejected - space", + "event": [ + { + "listen": "test", + "script": { + "exec": [ + "pm.test(\"Status code is 400 Bad Request\", function () {", + " pm.response.to.have.status(400);", + "});" + ], + "type": "text/javascript" + } + } + ], + "request": { + "method": "GET", + "header": [ + { + "key": "Content-Type", + "value": "application/json" + }, + { + "key": "x-tenant-id", + "value": "tenant a" + } + ], + "url": { + "raw": "{{protocol}}://{{host}}/api/v1/devices", + "protocol": "{{protocol}}", + "host": [ + "{{host}}" + ], + "path": [ + "api", + "v1", + "devices" + ] + } + }, + "response": [] + }, + { + "name": "Malformed tenant rejected - slash", + "event": [ + { + "listen": "test", + "script": { + "exec": [ + "pm.test(\"Status code is 400 Bad Request\", function () {", + " pm.response.to.have.status(400);", + "});" + ], + "type": "text/javascript" + } + } + ], + "request": { + "method": "GET", + "header": [ + { + "key": "Content-Type", + "value": "application/json" + }, + { + "key": "x-tenant-id", + "value": "tenant/a" + } + ], + "url": { + "raw": "{{protocol}}://{{host}}/api/v1/devices", + "protocol": "{{protocol}}", + "host": [ + "{{host}}" + ], + "path": [ + "api", + "v1", + "devices" + ] + } + }, + "response": [] + }, + { + "name": "Malformed tenant rejected - sql quote", + "event": [ + { + "listen": "test", + "script": { + "exec": [ + "pm.test(\"Status code is 400 Bad Request\", function () {", + " pm.response.to.have.status(400);", + "});" + ], + "type": "text/javascript" + } + } + ], + "request": { + "method": "GET", + "header": [ + { + "key": "Content-Type", + "value": "application/json" + }, + { + "key": "x-tenant-id", + "value": "tenant'a" + } + ], + "url": { + "raw": "{{protocol}}://{{host}}/api/v1/devices", + "protocol": "{{protocol}}", + "host": [ + "{{host}}" + ], + "path": [ + "api", + "v1", + "devices" + ] + } + }, + "response": [] + }, + { + "name": "Malformed tenant rejected - over max length", + "event": [ + { + "listen": "test", + "script": { + "exec": [ + "pm.test(\"Status code is 400 Bad Request\", function () {", + " pm.response.to.have.status(400);", + "});" + ], + "type": "text/javascript" + } + } + ], + "request": { + "method": "GET", + "header": [ + { + "key": "Content-Type", + "value": "application/json" + }, + { + "key": "x-tenant-id", + "value": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + } + ], + "url": { + "raw": "{{protocol}}://{{host}}/api/v1/devices", + "protocol": "{{protocol}}", + "host": [ + "{{host}}" + ], + "path": [ + "api", + "v1", + "devices" + ] + } + }, + "response": [] + }, + { + "name": "Malformed tenant rejected - dot", + "event": [{"listen": "test", "script": {"exec": ["pm.test(\"Status code is 400 Bad Request\", function () {", " pm.response.to.have.status(400);", "});"], "type": "text/javascript"}}], + "request": {"method": "GET", "header": [{"key": "x-tenant-id", "value": "tenant.a"}], "url": {"raw": "{{protocol}}://{{host}}/api/v1/devices", "protocol": "{{protocol}}", "host": ["{{host}}"], "path": ["api", "v1", "devices"]}}, + "response": [] + }, + { + "name": "Malformed tenant rejected - leading dot", + "event": [{"listen": "test", "script": {"exec": ["pm.test(\"Status code is 400 Bad Request\", function () {", " pm.response.to.have.status(400);", "});"], "type": "text/javascript"}}], + "request": {"method": "GET", "header": [{"key": "x-tenant-id", "value": ".tenant-a"}], "url": {"raw": "{{protocol}}://{{host}}/api/v1/devices", "protocol": "{{protocol}}", "host": ["{{host}}"], "path": ["api", "v1", "devices"]}}, + "response": [] + }, + { + "name": "Malformed tenant rejected - leading underscore", + "event": [{"listen": "test", "script": {"exec": ["pm.test(\"Status code is 400 Bad Request\", function () {", " pm.response.to.have.status(400);", "});"], "type": "text/javascript"}}], + "request": {"method": "GET", "header": [{"key": "x-tenant-id", "value": "_tenant-a"}], "url": {"raw": "{{protocol}}://{{host}}/api/v1/devices", "protocol": "{{protocol}}", "host": ["{{host}}"], "path": ["api", "v1", "devices"]}}, + "response": [] + }, + { + "name": "Malformed tenant rejected - leading hyphen", + "event": [{"listen": "test", "script": {"exec": ["pm.test(\"Status code is 400 Bad Request\", function () {", " pm.response.to.have.status(400);", "});"], "type": "text/javascript"}}], + "request": {"method": "GET", "header": [{"key": "x-tenant-id", "value": "-tenant-a"}], "url": {"raw": "{{protocol}}://{{host}}/api/v1/devices", "protocol": "{{protocol}}", "host": ["{{host}}"], "path": ["api", "v1", "devices"]}}, + "response": [] + } + ] } ], "event": [ @@ -2677,7 +3040,13 @@ "packages": {}, "requests": {}, "exec": [ - "" + "// Scope every request to {{tenantId}} when it is set. Leave it blank to", + "// exercise the default tenant, which is where single-tenant data lives.", + "const tenantId = pm.variables.get('tenantId');", + "", + "if (tenantId) {", + " pm.request.headers.upsert({ key: 'x-tenant-id', value: tenantId });", + "}" ] } }, diff --git a/integration-test/collections/console_rps_apis.postman_collection.json b/integration-test/collections/console_rps_apis.postman_collection.json index 9c8db15a7..282b77fbb 100644 --- a/integration-test/collections/console_rps_apis.postman_collection.json +++ b/integration-test/collections/console_rps_apis.postman_collection.json @@ -8390,6 +8390,547 @@ "response": [] } ] + }, + { + "name": "Tenancy", + "item": [ + { + "name": "Create profile in tenant A", + "event": [ + { + "listen": "test", + "script": { + "exec": [ + "pm.test(\"Status code is 201 Created\", function () {", + " pm.response.to.have.status(201);", + "});", + "", + "pm.test(\"Profile is stored under tenant A\", function () {", + " pm.expect(pm.response.json().tenantId).to.eql(pm.variables.get('tenantA'));", + "});" + ], + "type": "text/javascript" + } + } + ], + "request": { + "method": "POST", + "header": [ + { + "key": "Content-Type", + "value": "application/json" + }, + { + "key": "x-tenant-id", + "value": "{{tenantA}}" + } + ], + "url": { + "raw": "{{protocol}}://{{host}}/api/v1/admin/profiles", + "protocol": "{{protocol}}", + "host": [ + "{{host}}" + ], + "path": [ + "api", + "v1", + "admin", + "profiles" + ] + }, + "body": { + "mode": "raw", + "raw": "{\n \"profileName\": \"tenancy-probe\",\n \"activation\": \"ccmactivate\",\n \"generateRandomPassword\": true,\n \"generateRandomMEBxPassword\": false,\n \"tags\": [],\n \"dhcpEnabled\": true,\n \"tlsMode\": 0,\n \"userConsent\": \"All\",\n \"iderEnabled\": true,\n \"kvmEnabled\": true,\n \"solEnabled\": true\n}", + "options": { + "raw": { + "language": "json" + } + } + } + }, + "response": [] + }, + { + "name": "Tenant B cannot read tenant A profile", + "event": [ + { + "listen": "test", + "script": { + "exec": [ + "pm.test(\"Status code is 404 Not Found\", function () {", + " pm.response.to.have.status(404);", + "});", + "", + "pm.test(\"Cross-tenant read leaks nothing\", function () {", + " pm.expect(pm.response.text()).to.not.include('ccmactivate');", + "});" + ], + "type": "text/javascript" + } + } + ], + "request": { + "method": "GET", + "header": [ + { + "key": "Content-Type", + "value": "application/json" + }, + { + "key": "x-tenant-id", + "value": "{{tenantB}}" + } + ], + "url": { + "raw": "{{protocol}}://{{host}}/api/v1/admin/profiles/tenancy-probe", + "protocol": "{{protocol}}", + "host": [ + "{{host}}" + ], + "path": [ + "api", + "v1", + "admin", + "profiles", + "tenancy-probe" + ] + } + }, + "response": [] + }, + { + "name": "Default tenant cannot read tenant A profile", + "event": [ + { + "listen": "test", + "script": { + "exec": [ + "pm.test(\"Status code is 404 Not Found\", function () {", + " pm.response.to.have.status(404);", + "});" + ], + "type": "text/javascript" + } + } + ], + "request": { + "method": "GET", + "header": [ + { + "key": "Content-Type", + "value": "application/json" + } + ], + "url": { + "raw": "{{protocol}}://{{host}}/api/v1/admin/profiles/tenancy-probe", + "protocol": "{{protocol}}", + "host": [ + "{{host}}" + ], + "path": [ + "api", + "v1", + "admin", + "profiles", + "tenancy-probe" + ] + } + }, + "response": [] + }, + { + "name": "Tenant A can read its own profile", + "event": [ + { + "listen": "test", + "script": { + "exec": [ + "pm.test(\"Status code is 200 OK\", function () {", + " pm.response.to.have.status(200);", + "});" + ], + "type": "text/javascript" + } + } + ], + "request": { + "method": "GET", + "header": [ + { + "key": "Content-Type", + "value": "application/json" + }, + { + "key": "x-tenant-id", + "value": "{{tenantA}}" + } + ], + "url": { + "raw": "{{protocol}}://{{host}}/api/v1/admin/profiles/tenancy-probe", + "protocol": "{{protocol}}", + "host": [ + "{{host}}" + ], + "path": [ + "api", + "v1", + "admin", + "profiles", + "tenancy-probe" + ] + } + }, + "response": [] + }, + { + "name": "Body tenantId conflicting with header is rejected", + "event": [ + { + "listen": "test", + "script": { + "exec": [ + "pm.test(\"Status code is 400 Bad Request\", function () {", + " pm.response.to.have.status(400);", + "});" + ], + "type": "text/javascript" + } + } + ], + "request": { + "method": "POST", + "header": [ + { + "key": "Content-Type", + "value": "application/json" + }, + { + "key": "x-tenant-id", + "value": "{{tenantA}}" + } + ], + "url": { + "raw": "{{protocol}}://{{host}}/api/v1/admin/domains", + "protocol": "{{protocol}}", + "host": [ + "{{host}}" + ], + "path": [ + "api", + "v1", + "admin", + "domains" + ] + }, + "body": { + "mode": "raw", + "raw": "{\n \"profileName\": \"tenancy-conflict\",\n \"domainSuffix\": \"tenancy.example.com\",\n \"provisioningCert\": \"cert\",\n \"provisioningCertStorageFormat\": \"string\",\n \"provisioningCertPassword\": \"password\",\n \"tenantId\": \"{{tenantB}}\"\n}", + "options": { + "raw": { + "language": "json" + } + } + } + }, + "response": [] + }, + { + "name": "Domain rejects path traversal tenantId without header", + "event": [{"listen": "test", "script": {"exec": ["pm.test(\"Status code is 400 Bad Request\", function () {", " pm.response.to.have.status(400);", "});"], "type": "text/javascript"}}], + "request": {"method": "POST", "header": [{"key": "Content-Type", "value": "application/json"}], "body": {"mode": "raw", "raw": "{\n \"profileName\": \"tenant-path-traversal\",\n \"domainSuffix\": \"tenancy.example.com\",\n \"provisioningCert\": \"cert\",\n \"provisioningCertStorageFormat\": \"string\",\n \"provisioningCertPassword\": \"password\",\n \"tenantId\": \"../../other-tenant\"\n}", "options": {"raw": {"language": "json"}}}, "url": {"raw": "{{protocol}}://{{host}}/api/v1/admin/domains", "protocol": "{{protocol}}", "host": ["{{host}}"], "path": ["api", "v1", "admin", "domains"]}}, + "response": [] + }, + { + "name": "CIRA config body tenantId conflicting with header is rejected", + "event": [{"listen": "test", "script": {"exec": ["pm.test(\"Status code is 400 Bad Request\", function () {", " pm.response.to.have.status(400);", "});"], "type": "text/javascript"}}], + "request": {"method": "POST", "header": [{"key": "Content-Type", "value": "application/json"}, {"key": "x-tenant-id", "value": "{{tenantA}}"}], "body": {"mode": "raw", "raw": "{\n \"configName\": \"tenancy-conflict\",\n \"mpsServerAddress\": \"192.168.8.50\",\n \"mpsPort\": 4433,\n \"username\": \"admin\",\n \"commonName\": \"192.168.8.50\",\n \"serverAddressFormat\": 3,\n \"authMethod\": 2,\n \"mpsRootCertificate\": \"rootcert\",\n \"proxyDetails\": \"\",\n \"tenantId\": \"{{tenantB}}\"\n}", "options": {"raw": {"language": "json"}}}, "url": {"raw": "{{protocol}}://{{host}}/api/v1/admin/ciraconfigs", "protocol": "{{protocol}}", "host": ["{{host}}"], "path": ["api", "v1", "admin", "ciraconfigs"]}}, + "response": [] + }, + { + "name": "Tenant B cannot delete tenant A profile", + "event": [ + { + "listen": "test", + "script": { + "exec": [ + "pm.test(\"Status code is 404 Not Found\", function () {", + " pm.response.to.have.status(404);", + "});" + ], + "type": "text/javascript" + } + } + ], + "request": { + "method": "DELETE", + "header": [ + { + "key": "Content-Type", + "value": "application/json" + }, + { + "key": "x-tenant-id", + "value": "{{tenantB}}" + } + ], + "url": { + "raw": "{{protocol}}://{{host}}/api/v1/admin/profiles/tenancy-probe", + "protocol": "{{protocol}}", + "host": [ + "{{host}}" + ], + "path": [ + "api", + "v1", + "admin", + "profiles", + "tenancy-probe" + ] + } + }, + "response": [] + }, + { + "name": "Tenant A deletes its own profile", + "event": [ + { + "listen": "test", + "script": { + "exec": [ + "pm.test(\"Status code is 204 No Content\", function () {", + " pm.response.to.have.status(204);", + "});" + ], + "type": "text/javascript" + } + } + ], + "request": { + "method": "DELETE", + "header": [ + { + "key": "Content-Type", + "value": "application/json" + }, + { + "key": "x-tenant-id", + "value": "{{tenantA}}" + } + ], + "url": { + "raw": "{{protocol}}://{{host}}/api/v1/admin/profiles/tenancy-probe", + "protocol": "{{protocol}}", + "host": [ + "{{host}}" + ], + "path": [ + "api", + "v1", + "admin", + "profiles", + "tenancy-probe" + ] + } + }, + "response": [] + }, + { + "name": "Malformed tenant rejected - space", + "event": [ + { + "listen": "test", + "script": { + "exec": [ + "pm.test(\"Status code is 400 Bad Request\", function () {", + " pm.response.to.have.status(400);", + "});" + ], + "type": "text/javascript" + } + } + ], + "request": { + "method": "GET", + "header": [ + { + "key": "Content-Type", + "value": "application/json" + }, + { + "key": "x-tenant-id", + "value": "tenant a" + } + ], + "url": { + "raw": "{{protocol}}://{{host}}/api/v1/admin/profiles", + "protocol": "{{protocol}}", + "host": [ + "{{host}}" + ], + "path": [ + "api", + "v1", + "admin", + "profiles" + ] + } + }, + "response": [] + }, + { + "name": "Malformed tenant rejected - slash", + "event": [ + { + "listen": "test", + "script": { + "exec": [ + "pm.test(\"Status code is 400 Bad Request\", function () {", + " pm.response.to.have.status(400);", + "});" + ], + "type": "text/javascript" + } + } + ], + "request": { + "method": "GET", + "header": [ + { + "key": "Content-Type", + "value": "application/json" + }, + { + "key": "x-tenant-id", + "value": "tenant/a" + } + ], + "url": { + "raw": "{{protocol}}://{{host}}/api/v1/admin/profiles", + "protocol": "{{protocol}}", + "host": [ + "{{host}}" + ], + "path": [ + "api", + "v1", + "admin", + "profiles" + ] + } + }, + "response": [] + }, + { + "name": "Malformed tenant rejected - sql quote", + "event": [ + { + "listen": "test", + "script": { + "exec": [ + "pm.test(\"Status code is 400 Bad Request\", function () {", + " pm.response.to.have.status(400);", + "});" + ], + "type": "text/javascript" + } + } + ], + "request": { + "method": "GET", + "header": [ + { + "key": "Content-Type", + "value": "application/json" + }, + { + "key": "x-tenant-id", + "value": "tenant'a" + } + ], + "url": { + "raw": "{{protocol}}://{{host}}/api/v1/admin/profiles", + "protocol": "{{protocol}}", + "host": [ + "{{host}}" + ], + "path": [ + "api", + "v1", + "admin", + "profiles" + ] + } + }, + "response": [] + }, + { + "name": "Malformed tenant rejected - over max length", + "event": [ + { + "listen": "test", + "script": { + "exec": [ + "pm.test(\"Status code is 400 Bad Request\", function () {", + " pm.response.to.have.status(400);", + "});" + ], + "type": "text/javascript" + } + } + ], + "request": { + "method": "GET", + "header": [ + { + "key": "Content-Type", + "value": "application/json" + }, + { + "key": "x-tenant-id", + "value": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + } + ], + "url": { + "raw": "{{protocol}}://{{host}}/api/v1/admin/profiles", + "protocol": "{{protocol}}", + "host": [ + "{{host}}" + ], + "path": [ + "api", + "v1", + "admin", + "profiles" + ] + } + }, + "response": [] + }, + { + "name": "Malformed tenant rejected - dot", + "event": [{"listen": "test", "script": {"exec": ["pm.test(\"Status code is 400 Bad Request\", function () {", " pm.response.to.have.status(400);", "});"], "type": "text/javascript"}}], + "request": {"method": "GET", "header": [{"key": "x-tenant-id", "value": "tenant.a"}], "url": {"raw": "{{protocol}}://{{host}}/api/v1/admin/profiles", "protocol": "{{protocol}}", "host": ["{{host}}"], "path": ["api", "v1", "admin", "profiles"]}}, + "response": [] + }, + { + "name": "Malformed tenant rejected - leading dot", + "event": [{"listen": "test", "script": {"exec": ["pm.test(\"Status code is 400 Bad Request\", function () {", " pm.response.to.have.status(400);", "});"], "type": "text/javascript"}}], + "request": {"method": "GET", "header": [{"key": "x-tenant-id", "value": ".tenant-a"}], "url": {"raw": "{{protocol}}://{{host}}/api/v1/admin/profiles", "protocol": "{{protocol}}", "host": ["{{host}}"], "path": ["api", "v1", "admin", "profiles"]}}, + "response": [] + }, + { + "name": "Malformed tenant rejected - leading underscore", + "event": [{"listen": "test", "script": {"exec": ["pm.test(\"Status code is 400 Bad Request\", function () {", " pm.response.to.have.status(400);", "});"], "type": "text/javascript"}}], + "request": {"method": "GET", "header": [{"key": "x-tenant-id", "value": "_tenant-a"}], "url": {"raw": "{{protocol}}://{{host}}/api/v1/admin/profiles", "protocol": "{{protocol}}", "host": ["{{host}}"], "path": ["api", "v1", "admin", "profiles"]}}, + "response": [] + }, + { + "name": "Malformed tenant rejected - leading hyphen", + "event": [{"listen": "test", "script": {"exec": ["pm.test(\"Status code is 400 Bad Request\", function () {", " pm.response.to.have.status(400);", "});"], "type": "text/javascript"}}], + "request": {"method": "GET", "header": [{"key": "x-tenant-id", "value": "-tenant-a"}], "url": {"raw": "{{protocol}}://{{host}}/api/v1/admin/profiles", "protocol": "{{protocol}}", "host": ["{{host}}"], "path": ["api", "v1", "admin", "profiles"]}}, + "response": [] + } + ] } ], "auth": { @@ -8409,7 +8950,13 @@ "type": "text/javascript", "requests": {}, "exec": [ - "" + "// Scope every request to {{tenantId}} when it is set. Leave it blank to", + "// exercise the default tenant, which is where single-tenant data lives.", + "const tenantId = pm.variables.get('tenantId');", + "", + "if (tenantId) {", + " pm.request.headers.upsert({ key: 'x-tenant-id', value: tenantId });", + "}" ] } }, diff --git a/internal/controller/httpapi/middleware/tenant.go b/internal/controller/httpapi/middleware/tenant.go new file mode 100644 index 000000000..a35aa685d --- /dev/null +++ b/internal/controller/httpapi/middleware/tenant.go @@ -0,0 +1,38 @@ +package middleware + +import ( + "net/http" + + "github.com/gin-gonic/gin" + + "github.com/device-management-toolkit/console/internal/tenant" + "github.com/device-management-toolkit/console/pkg/logger" +) + +// TenantHeaderName is the request header carrying the tenant identifier. +const TenantHeaderName = "x-tenant-id" + +// Tenant validates the tenant header and scopes the request context to it. An +// absent header yields the empty tenant, which is what existing single-tenant +// rows are stored under. +func Tenant(l logger.Interface) gin.HandlerFunc { + return func(c *gin.Context) { + tenantID := c.GetHeader(TenantHeaderName) + + if !tenant.Valid(tenantID) { + c.AbortWithStatusJSON(http.StatusBadRequest, gin.H{"error": tenant.Hint, "message": tenant.Hint}) + + return + } + + l.Debug("REST request tenant ID", "tenant_id", tenantID) + + // Leave the context untouched for the default tenant so single-tenant + // requests carry no extra value. + if tenantID != "" { + c.Request = c.Request.WithContext(tenant.WithContext(c.Request.Context(), tenantID)) + } + + c.Next() + } +} diff --git a/internal/controller/httpapi/middleware/tenant_test.go b/internal/controller/httpapi/middleware/tenant_test.go new file mode 100644 index 000000000..c292822a0 --- /dev/null +++ b/internal/controller/httpapi/middleware/tenant_test.go @@ -0,0 +1,63 @@ +package middleware_test + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/gin-gonic/gin" + "github.com/stretchr/testify/require" + + "github.com/device-management-toolkit/console/internal/controller/httpapi/middleware" + "github.com/device-management-toolkit/console/internal/tenant" + "github.com/device-management-toolkit/console/pkg/logger" +) + +func serve(t *testing.T, headerValue string) (recorder *httptest.ResponseRecorder, seen string) { + t.Helper() + + engine := gin.New() + engine.Use(middleware.Tenant(logger.New("error"))) + engine.GET("/", func(c *gin.Context) { + seen = tenant.FromContext(c.Request.Context()) + c.Status(http.StatusOK) + }) + + req := httptest.NewRequest(http.MethodGet, "/", http.NoBody) + if headerValue != "" { + req.Header.Set(middleware.TenantHeaderName, headerValue) + } + + recorder = httptest.NewRecorder() + engine.ServeHTTP(recorder, req) + + return recorder, seen +} + +func TestTenantScopesRequestContext(t *testing.T) { + t.Parallel() + + recorder, seen := serve(t, "tenant-a") + + require.Equal(t, http.StatusOK, recorder.Code) + require.Equal(t, "tenant-a", seen) +} + +func TestTenantWithoutHeaderYieldsEmptyTenant(t *testing.T) { + t.Parallel() + + recorder, seen := serve(t, "") + + require.Equal(t, http.StatusOK, recorder.Code) + require.Empty(t, seen) +} + +func TestTenantRejectsMalformedHeader(t *testing.T) { + t.Parallel() + + recorder, seen := serve(t, "tenant a") + + require.Equal(t, http.StatusBadRequest, recorder.Code) + require.Contains(t, recorder.Body.String(), "x-tenant-id must match") + require.Empty(t, seen) +} diff --git a/internal/controller/httpapi/router.go b/internal/controller/httpapi/router.go index 3a36681c9..c52a8435f 100644 --- a/internal/controller/httpapi/router.go +++ b/internal/controller/httpapi/router.go @@ -11,6 +11,7 @@ import ( ginprometheus "github.com/zsais/go-gin-prometheus" "github.com/device-management-toolkit/console/config" + "github.com/device-management-toolkit/console/internal/controller/httpapi/middleware" v1 "github.com/device-management-toolkit/console/internal/controller/httpapi/v1" v2 "github.com/device-management-toolkit/console/internal/controller/httpapi/v2" openapi "github.com/device-management-toolkit/console/internal/controller/openapi" @@ -64,6 +65,8 @@ func NewRouter(handler *gin.Engine, l logger.Interface, t usecase.Usecases, cfg protected = handler.Group("/api", login.JWTAuthMiddleware()) } + protected.Use(middleware.Tenant(l)) + registerCustomValidators(l) // Routers diff --git a/internal/controller/httpapi/v1/ciraconfigs.go b/internal/controller/httpapi/v1/ciraconfigs.go index be83bf9db..7f69cd065 100644 --- a/internal/controller/httpapi/v1/ciraconfigs.go +++ b/internal/controller/httpapi/v1/ciraconfigs.go @@ -31,6 +31,8 @@ func NewCIRAConfigRoutes(handler *gin.RouterGroup, t ciraconfigs.Feature, l logg } func (r *ciraConfigRoutes) get(c *gin.Context) { + tenantID := tenantIDFromHeader(c) + var odata OData if err := odata.BindAndValidate(c); err != nil { r.l.Error(err, "http - CIRA configs - v1 - get") @@ -39,7 +41,7 @@ func (r *ciraConfigRoutes) get(c *gin.Context) { return } - configs, err := r.cira.Get(c.Request.Context(), odata.Top, odata.Skip, "") + configs, err := r.cira.Get(c.Request.Context(), odata.Top, odata.Skip, tenantID) if err != nil { r.l.Error(err, "http - CIRA configs - v1 - get") ErrorResponse(c, err) @@ -48,7 +50,7 @@ func (r *ciraConfigRoutes) get(c *gin.Context) { } if odata.Count { - count, err := r.cira.GetCount(c.Request.Context(), "") + count, err := r.cira.GetCount(c.Request.Context(), tenantID) if err != nil { r.l.Error(err, "http - CIRA configs - v1 - getCount") ErrorResponse(c, err) @@ -69,8 +71,9 @@ func (r *ciraConfigRoutes) get(c *gin.Context) { func (r *ciraConfigRoutes) getByName(c *gin.Context) { configName := c.Param("ciraConfigName") + tenantID := tenantIDFromHeader(c) - foundConfig, err := r.cira.GetByName(c.Request.Context(), configName, "") + foundConfig, err := r.cira.GetByName(c.Request.Context(), configName, tenantID) if err != nil { r.l.Error(err, "http - CIRA configs - v1 - getByName") ErrorResponse(c, err) @@ -90,6 +93,12 @@ func (r *ciraConfigRoutes) insert(c *gin.Context) { return } + if err := applyTenantID(c, &ciraConfig.TenantID); err != nil { + ErrorResponse(c, err) + + return + } + newCiraConfig, err := r.cira.Insert(c.Request.Context(), &ciraConfig) if err != nil { r.l.Error(err, "http - CIRA configs - v1 - insert") @@ -110,6 +119,12 @@ func (r *ciraConfigRoutes) update(c *gin.Context) { return } + if err := applyTenantID(c, &ciraConfig.TenantID); err != nil { + ErrorResponse(c, err) + + return + } + updatedConfig, err := r.cira.Update(c.Request.Context(), &ciraConfig) if err != nil { r.l.Error(err, "http - CIRA configs - v1 - update") @@ -123,8 +138,9 @@ func (r *ciraConfigRoutes) update(c *gin.Context) { func (r *ciraConfigRoutes) delete(c *gin.Context) { configName := c.Param("ciraConfigName") + tenantID := tenantIDFromHeader(c) - err := r.cira.Delete(c.Request.Context(), configName, "") + err := r.cira.Delete(c.Request.Context(), configName, tenantID) if err != nil { r.l.Error(err, "http - CIRA configs - v1 - delete") ErrorResponse(c, err) diff --git a/internal/controller/httpapi/v1/devices.go b/internal/controller/httpapi/v1/devices.go index 57485ec84..3d07622fc 100644 --- a/internal/controller/httpapi/v1/devices.go +++ b/internal/controller/httpapi/v1/devices.go @@ -13,6 +13,7 @@ import ( "github.com/device-management-toolkit/console/config" "github.com/device-management-toolkit/console/internal/entity/dto/v1" + "github.com/device-management-toolkit/console/internal/tenant" "github.com/device-management-toolkit/console/internal/usecase/devices" "github.com/device-management-toolkit/console/pkg/consoleerrors" "github.com/device-management-toolkit/console/pkg/logger" @@ -47,7 +48,9 @@ func NewDeviceRoutes(handler *gin.RouterGroup, t devices.Feature, l logger.Inter } func (dr *deviceRoutes) getStats(c *gin.Context) { - count, err := dr.t.GetCount(c.Request.Context(), "") + tenantID := tenantIDFromHeader(c) + + count, err := dr.t.GetCount(c.Request.Context(), tenantID) if err != nil { dr.l.Error(err, "http - devices - v1 - getCount") ErrorResponse(c, err) @@ -64,8 +67,9 @@ func (dr *deviceRoutes) getStats(c *gin.Context) { func (dr *deviceRoutes) LoginRedirection(c *gin.Context) { deviceID := c.Param("id") + tenantID := tenantIDFromHeader(c) - _, err := dr.t.GetByID(c.Request.Context(), deviceID, "", false) + device, err := dr.t.GetByID(c.Request.Context(), deviceID, tenantID, false) if err != nil { dr.l.Error(err, "http - devices - v1 - LoginRedirection") ErrorResponse(c, err) @@ -76,9 +80,10 @@ func (dr *deviceRoutes) LoginRedirection(c *gin.Context) { // GUIDs are stored and matched lowercase, so the claim is normalized to match. expirationTime := time.Now().Add(config.ConsoleConfig.RedirectionJWTExpiration) claims := jwt.MapClaims{ - "exp": expirationTime.Unix(), - "iss": config.ConsoleConfig.Issuer, - "deviceId": strings.ToLower(deviceID), + "exp": expirationTime.Unix(), + "iss": config.ConsoleConfig.Issuer, + "deviceId": strings.ToLower(deviceID), + tenant.TenantIDClaim: device.TenantID, } token := jwt.NewWithClaims(jwt.SigningMethodHS256, claims) @@ -94,6 +99,8 @@ func (dr *deviceRoutes) LoginRedirection(c *gin.Context) { } func (dr *deviceRoutes) get(c *gin.Context) { + tenantID := tenantIDFromHeader(c) + var odata OData if err := odata.BindAndValidate(c); err != nil { ErrorResponse(c, err) @@ -111,16 +118,16 @@ func (dr *deviceRoutes) get(c *gin.Context) { switch { case hostname != "": - items, err = dr.getByColumnOrTags(c, "HostName", hostname, odata.Top, odata.Skip, "") + items, err = dr.getByColumnOrTags(c, "HostName", hostname, odata.Top, odata.Skip, tenantID) case friendlyName != "": - items, err = dr.getByColumnOrTags(c, "FriendlyName", friendlyName, odata.Top, odata.Skip, "") + items, err = dr.getByColumnOrTags(c, "FriendlyName", friendlyName, odata.Top, odata.Skip, tenantID) case tags != "": - items, err = dr.getByColumnOrTags(c, "Tags", tags, odata.Top, odata.Skip, "") + items, err = dr.getByColumnOrTags(c, "Tags", tags, odata.Top, odata.Skip, tenantID) default: - items, err = dr.t.Get(c.Request.Context(), odata.Top, odata.Skip, "") + items, err = dr.t.Get(c.Request.Context(), odata.Top, odata.Skip, tenantID) } if err != nil { @@ -131,7 +138,7 @@ func (dr *deviceRoutes) get(c *gin.Context) { } if odata.Count { - count, err := dr.t.GetCount(c.Request.Context(), "") + count, err := dr.t.GetCount(c.Request.Context(), tenantID) if err != nil { dr.l.Error(err, "http - devices - v1 - get") ErrorResponse(c, err) @@ -159,7 +166,7 @@ func (dr *deviceRoutes) getByColumnOrTags(c *gin.Context, column, value string, if column == "Tags" { items, err = dr.t.GetByTags(ctx, value, c.Query("method"), limit, skip, tenantID) } else { - items, err = dr.t.GetByColumn(ctx, column, value, "") + items, err = dr.t.GetByColumn(ctx, column, value, tenantID) } if err != nil { @@ -170,6 +177,8 @@ func (dr *deviceRoutes) getByColumnOrTags(c *gin.Context, column, value string, } func (dr *deviceRoutes) getByID(c *gin.Context) { + tenantID := tenantIDFromHeader(c) + var odata OData if err := odata.BindAndValidate(c); err != nil { ErrorResponse(c, err) @@ -179,7 +188,7 @@ func (dr *deviceRoutes) getByID(c *gin.Context) { guid := c.Param("guid") - item, err := dr.t.GetByID(c.Request.Context(), guid, "", false) + item, err := dr.t.GetByID(c.Request.Context(), guid, tenantID, false) if err != nil { dr.l.Error(err, "http - devices - v1 - get") ErrorResponse(c, err) @@ -207,6 +216,12 @@ func (dr *deviceRoutes) insert(c *gin.Context) { return } + if err := applyTenantID(c, &device.TenantID); err != nil { + ErrorResponse(c, err) + + return + } + var raw map[string]json.RawMessage if err := json.Unmarshal(body, &raw); err != nil { validationErr := ErrValidationDevices.Wrap("insert", "json.Unmarshal", err) @@ -314,6 +329,12 @@ func (dr *deviceRoutes) update(c *gin.Context) { return } + if err := applyTenantID(c, &device.TenantID); err != nil { + ErrorResponse(c, err) + + return + } + fields, err := providedJSONFieldsFromBody(body) if err != nil { ErrorResponse(c, err) @@ -334,8 +355,9 @@ func (dr *deviceRoutes) update(c *gin.Context) { func (dr *deviceRoutes) delete(c *gin.Context) { guid := c.Param("guid") + tenantID := tenantIDFromHeader(c) - err := dr.t.Delete(c.Request.Context(), guid, "") + err := dr.t.Delete(c.Request.Context(), guid, tenantID) if err != nil { dr.l.Error(err, "http - devices - v1 - delete") ErrorResponse(c, err) @@ -356,7 +378,9 @@ func (dr *deviceRoutes) redirectStatus(c *gin.Context) { } func (dr *deviceRoutes) getTags(c *gin.Context) { - tags, err := dr.t.GetDistinctTags(c.Request.Context(), "") + tenantID := tenantIDFromHeader(c) + + tags, err := dr.t.GetDistinctTags(c.Request.Context(), tenantID) if err != nil { dr.l.Error(err, "http - devices - v1 - tags") ErrorResponse(c, err) @@ -368,6 +392,8 @@ func (dr *deviceRoutes) getTags(c *gin.Context) { } func (dr *deviceRoutes) getDeviceCertificate(c *gin.Context) { + tenantID := tenantIDFromHeader(c) + var odata OData if err := odata.BindAndValidate(c); err != nil { ErrorResponse(c, err) @@ -377,7 +403,7 @@ func (dr *deviceRoutes) getDeviceCertificate(c *gin.Context) { guid := c.Param("guid") - item, err := dr.t.GetByID(c.Request.Context(), guid, "", false) + item, err := dr.t.GetByID(c.Request.Context(), guid, tenantID, false) if err != nil { dr.l.Error(err, "http - devices - v1 - cert") ErrorResponse(c, err) @@ -399,6 +425,8 @@ func (dr *deviceRoutes) getDeviceCertificate(c *gin.Context) { } func (dr *deviceRoutes) pinDeviceCertificate(c *gin.Context) { + tenantID := tenantIDFromHeader(c) + var certToPin dto.PinCertificate if err := c.ShouldBindBodyWithJSON(&certToPin); err != nil { ErrorResponse(c, err) @@ -408,7 +436,7 @@ func (dr *deviceRoutes) pinDeviceCertificate(c *gin.Context) { guid := c.Param("guid") - item, err := dr.t.GetByID(c.Request.Context(), guid, "", true) + item, err := dr.t.GetByID(c.Request.Context(), guid, tenantID, true) if err != nil { dr.l.Error(err, "http - devices - v1 - deleteDeviceCertificate - getById") ErrorResponse(c, err) @@ -430,6 +458,8 @@ func (dr *deviceRoutes) pinDeviceCertificate(c *gin.Context) { } func (dr *deviceRoutes) deleteDeviceCertificate(c *gin.Context) { + tenantID := tenantIDFromHeader(c) + var odata OData if err := odata.BindAndValidate(c); err != nil { ErrorResponse(c, err) @@ -439,7 +469,7 @@ func (dr *deviceRoutes) deleteDeviceCertificate(c *gin.Context) { guid := c.Param("guid") - item, err := dr.t.GetByID(c.Request.Context(), guid, "", true) + item, err := dr.t.GetByID(c.Request.Context(), guid, tenantID, true) if err != nil { dr.l.Error(err, "http - devices - v1 - deleteDeviceCertificate - getById") ErrorResponse(c, err) diff --git a/internal/controller/httpapi/v1/devices_test.go b/internal/controller/httpapi/v1/devices_test.go index a3319fc7e..898a432ad 100644 --- a/internal/controller/httpapi/v1/devices_test.go +++ b/internal/controller/httpapi/v1/devices_test.go @@ -763,20 +763,22 @@ func TestLoginRedirection(t *testing.T) { // deviceID is the GUID as it appears in the request path. deviceID string // expectedClaim is the deviceId the token must carry; empty means deviceID. - expectedClaim string - mock func(devFeature *mocks.MockDeviceManagementFeature) - expectedCode int - expectedErr bool + expectedClaim string + expectedTenant string + mock func(devFeature *mocks.MockDeviceManagementFeature) + expectedCode int + expectedErr bool }{ { name: "login redirection - success", deviceID: "test-device-guid", mock: func(devFeature *mocks.MockDeviceManagementFeature) { devFeature.EXPECT().GetByID(context.Background(), "test-device-guid", "", false). - Return(&dto.Device{GUID: "test-device-guid", Hostname: "test-host"}, nil) + Return(&dto.Device{GUID: "test-device-guid", Hostname: "test-host", TenantID: "tenant-a"}, nil) }, - expectedCode: http.StatusOK, - expectedErr: false, + expectedCode: http.StatusOK, + expectedTenant: "tenant-a", + expectedErr: false, }, { name: "login redirection - mixed-case guid is normalized in claim", @@ -845,14 +847,14 @@ func TestLoginRedirection(t *testing.T) { } // Decode and verify token expiration and device binding - verifyRedirectionToken(t, tokenString, expectedClaim) + verifyRedirectionToken(t, tokenString, expectedClaim, tc.expectedTenant) } }) } } // verifyRedirectionToken checks the token's expiration and AMT-GUID (deviceId) binding. -func verifyRedirectionToken(t *testing.T, tokenString, expectedDeviceID string) { +func verifyRedirectionToken(t *testing.T, tokenString, expectedDeviceID, expectedTenant string) { t.Helper() // Parse the token, verifying its signature against the test signing key. @@ -864,6 +866,7 @@ func verifyRedirectionToken(t *testing.T, tokenString, expectedDeviceID string) // deviceId must be the device GUID require.Equal(t, expectedDeviceID, claims["deviceId"], "token deviceId should be the device GUID") + require.Equal(t, expectedTenant, claims["tenantId"], "token tenantId should be the device tenant") // Verify expiration is set exp, err := claims.GetExpirationTime() diff --git a/internal/controller/httpapi/v1/domains.go b/internal/controller/httpapi/v1/domains.go index 6a3ce63cc..ab2107947 100644 --- a/internal/controller/httpapi/v1/domains.go +++ b/internal/controller/httpapi/v1/domains.go @@ -37,6 +37,8 @@ type DomainCountResponse struct { } func (r *domainRoutes) get(c *gin.Context) { + tenantID := tenantIDFromHeader(c) + var odata OData if err := odata.BindAndValidate(c); err != nil { validationErr := ErrValidationDomains.Wrap("get", "BindAndValidate", err) @@ -45,7 +47,7 @@ func (r *domainRoutes) get(c *gin.Context) { return } - items, err := r.t.Get(c.Request.Context(), odata.Top, odata.Skip, "") + items, err := r.t.Get(c.Request.Context(), odata.Top, odata.Skip, tenantID) if err != nil { r.l.Error(err, "http - v1 - getCount") ErrorResponse(c, err) @@ -54,7 +56,7 @@ func (r *domainRoutes) get(c *gin.Context) { } if odata.Count { - count, err := r.t.GetCount(c.Request.Context(), "") + count, err := r.t.GetCount(c.Request.Context(), tenantID) if err != nil { r.l.Error(err, "http - v1 - getCount") ErrorResponse(c, err) @@ -73,8 +75,9 @@ func (r *domainRoutes) get(c *gin.Context) { func (r *domainRoutes) getByName(c *gin.Context) { name := c.Param("name") + tenantID := tenantIDFromHeader(c) - item, err := r.t.GetByName(c.Request.Context(), name, "") + item, err := r.t.GetByName(c.Request.Context(), name, tenantID) if err != nil { r.l.Error(err, "http - v1 - getByName") ErrorResponse(c, err) @@ -94,6 +97,12 @@ func (r *domainRoutes) insert(c *gin.Context) { return } + if err := applyTenantID(c, &domain.TenantID); err != nil { + ErrorResponse(c, err) + + return + } + newDomain, err := r.t.Insert(c.Request.Context(), &domain) if err != nil { r.l.Error(err, "http - v1 - insert") @@ -114,6 +123,12 @@ func (r *domainRoutes) update(c *gin.Context) { return } + if err := applyTenantID(c, &domain.TenantID); err != nil { + ErrorResponse(c, err) + + return + } + updatedDomain, err := r.t.Update(c.Request.Context(), &domain) if err != nil { r.l.Error(err, "http - v1 - update") @@ -127,8 +142,9 @@ func (r *domainRoutes) update(c *gin.Context) { func (r *domainRoutes) delete(c *gin.Context) { name := c.Param("name") + tenantID := tenantIDFromHeader(c) - err := r.t.Delete(c.Request.Context(), name, "") + err := r.t.Delete(c.Request.Context(), name, tenantID) if err != nil { r.l.Error(err, "http - v1 - delete") ErrorResponse(c, err) diff --git a/internal/controller/httpapi/v1/error.go b/internal/controller/httpapi/v1/error.go index 170aa3580..f275371ff 100644 --- a/internal/controller/httpapi/v1/error.go +++ b/internal/controller/httpapi/v1/error.go @@ -139,6 +139,12 @@ func handleSentinelErrors(c *gin.Context, err error) bool { msg := wsmanAPI.ErrCIRADeviceNotConnected.Error() c.AbortWithStatusJSON(http.StatusServiceUnavailable, response{Error: msg, Message: msg}) + return true + case errors.Is(err, wsmanAPI.ErrCIRATenantMismatch): + // Matches MPS ciraMiddleware, which answers 401 on a tenant mismatch. + msg := "Unauthorized" + c.AbortWithStatusJSON(http.StatusUnauthorized, response{Error: msg, Message: msg}) + return true } diff --git a/internal/controller/httpapi/v1/explorer.go b/internal/controller/httpapi/v1/explorer.go index e6029d369..2d23087ff 100644 --- a/internal/controller/httpapi/v1/explorer.go +++ b/internal/controller/httpapi/v1/explorer.go @@ -16,7 +16,7 @@ func (r *deviceManagementRoutes) executeCall(c *gin.Context) { guid := c.Param("guid") call := c.Param("call") - result, err := r.a.ExecuteCall(c.Request.Context(), guid, call, "") + result, err := r.a.ExecuteCall(c.Request.Context(), guid, call, tenantIDFromHeader(c)) if err != nil { r.l.Error(err, "http - explorer - v1 - executeCall") ErrorResponse(c, err) diff --git a/internal/controller/httpapi/v1/explorer_test.go b/internal/controller/httpapi/v1/explorer_test.go new file mode 100644 index 000000000..c9ef28ac4 --- /dev/null +++ b/internal/controller/httpapi/v1/explorer_test.go @@ -0,0 +1,66 @@ +package v1 + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/gin-gonic/gin" + "github.com/stretchr/testify/require" + gomock "go.uber.org/mock/gomock" + + "github.com/device-management-toolkit/console/internal/controller/httpapi/middleware" + dto "github.com/device-management-toolkit/console/internal/entity/dto/v1" + "github.com/device-management-toolkit/console/internal/mocks" + "github.com/device-management-toolkit/console/pkg/logger" +) + +func explorerTest(t *testing.T) (*mocks.MockAMTExplorerFeature, *gin.Engine) { + t.Helper() + + mockCtl := gomock.NewController(t) + defer mockCtl.Finish() + + log := logger.New("error") + amtExplorer := mocks.NewMockAMTExplorerFeature(mockCtl) + + engine := gin.New() + engine.Use(middleware.Tenant(logger.New("error"))) + handler := engine.Group("/api/v1") + + NewAmtRoutes(handler, mocks.NewMockDeviceManagementFeature(mockCtl), amtExplorer, mocks.NewMockExporter(mockCtl), log) + + return amtExplorer, engine +} + +func executeExplorerCall(t *testing.T, tenantHeader string) *httptest.ResponseRecorder { + t.Helper() + + amtExplorer, engine := explorerTest(t) + + amtExplorer.EXPECT(). + ExecuteCall(gomock.Any(), "device-guid", "GetGeneralSettings", tenantHeader). + Return(&dto.Explorer{XMLInput: "", XMLOutput: ""}, nil) + + req := httptest.NewRequest(http.MethodGet, "/api/v1/amt/explorer/device-guid/GetGeneralSettings", http.NoBody) + if tenantHeader != "" { + req.Header.Set(middleware.TenantHeaderName, tenantHeader) + } + + recorder := httptest.NewRecorder() + engine.ServeHTTP(recorder, req) + + return recorder +} + +func TestExplorerExecuteCallUsesTenantHeader(t *testing.T) { + t.Parallel() + + require.Equal(t, http.StatusOK, executeExplorerCall(t, "acme-corp").Code) +} + +func TestExplorerExecuteCallWithoutTenantHeader(t *testing.T) { + t.Parallel() + + require.Equal(t, http.StatusOK, executeExplorerCall(t, "").Code) +} diff --git a/internal/controller/httpapi/v1/ieee8021xconfigs.go b/internal/controller/httpapi/v1/ieee8021xconfigs.go index 911cf0430..1616c4070 100644 --- a/internal/controller/httpapi/v1/ieee8021xconfigs.go +++ b/internal/controller/httpapi/v1/ieee8021xconfigs.go @@ -43,6 +43,8 @@ func NewIEEE8021xConfigRoutes(handler *gin.RouterGroup, t ieee8021xconfigs.Featu } func (r *ieee8021xConfigRoutes) get(c *gin.Context) { + tenantID := tenantIDFromHeader(c) + var odata OData if err := odata.BindAndValidate(c); err != nil { validationErr := ErrValidation8021xConfig.Wrap("get", "BindAndValidate", err) @@ -51,7 +53,7 @@ func (r *ieee8021xConfigRoutes) get(c *gin.Context) { return } - items, err := r.t.Get(c.Request.Context(), odata.Top, odata.Skip, "") + items, err := r.t.Get(c.Request.Context(), odata.Top, odata.Skip, tenantID) if err != nil { r.l.Error(err, "http - IEEE8021x configs - v1 - getCount") ErrorResponse(c, err) @@ -60,7 +62,7 @@ func (r *ieee8021xConfigRoutes) get(c *gin.Context) { } if odata.Count { - count, err := r.t.GetCount(c.Request.Context(), "") + count, err := r.t.GetCount(c.Request.Context(), tenantID) if err != nil { r.l.Error(err, "http - IEEE8021x configs - v1 - getCount") ErrorResponse(c, err) @@ -79,8 +81,9 @@ func (r *ieee8021xConfigRoutes) get(c *gin.Context) { func (r *ieee8021xConfigRoutes) getByName(c *gin.Context) { configName := c.Param("profileName") + tenantID := tenantIDFromHeader(c) - config, err := r.t.GetByName(c.Request.Context(), configName, "") + config, err := r.t.GetByName(c.Request.Context(), configName, tenantID) if err != nil { r.l.Error(err, "http - IEEE8021x configs - v1 - getByName") ErrorResponse(c, err) @@ -100,6 +103,12 @@ func (r *ieee8021xConfigRoutes) insert(c *gin.Context) { return } + if err := applyTenantID(c, &config.TenantID); err != nil { + ErrorResponse(c, err) + + return + } + newConfig, err := r.t.Insert(c.Request.Context(), &config) if err != nil { r.l.Error(err, "http - IEEE8021x configs - v1 - insert") @@ -120,6 +129,12 @@ func (r *ieee8021xConfigRoutes) update(c *gin.Context) { return } + if err := applyTenantID(c, &config.TenantID); err != nil { + ErrorResponse(c, err) + + return + } + updatedConfig, err := r.t.Update(c.Request.Context(), &config) if err != nil { r.l.Error(err, "http - IEEE8021x configs - v1 - update") @@ -133,8 +148,9 @@ func (r *ieee8021xConfigRoutes) update(c *gin.Context) { func (r *ieee8021xConfigRoutes) delete(c *gin.Context) { configName := c.Param("profileName") + tenantID := tenantIDFromHeader(c) - err := r.t.Delete(c.Request.Context(), configName, "") + err := r.t.Delete(c.Request.Context(), configName, tenantID) if err != nil { r.l.Error(err, "http - IEEE8021x configs - v1 - delete") ErrorResponse(c, err) diff --git a/internal/controller/httpapi/v1/profiles.go b/internal/controller/httpapi/v1/profiles.go index 26dcef9bb..269a03ca8 100644 --- a/internal/controller/httpapi/v1/profiles.go +++ b/internal/controller/httpapi/v1/profiles.go @@ -46,6 +46,8 @@ func NewProfileRoutes(handler *gin.RouterGroup, t profiles.Feature, l logger.Int } func (r *profileRoutes) get(c *gin.Context) { + tenantID := tenantIDFromHeader(c) + var odata OData if err := odata.BindAndValidate(c); err != nil { validationErr := ErrValidationProfile.Wrap("get", "BindAndValidate", err) @@ -54,7 +56,7 @@ func (r *profileRoutes) get(c *gin.Context) { return } - items, err := r.t.Get(c.Request.Context(), odata.Top, odata.Skip, "") + items, err := r.t.Get(c.Request.Context(), odata.Top, odata.Skip, tenantID) if err != nil { r.l.Error(err, "http - v1 - get") ErrorResponse(c, err) @@ -63,7 +65,7 @@ func (r *profileRoutes) get(c *gin.Context) { } if odata.Count { - count, err := r.t.GetCount(c.Request.Context(), "") + count, err := r.t.GetCount(c.Request.Context(), tenantID) if err != nil { r.l.Error(err, "http - v1 - getCount") ErrorResponse(c, err) @@ -82,8 +84,9 @@ func (r *profileRoutes) get(c *gin.Context) { func (r *profileRoutes) getByName(c *gin.Context) { name := c.Param("name") + tenantID := tenantIDFromHeader(c) - item, err := r.t.GetByName(c.Request.Context(), name, "") + item, err := r.t.GetByName(c.Request.Context(), name, tenantID) if err != nil { r.l.Error(err, "http - v1 - getByName") ErrorResponse(c, err) @@ -97,8 +100,9 @@ func (r *profileRoutes) getByName(c *gin.Context) { func (r *profileRoutes) export(c *gin.Context) { name := c.Param("name") domainName := c.Query("domainName") + tenantID := tenantIDFromHeader(c) - item, key, err := r.t.Export(c.Request.Context(), name, domainName, "") + item, key, err := r.t.Export(c.Request.Context(), name, domainName, tenantID) if err != nil { r.l.Error(err, "http - v1 - export") ErrorResponse(c, err) @@ -125,6 +129,12 @@ func (r *profileRoutes) insert(c *gin.Context) { return } + if err := applyTenantID(c, &profile.TenantID); err != nil { + ErrorResponse(c, err) + + return + } + newProfile, err := r.t.Insert(c.Request.Context(), &profile) if err != nil { r.l.Error(err, "http - v1 - insert") @@ -153,6 +163,12 @@ func (r *profileRoutes) update(c *gin.Context) { return } + if err := applyTenantID(c, &profile.TenantID); err != nil { + ErrorResponse(c, err) + + return + } + fields, err := providedJSONFieldsFromBody(body) if err != nil { validationErr := ErrValidationProfile.Wrap("update", "providedJSONFieldsFromBody", err) @@ -174,8 +190,9 @@ func (r *profileRoutes) update(c *gin.Context) { func (r *profileRoutes) delete(c *gin.Context) { name := c.Param("name") + tenantID := tenantIDFromHeader(c) - err := r.t.Delete(c.Request.Context(), name, "") + err := r.t.Delete(c.Request.Context(), name, tenantID) if err != nil { r.l.Error(err, "http - v1 - delete") ErrorResponse(c, err) diff --git a/internal/controller/httpapi/v1/profiles_test.go b/internal/controller/httpapi/v1/profiles_test.go index 3e3b4b88b..06ab3b194 100644 --- a/internal/controller/httpapi/v1/profiles_test.go +++ b/internal/controller/httpapi/v1/profiles_test.go @@ -12,8 +12,10 @@ import ( "github.com/stretchr/testify/require" gomock "go.uber.org/mock/gomock" + "github.com/device-management-toolkit/console/internal/controller/httpapi/middleware" "github.com/device-management-toolkit/console/internal/entity/dto/v1" "github.com/device-management-toolkit/console/internal/mocks" + "github.com/device-management-toolkit/console/internal/tenant" "github.com/device-management-toolkit/console/internal/usecase/profiles" "github.com/device-management-toolkit/console/pkg/logger" ) @@ -28,6 +30,7 @@ func profilesTest(t *testing.T) (*mocks.MockProfilesFeature, *gin.Engine) { mockProfiles := mocks.NewMockProfilesFeature(mockCtl) engine := gin.New() + engine.Use(middleware.Tenant(log)) handler := engine.Group("/api/v1/admin") NewProfileRoutes(handler, mockProfiles, log) @@ -414,6 +417,52 @@ func TestProfilesUpdatePatchWithoutPasswords(t *testing.T) { require.Equal(t, http.StatusOK, w.Code) } +func TestProfilesGetUsesTenantHeader(t *testing.T) { + t.Parallel() + + profileFeature, engine := profilesTest(t) + + profileFeature.EXPECT(). + Get(tenant.WithContext(context.Background(), "tenant-a"), 25, 0, "tenant-a"). + Return([]dto.Profile{{ProfileName: "profile", TenantID: "tenant-a"}}, nil) + + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "/api/v1/admin/profiles", http.NoBody) + require.NoError(t, err) + req.Header.Set(middleware.TenantHeaderName, "tenant-a") + + w := httptest.NewRecorder() + engine.ServeHTTP(w, req) + + require.Equal(t, http.StatusOK, w.Code) + require.Contains(t, w.Body.String(), "tenant-a") +} + +func TestProfilesInsertUsesTenantHeader(t *testing.T) { + t.Parallel() + + profileFeature, engine := profilesTest(t) + + expected := profileTest + expected.TenantID = "tenant-a" + + profileFeature.EXPECT(). + Insert(tenant.WithContext(context.Background(), "tenant-a"), &expected). + Return(&expected, nil) + + body, err := json.Marshal(profileTest) + require.NoError(t, err) + + req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, "/api/v1/admin/profiles", bytes.NewBuffer(body)) + require.NoError(t, err) + req.Header.Set(middleware.TenantHeaderName, "tenant-a") + + w := httptest.NewRecorder() + engine.ServeHTTP(w, req) + + require.Equal(t, http.StatusCreated, w.Code) + require.Contains(t, w.Body.String(), "tenant-a") +} + func TestProfilesInsertWithoutPasswordFails(t *testing.T) { t.Parallel() diff --git a/internal/controller/httpapi/v1/tenant.go b/internal/controller/httpapi/v1/tenant.go new file mode 100644 index 000000000..ff524f5aa --- /dev/null +++ b/internal/controller/httpapi/v1/tenant.go @@ -0,0 +1,45 @@ +package v1 + +import ( + "errors" + + "github.com/gin-gonic/gin" + + "github.com/device-management-toolkit/console/internal/entity/dto/v1" + "github.com/device-management-toolkit/console/internal/tenant" + "github.com/device-management-toolkit/console/pkg/consoleerrors" +) + +var ( + ErrValidationTenant = dto.NotValidError{Console: consoleerrors.CreateConsoleError("TenantAPI")} + errTenantMismatch = errors.New("tenantId in body conflicts with x-tenant-id header") + errInvalidTenantID = errors.New(tenant.Hint) +) + +func tenantIDFromHeader(c *gin.Context) string { + return tenant.FromContext(c.Request.Context()) +} + +// applyTenantID overwrites target with the request's tenant. An absent header +// leaves the body-supplied value untouched if valid; a conflicting or invalid +// tenant ID is rejected so a caller cannot write outside the tenant it asked for +// or supply a malformed tenant ID. +func applyTenantID(c *gin.Context, target *string) error { + tenantID := tenantIDFromHeader(c) + + if tenantID == "" { + if !tenant.Valid(*target) { + return ErrValidationTenant.Wrap("applyTenantID", "tenantId", errInvalidTenantID) + } + + return nil + } + + if *target != "" && *target != tenantID { + return ErrValidationTenant.Wrap("applyTenantID", "tenantId", errTenantMismatch) + } + + *target = tenantID + + return nil +} diff --git a/internal/controller/httpapi/v1/tenant_test.go b/internal/controller/httpapi/v1/tenant_test.go new file mode 100644 index 000000000..f9ee81b51 --- /dev/null +++ b/internal/controller/httpapi/v1/tenant_test.go @@ -0,0 +1,79 @@ +package v1 + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/gin-gonic/gin" + "github.com/stretchr/testify/require" + + dto "github.com/device-management-toolkit/console/internal/entity/dto/v1" + "github.com/device-management-toolkit/console/internal/tenant" +) + +// tenantContext returns a context scoped as the tenant middleware would leave it. +func tenantContext(t *testing.T, tenantID string) *gin.Context { + t.Helper() + + c, _ := gin.CreateTestContext(httptest.NewRecorder()) + req := httptest.NewRequest(http.MethodGet, "/", http.NoBody) + c.Request = req.WithContext(tenant.WithContext(req.Context(), tenantID)) + + return c +} + +func TestTenantIDFromHeader(t *testing.T) { + t.Parallel() + + require.Equal(t, "tenant-a", tenantIDFromHeader(tenantContext(t, "tenant-a"))) + require.Empty(t, tenantIDFromHeader(tenantContext(t, ""))) +} + +func TestApplyTenantID(t *testing.T) { + t.Parallel() + + c := tenantContext(t, "tenant-a") + + profile := dto.Profile{} + require.NoError(t, applyTenantID(c, &profile.TenantID)) + require.Equal(t, "tenant-a", profile.TenantID) + + // Matching body value is accepted. + require.NoError(t, applyTenantID(c, &profile.TenantID)) + require.Equal(t, "tenant-a", profile.TenantID) +} + +func TestApplyTenantIDRejectsBodyMismatch(t *testing.T) { + t.Parallel() + + c := tenantContext(t, "tenant-a") + + profile := dto.Profile{TenantID: "tenant-b"} + require.ErrorIs(t, applyTenantID(c, &profile.TenantID), errTenantMismatch) + require.Equal(t, "tenant-b", profile.TenantID) +} + +func TestApplyTenantIDWithoutTenantKeepsBodyValue(t *testing.T) { + t.Parallel() + + c := tenantContext(t, "") + + profile := dto.Profile{TenantID: "tenant-b"} + require.NoError(t, applyTenantID(c, &profile.TenantID)) + require.Equal(t, "tenant-b", profile.TenantID) + + profileEmpty := dto.Profile{TenantID: ""} + require.NoError(t, applyTenantID(c, &profileEmpty.TenantID)) + require.Equal(t, "", profileEmpty.TenantID) +} + +func TestApplyTenantIDWithoutTenantRejectsInvalidBodyValue(t *testing.T) { + t.Parallel() + + c := tenantContext(t, "") + + profile := dto.Profile{TenantID: "../../other-tenant"} + require.ErrorIs(t, applyTenantID(c, &profile.TenantID), errInvalidTenantID) + require.Equal(t, "../../other-tenant", profile.TenantID) +} diff --git a/internal/controller/httpapi/v1/wificonfigs.go b/internal/controller/httpapi/v1/wificonfigs.go index 77e235d71..20984f202 100644 --- a/internal/controller/httpapi/v1/wificonfigs.go +++ b/internal/controller/httpapi/v1/wificonfigs.go @@ -43,6 +43,8 @@ func NewWirelessConfigRoutes(handler *gin.RouterGroup, t wificonfigs.Feature, l } func (r *WirelessConfigRoutes) get(c *gin.Context) { + tenantID := tenantIDFromHeader(c) + var odata OData if err := odata.BindAndValidate(c); err != nil { validationErr := ErrValidationWifiConfig.Wrap("get", "BindAndValidate", err) @@ -51,7 +53,7 @@ func (r *WirelessConfigRoutes) get(c *gin.Context) { return } - items, err := r.t.Get(c.Request.Context(), odata.Top, odata.Skip, "") + items, err := r.t.Get(c.Request.Context(), odata.Top, odata.Skip, tenantID) if err != nil { r.l.Error(err, "http - wireless configs - v1 - getCount") ErrorResponse(c, err) @@ -60,7 +62,7 @@ func (r *WirelessConfigRoutes) get(c *gin.Context) { } if odata.Count { - count, err := r.t.GetCount(c.Request.Context(), "") + count, err := r.t.GetCount(c.Request.Context(), tenantID) if err != nil { r.l.Error(err, "http - wireless configs - v1 - getCount") ErrorResponse(c, err) @@ -79,8 +81,9 @@ func (r *WirelessConfigRoutes) get(c *gin.Context) { func (r *WirelessConfigRoutes) getByName(c *gin.Context) { profileName := c.Param("profileName") + tenantID := tenantIDFromHeader(c) - config, err := r.t.GetByName(c.Request.Context(), profileName, "") + config, err := r.t.GetByName(c.Request.Context(), profileName, tenantID) if err != nil { r.l.Error(err, "http - wireless configs - v1 - getByName") ErrorResponse(c, err) @@ -100,6 +103,12 @@ func (r *WirelessConfigRoutes) insert(c *gin.Context) { return } + if err := applyTenantID(c, &config.TenantID); err != nil { + ErrorResponse(c, err) + + return + } + insertedConfig, err := r.t.Insert(c.Request.Context(), &config) if err != nil { r.l.Error(err, "http - wireless configs - v1 - insert") @@ -121,6 +130,12 @@ func (r *WirelessConfigRoutes) update(c *gin.Context) { return } + if err := applyTenantID(c, &config.TenantID); err != nil { + ErrorResponse(c, err) + + return + } + updatedWirelessConfig, err := r.t.Update(c.Request.Context(), &config) if err != nil { r.l.Error(err, "http - wireless configs - v1 - update") @@ -134,8 +149,9 @@ func (r *WirelessConfigRoutes) update(c *gin.Context) { func (r *WirelessConfigRoutes) delete(c *gin.Context) { configName := c.Param("profileName") + tenantID := tenantIDFromHeader(c) - err := r.t.Delete(c.Request.Context(), configName, "") + err := r.t.Delete(c.Request.Context(), configName, tenantID) if err != nil { r.l.Error(err, "http - wireless configs - v1 - delete") ErrorResponse(c, err) diff --git a/internal/controller/openapi/route_options.go b/internal/controller/openapi/route_options.go index 4b68bcd63..40db33ace 100644 --- a/internal/controller/openapi/route_options.go +++ b/internal/controller/openapi/route_options.go @@ -5,8 +5,11 @@ import ( "github.com/getkin/kin-openapi/openapi3" "github.com/go-fuego/fuego" + "github.com/go-fuego/fuego/param" "github.com/device-management-toolkit/console/config" + "github.com/device-management-toolkit/console/internal/controller/httpapi/middleware" + "github.com/device-management-toolkit/console/internal/tenant" ) // specCookieAuthEnabled mirrors the auth middleware. cmd/openapi-gen runs @@ -38,6 +41,8 @@ func protectedRouteOptions() fuego.RouteOption { return routeOptionGroup( apiRouteOptions(), fuego.OptionSecurity(security...), + tenantHeaderOption(), + errorResponseOption(http.StatusBadRequest, "Bad Request"), errorResponseOption(http.StatusNotFound, "Not Found"), errorResponseOption(http.StatusRequestTimeout, "Request Timeout"), errorResponseOption(http.StatusConflict, "Conflict"), @@ -46,6 +51,17 @@ func protectedRouteOptions() fuego.RouteOption { ) } +// tenantHeaderOption documents the optional tenant scope. Omitting the header +// selects the default tenant, which is where single-tenant data lives. +func tenantHeaderOption() fuego.RouteOption { + return fuego.OptionHeader( + middleware.TenantHeaderName, + "Scopes the request to a tenant. "+tenant.Hint+". Omit for the default tenant.", + param.Nullable(), + param.Example("tenant", "acme-corp"), + ) +} + func errorResponseOption(statusCode int, description string) fuego.RouteOption { return fuego.OptionAddResponse(statusCode, description, fuego.Response{Type: fuego.HTTPError{}}) } diff --git a/internal/controller/openapi/tenant_test.go b/internal/controller/openapi/tenant_test.go new file mode 100644 index 000000000..13d074722 --- /dev/null +++ b/internal/controller/openapi/tenant_test.go @@ -0,0 +1,88 @@ +package openapi + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/device-management-toolkit/console/internal/controller/httpapi/middleware" +) + +// tenantHeaderNames returns, per operation, whether x-tenant-id is declared. +func tenantHeaderNames(t *testing.T, spec map[string]interface{}) map[string]bool { + t.Helper() + + paths, ok := spec["paths"].(map[string]interface{}) + require.True(t, ok) + + declared := map[string]bool{} + + for path, item := range paths { + operations, ok := item.(map[string]interface{}) + require.True(t, ok) + + for method, raw := range operations { + operation, ok := raw.(map[string]interface{}) + if !ok { + continue + } + + declared[method+" "+path] = hasTenantHeader(operation) + } + } + + return declared +} + +func hasTenantHeader(operation map[string]interface{}) bool { + params, ok := operation["parameters"].([]interface{}) + if !ok { + return false + } + + for _, raw := range params { + param, ok := raw.(map[string]interface{}) + if !ok { + continue + } + + if param["name"] == middleware.TenantHeaderName && param["in"] == "header" { + return true + } + } + + return false +} + +func TestProtectedRoutesDeclareTenantHeader(t *testing.T) { + t.Parallel() + + f := newTestAdapter() + f.RegisterRoutes() + + specBytes, err := f.GetOpenAPISpec() + require.NoError(t, err) + + var spec map[string]interface{} + require.NoError(t, json.Unmarshal(specBytes, &spec)) + + declared := tenantHeaderNames(t, spec) + require.NotEmpty(t, declared) + + // Only the public authorize endpoints are unscoped. + public := map[string]bool{ + "post /api/v1/authorize": true, + "post /api/v1/authorize/logout": true, + } + + for operation, hasHeader := range declared { + if public[operation] { + require.False(t, hasHeader, "%s is public and must not advertise a tenant header", operation) + + continue + } + + require.True(t, hasHeader, "%s must declare the tenant header", operation) + } +} diff --git a/internal/controller/tcp/cira/handler.go b/internal/controller/tcp/cira/handler.go index 02f68dd75..59fb9c981 100644 --- a/internal/controller/tcp/cira/handler.go +++ b/internal/controller/tcp/cira/handler.go @@ -10,14 +10,17 @@ import ( "github.com/device-management-toolkit/console/pkg/logger" ) -// globalRequestThreshold is the number of global requests required before sending keep-alive. -const globalRequestThreshold = 4 +const ( + globalRequestThreshold = 4 + authMethodPassword = "password" +) // APFHandler implements apf.Handler for the CIRA server. // It provides application-specific logic for authentication and device registration. type APFHandler struct { devices devices.Feature deviceID string + tenantID string globalRequestCount int log logger.Interface } @@ -35,6 +38,12 @@ func (h *APFHandler) DeviceID() string { return h.deviceID } +// TenantID returns the tenant the authenticated device belongs to, learned from +// its database row rather than from the device itself. +func (h *APFHandler) TenantID() string { + return h.tenantID +} + // OnProtocolVersion is called when an APF_PROTOCOLVERSION message is received. // Extracts and stores the device UUID for later use. // The UUID is normalized to lowercase to ensure case-insensitive matching @@ -55,7 +64,7 @@ func (h *APFHandler) OnAuthRequest(request apf.AuthRequest) apf.AuthResponse { h.deviceID, request.Username, request.MethodName) // Only support password authentication - if request.MethodName != "password" { + if request.MethodName != authMethodPassword { h.log.Warn("Unsupported authentication method: %s", request.MethodName) return apf.AuthResponse{Authenticated: false} @@ -85,7 +94,9 @@ func (h *APFHandler) validateCredentials(username, password string) bool { ctx := context.Background() // Fetch device from database using the UUID - device, err := h.devices.GetByID(ctx, h.deviceID, "", true) + // CIRA devices authenticate by GUID and cannot present a tenant, so the + // lookup must span tenants rather than defaulting to the empty one. + device, err := h.devices.GetByGUID(ctx, h.deviceID, true) if err != nil { h.log.Warn("Failed to fetch device %s from database: %v", h.deviceID, err) @@ -113,6 +124,9 @@ func (h *APFHandler) validateCredentials(username, password string) bool { return false } + h.tenantID = device.TenantID + h.log.Debug("CIRA tenant resolved", "device_id", h.deviceID, "tenant_id", h.tenantID) + return true } diff --git a/internal/controller/tcp/cira/handler_test.go b/internal/controller/tcp/cira/handler_test.go new file mode 100644 index 000000000..fa642b229 --- /dev/null +++ b/internal/controller/tcp/cira/handler_test.go @@ -0,0 +1,108 @@ +package cira + +import ( + "testing" + + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + + "github.com/device-management-toolkit/go-wsman-messages/v2/pkg/apf" + + dto "github.com/device-management-toolkit/console/internal/entity/dto/v1" + "github.com/device-management-toolkit/console/internal/mocks" + "github.com/device-management-toolkit/console/pkg/logger" +) + +const ( + testGUID = "4c4c4544-0043-4810-8053-b8c04f595931" + testUsername = "mpsuser" + testPassword = "mpspass" +) + +func authHandler(t *testing.T, device *dto.Device, err error) *APFHandler { + t.Helper() + + ctrl := gomock.NewController(t) + mockDevices := mocks.NewMockDeviceManagementFeature(ctrl) + mockDevices.EXPECT().GetByGUID(gomock.Any(), testGUID, true).Return(device, err) + + handler := NewAPFHandler(mockDevices, logger.New("error")) + require.NoError(t, handler.OnProtocolVersion(apf.ProtocolVersionInfo{UUID: testGUID})) + + return handler +} + +// A device whose row carries a non-empty tenant must still authenticate: CIRA +// devices present only a GUID and MPS credentials. +func TestOnAuthRequestAcceptsDeviceInNonDefaultTenant(t *testing.T) { + t.Parallel() + + handler := authHandler(t, &dto.Device{ + GUID: testGUID, + MPSUsername: testUsername, + MPSPassword: testPassword, + TenantID: "acme-corp", + }, nil) + + response := handler.OnAuthRequest(apf.AuthRequest{ + MethodName: "password", + Username: testUsername, + Password: testPassword, + }) + + require.True(t, response.Authenticated) + require.Equal(t, "acme-corp", handler.TenantID()) +} + +func TestOnAuthRequestLearnsDefaultTenant(t *testing.T) { + t.Parallel() + + handler := authHandler(t, &dto.Device{ + GUID: testGUID, + MPSUsername: testUsername, + MPSPassword: testPassword, + }, nil) + + response := handler.OnAuthRequest(apf.AuthRequest{ + MethodName: "password", + Username: testUsername, + Password: testPassword, + }) + + require.True(t, response.Authenticated) + require.Empty(t, handler.TenantID()) +} + +func TestOnAuthRequestRejectsBadPasswordWithoutLearningTenant(t *testing.T) { + t.Parallel() + + handler := authHandler(t, &dto.Device{ + GUID: testGUID, + MPSUsername: testUsername, + MPSPassword: testPassword, + TenantID: "acme-corp", + }, nil) + + response := handler.OnAuthRequest(apf.AuthRequest{ + MethodName: "password", + Username: testUsername, + Password: "wrong", + }) + + require.False(t, response.Authenticated) + require.Empty(t, handler.TenantID()) +} + +func TestOnAuthRequestRejectsUnsupportedMethod(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + mockDevices := mocks.NewMockDeviceManagementFeature(ctrl) + + handler := NewAPFHandler(mockDevices, logger.New("error")) + require.NoError(t, handler.OnProtocolVersion(apf.ProtocolVersionInfo{UUID: testGUID})) + + response := handler.OnAuthRequest(apf.AuthRequest{MethodName: "publickey"}) + + require.False(t, response.Authenticated) +} diff --git a/internal/controller/tcp/cira/tunnel.go b/internal/controller/tcp/cira/tunnel.go index ebf23b8ec..19371171c 100644 --- a/internal/controller/tcp/cira/tunnel.go +++ b/internal/controller/tcp/cira/tunnel.go @@ -6,7 +6,6 @@ import ( "context" "crypto/tls" "encoding/binary" - "encoding/hex" "errors" "fmt" "net" @@ -88,7 +87,8 @@ func (s *Server) ListenAndServe() error { config.CipherSuites = append(config.CipherSuites, suite.ID) } // add the weak cipher suites for AMT device compatibility - config.CipherSuites = append(config.CipherSuites, + config.CipherSuites = append( + config.CipherSuites, tls.TLS_RSA_WITH_AES_128_GCM_SHA256, tls.TLS_RSA_WITH_AES_128_CBC_SHA, tls.TLS_RSA_WITH_AES_256_CBC_SHA, @@ -254,7 +254,7 @@ func (ctx *connectionContext) readData() ([]byte, error) { } data := buf[:n] - ctx.log.Debug("Received data from %s: %s", ctx.handler.DeviceID(), hex.EncodeToString(data)) + ctx.log.Debug("Received data from device", "device_id", ctx.handler.DeviceID(), "bytes_received", len(data)) return data, nil } @@ -293,6 +293,7 @@ func (ctx *connectionContext) registerDevice() { ctx.device = &wsman.ConnectionEntry{ IsCIRA: true, + TenantID: ctx.handler.TenantID(), Conny: ctx.conn, Timer: time.NewTimer(maxIdleTime), WsmanMessages: wsman2.NewMessages(client.Parameters{}), @@ -304,7 +305,7 @@ func (ctx *connectionContext) registerDevice() { ctx.log.Error("Failed to update connection status for device %s: %v", deviceID, err) } - ctx.log.Info("Device authenticated and registered: %s", deviceID) + ctx.log.Info("Device authenticated and registered", "device_id", deviceID, "tenant_id", ctx.handler.TenantID()) } func (ctx *connectionContext) writeResponse(response bytes.Buffer) error { @@ -487,6 +488,7 @@ func (ctx *connectionContext) handleChannelClose(data []byte) bool { return false } + ctx.log.Info("AMT closed APF channel", "device_id", ctx.handler.DeviceID(), "channel_id", ourChannel) ctx.device.UnregisterAPFChannel(ourChannel) return true diff --git a/internal/controller/ws/v1/redirect.go b/internal/controller/ws/v1/redirect.go index 3efb95a98..faeff1015 100644 --- a/internal/controller/ws/v1/redirect.go +++ b/internal/controller/ws/v1/redirect.go @@ -13,6 +13,7 @@ import ( "github.com/gorilla/websocket" "github.com/device-management-toolkit/console/config" + "github.com/device-management-toolkit/console/internal/tenant" "github.com/device-management-toolkit/console/internal/usecase/devices" "github.com/device-management-toolkit/console/pkg/logger" ) @@ -75,14 +76,24 @@ func (r *RedirectRoutes) websocketHandler(c *gin.Context) { // KVM_TIMING: Measure total connection time totalStart := time.Now() - err = r.d.Redirect(c, conn, c.Query("host"), c.Query("mode")) + // The JWT tenant claim is stored on the request context and is required for + // the use case to locate the device in the correct tenant. + err = r.d.Redirect(c.Request.Context(), conn, c.Query("host"), c.Query("mode")) totalDuration := time.Since(totalStart) devices.RecordTotalConnection(totalDuration, c.Query("mode")) r.l.Debug("KVM_TIMING: Total connection time", "duration_ms", totalDuration.Milliseconds(), "mode", c.Query("mode")) if err != nil { r.l.Error(err, "http - devices - v1 - redirect") - errorResponse(c, http.StatusInternalServerError, "redirect failed") + + // Upgrade hijacks the HTTP connection, so errors after this point must be + // reported over the WebSocket rather than through Gin's response writer. + _ = conn.WriteControl( + websocket.CloseMessage, + websocket.FormatCloseMessage(websocket.CloseInternalServerErr, "redirect failed"), + time.Now().Add(time.Second), + ) + _ = conn.Close() } } @@ -124,5 +135,24 @@ func (r *RedirectRoutes) validateRedirectionToken(c *gin.Context, tokenString st return false } + tenantID, hasTenantID := (*claims)[tenant.TenantIDClaim] + if hasTenantID { + tenantValue, ok := tenantID.(string) + if !ok || !tenant.Valid(tenantValue) { + r.l.Warn("redirection token contains invalid tenant", "host", c.Query("host")) + http.Error(c.Writer, "token contains invalid tenant", http.StatusForbidden) + + return false + } + + r.l.Debug("WebSocket tenant ID from claims", "tenant_id", tenantValue) + + c.Request = c.Request.WithContext(tenant.WithContext(c.Request.Context(), tenantValue)) + } else { + r.l.Debug("WebSocket tenant ID from claims is not present") + + c.Request = c.Request.WithContext(tenant.WithContext(c.Request.Context(), "")) + } + return true } diff --git a/internal/controller/ws/v1/redirect_test.go b/internal/controller/ws/v1/redirect_test.go index 235aa709f..7e02a6bd5 100644 --- a/internal/controller/ws/v1/redirect_test.go +++ b/internal/controller/ws/v1/redirect_test.go @@ -1,9 +1,11 @@ package v1 import ( + "context" "errors" "net/http" "net/http/httptest" + "strings" "testing" "time" @@ -11,10 +13,12 @@ import ( "github.com/golang-jwt/jwt/v5" "github.com/gorilla/websocket" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" "github.com/device-management-toolkit/console/config" "github.com/device-management-toolkit/console/internal/mocks" + "github.com/device-management-toolkit/console/internal/tenant" ) var ( @@ -51,12 +55,6 @@ func TestWebSocketHandler(t *testing.T) { //nolint:paralleltest // logging libra redirectError: nil, expectedStatus: http.StatusInternalServerError, }, - { - name: "Redirect error", - upgraderError: nil, - redirectError: ErrRedirect, - expectedStatus: http.StatusInternalServerError, - }, } for _, tc := range tests { //nolint:paralleltest // logging library is not thread-safe for tests @@ -102,6 +100,44 @@ func TestWebSocketHandler(t *testing.T) { //nolint:paralleltest // logging libra } } +func TestWebSocketHandlerRedirectErrorClosesWebSocket(t *testing.T) { //nolint:paralleltest // shared configuration and logger + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + _, _ = config.NewConfig() + config.ConsoleConfig.Disabled = true + + mockFeature := mocks.NewMockDeviceManagementFeature(ctrl) + mockLogger := mocks.NewMockLogger(ctrl) + mockLogger.EXPECT().Debug("KVM_TIMING: WebSocket upgrade", "duration_ms", gomock.Any()) + mockLogger.EXPECT().Info("Websocket connection opened") + mockFeature.EXPECT().Redirect(gomock.Any(), gomock.Any(), "someHost", "someMode").Return(ErrRedirect) + mockLogger.EXPECT().Debug("KVM_TIMING: Total connection time", "duration_ms", gomock.Any(), "mode", "someMode") + mockLogger.EXPECT().Error(ErrRedirect, "http - devices - v1 - redirect") + + r := gin.Default() + RegisterRoutes(r, mockLogger, mockFeature, &websocket.Upgrader{}) + server := httptest.NewServer(r) + t.Cleanup(server.Close) + + connection, response, err := websocket.DefaultDialer.Dial( + "ws"+strings.TrimPrefix(server.URL, "http")+"/relay/webrelay.ashx?host=someHost&mode=someMode", + nil, + ) + if response != nil { + t.Cleanup(func() { _ = response.Body.Close() }) + } + + require.NoError(t, err) + t.Cleanup(func() { _ = connection.Close() }) + + _, _, err = connection.ReadMessage() + closeError := &websocket.CloseError{} + require.ErrorAs(t, err, &closeError) + require.Equal(t, websocket.CloseInternalServerErr, closeError.Code) + require.Equal(t, "redirect failed", closeError.Text) +} + // TestWebSocketHandlerDeviceBinding: WS accepts only a token whose deviceId matches host. func TestWebSocketHandlerDeviceBinding(t *testing.T) { //nolint:paralleltest // logging library is not thread-safe for tests ctrl := gomock.NewController(t) @@ -125,6 +161,17 @@ func TestWebSocketHandlerDeviceBinding(t *testing.T) { //nolint:paralleltest // return s } + tokenForTenant := func(deviceID, tenantID string) string { + claims := jwt.MapClaims{ + "exp": time.Now().Add(5 * time.Minute).Unix(), + "deviceId": deviceID, + "tenantId": tenantID, + } + + s, _ := jwt.NewWithClaims(jwt.SigningMethodHS256, claims).SignedString([]byte(config.ConsoleConfig.JWTKey)) + + return s + } t.Run("rejects token whose deviceId does not match host", func(t *testing.T) { //nolint:paralleltest // shared logger mockFeature := mocks.NewMockDeviceManagementFeature(ctrl) @@ -186,6 +233,7 @@ func TestWebSocketHandlerDeviceBinding(t *testing.T) { //nolint:paralleltest // mockLogger := mocks.NewMockLogger(ctrl) mockUpgrader.EXPECT().Upgrade(gomock.Any(), gomock.Any(), nil).Return(&websocket.Conn{}, nil) + mockLogger.EXPECT().Debug("WebSocket tenant ID from claims is not present") mockLogger.EXPECT().Debug("failed to cast Upgrader to *websocket.Upgrader") mockLogger.EXPECT().Debug("KVM_TIMING: WebSocket upgrade", "duration_ms", gomock.Any()) mockLogger.EXPECT().Info("Websocket connection opened") @@ -210,6 +258,7 @@ func TestWebSocketHandlerDeviceBinding(t *testing.T) { //nolint:paralleltest // mockLogger := mocks.NewMockLogger(ctrl) mockUpgrader.EXPECT().Upgrade(gomock.Any(), gomock.Any(), nil).Return(&websocket.Conn{}, nil) + mockLogger.EXPECT().Debug("WebSocket tenant ID from claims is not present") mockLogger.EXPECT().Debug("failed to cast Upgrader to *websocket.Upgrader") mockLogger.EXPECT().Debug("KVM_TIMING: WebSocket upgrade", "duration_ms", gomock.Any()) mockLogger.EXPECT().Info("Websocket connection opened") @@ -227,6 +276,55 @@ func TestWebSocketHandlerDeviceBinding(t *testing.T) { //nolint:paralleltest // assert.Equal(t, http.StatusOK, w.Code) }) + + t.Run("propagates token tenant to redirect context", func(t *testing.T) { //nolint:paralleltest // shared logger + mockFeature := mocks.NewMockDeviceManagementFeature(ctrl) + mockUpgrader := mocks.NewMockUpgrader(ctrl) + mockLogger := mocks.NewMockLogger(ctrl) + + mockUpgrader.EXPECT().Upgrade(gomock.Any(), gomock.Any(), nil).Return(&websocket.Conn{}, nil) + mockLogger.EXPECT().Debug("WebSocket tenant ID from claims", "tenant_id", "tenant-a") + mockLogger.EXPECT().Debug("failed to cast Upgrader to *websocket.Upgrader") + mockLogger.EXPECT().Debug("KVM_TIMING: WebSocket upgrade", "duration_ms", gomock.Any()) + mockLogger.EXPECT().Info("Websocket connection opened") + mockFeature.EXPECT().Redirect(gomock.Any(), gomock.Any(), "deviceA", "kvm").DoAndReturn( + func(ctx context.Context, _ *websocket.Conn, _, _ string) error { + assert.Equal(t, "tenant-a", tenant.FromContext(ctx)) + + return nil + }, + ) + mockLogger.EXPECT().Debug("KVM_TIMING: Total connection time", "duration_ms", gomock.Any(), "mode", "kvm") + + r := gin.Default() + RegisterRoutes(r, mockLogger, mockFeature, mockUpgrader) + + req := httptest.NewRequest(http.MethodGet, "/relay/webrelay.ashx?host=deviceA&mode=kvm", http.NoBody) + req.Header.Set("Sec-Websocket-Protocol", tokenForTenant("deviceA", "tenant-a")) + + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + + assert.Equal(t, http.StatusOK, w.Code) + }) + + t.Run("rejects token with invalid tenant", func(t *testing.T) { //nolint:paralleltest // shared logger + mockFeature := mocks.NewMockDeviceManagementFeature(ctrl) + mockUpgrader := mocks.NewMockUpgrader(ctrl) + mockLogger := mocks.NewMockLogger(ctrl) + mockLogger.EXPECT().Warn("redirection token contains invalid tenant", "host", "deviceA") + + r := gin.Default() + RegisterRoutes(r, mockLogger, mockFeature, mockUpgrader) + + req := httptest.NewRequest(http.MethodGet, "/relay/webrelay.ashx?host=deviceA&mode=kvm", http.NoBody) + req.Header.Set("Sec-Websocket-Protocol", tokenForTenant("deviceA", ".tenant-a")) + + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + + assert.Equal(t, http.StatusForbidden, w.Code) + }) } // TestWebSocketHandlerTokenValidation: WS rejects missing and unverifiable tokens. diff --git a/internal/mocks/devicemanagement_mocks.go b/internal/mocks/devicemanagement_mocks.go index a5266d57d..cfc75e0d5 100644 --- a/internal/mocks/devicemanagement_mocks.go +++ b/internal/mocks/devicemanagement_mocks.go @@ -322,6 +322,21 @@ func (mr *MockDeviceManagementRepositoryMockRecorder) GetByColumn(ctx, columnNam return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetByColumn", reflect.TypeOf((*MockDeviceManagementRepository)(nil).GetByColumn), ctx, columnName, queryValue, tenantID) } +// GetByGUID mocks base method. +func (m *MockDeviceManagementRepository) GetByGUID(ctx context.Context, guid string) (*entity.Device, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetByGUID", ctx, guid) + ret0, _ := ret[0].(*entity.Device) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetByGUID indicates an expected call of GetByGUID. +func (mr *MockDeviceManagementRepositoryMockRecorder) GetByGUID(ctx, guid any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetByGUID", reflect.TypeOf((*MockDeviceManagementRepository)(nil).GetByGUID), ctx, guid) +} + // GetByID mocks base method. func (m *MockDeviceManagementRepository) GetByID(ctx context.Context, guid, tenantID string) (*entity.Device, error) { m.ctrl.T.Helper() @@ -654,6 +669,21 @@ func (mr *MockDeviceManagementFeatureMockRecorder) GetByColumn(ctx, columnName, return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetByColumn", reflect.TypeOf((*MockDeviceManagementFeature)(nil).GetByColumn), ctx, columnName, queryValue, tenantID) } +// GetByGUID mocks base method. +func (m *MockDeviceManagementFeature) GetByGUID(ctx context.Context, guid string, includeSecrets bool) (*dto.Device, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetByGUID", ctx, guid, includeSecrets) + ret0, _ := ret[0].(*dto.Device) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetByGUID indicates an expected call of GetByGUID. +func (mr *MockDeviceManagementFeatureMockRecorder) GetByGUID(ctx, guid, includeSecrets any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetByGUID", reflect.TypeOf((*MockDeviceManagementFeature)(nil).GetByGUID), ctx, guid, includeSecrets) +} + // GetByID mocks base method. func (m *MockDeviceManagementFeature) GetByID(ctx context.Context, guid, tenantID string, includeSecrets bool) (*dto.Device, error) { m.ctrl.T.Helper() diff --git a/internal/tenant/tenant.go b/internal/tenant/tenant.go new file mode 100644 index 000000000..30c703226 --- /dev/null +++ b/internal/tenant/tenant.go @@ -0,0 +1,46 @@ +// Package tenant carries the request-scoped tenant identifier between the HTTP +// layer and the use cases. It has no internal dependencies so every layer can +// import it without creating a cycle or coupling features to each other. +package tenant + +import ( + "context" + "regexp" +) + +// MaxLength bounds the identifier. tenant_id is part of the composite primary +// key on profiles, domains, wirelessconfigs, ieee8021xconfigs and ciraconfigs, +// so an unbounded value becomes a permanently addressable row. +const MaxLength = 64 + +// Hint describes the accepted format in API error responses. +const ( + Pattern = `^[A-Za-z0-9][A-Za-z0-9_-]{0,63}$` + Hint = "x-tenant-id must match " + Pattern + TenantIDClaim = "tenantId" +) + +// pattern excludes whitespace, control characters and non-ASCII so two visually +// identical identifiers cannot map to two different primary key values. +var pattern = regexp.MustCompile(Pattern) + +type contextKey struct{} + +// Valid reports whether tenantID is storable. The empty tenant is the default +// single-tenant value and is always allowed. +func Valid(tenantID string) bool { + return tenantID == "" || pattern.MatchString(tenantID) +} + +// WithContext scopes ctx to tenantID. +func WithContext(ctx context.Context, tenantID string) context.Context { + return context.WithValue(ctx, contextKey{}, tenantID) +} + +// FromContext returns the tenant scoping ctx, or the empty tenant when none was +// set. +func FromContext(ctx context.Context) string { + tenantID, _ := ctx.Value(contextKey{}).(string) + + return tenantID +} diff --git a/internal/tenant/tenant_test.go b/internal/tenant/tenant_test.go new file mode 100644 index 000000000..2f9c404ab --- /dev/null +++ b/internal/tenant/tenant_test.go @@ -0,0 +1,98 @@ +package tenant_test + +import ( + "context" + "strings" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/device-management-toolkit/console/internal/tenant" +) + +func TestValidRejects(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + value string + }{ + {"single space", " "}, + {"embedded space", "tenant a"}, + {"leading space", " tenant-a"}, + {"leading dot", ".tenant-a"}, + {"leading underscore", "_tenant-a"}, + {"leading hyphen", "-tenant-a"}, + {"trailing space", "tenant-a "}, + {"tab", "tenant\tb"}, + {"newline", "tenant\nb"}, + {"carriage return", "tenant\rb"}, + {"null byte", "tenant\x00b"}, + {"forward slash", "tenant/a"}, + {"backslash", "tenant\\a"}, + {"colon", "tenant:a"}, + {"at sign", "tenant@a"}, + {"percent encoding", "tenant%20a"}, + {"sql quote", "tenant'a"}, + {"sql injection", "a' OR '1'='1"}, + {"wildcard", "tenant*"}, + {"comma", "tenant,a"}, + {"path traversal", "../tenant"}, + {"cyrillic homoglyph", "tenant-\u0430"}, + {"zero width space", "tenant\u200ba"}, + {"emoji", "tenant-\U0001F600"}, + {"dot", "tenant.a"}, + {"one over max length", strings.Repeat("a", tenant.MaxLength+1)}, + {"far over max length", strings.Repeat("a", 4096)}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + require.False(t, tenant.Valid(tt.value)) + }) + } +} + +func TestValidAccepts(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + value string + }{ + {"empty is the default tenant", ""}, + {"single character", "a"}, + {"single digit", "1"}, + {"lower case", "tenant"}, + {"upper case", "TENANT"}, + {"mixed case", "TenantA"}, + {"hyphen", "tenant-a"}, + {"underscore", "tenant_a"}, + {"all separators", "a-b_c-d"}, + {"uuid", "3a1b0c8e-4f2d-4a6b-9c1e-7d5f8a0b2c3d"}, + {"at max length", strings.Repeat("a", tenant.MaxLength)}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + require.True(t, tenant.Valid(tt.value)) + }) + } +} + +func TestContextRoundTrip(t *testing.T) { + t.Parallel() + + ctx := tenant.WithContext(context.Background(), "tenant-a") + require.Equal(t, "tenant-a", tenant.FromContext(ctx)) +} + +func TestFromContextWithoutTenant(t *testing.T) { + t.Parallel() + + require.Empty(t, tenant.FromContext(context.Background())) +} diff --git a/internal/usecase/devices/alarms.go b/internal/usecase/devices/alarms.go index ee0776b24..591f68487 100644 --- a/internal/usecase/devices/alarms.go +++ b/internal/usecase/devices/alarms.go @@ -18,7 +18,7 @@ const ( ) func (uc *UseCase) GetAlarmOccurrences(c context.Context, guid string) ([]dto.AlarmClockOccurrence, error) { - item, err := uc.repo.GetByID(c, guid, "") + item, err := uc.deviceInTenant(c, guid) if err != nil { return nil, err } @@ -53,7 +53,7 @@ func (uc *UseCase) GetAlarmOccurrences(c context.Context, guid string) ([]dto.Al } func (uc *UseCase) CreateAlarmOccurrences(c context.Context, guid string, alarm dto.AlarmClockOccurrenceInput) (dto.AddAlarmOutput, error) { - item, err := uc.repo.GetByID(c, guid, "") + item, err := uc.deviceInTenant(c, guid) if err != nil { return dto.AddAlarmOutput{}, err } @@ -80,7 +80,7 @@ func (uc *UseCase) CreateAlarmOccurrences(c context.Context, guid string, alarm } func (uc *UseCase) DeleteAlarmOccurrences(c context.Context, guid, instanceID string) error { - item, err := uc.repo.GetByID(c, guid, "") + item, err := uc.deviceInTenant(c, guid) if err != nil { return err } diff --git a/internal/usecase/devices/boot.go b/internal/usecase/devices/boot.go index 1c72ac9f4..4f8754047 100644 --- a/internal/usecase/devices/boot.go +++ b/internal/usecase/devices/boot.go @@ -10,7 +10,7 @@ import ( ) func (uc *UseCase) GetRemoteEraseCapabilities(c context.Context, guid string) (dto.BootCapabilities, error) { - item, err := uc.repo.GetByID(c, guid, "") + item, err := uc.deviceInTenant(c, guid) if err != nil { return dto.BootCapabilities{}, err } @@ -40,7 +40,7 @@ func (uc *UseCase) GetRemoteEraseCapabilities(c context.Context, guid string) (d } func (uc *UseCase) SetRemoteEraseOptions(c context.Context, guid string, req dto.RemoteEraseRequest) error { - item, err := uc.repo.GetByID(c, guid, "") + item, err := uc.deviceInTenant(c, guid) if err != nil { return err } diff --git a/internal/usecase/devices/certificates.go b/internal/usecase/devices/certificates.go index aaa72334a..f688af893 100644 --- a/internal/usecase/devices/certificates.go +++ b/internal/usecase/devices/certificates.go @@ -160,7 +160,7 @@ func processCertificates(contextItems []credential.CredentialContext, response w } func (uc *UseCase) GetCertificates(c context.Context, guid string) (dto.SecuritySettings, error) { - item, err := uc.repo.GetByID(c, guid, "") + item, err := uc.deviceInTenant(c, guid) if err != nil { return dto.SecuritySettings{}, err } @@ -257,7 +257,7 @@ func KeysToDTO(r *publicprivate.RefinedPullResponse) dto.KeyPullResponse { } func (uc *UseCase) GetDeviceCertificate(c context.Context, guid string) (dto.Certificate, error) { - item, err := uc.repo.GetByID(c, guid, "") + item, err := uc.deviceInTenant(c, guid) if err != nil { return dto.Certificate{}, err } @@ -330,7 +330,7 @@ func populateCertificateDTO(cert *x509.Certificate) dto.Certificate { func (uc *UseCase) AddCertificate(c context.Context, guid string, certInfo dto.CertInfo) (handle string, err error) { var certData []byte - item, err := uc.repo.GetByID(c, guid, "") + item, err := uc.deviceInTenant(c, guid) if err != nil { return "", err } @@ -397,7 +397,7 @@ func (uc *UseCase) AddCertificate(c context.Context, guid string, certInfo dto.C } func (uc *UseCase) DeleteCertificate(c context.Context, guid, instanceID string) error { - item, err := uc.repo.GetByID(c, guid, "") + item, err := uc.deviceInTenant(c, guid) if err != nil { return err } diff --git a/internal/usecase/devices/connections.go b/internal/usecase/devices/connections.go index 43ecf5100..89e89aced 100644 --- a/internal/usecase/devices/connections.go +++ b/internal/usecase/devices/connections.go @@ -9,7 +9,7 @@ import ( ) func (uc *UseCase) GetTLSSettingData(c context.Context, guid string) ([]dto.SettingDataResponse, error) { - item, err := uc.repo.GetByID(c, guid, "") + item, err := uc.deviceInTenant(c, guid) if err != nil { return nil, err } diff --git a/internal/usecase/devices/consent.go b/internal/usecase/devices/consent.go index d4f3caffa..6f544bd70 100644 --- a/internal/usecase/devices/consent.go +++ b/internal/usecase/devices/consent.go @@ -8,7 +8,7 @@ import ( ) func (uc *UseCase) CancelUserConsent(c context.Context, guid string) (dto.UserConsentMessage, error) { - item, err := uc.repo.GetByID(c, guid, "") + item, err := uc.deviceInTenant(c, guid) if err != nil { return dto.UserConsentMessage{}, err } @@ -48,7 +48,7 @@ func (uc *UseCase) CancelUserConsent(c context.Context, guid string) (dto.UserCo } func (uc *UseCase) GetUserConsentCode(c context.Context, guid string) (dto.UserConsentMessage, error) { - item, err := uc.repo.GetByID(c, guid, "") + item, err := uc.deviceInTenant(c, guid) if err != nil { return dto.UserConsentMessage{}, err } @@ -88,7 +88,7 @@ func (uc *UseCase) GetUserConsentCode(c context.Context, guid string) (dto.UserC } func (uc *UseCase) SendConsentCode(c context.Context, userConsent dto.UserConsentCode, guid string) (dto.UserConsentMessage, error) { - item, err := uc.repo.GetByID(c, guid, "") + item, err := uc.deviceInTenant(c, guid) if err != nil { return dto.UserConsentMessage{}, err } diff --git a/internal/usecase/devices/features.go b/internal/usecase/devices/features.go index 65e517743..313ccb0fc 100644 --- a/internal/usecase/devices/features.go +++ b/internal/usecase/devices/features.go @@ -54,7 +54,7 @@ type BootConfiguration struct { } func (uc *UseCase) GetFeatures(c context.Context, guid string) (settingsResults dto.Features, settingsResultsV2 dtov2.Features, err error) { - item, err := uc.repo.GetByID(c, guid, "") + item, err := uc.deviceInTenant(c, guid) if err != nil { return dto.Features{}, dtov2.Features{}, err } @@ -223,7 +223,7 @@ func FindBootSettingInstances(bootSourceSettings []cimBoot.BootSourceSetting) dt } func (uc *UseCase) SetFeatures(c context.Context, guid string, features dto.Features) (settingsResults dto.Features, settingsResultsV2 dtov2.Features, err error) { - item, err := uc.repo.GetByID(c, guid, "") + item, err := uc.deviceInTenant(c, guid) if err != nil { return settingsResults, settingsResultsV2, err } diff --git a/internal/usecase/devices/features_test.go b/internal/usecase/devices/features_test.go index f052510f0..974e183bf 100644 --- a/internal/usecase/devices/features_test.go +++ b/internal/usecase/devices/features_test.go @@ -18,6 +18,7 @@ import ( "github.com/device-management-toolkit/console/internal/entity/dto/v1" dtov2 "github.com/device-management-toolkit/console/internal/entity/dto/v2" "github.com/device-management-toolkit/console/internal/mocks" + "github.com/device-management-toolkit/console/internal/tenant" devices "github.com/device-management-toolkit/console/internal/usecase/devices" ) @@ -2945,6 +2946,18 @@ func TestSetFeatures(t *testing.T) { } } +func TestSetFeaturesRejectsDeviceOutsideRequestTenant(t *testing.T) { + t.Parallel() + + useCase, _, _, repo := initInfoTest(t) + ctx := tenant.WithContext(context.Background(), "tenant-b") + + repo.EXPECT().GetByID(ctx, "tenant-a-device", "tenant-b").Return(nil, nil) + + _, _, err := useCase.SetFeatures(ctx, "tenant-a-device", dto.Features{}) + require.IsType(t, devices.ErrNotFound, err) +} + func TestFindBootSettingInstances(t *testing.T) { t.Parallel() diff --git a/internal/usecase/devices/info.go b/internal/usecase/devices/info.go index 69e5b1a55..79f148734 100644 --- a/internal/usecase/devices/info.go +++ b/internal/usecase/devices/info.go @@ -13,7 +13,7 @@ import ( ) func (uc *UseCase) GetVersion(c context.Context, guid string) (v1 dto.Version, v2 dtov2.Version, err error) { - item, err := uc.repo.GetByID(c, guid, "") + item, err := uc.deviceInTenant(c, guid) if err != nil { return v1, v2, err } @@ -64,7 +64,7 @@ func (uc *UseCase) GetVersion(c context.Context, guid string) (v1 dto.Version, v } func (uc *UseCase) GetHardwareInfo(c context.Context, guid string) (dto.HardwareInfo, error) { - item, err := uc.repo.GetByID(c, guid, "") + item, err := uc.deviceInTenant(c, guid) if err != nil { return dto.HardwareInfo{}, err } @@ -113,7 +113,7 @@ func (uc *UseCase) hardwareInfoToDTO(hw wsmanAPI.HWResults) dto.HardwareInfo { } func (uc *UseCase) GetDiskInfo(c context.Context, guid string) (dto.DiskInfo, error) { - item, err := uc.repo.GetByID(c, guid, "") + item, err := uc.deviceInTenant(c, guid) if err != nil { return dto.DiskInfo{}, err } @@ -143,7 +143,7 @@ func (uc *UseCase) diskInfoToDTO(diskInfo wsmanAPI.DiskResults) dto.DiskInfo { } func (uc *UseCase) GetAuditLog(c context.Context, startIndex int, guid string) (dto.AuditLog, error) { - item, err := uc.repo.GetByID(c, guid, "") + item, err := uc.deviceInTenant(c, guid) if err != nil { return dto.AuditLog{}, err } @@ -170,7 +170,7 @@ func (uc *UseCase) GetAuditLog(c context.Context, startIndex int, guid string) ( } func (uc *UseCase) GetEventLog(c context.Context, startIndex, maxReadRecords int, guid string) (dto.EventLogs, error) { - item, err := uc.repo.GetByID(c, guid, "") + item, err := uc.deviceInTenant(c, guid) if err != nil { return dto.EventLogs{}, err } @@ -224,7 +224,7 @@ func (uc *UseCase) GetEventLog(c context.Context, startIndex, maxReadRecords int } func (uc *UseCase) GetGeneralSettings(c context.Context, guid string) (dto.GeneralSettings, error) { - item, err := uc.repo.GetByID(c, guid, "") + item, err := uc.deviceInTenant(c, guid) if err != nil { return dto.GeneralSettings{}, err } diff --git a/internal/usecase/devices/interceptor.go b/internal/usecase/devices/interceptor.go index 752e00cf8..70a3f6cc7 100644 --- a/internal/usecase/devices/interceptor.go +++ b/internal/usecase/devices/interceptor.go @@ -52,7 +52,7 @@ type DeviceConnection struct { func (uc *UseCase) Redirect(c context.Context, conn *websocket.Conn, guid, mode string) error { // KVM_TIMING: Measure device lookup latency lookupStart := time.Now() - device, err := uc.repo.GetByID(c, guid, "") + device, err := uc.deviceInTenant(c, guid) RecordDeviceLookup(time.Since(lookupStart)) uc.log.Debug("KVM_TIMING: Device lookup", "duration_ms", time.Since(lookupStart).Milliseconds(), "guid", guid) @@ -137,7 +137,10 @@ func (uc *UseCase) createNewConnection(c context.Context, conn *websocket.Conn, device.Password = decryptedPassword - ctx, cancel := context.WithCancel(c) + // Preserve request values such as tenant ID, but do not inherit request + // cancellation: the HTTP handler returns immediately after upgrading the + // connection while the KVM session must remain active in its goroutines. + ctx, cancel := context.WithCancel(context.WithoutCancel(c)) now := time.Now() deviceConnection := &DeviceConnection{ Conn: conn, @@ -212,7 +215,11 @@ func (uc *UseCase) startConnectionGoroutines(c context.Context, deviceConnection } func (uc *UseCase) closeDeviceWebSocket(conn WebSocketConn, deviceConnection *DeviceConnection) { - uc.log.Debug("KVM session closed by AMT", "guid", deviceConnection.Device.GUID) + if deviceConnection.ctx.Err() != nil { + uc.log.Debug("KVM session closed after browser disconnect", "guid", deviceConnection.Device.GUID) + } else { + uc.log.Debug("KVM session closed by AMT", "guid", deviceConnection.Device.GUID) + } if conn != nil { _ = conn.WriteMessage( @@ -264,6 +271,8 @@ func (uc *UseCase) ListenToDevice(deviceConnection *DeviceConnection) { uc.observeDeviceReceive(deviceConnection, time.Since(recvStart)) if err != nil { + uc.log.Debug("KVM device listener stopped", "guid", deviceConnection.Device.GUID, "error", err) + break } @@ -337,6 +346,8 @@ func (uc *UseCase) ListenToBrowser(deviceConnection *DeviceConnection) { uc.observeBrowserRead(deviceConnection, time.Since(readStart)) if err != nil { + uc.log.Debug("KVM browser listener stopped", "guid", deviceConnection.Device.GUID, "error", err) + if websocket.IsUnexpectedCloseError(err, websocket.CloseGoingAway, websocket.CloseAbnormalClosure) { _ = fmt.Errorf("interceptor - listenToBrowser - websocket closed unexpectedly (reading from browser): %w", err) } diff --git a/internal/usecase/devices/interceptor_private_test.go b/internal/usecase/devices/interceptor_private_test.go index 06381b812..e889a5bf6 100644 --- a/internal/usecase/devices/interceptor_private_test.go +++ b/internal/usecase/devices/interceptor_private_test.go @@ -17,6 +17,7 @@ import ( "github.com/device-management-toolkit/go-wsman-messages/v2/pkg/wsman/client" "github.com/device-management-toolkit/console/internal/entity" + crypto "github.com/device-management-toolkit/console/internal/mocks/crypto" "github.com/device-management-toolkit/console/pkg/logger" ) @@ -923,6 +924,32 @@ func TestListenToDeviceClosesWebSocketOnAMTDisconnect(t *testing.T) { require.True(t, spy.closeCalled, "expected Close() to be called on browser WebSocket") } +func TestCreateNewConnectionSurvivesRequestCancellation(t *testing.T) { + t.Parallel() + + device := &entity.Device{ + GUID: "test-guid", + Username: "admin", + Password: "password", + } + + uc := &UseCase{ + redirection: &spyRedirection{}, + safeRequirements: crypto.MockCrypto{}, + redirConnections: make(map[string]*DeviceConnection), + } + requestCtx, requestCancel := context.WithCancel(context.Background()) + requestCancel() + + connection, err := uc.createNewConnection(requestCtx, &websocket.Conn{}, "test-guid-kvm", device) + require.NoError(t, err) + t.Cleanup(func() { + connection.cancel() + connection.healthTicker.Stop() + }) + require.NoError(t, connection.ctx.Err()) +} + func entityDevice() entity.Device { return entity.Device{ GUID: "test-guid", diff --git a/internal/usecase/devices/interfaces.go b/internal/usecase/devices/interfaces.go index 931f49c43..e59ca9385 100644 --- a/internal/usecase/devices/interfaces.go +++ b/internal/usecase/devices/interfaces.go @@ -40,6 +40,7 @@ type ( GetCount(context.Context, string) (int, error) Get(ctx context.Context, top, skip int, tenantID string) ([]entity.Device, error) GetByID(ctx context.Context, guid, tenantID string) (*entity.Device, error) + GetByGUID(ctx context.Context, guid string) (*entity.Device, error) GetDistinctTags(ctx context.Context, tenantID string) ([]string, error) GetByTags(ctx context.Context, tags []string, method string, limit, offset int, tenantID string) ([]entity.Device, error) Delete(ctx context.Context, guid, tenantID string) (bool, error) @@ -54,6 +55,9 @@ type ( GetCount(context.Context, string) (int, error) Get(ctx context.Context, top, skip int, tenantID string) ([]dto.Device, error) GetByID(ctx context.Context, guid, tenantID string, includeSecrets bool) (*dto.Device, error) + // GetByGUID resolves a device without a tenant filter. CIRA devices + // authenticate by GUID and cannot present a tenant. + GetByGUID(ctx context.Context, guid string, includeSecrets bool) (*dto.Device, error) UpdateConnectionStatus(ctx context.Context, guid string, status bool) error UpdateLastSeen(ctx context.Context, guid string) error GetDistinctTags(ctx context.Context, tenantID string) ([]string, error) diff --git a/internal/usecase/devices/kvm.go b/internal/usecase/devices/kvm.go index 4e5d3204d..64bfbbed6 100644 --- a/internal/usecase/devices/kvm.go +++ b/internal/usecase/devices/kvm.go @@ -13,7 +13,7 @@ var ErrNotSupportedUseCase = NotSupportedError{Console: consoleerrors.CreateCons // GetKVMScreenSettings returns IPS_ScreenSettingData for the device. func (uc *UseCase) GetKVMScreenSettings(c context.Context, guid string) (dto.KVMScreenSettings, error) { - item, err := uc.repo.GetByID(c, guid, "") + item, err := uc.deviceInTenant(c, guid) if err != nil { return dto.KVMScreenSettings{}, err } @@ -74,7 +74,7 @@ func (uc *UseCase) GetKVMScreenSettings(c context.Context, guid string) (dto.KVM // SetKVMScreenSettings updates IPS_ScreenSettingData; currently not supported via wsman lib // We accept payload but return NotSupported to preserve API contract for future. func (uc *UseCase) SetKVMScreenSettings(c context.Context, guid string, reqData dto.KVMScreenSettingsRequest) (dto.KVMScreenSettings, error) { - item, err := uc.repo.GetByID(c, guid, "") + item, err := uc.deviceInTenant(c, guid) if err != nil { return dto.KVMScreenSettings{}, err } diff --git a/internal/usecase/devices/linkpreference.go b/internal/usecase/devices/linkpreference.go index c5f8af926..d4cd42d58 100644 --- a/internal/usecase/devices/linkpreference.go +++ b/internal/usecase/devices/linkpreference.go @@ -8,7 +8,7 @@ import ( // SetLinkPreference sets the link preference (ME or Host) on a device's WiFi interface. func (uc *UseCase) SetLinkPreference(c context.Context, guid string, req dto.LinkPreferenceRequest) (dto.LinkPreferenceResponse, error) { - item, err := uc.repo.GetByID(c, guid, "") + item, err := uc.deviceInTenant(c, guid) if err != nil { return dto.LinkPreferenceResponse{}, err } diff --git a/internal/usecase/devices/network.go b/internal/usecase/devices/network.go index fe8c53cc6..cc6e12b25 100644 --- a/internal/usecase/devices/network.go +++ b/internal/usecase/devices/network.go @@ -17,7 +17,7 @@ const ( ) func (uc *UseCase) GetNetworkSettings(c context.Context, guid string) (dto.NetworkSettings, error) { - item, err := uc.repo.GetByID(c, guid, "") + item, err := uc.deviceInTenant(c, guid) if err != nil { return dto.NetworkSettings{}, err } @@ -188,7 +188,7 @@ func (uc *UseCase) PatchWiredNetworkSettings(c context.Context, guid string, req return err } - item, err := uc.repo.GetByID(c, guid, "") + item, err := uc.deviceInTenant(c, guid) if err != nil { return err } diff --git a/internal/usecase/devices/power.go b/internal/usecase/devices/power.go index 75735365a..0913a35fb 100644 --- a/internal/usecase/devices/power.go +++ b/internal/usecase/devices/power.go @@ -44,7 +44,7 @@ var ( ) func (uc *UseCase) SendPowerAction(c context.Context, guid string, action int) (power.PowerActionResponse, error) { - item, err := uc.repo.GetByID(c, guid, "") + item, err := uc.deviceInTenant(c, guid) if err != nil { return power.PowerActionResponse{}, err } @@ -122,7 +122,7 @@ func ensureFullPowerBeforeReset(device wsman.Management) (power.PowerActionRespo } func (uc *UseCase) GetPowerState(c context.Context, guid string) (dto.PowerState, error) { - item, err := uc.repo.GetByID(c, guid, "") + item, err := uc.deviceInTenant(c, guid) if err != nil { return dto.PowerState{}, err } @@ -160,7 +160,7 @@ func (uc *UseCase) GetPowerState(c context.Context, guid string) (dto.PowerState } func (uc *UseCase) GetPowerCapabilities(c context.Context, guid string) (dto.PowerCapabilities, error) { - item, err := uc.repo.GetByID(c, guid, "") + item, err := uc.deviceInTenant(c, guid) if err != nil { return dto.PowerCapabilities{}, err } @@ -280,7 +280,7 @@ func buildBootSettingData(bootData boot.BootSettingDataResponse, bootSetting dto } func (uc *UseCase) SetBootOptions(c context.Context, guid string, bootSetting dto.BootSetting) (power.PowerActionResponse, error) { - item, err := uc.repo.GetByID(c, guid, "") + item, err := uc.deviceInTenant(c, guid) if err != nil { return power.PowerActionResponse{}, err } @@ -593,7 +593,7 @@ func parseVersion(version []software.SoftwareIdentity) (int, error) { } func (uc *UseCase) GetBootSourceSetting(c context.Context, guid string) ([]dto.BootSources, error) { - item, err := uc.repo.GetByID(c, guid, "") + item, err := uc.deviceInTenant(c, guid) if err != nil { return nil, err } diff --git a/internal/usecase/devices/redirection.go b/internal/usecase/devices/redirection.go index 60c7ce51a..9d7607ac9 100644 --- a/internal/usecase/devices/redirection.go +++ b/internal/usecase/devices/redirection.go @@ -8,6 +8,7 @@ import ( "github.com/device-management-toolkit/go-wsman-messages/v2/pkg/wsman/client" "github.com/device-management-toolkit/console/internal/entity" + "github.com/device-management-toolkit/console/internal/tenant" wsmanAPI "github.com/device-management-toolkit/console/internal/usecase/devices/wsman" ) @@ -15,7 +16,7 @@ type Redirector struct { SafeRequirements security.Cryptor } -func (g *Redirector) SetupWsmanClient(_ context.Context, device entity.Device, isRedirection, logAMTMessages bool) (wsman.Messages, error) { +func (g *Redirector) SetupWsmanClient(ctx context.Context, device entity.Device, isRedirection, logAMTMessages bool) (wsman.Messages, error) { // CIRA device: route redirection through the APF tunnel if isRedirection && device.MPSUsername != "" { connection := wsmanAPI.GetConnectionEntry(device.GUID) @@ -23,6 +24,12 @@ func (g *Redirector) SetupWsmanClient(_ context.Context, device entity.Device, i return wsman.Messages{}, wsmanAPI.ErrCIRADeviceNotConnected } + // The CIRA socket is registered after device authentication. Verify the + // caller owns that socket before forwarding redirection traffic to AMT. + if connection.TenantID != tenant.FromContext(ctx) { + return wsman.Messages{}, wsmanAPI.ErrCIRATenantMismatch + } + return wsman.NewCIRARedirectionMessages(connection), nil } diff --git a/internal/usecase/devices/redirection_test.go b/internal/usecase/devices/redirection_test.go index 8c305edac..ef61394c9 100644 --- a/internal/usecase/devices/redirection_test.go +++ b/internal/usecase/devices/redirection_test.go @@ -13,6 +13,7 @@ import ( "github.com/device-management-toolkit/console/internal/entity" "github.com/device-management-toolkit/console/internal/mocks" + "github.com/device-management-toolkit/console/internal/tenant" devices "github.com/device-management-toolkit/console/internal/usecase/devices" wsmanAPI "github.com/device-management-toolkit/console/internal/usecase/devices/wsman" ) @@ -114,6 +115,27 @@ func TestSetupWsmanClient_CIRARedirection(t *testing.T) { require.NotNil(t, msgs.Client) }) + t.Run("rejects a CIRA socket owned by another tenant", func(t *testing.T) { + t.Parallel() + + guid := "cira-device-tenant-mismatch" + wsmanAPI.SetConnectionEntry(guid, &wsmanAPI.ConnectionEntry{ + IsCIRA: true, + TenantID: "tenant-a", + }) + t.Cleanup(func() { wsmanAPI.RemoveConnection(guid) }) + + device := entity.Device{ + GUID: guid, + MPSUsername: "admin", + } + redirector := &devices.Redirector{SafeRequirements: mocks.MockCrypto{}} + ctx := tenant.WithContext(context.Background(), "tenant-b") + + _, err := redirector.SetupWsmanClient(ctx, device, true, false) + require.ErrorIs(t, err, wsmanAPI.ErrCIRATenantMismatch) + }) + t.Run("non-CIRA device skips CIRA path", func(t *testing.T) { t.Parallel() diff --git a/internal/usecase/devices/repo.go b/internal/usecase/devices/repo.go index 28e067d52..9509c4bbf 100644 --- a/internal/usecase/devices/repo.go +++ b/internal/usecase/devices/repo.go @@ -100,6 +100,32 @@ func (uc *UseCase) GetByID(ctx context.Context, guid, tenantID string, includeSe return d2, nil } +// GetByGUID resolves a device without a tenant filter, for callers that +// identify themselves by GUID alone and cannot supply one. +func (uc *UseCase) GetByGUID(ctx context.Context, guid string, includeSecrets bool) (*dto.Device, error) { + data, err := uc.repo.GetByGUID(ctx, strings.ToLower(guid)) + if err != nil { + return nil, ErrDatabase.Wrap("GetByGUID", "uc.repo.GetByGUID", err) + } + + if data == nil || data.GUID == "" { + return nil, ErrNotFound + } + + d2, err := uc.entityToDTO(data) + if err != nil { + return nil, err + } + + if includeSecrets { + if err := uc.decryptSecrets(d2, data); err != nil { + return nil, err + } + } + + return d2, nil +} + func (uc *UseCase) decryptSecrets(d2 *dto.Device, data *entity.Device) error { var err error diff --git a/internal/usecase/devices/tenant.go b/internal/usecase/devices/tenant.go new file mode 100644 index 000000000..dc88ed086 --- /dev/null +++ b/internal/usecase/devices/tenant.go @@ -0,0 +1,14 @@ +package devices + +import ( + "context" + + "github.com/device-management-toolkit/console/internal/entity" + "github.com/device-management-toolkit/console/internal/tenant" +) + +// deviceInTenant is the single tenant-scoped device lookup used by every +// management call, so scoping cannot be forgotten per-operation. +func (uc *UseCase) deviceInTenant(ctx context.Context, guid string) (*entity.Device, error) { + return uc.repo.GetByID(ctx, guid, tenant.FromContext(ctx)) +} diff --git a/internal/usecase/devices/tenant_test.go b/internal/usecase/devices/tenant_test.go new file mode 100644 index 000000000..148f8dd3d --- /dev/null +++ b/internal/usecase/devices/tenant_test.go @@ -0,0 +1,38 @@ +package devices_test + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + + "github.com/device-management-toolkit/go-wsman-messages/v2/pkg/wsman/cim/power" + + "github.com/device-management-toolkit/console/internal/entity" + "github.com/device-management-toolkit/console/internal/mocks" + "github.com/device-management-toolkit/console/internal/tenant" + devices "github.com/device-management-toolkit/console/internal/usecase/devices" + "github.com/device-management-toolkit/console/pkg/logger" +) + +func TestSendPowerActionUsesRequestTenant(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + repo := mocks.NewMockDeviceManagementRepository(ctrl) + wsman := mocks.NewMockWSMAN(ctrl) + management := mocks.NewMockManagement(ctrl) + + wsman.EXPECT().Worker().AnyTimes() + uc := devices.New(repo, wsman, mocks.NewMockRedirection(ctrl), logger.New("error"), mocks.MockCrypto{}) + ctx := tenant.WithContext(context.Background(), "tenant-a") + device := &entity.Device{GUID: "device-guid", TenantID: "tenant-a", Password: "encrypted"} + + repo.EXPECT().GetByID(ctx, device.GUID, "tenant-a").Return(device, nil) + wsman.EXPECT().SetupWsmanClient(ctx, gomock.Any(), false, true).Return(management, nil) + management.EXPECT().SendPowerAction(0).Return(power.PowerActionResponse{}, nil) + + _, err := uc.SendPowerAction(ctx, device.GUID, 0) + require.NoError(t, err) +} diff --git a/internal/usecase/devices/wifiprofile.go b/internal/usecase/devices/wifiprofile.go index ce80844b1..958403e46 100644 --- a/internal/usecase/devices/wifiprofile.go +++ b/internal/usecase/devices/wifiprofile.go @@ -182,7 +182,7 @@ func (uc *UseCase) UpdateWirelessProfile(c context.Context, guid string, profile } func (uc *UseCase) setupWirelessProfileManagement(c context.Context, guid string) (wsman.Management, error) { - item, err := uc.repo.GetByID(c, guid, "") + item, err := uc.deviceInTenant(c, guid) if err != nil { return nil, err } diff --git a/internal/usecase/devices/wifistate.go b/internal/usecase/devices/wifistate.go index 7b00bbedc..664b59a2c 100644 --- a/internal/usecase/devices/wifistate.go +++ b/internal/usecase/devices/wifistate.go @@ -11,7 +11,7 @@ func (uc *UseCase) RequestWirelessStateChange(c context.Context, guid string, re return 0, ErrValidationUseCase.Wrap("RequestWirelessStateChange", "validate requested state", "state must be one of 3, 32768, 32769") } - item, err := uc.repo.GetByID(c, guid, "") + item, err := uc.deviceInTenant(c, guid) if err != nil { return 0, err } @@ -51,7 +51,7 @@ func isWirelessRequestedStateSupported(requestedState wifi.RequestedState) bool } func (uc *UseCase) GetWirelessState(c context.Context, guid string) (wifi.EnabledState, error) { - item, err := uc.repo.GetByID(c, guid, "") + item, err := uc.deviceInTenant(c, guid) if err != nil { return 0, err } diff --git a/internal/usecase/devices/wsman/message.go b/internal/usecase/devices/wsman/message.go index ad0f4ddc2..88d188816 100644 --- a/internal/usecase/devices/wsman/message.go +++ b/internal/usecase/devices/wsman/message.go @@ -83,6 +83,8 @@ var ( // ErrCIRADeviceNotConnected is returned when a CIRA device is not connected or not found. ErrCIRADeviceNotConnected = errors.New("CIRA device not connected/not found") + // ErrCIRATenantMismatch is returned when a caller's tenant does not own the CIRA socket. + ErrCIRATenantMismatch = errors.New("CIRA device belongs to a different tenant") // ErrNoWiFiPort is returned when no WiFi interface is found on the device. ErrNoWiFiPort = errors.New("no WiFi interface found (InstanceID == Intel(r) AMT Ethernet Port Settings 1)") @@ -92,8 +94,11 @@ var ( type ConnectionEntry struct { WsmanMessages wsman.Messages IsCIRA bool - Conny net.Conn - Timer *time.Timer + // TenantID is learned from the device row during APF auth, so REST callers + // can be checked against the tenant that actually owns the socket. + TenantID string + Conny net.Conn + Timer *time.Timer // APF channel management for CIRA connections (uses types from go-wsman-messages) APFChannelStore *client.APFChannelStore @@ -169,6 +174,14 @@ func (g GoWSMANMessages) SetupWsmanClient(ctx context.Context, device entity.Dev return } + // Defense in depth behind the repository's tenant filter: the socket is + // owned by the tenant that authenticated it, whatever the caller asked for. + if connection.TenantID != device.TenantID { + errChan <- ErrCIRATenantMismatch + + return + } + cp := client.Parameters{ Target: device.GUID, // Use GUID as Host for CIRA connections IsRedirection: false, diff --git a/internal/usecase/devices/wsman/message_test.go b/internal/usecase/devices/wsman/message_test.go index 7a90ac67a..f0cbc80af 100644 --- a/internal/usecase/devices/wsman/message_test.go +++ b/internal/usecase/devices/wsman/message_test.go @@ -173,3 +173,63 @@ func TestDestroyWsmanClient_MissingEntryIsNoop(t *testing.T) { // Should not panic when the entry is absent. g.DestroyWsmanClient(dto.Device{GUID: "destroy-missing-entry"}) } + +// The CIRA socket belongs to the tenant that authenticated it, so a caller from +// another tenant must not be handed the connection. Mirrors MPS ciraMiddleware. +func TestSetupWsmanClient_CIRATenantMismatchIsRejected(t *testing.T) { //nolint:paralleltest // mutates package-level state (requestQueue, queueTickTime, connections) + origTick := queueTickTime + queueTickTime = 1 * time.Millisecond + + t.Cleanup(func() { queueTickTime = origTick }) + + guid := "cira-tenant-mismatch-guid" + + t.Cleanup(func() { RemoveConnection(guid) }) + + SetConnectionEntry(guid, &ConnectionEntry{ + IsCIRA: true, + TenantID: "acme-corp", + Timer: time.AfterFunc(time.Hour, func() {}), + }) + + g := NewGoWSMANMessages(logger.New("error"), passthroughCryptor{}) + + stopWorker := make(chan struct{}) + workerDone := make(chan struct{}) + + go func() { + defer close(workerDone) + + for { + select { + case request := <-requestQueue: + request() + case <-stopWorker: + return + } + } + }() + + t.Cleanup(func() { + close(stopWorker) + <-workerDone + }) + + mismatched := entity.Device{ + GUID: guid, + MPSUsername: "mpsuser", + TenantID: "globex", + } + + _, err := g.SetupWsmanClient(context.Background(), mismatched, false, false) + require.ErrorIs(t, err, ErrCIRATenantMismatch) + + owner := entity.Device{ + GUID: guid, + MPSUsername: "mpsuser", + TenantID: "acme-corp", + } + + _, err = g.SetupWsmanClient(context.Background(), owner, false, false) + require.NoError(t, err) +} diff --git a/internal/usecase/nosqldb/mongo/device.go b/internal/usecase/nosqldb/mongo/device.go index c1e5a45f3..7435f266b 100644 --- a/internal/usecase/nosqldb/mongo/device.go +++ b/internal/usecase/nosqldb/mongo/device.go @@ -48,6 +48,8 @@ type deviceUpdateDocument struct { Set deviceUpdateFields `bson:"$set"` } +const maxGUIDMatches = 2 + var _ devices.Repository = (*DeviceRepo)(nil) func NewDeviceRepo(db *mongo.Database) *DeviceRepo { @@ -120,6 +122,32 @@ func (r *DeviceRepo) GetByID(ctx context.Context, guid, tenantID string) (*entit return &d, nil } +func (r *DeviceRepo) GetByGUID(ctx context.Context, guid string) (*entity.Device, error) { + if !identifierRegex.MatchString(guid) { + return nil, nil + } + + cur, err := r.col.Find(ctx, bson.M{fieldGUID: guid}, options.Find().SetLimit(maxGUIDMatches)) + if err != nil { + return nil, errDeviceDatabase.Wrap("GetByGUID", "Find", err) + } + defer cur.Close(ctx) + + deviceMatches := make([]entity.Device, 0, maxGUIDMatches) + if err := cur.All(ctx, &deviceMatches); err != nil { + return nil, errDeviceDatabase.Wrap("GetByGUID", "Cursor.All", err) + } + + switch len(deviceMatches) { + case 0: + return nil, nil + case 1: + return &deviceMatches[0], nil + default: + return nil, errDeviceNotUnique.Wrap("multiple devices found for guid") + } +} + func (r *DeviceRepo) GetDistinctTags(ctx context.Context, tenantID string) ([]string, error) { if tenantID != "" && !identifierRegex.MatchString(tenantID) { return []string{}, nil @@ -227,7 +255,8 @@ func (r *DeviceRepo) Update(ctx context.Context, d *entity.Device) (bool, error) } // Explicit field list mirrors sqldb/device.go:Update so a new field must be wired in intentionally. - res, err := r.col.UpdateOne(ctx, + res, err := r.col.UpdateOne( + ctx, deviceFilter{GUID: d.GUID, TenantID: d.TenantID}, deviceUpdateDocument{Set: deviceUpdateFields{ GUID: d.GUID, @@ -283,7 +312,8 @@ func (r *DeviceRepo) UpdateLastSeen(ctx context.Context, guid string) error { return errDeviceDatabase.Wrap("UpdateLastSeen", "validate", nil) } - _, err := r.col.UpdateOne(ctx, + _, err := r.col.UpdateOne( + ctx, bson.M{fieldGUID: guid}, bson.M{opSet: bson.M{"lastseen": time.Now()}}, ) diff --git a/internal/usecase/nosqldb/mongo/device_test.go b/internal/usecase/nosqldb/mongo/device_test.go index 8fedd41ee..0cb88b977 100644 --- a/internal/usecase/nosqldb/mongo/device_test.go +++ b/internal/usecase/nosqldb/mongo/device_test.go @@ -67,6 +67,74 @@ func TestDeviceRepo_GetByID_NotFound(t *testing.T) { require.Nil(t, got) } +func TestDeviceRepo_GetByGUID_FoundInNonDefaultTenant(t *testing.T) { + t.Parallel() + + db, md := newMockedDB(t) + + md.AddResponses(findResponse( + "testdb."+mongo.CollectionDevices, + bson.D{ + {Key: "guid", Value: "g1"}, + {Key: "friendlyname", Value: "lab-host-1"}, + {Key: "tenantid", Value: "acme-corp"}, + }, + )) + + repo := mongo.NewDeviceRepo(db) + + got, err := repo.GetByGUID(context.Background(), "g1") + require.NoError(t, err) + require.NotNil(t, got) + require.Equal(t, "g1", got.GUID) + require.Equal(t, "acme-corp", got.TenantID) +} + +func TestDeviceRepo_GetByGUID_NotFound(t *testing.T) { + t.Parallel() + + db, md := newMockedDB(t) + + md.AddResponses(findResponse("testdb." + mongo.CollectionDevices)) + + repo := mongo.NewDeviceRepo(db) + + got, err := repo.GetByGUID(context.Background(), "ghost") + require.NoError(t, err) + require.Nil(t, got) +} + +func TestDeviceRepo_GetByGUID_AmbiguousAcrossTenantsReturnsNotUniqueError(t *testing.T) { + t.Parallel() + + db, md := newMockedDB(t) + md.AddResponses(findResponse( + "testdb."+mongo.CollectionDevices, + bson.D{{Key: "guid", Value: "g1"}, {Key: "tenantid", Value: "tenant-a"}}, + bson.D{{Key: "guid", Value: "g1"}, {Key: "tenantid", Value: "tenant-b"}}, + )) + + repo := mongo.NewDeviceRepo(db) + + got, err := repo.GetByGUID(context.Background(), "g1") + require.Nil(t, got) + + var notUnique repoerrors.NotUniqueError + require.ErrorAs(t, err, ¬Unique) +} + +func TestDeviceRepo_GetByGUID_RejectsMalformedGUID(t *testing.T) { + t.Parallel() + + db, _ := newMockedDB(t) + + repo := mongo.NewDeviceRepo(db) + + got, err := repo.GetByGUID(context.Background(), "bad guid") + require.NoError(t, err) + require.Nil(t, got) +} + func TestDeviceRepo_Get(t *testing.T) { t.Parallel() diff --git a/internal/usecase/sqldb/device.go b/internal/usecase/sqldb/device.go index 7766d3523..e6cf283d6 100644 --- a/internal/usecase/sqldb/device.go +++ b/internal/usecase/sqldb/device.go @@ -23,6 +23,7 @@ type DeviceRepo struct { var ( ErrDeviceDatabase = repoerrors.DatabaseError{Console: consoleerrors.CreateConsoleError("DeviceRepo")} ErrDeviceNotUnique = repoerrors.NotUniqueError{Console: consoleerrors.CreateConsoleError("DeviceRepo")} + errDuplicateDevice = errors.New("duplicate device found for guid and tenant") ) // New -. @@ -166,24 +167,87 @@ func (r *DeviceRepo) GetByID(_ context.Context, guid, tenantID string) (*entity. return nil, ErrDeviceDatabase.Wrap("Get", "rows.Err", rows.Err()) } - devices := make([]*entity.Device, 0) + if !rows.Next() { + if err := rows.Err(); err != nil { + return nil, ErrDeviceDatabase.Wrap("Get", "rows.Err", err) + } - for rows.Next() { + return nil, nil + } + + d := &entity.Device{} + + err = rows.Scan(&d.GUID, &d.Hostname, &d.Tags, &d.MPSInstance, &d.ConnectionStatus, &d.MPSUsername, &d.TenantID, &d.FriendlyName, &d.DNSSuffix, &d.DeviceInfo, &d.Username, &d.Password, &d.MPSPassword, &d.MEBXPassword, &d.UseTLS, &d.AllowSelfSigned, &d.CertHash) + if err != nil { + return d, ErrDeviceDatabase.Wrap("Get", "rows.Scan: ", err) + } + + if rows.Next() { + return nil, ErrDeviceNotUnique.Wrap(errDuplicateDevice.Error()) + } + + if err := rows.Err(); err != nil { + return nil, ErrDeviceDatabase.Wrap("Get", "rows.Err", err) + } + + return d, nil +} + +func (r *DeviceRepo) GetByGUID(ctx context.Context, guid string) (*entity.Device, error) { + sqlQuery, _, err := r.Builder. + Select( + "guid", + "hostname", + "tags", + "mpsinstance", + "connectionstatus", + "mpsusername", + "tenantid", + "friendlyname", + "dnssuffix", + "deviceinfo", + "username", + "password", + "mpspassword", + "mebxpassword", + "usetls", + "allowselfsigned", + "certhash", + ). + From("devices"). + Where("guid = ?"). + ToSql() + if err != nil { + return nil, ErrDeviceDatabase.Wrap("GetByGUID", "r.Builder: ", err) + } + + rows, err := r.Pool.QueryContext(ctx, sqlQuery, guid) + if err != nil { + return nil, ErrDeviceDatabase.Wrap("GetByGUID", "r.Pool.Query", err) + } + + defer rows.Close() + + if rows.Err() != nil { + return nil, ErrDeviceDatabase.Wrap("GetByGUID", "rows.Err", rows.Err()) + } + + if rows.Next() { d := &entity.Device{} err = rows.Scan(&d.GUID, &d.Hostname, &d.Tags, &d.MPSInstance, &d.ConnectionStatus, &d.MPSUsername, &d.TenantID, &d.FriendlyName, &d.DNSSuffix, &d.DeviceInfo, &d.Username, &d.Password, &d.MPSPassword, &d.MEBXPassword, &d.UseTLS, &d.AllowSelfSigned, &d.CertHash) if err != nil { - return d, ErrDeviceDatabase.Wrap("Get", "rows.Scan: ", err) + return d, ErrDeviceDatabase.Wrap("GetByGUID", "rows.Scan: ", err) } - devices = append(devices, d) + return d, nil } - if len(devices) == 0 { - return nil, nil + if err := rows.Err(); err != nil { + return nil, ErrDeviceDatabase.Wrap("GetByGUID", "rows.Err", err) } - return devices[0], nil + return nil, nil } func (r *DeviceRepo) GetDistinctTags(_ context.Context, tenantID string) ([]string, error) { @@ -247,7 +311,6 @@ func (r *DeviceRepo) GetByTags(_ context.Context, tags []string, method string, // All tags must be present (simulating an 'AND' operation) for _, tag := range tags { builder = builder.Where("(',' || tags || ',') LIKE ? AND tenantId = ?", "%,"+tag+",%", tenantID) - params = append(params, "%,"+tag+",%", tenantID) //nolint:staticcheck // intentionally retained; the AND branch passes its args to Where inline } } else { // Any tag is present (simulating an 'OR' operation) diff --git a/internal/usecase/sqldb/device_test.go b/internal/usecase/sqldb/device_test.go index 67d7d806e..6f7916337 100644 --- a/internal/usecase/sqldb/device_test.go +++ b/internal/usecase/sqldb/device_test.go @@ -276,8 +276,8 @@ func TestDeviceRepo_GetByID(t *testing.T) { { name: "Successful query", setup: func(dbConn *sql.DB) { - _, err := dbConn.ExecContext(context.Background(), `INSERT INTO devices (guid, hostname, tags, mpsinstance, connectionstatus, mpsusername, tenantid, friendlyname, dnssuffix, deviceinfo, username, password, usetls, allowselfsigned, certhash) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, - "guid1", "hostname1", "tag1", "mpsinstance1", true, "mpsusername1", "tenant1", "friendlyname1", "dnssuffix1", "deviceinfo1", "username1", "password1", true, false, Certhash) + _, err := dbConn.ExecContext(context.Background(), `INSERT INTO devices (guid, hostname, tags, mpsinstance, connectionstatus, mpsusername, tenantid, friendlyname, dnssuffix, deviceinfo, username, password, mpspassword, mebxpassword, usetls, allowselfsigned, certhash) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + "guid1", "hostname1", "tag1", "mpsinstance1", true, "mpsusername1", "tenant1", "friendlyname1", "dnssuffix1", "deviceinfo1", "username1", "password1", "mpspassword1", "mebxpassword1", true, false, Certhash) require.NoError(t, err) }, guid: "guid1", @@ -295,12 +295,58 @@ func TestDeviceRepo_GetByID(t *testing.T) { DeviceInfo: "deviceinfo1", Username: "username1", Password: "password1", + MPSPassword: StringPtr("mpspassword1"), + MEBXPassword: StringPtr("mebxpassword1"), UseTLS: true, AllowSelfSigned: false, CertHash: Certhash, }, err: nil, }, + { + name: "Duplicate rows for same guid and tenant", + setup: func(dbConn *sql.DB) { + _, err := dbConn.ExecContext(context.Background(), `DROP TABLE devices`) + require.NoError(t, err) + + _, err = dbConn.ExecContext(context.Background(), ` + CREATE TABLE devices ( + guid TEXT NOT NULL, + hostname TEXT NOT NULL DEFAULT '', + tags TEXT NOT NULL DEFAULT '', + mpsinstance TEXT NOT NULL DEFAULT '', + connectionstatus BOOLEAN NOT NULL DEFAULT FALSE, + mpsusername TEXT NOT NULL DEFAULT '', + tenantid TEXT NOT NULL, + friendlyname TEXT NOT NULL DEFAULT '', + dnssuffix TEXT NOT NULL DEFAULT '', + deviceinfo TEXT NOT NULL DEFAULT '', + username TEXT NOT NULL DEFAULT '', + password TEXT NOT NULL DEFAULT '', + mpspassword TEXT, + mebxpassword TEXT, + usetls BOOLEAN NOT NULL DEFAULT FALSE, + allowselfsigned BOOLEAN NOT NULL DEFAULT FALSE, + certhash TEXT NOT NULL DEFAULT '', + lastconnected TEXT, + lastdisconnected TEXT, + lastseen TEXT + ); + `) + require.NoError(t, err) + + _, err = dbConn.ExecContext(context.Background(), `INSERT INTO devices (guid, hostname, tags, mpsinstance, connectionstatus, mpsusername, tenantid, friendlyname, dnssuffix, deviceinfo, username, password, mpspassword, mebxpassword, usetls, allowselfsigned, certhash) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + "guid1", "hostname1", "tag1", "mpsinstance1", true, "mpsusername1", "tenant1", "friendlyname1", "dnssuffix1", "deviceinfo1", "username1", "password1", "mpspassword1", "mebxpassword1", true, false, Certhash) + require.NoError(t, err) + _, err = dbConn.ExecContext(context.Background(), `INSERT INTO devices (guid, hostname, tags, mpsinstance, connectionstatus, mpsusername, tenantid, friendlyname, dnssuffix, deviceinfo, username, password, mpspassword, mebxpassword, usetls, allowselfsigned, certhash) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + "guid1", "hostname2", "tag2", "mpsinstance2", false, "mpsusername2", "tenant1", "friendlyname2", "dnssuffix2", "deviceinfo2", "username2", "password2", "mpspassword2", "mebxpassword2", false, true, Certhash) + require.NoError(t, err) + }, + guid: "guid1", + tenantID: "tenant1", + expected: nil, + err: repoerrors.NotUniqueError{}, + }, { name: "No device found", setup: func(_ *sql.DB) {}, @@ -366,6 +412,103 @@ func TestDeviceRepo_GetByID(t *testing.T) { } } +func TestDeviceRepo_GetByGUID(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + setup func(dbConn *sql.DB) + guid string + expected *entity.Device + err error + }{ + { + name: "Found regardless of tenant", + setup: func(dbConn *sql.DB) { + _, err := dbConn.ExecContext(context.Background(), `INSERT INTO devices (guid, hostname, tags, mpsinstance, connectionstatus, mpsusername, tenantid, friendlyname, dnssuffix, deviceinfo, username, password, usetls, allowselfsigned, certhash) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + "guid1", "hostname1", "tag1", "mpsinstance1", true, "mpsusername1", "acme-corp", "friendlyname1", "dnssuffix1", "deviceinfo1", "username1", "password1", true, false, Certhash) + require.NoError(t, err) + }, + guid: "guid1", + expected: &entity.Device{GUID: "guid1", TenantID: "acme-corp"}, + err: nil, + }, + { + name: "No device found", + setup: func(_ *sql.DB) {}, + guid: "guid2", + expected: nil, + err: nil, + }, + { + name: QueryExecutionErrorTestName, + setup: func(_ *sql.DB) {}, + guid: "guid1", + expected: nil, + err: repoerrors.DatabaseError{}, + }, + } + + for _, tc := range tests { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + dbConn := setupDeviceTable(t) + defer dbConn.Close() + + tc.setup(dbConn) + + sqlConfig := &db.SQL{ + Builder: squirrel.StatementBuilder.PlaceholderFormat(squirrel.Question), + Pool: dbConn, + IsEmbedded: true, + } + + if tc.name == QueryExecutionErrorTestName { + sqlConfig.Builder = squirrel.StatementBuilder.PlaceholderFormat(squirrel.AtP) + } + + mockLog := mocks.NewMockLogger(nil) + repo := sqldb.NewDeviceRepo(sqlConfig, mockLog) + + device, err := repo.GetByGUID(context.Background(), tc.guid) + + checkDeviceError(t, err, tc.err) + + if tc.expected == nil { + assert.Nil(t, device) + + return + } + + require.NotNil(t, device) + assert.Equal(t, tc.expected.GUID, device.GUID) + assert.Equal(t, tc.expected.TenantID, device.TenantID) + }) + } +} + +func TestDeviceRepo_GetByGUIDUsesCallerContext(t *testing.T) { + t.Parallel() + + dbConn := setupDeviceTable(t) + t.Cleanup(func() { _ = dbConn.Close() }) + + repo := sqldb.NewDeviceRepo(&db.SQL{ + Builder: squirrel.StatementBuilder.PlaceholderFormat(squirrel.Question), + Pool: dbConn, + IsEmbedded: true, + }, mocks.NewMockLogger(nil)) + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + device, err := repo.GetByGUID(ctx, "guid1") + require.Nil(t, device) + require.Error(t, err) + require.Contains(t, err.Error(), context.Canceled.Error()) +} + func TestDeviceRepo_GetDistinctTags(t *testing.T) { t.Parallel() From ba38d54889597cd57d0e1d03f3d99de18fe2e5de Mon Sep 17 00:00:00 2001 From: "Pola, Sudhir" Date: Wed, 2 Sep 2026 13:47:50 +0530 Subject: [PATCH 2/3] fix(api): ensuring the guid unqiueness for mongo-db --- cmd/app/tray_windows.go | 6 ++++-- internal/usecase/nosqldb/mongo/client.go | 8 ++++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/cmd/app/tray_windows.go b/cmd/app/tray_windows.go index a07ae0f2a..a4a182423 100644 --- a/cmd/app/tray_windows.go +++ b/cmd/app/tray_windows.go @@ -19,8 +19,10 @@ import ( const detachedProcess = 0x00000008 // CreateMutexW returns ERROR_ALREADY_EXISTS when another instance holds the named mutex. -const mutexName = "Local\\DMTConsoleTray" -const errorAlreadyExists uint32 = 183 +const ( + mutexName = "Local\\DMTConsoleTray" + errorAlreadyExists uint32 = 183 +) // ensureSingleInstance prevents concurrent tray processes via a named mutex. // diff --git a/internal/usecase/nosqldb/mongo/client.go b/internal/usecase/nosqldb/mongo/client.go index 968979981..e3ee478be 100644 --- a/internal/usecase/nosqldb/mongo/client.go +++ b/internal/usecase/nosqldb/mongo/client.go @@ -73,7 +73,6 @@ func ensureIndexes(ctx context.Context, db *mongo.Database, log logger.Interface } tenantScoped := []idx{ - {CollectionDevices, bson.D{{Key: fieldGUID, Value: 1}, {Key: fieldTenantID, Value: 1}}}, {CollectionProfiles, bson.D{{Key: fieldProfileName, Value: 1}, {Key: fieldTenantID, Value: 1}}}, {CollectionCIRAConfigs, bson.D{{Key: fieldConfigName, Value: 1}, {Key: fieldTenantID, Value: 1}}}, {CollectionDomains, bson.D{{Key: fieldProfileName, Value: 1}, {Key: fieldTenantID, Value: 1}}}, @@ -90,7 +89,12 @@ func ensureIndexes(ctx context.Context, db *mongo.Database, log logger.Interface }}, } - for _, i := range tenantScoped { + // CIRA authenticates devices by GUID alone, so GUIDs must be globally unique. + globalIndexes := []idx{ + {CollectionDevices, bson.D{{Key: fieldGUID, Value: 1}}}, + } + + for _, i := range append(tenantScoped, globalIndexes...) { _, err := db.Collection(i.coll).Indexes().CreateOne(ctx, mongo.IndexModel{ Keys: i.keys, Options: options.Index().SetUnique(true), From 6f92a60284419ae5bf3e679f58d758ec61931b3e Mon Sep 17 00:00:00 2001 From: "Pola, Sudhir" Date: Wed, 9 Sep 2026 13:44:34 +0530 Subject: [PATCH 3/3] refactor(internal): removed the redundant tenant-id in CIRA and redirect flows --- internal/controller/httpapi/v1/devices.go | 10 ++- .../controller/httpapi/v1/devices_test.go | 21 +++--- internal/controller/httpapi/v1/error.go | 6 -- internal/controller/tcp/cira/handler.go | 10 --- internal/controller/tcp/cira/handler_test.go | 3 - internal/controller/tcp/cira/tunnel.go | 3 +- internal/controller/ws/v1/redirect.go | 22 ------ internal/controller/ws/v1/redirect_test.go | 72 ++----------------- internal/usecase/devices/interceptor.go | 2 +- internal/usecase/devices/interceptor_test.go | 20 +++--- internal/usecase/devices/redirection.go | 9 +-- internal/usecase/devices/redirection_test.go | 22 ------ internal/usecase/devices/wsman/message.go | 17 +---- .../usecase/devices/wsman/message_test.go | 60 ---------------- 14 files changed, 35 insertions(+), 242 deletions(-) diff --git a/internal/controller/httpapi/v1/devices.go b/internal/controller/httpapi/v1/devices.go index 3d07622fc..fb20e3980 100644 --- a/internal/controller/httpapi/v1/devices.go +++ b/internal/controller/httpapi/v1/devices.go @@ -13,7 +13,6 @@ import ( "github.com/device-management-toolkit/console/config" "github.com/device-management-toolkit/console/internal/entity/dto/v1" - "github.com/device-management-toolkit/console/internal/tenant" "github.com/device-management-toolkit/console/internal/usecase/devices" "github.com/device-management-toolkit/console/pkg/consoleerrors" "github.com/device-management-toolkit/console/pkg/logger" @@ -69,7 +68,7 @@ func (dr *deviceRoutes) LoginRedirection(c *gin.Context) { deviceID := c.Param("id") tenantID := tenantIDFromHeader(c) - device, err := dr.t.GetByID(c.Request.Context(), deviceID, tenantID, false) + _, err := dr.t.GetByID(c.Request.Context(), deviceID, tenantID, false) if err != nil { dr.l.Error(err, "http - devices - v1 - LoginRedirection") ErrorResponse(c, err) @@ -80,10 +79,9 @@ func (dr *deviceRoutes) LoginRedirection(c *gin.Context) { // GUIDs are stored and matched lowercase, so the claim is normalized to match. expirationTime := time.Now().Add(config.ConsoleConfig.RedirectionJWTExpiration) claims := jwt.MapClaims{ - "exp": expirationTime.Unix(), - "iss": config.ConsoleConfig.Issuer, - "deviceId": strings.ToLower(deviceID), - tenant.TenantIDClaim: device.TenantID, + "exp": expirationTime.Unix(), + "iss": config.ConsoleConfig.Issuer, + "deviceId": strings.ToLower(deviceID), } token := jwt.NewWithClaims(jwt.SigningMethodHS256, claims) diff --git a/internal/controller/httpapi/v1/devices_test.go b/internal/controller/httpapi/v1/devices_test.go index 898a432ad..40d78f74a 100644 --- a/internal/controller/httpapi/v1/devices_test.go +++ b/internal/controller/httpapi/v1/devices_test.go @@ -763,11 +763,10 @@ func TestLoginRedirection(t *testing.T) { // deviceID is the GUID as it appears in the request path. deviceID string // expectedClaim is the deviceId the token must carry; empty means deviceID. - expectedClaim string - expectedTenant string - mock func(devFeature *mocks.MockDeviceManagementFeature) - expectedCode int - expectedErr bool + expectedClaim string + mock func(devFeature *mocks.MockDeviceManagementFeature) + expectedCode int + expectedErr bool }{ { name: "login redirection - success", @@ -776,9 +775,8 @@ func TestLoginRedirection(t *testing.T) { devFeature.EXPECT().GetByID(context.Background(), "test-device-guid", "", false). Return(&dto.Device{GUID: "test-device-guid", Hostname: "test-host", TenantID: "tenant-a"}, nil) }, - expectedCode: http.StatusOK, - expectedTenant: "tenant-a", - expectedErr: false, + expectedCode: http.StatusOK, + expectedErr: false, }, { name: "login redirection - mixed-case guid is normalized in claim", @@ -847,14 +845,14 @@ func TestLoginRedirection(t *testing.T) { } // Decode and verify token expiration and device binding - verifyRedirectionToken(t, tokenString, expectedClaim, tc.expectedTenant) + verifyRedirectionToken(t, tokenString, expectedClaim) } }) } } // verifyRedirectionToken checks the token's expiration and AMT-GUID (deviceId) binding. -func verifyRedirectionToken(t *testing.T, tokenString, expectedDeviceID, expectedTenant string) { +func verifyRedirectionToken(t *testing.T, tokenString, expectedDeviceID string) { t.Helper() // Parse the token, verifying its signature against the test signing key. @@ -866,7 +864,8 @@ func verifyRedirectionToken(t *testing.T, tokenString, expectedDeviceID, expecte // deviceId must be the device GUID require.Equal(t, expectedDeviceID, claims["deviceId"], "token deviceId should be the device GUID") - require.Equal(t, expectedTenant, claims["tenantId"], "token tenantId should be the device tenant") + _, hasTenantID := claims["tenantId"] + require.False(t, hasTenantID, "token should not contain a tenantId claim") // Verify expiration is set exp, err := claims.GetExpirationTime() diff --git a/internal/controller/httpapi/v1/error.go b/internal/controller/httpapi/v1/error.go index f275371ff..170aa3580 100644 --- a/internal/controller/httpapi/v1/error.go +++ b/internal/controller/httpapi/v1/error.go @@ -139,12 +139,6 @@ func handleSentinelErrors(c *gin.Context, err error) bool { msg := wsmanAPI.ErrCIRADeviceNotConnected.Error() c.AbortWithStatusJSON(http.StatusServiceUnavailable, response{Error: msg, Message: msg}) - return true - case errors.Is(err, wsmanAPI.ErrCIRATenantMismatch): - // Matches MPS ciraMiddleware, which answers 401 on a tenant mismatch. - msg := "Unauthorized" - c.AbortWithStatusJSON(http.StatusUnauthorized, response{Error: msg, Message: msg}) - return true } diff --git a/internal/controller/tcp/cira/handler.go b/internal/controller/tcp/cira/handler.go index 59fb9c981..81e3b8f0b 100644 --- a/internal/controller/tcp/cira/handler.go +++ b/internal/controller/tcp/cira/handler.go @@ -20,7 +20,6 @@ const ( type APFHandler struct { devices devices.Feature deviceID string - tenantID string globalRequestCount int log logger.Interface } @@ -38,12 +37,6 @@ func (h *APFHandler) DeviceID() string { return h.deviceID } -// TenantID returns the tenant the authenticated device belongs to, learned from -// its database row rather than from the device itself. -func (h *APFHandler) TenantID() string { - return h.tenantID -} - // OnProtocolVersion is called when an APF_PROTOCOLVERSION message is received. // Extracts and stores the device UUID for later use. // The UUID is normalized to lowercase to ensure case-insensitive matching @@ -124,9 +117,6 @@ func (h *APFHandler) validateCredentials(username, password string) bool { return false } - h.tenantID = device.TenantID - h.log.Debug("CIRA tenant resolved", "device_id", h.deviceID, "tenant_id", h.tenantID) - return true } diff --git a/internal/controller/tcp/cira/handler_test.go b/internal/controller/tcp/cira/handler_test.go index fa642b229..81cda7a88 100644 --- a/internal/controller/tcp/cira/handler_test.go +++ b/internal/controller/tcp/cira/handler_test.go @@ -51,7 +51,6 @@ func TestOnAuthRequestAcceptsDeviceInNonDefaultTenant(t *testing.T) { }) require.True(t, response.Authenticated) - require.Equal(t, "acme-corp", handler.TenantID()) } func TestOnAuthRequestLearnsDefaultTenant(t *testing.T) { @@ -70,7 +69,6 @@ func TestOnAuthRequestLearnsDefaultTenant(t *testing.T) { }) require.True(t, response.Authenticated) - require.Empty(t, handler.TenantID()) } func TestOnAuthRequestRejectsBadPasswordWithoutLearningTenant(t *testing.T) { @@ -90,7 +88,6 @@ func TestOnAuthRequestRejectsBadPasswordWithoutLearningTenant(t *testing.T) { }) require.False(t, response.Authenticated) - require.Empty(t, handler.TenantID()) } func TestOnAuthRequestRejectsUnsupportedMethod(t *testing.T) { diff --git a/internal/controller/tcp/cira/tunnel.go b/internal/controller/tcp/cira/tunnel.go index 19371171c..1fa91caea 100644 --- a/internal/controller/tcp/cira/tunnel.go +++ b/internal/controller/tcp/cira/tunnel.go @@ -293,7 +293,6 @@ func (ctx *connectionContext) registerDevice() { ctx.device = &wsman.ConnectionEntry{ IsCIRA: true, - TenantID: ctx.handler.TenantID(), Conny: ctx.conn, Timer: time.NewTimer(maxIdleTime), WsmanMessages: wsman2.NewMessages(client.Parameters{}), @@ -305,7 +304,7 @@ func (ctx *connectionContext) registerDevice() { ctx.log.Error("Failed to update connection status for device %s: %v", deviceID, err) } - ctx.log.Info("Device authenticated and registered", "device_id", deviceID, "tenant_id", ctx.handler.TenantID()) + ctx.log.Info("Device authenticated and registered", "device_id", deviceID) } func (ctx *connectionContext) writeResponse(response bytes.Buffer) error { diff --git a/internal/controller/ws/v1/redirect.go b/internal/controller/ws/v1/redirect.go index faeff1015..45e0c79b3 100644 --- a/internal/controller/ws/v1/redirect.go +++ b/internal/controller/ws/v1/redirect.go @@ -13,7 +13,6 @@ import ( "github.com/gorilla/websocket" "github.com/device-management-toolkit/console/config" - "github.com/device-management-toolkit/console/internal/tenant" "github.com/device-management-toolkit/console/internal/usecase/devices" "github.com/device-management-toolkit/console/pkg/logger" ) @@ -76,8 +75,6 @@ func (r *RedirectRoutes) websocketHandler(c *gin.Context) { // KVM_TIMING: Measure total connection time totalStart := time.Now() - // The JWT tenant claim is stored on the request context and is required for - // the use case to locate the device in the correct tenant. err = r.d.Redirect(c.Request.Context(), conn, c.Query("host"), c.Query("mode")) totalDuration := time.Since(totalStart) devices.RecordTotalConnection(totalDuration, c.Query("mode")) @@ -135,24 +132,5 @@ func (r *RedirectRoutes) validateRedirectionToken(c *gin.Context, tokenString st return false } - tenantID, hasTenantID := (*claims)[tenant.TenantIDClaim] - if hasTenantID { - tenantValue, ok := tenantID.(string) - if !ok || !tenant.Valid(tenantValue) { - r.l.Warn("redirection token contains invalid tenant", "host", c.Query("host")) - http.Error(c.Writer, "token contains invalid tenant", http.StatusForbidden) - - return false - } - - r.l.Debug("WebSocket tenant ID from claims", "tenant_id", tenantValue) - - c.Request = c.Request.WithContext(tenant.WithContext(c.Request.Context(), tenantValue)) - } else { - r.l.Debug("WebSocket tenant ID from claims is not present") - - c.Request = c.Request.WithContext(tenant.WithContext(c.Request.Context(), "")) - } - return true } diff --git a/internal/controller/ws/v1/redirect_test.go b/internal/controller/ws/v1/redirect_test.go index c71269c0c..bc6f0701d 100644 --- a/internal/controller/ws/v1/redirect_test.go +++ b/internal/controller/ws/v1/redirect_test.go @@ -1,7 +1,6 @@ package v1 import ( - "context" "errors" "net/http" "net/http/httptest" @@ -18,7 +17,6 @@ import ( "github.com/device-management-toolkit/console/config" "github.com/device-management-toolkit/console/internal/mocks" - "github.com/device-management-toolkit/console/internal/tenant" ) var ( @@ -103,11 +101,15 @@ func TestWebSocketHandler(t *testing.T) { } } -func TestWebSocketHandlerRedirectErrorClosesWebSocket(t *testing.T) { //nolint:paralleltest // shared configuration and logger +func TestWebSocketHandlerRedirectErrorClosesWebSocket(t *testing.T) { ctrl := gomock.NewController(t) t.Cleanup(ctrl.Finish) - _, _ = config.NewConfig() + t.Setenv("AUTH_JWT_KEY", "test-jwt-key") + + _, err := config.NewConfig() + require.NoError(t, err) + config.ConsoleConfig.Disabled = true mockFeature := mocks.NewMockDeviceManagementFeature(ctrl) @@ -166,17 +168,6 @@ func TestWebSocketHandlerDeviceBinding(t *testing.T) { return s } - tokenForTenant := func(deviceID, tenantID string) string { - claims := jwt.MapClaims{ - "exp": time.Now().Add(5 * time.Minute).Unix(), - "deviceId": deviceID, - "tenantId": tenantID, - } - - s, _ := jwt.NewWithClaims(jwt.SigningMethodHS256, claims).SignedString([]byte(config.ConsoleConfig.JWTKey)) - - return s - } t.Run("rejects token whose deviceId does not match host", func(t *testing.T) { //nolint:paralleltest // shared logger mockFeature := mocks.NewMockDeviceManagementFeature(ctrl) @@ -238,7 +229,6 @@ func TestWebSocketHandlerDeviceBinding(t *testing.T) { mockLogger := mocks.NewMockLogger(ctrl) mockUpgrader.EXPECT().Upgrade(gomock.Any(), gomock.Any(), nil).Return(&websocket.Conn{}, nil) - mockLogger.EXPECT().Debug("WebSocket tenant ID from claims is not present") mockLogger.EXPECT().Debug("failed to cast Upgrader to *websocket.Upgrader") mockLogger.EXPECT().Debug("KVM_TIMING: WebSocket upgrade", "duration_ms", gomock.Any()) mockLogger.EXPECT().Info("Websocket connection opened") @@ -263,7 +253,6 @@ func TestWebSocketHandlerDeviceBinding(t *testing.T) { mockLogger := mocks.NewMockLogger(ctrl) mockUpgrader.EXPECT().Upgrade(gomock.Any(), gomock.Any(), nil).Return(&websocket.Conn{}, nil) - mockLogger.EXPECT().Debug("WebSocket tenant ID from claims is not present") mockLogger.EXPECT().Debug("failed to cast Upgrader to *websocket.Upgrader") mockLogger.EXPECT().Debug("KVM_TIMING: WebSocket upgrade", "duration_ms", gomock.Any()) mockLogger.EXPECT().Info("Websocket connection opened") @@ -281,55 +270,6 @@ func TestWebSocketHandlerDeviceBinding(t *testing.T) { assert.Equal(t, http.StatusOK, w.Code) }) - - t.Run("propagates token tenant to redirect context", func(t *testing.T) { //nolint:paralleltest // shared logger - mockFeature := mocks.NewMockDeviceManagementFeature(ctrl) - mockUpgrader := mocks.NewMockUpgrader(ctrl) - mockLogger := mocks.NewMockLogger(ctrl) - - mockUpgrader.EXPECT().Upgrade(gomock.Any(), gomock.Any(), nil).Return(&websocket.Conn{}, nil) - mockLogger.EXPECT().Debug("WebSocket tenant ID from claims", "tenant_id", "tenant-a") - mockLogger.EXPECT().Debug("failed to cast Upgrader to *websocket.Upgrader") - mockLogger.EXPECT().Debug("KVM_TIMING: WebSocket upgrade", "duration_ms", gomock.Any()) - mockLogger.EXPECT().Info("Websocket connection opened") - mockFeature.EXPECT().Redirect(gomock.Any(), gomock.Any(), "deviceA", "kvm").DoAndReturn( - func(ctx context.Context, _ *websocket.Conn, _, _ string) error { - assert.Equal(t, "tenant-a", tenant.FromContext(ctx)) - - return nil - }, - ) - mockLogger.EXPECT().Debug("KVM_TIMING: Total connection time", "duration_ms", gomock.Any(), "mode", "kvm") - - r := gin.Default() - RegisterRoutes(r, mockLogger, mockFeature, mockUpgrader) - - req := httptest.NewRequest(http.MethodGet, "/relay/webrelay.ashx?host=deviceA&mode=kvm", http.NoBody) - req.Header.Set("Sec-Websocket-Protocol", tokenForTenant("deviceA", "tenant-a")) - - w := httptest.NewRecorder() - r.ServeHTTP(w, req) - - assert.Equal(t, http.StatusOK, w.Code) - }) - - t.Run("rejects token with invalid tenant", func(t *testing.T) { //nolint:paralleltest // shared logger - mockFeature := mocks.NewMockDeviceManagementFeature(ctrl) - mockUpgrader := mocks.NewMockUpgrader(ctrl) - mockLogger := mocks.NewMockLogger(ctrl) - mockLogger.EXPECT().Warn("redirection token contains invalid tenant", "host", "deviceA") - - r := gin.Default() - RegisterRoutes(r, mockLogger, mockFeature, mockUpgrader) - - req := httptest.NewRequest(http.MethodGet, "/relay/webrelay.ashx?host=deviceA&mode=kvm", http.NoBody) - req.Header.Set("Sec-Websocket-Protocol", tokenForTenant("deviceA", ".tenant-a")) - - w := httptest.NewRecorder() - r.ServeHTTP(w, req) - - assert.Equal(t, http.StatusForbidden, w.Code) - }) } // TestWebSocketHandlerTokenValidation: WS rejects missing and unverifiable tokens. diff --git a/internal/usecase/devices/interceptor.go b/internal/usecase/devices/interceptor.go index 70a3f6cc7..9cd6e6523 100644 --- a/internal/usecase/devices/interceptor.go +++ b/internal/usecase/devices/interceptor.go @@ -52,7 +52,7 @@ type DeviceConnection struct { func (uc *UseCase) Redirect(c context.Context, conn *websocket.Conn, guid, mode string) error { // KVM_TIMING: Measure device lookup latency lookupStart := time.Now() - device, err := uc.deviceInTenant(c, guid) + device, err := uc.repo.GetByGUID(c, guid) RecordDeviceLookup(time.Since(lookupStart)) uc.log.Debug("KVM_TIMING: Device lookup", "duration_ms", time.Since(lookupStart).Milliseconds(), "guid", guid) diff --git a/internal/usecase/devices/interceptor_test.go b/internal/usecase/devices/interceptor_test.go index ead512e3e..720ba5bfc 100644 --- a/internal/usecase/devices/interceptor_test.go +++ b/internal/usecase/devices/interceptor_test.go @@ -51,7 +51,7 @@ func TestRedirect(t *testing.T) { mockWSMAN.EXPECT().Worker().Do(func() { defer wg.Done() }).Times(1) - mockRepo.EXPECT().GetByID(gomock.Any(), guid, "").Return(nil, ErrInterceptorGeneral) + mockRepo.EXPECT().GetByGUID(gomock.Any(), guid).Return(nil, ErrInterceptorGeneral) }, expectedErr: ErrInterceptorGeneral, }, @@ -61,7 +61,7 @@ func TestRedirect(t *testing.T) { mockWSMAN.EXPECT().Worker().Do(func() { defer wg.Done() }).Times(1) - mockRepo.EXPECT().GetByID(gomock.Any(), guid, "").Return(&entity.Device{ + mockRepo.EXPECT().GetByGUID(gomock.Any(), guid).Return(&entity.Device{ GUID: guid, Username: "user", Password: "pass", @@ -137,7 +137,7 @@ func TestRedirectSuccessfulFlow(t *testing.T) { } // Mock successful flow up to RedirectConnect, then fail to avoid goroutines - mockRepo.EXPECT().GetByID(gomock.Any(), testGUID, "").Return(device, nil) + mockRepo.EXPECT().GetByGUID(gomock.Any(), testGUID).Return(device, nil) mockRedirection.EXPECT().SetupWsmanClient(gomock.Any(), *device, true, true).Return(wsman.Messages{}, nil) // Return error to avoid starting problematic goroutines but still test the flow mockRedirection.EXPECT().RedirectConnect(gomock.Any(), gomock.Any()).Return(ErrConnectionFailed) @@ -172,7 +172,7 @@ func TestRedirectDeviceNotFound(t *testing.T) { wg.Wait() // Mock device not found - mockRepo.EXPECT().GetByID(gomock.Any(), testGUID, "").Return(nil, nil) + mockRepo.EXPECT().GetByGUID(gomock.Any(), testGUID).Return(nil, nil) // Test device not found err := uc.Redirect(context.Background(), mockConn, testGUID, testMode) @@ -210,7 +210,7 @@ func TestRedirectConnectionReuse(t *testing.T) { } // First call - create new connection but fail at connect to avoid goroutines - mockRepo.EXPECT().GetByID(gomock.Any(), testGUID, "").Return(device, nil) + mockRepo.EXPECT().GetByGUID(gomock.Any(), testGUID).Return(device, nil) mockRedirection.EXPECT().SetupWsmanClient(gomock.Any(), *device, true, true).Return(wsman.Messages{}, nil) mockRedirection.EXPECT().RedirectConnect(gomock.Any(), gomock.Any()).Return(ErrFirstConnectionFailed) @@ -218,7 +218,7 @@ func TestRedirectConnectionReuse(t *testing.T) { require.Error(t, err) // Second call - also fail to avoid goroutines but test reuse logic - mockRepo.EXPECT().GetByID(gomock.Any(), testGUID, "").Return(device, nil) + mockRepo.EXPECT().GetByGUID(gomock.Any(), testGUID).Return(device, nil) mockRedirection.EXPECT().SetupWsmanClient(gomock.Any(), *device, true, true).Return(wsman.Messages{}, nil) mockRedirection.EXPECT().RedirectConnect(gomock.Any(), gomock.Any()).Return(ErrSecondConnectionFailed) @@ -304,7 +304,7 @@ func TestRedirectWithErrorScenarios(t *testing.T) { }).Times(1) device := &entity.Device{GUID: testGUID, Username: "user", Password: "pass"} - mockRepo.EXPECT().GetByID(gomock.Any(), testGUID, "").Return(device, nil) + mockRepo.EXPECT().GetByGUID(gomock.Any(), testGUID).Return(device, nil) mockRedir.EXPECT().SetupWsmanClient(gomock.Any(), *device, true, true).Return(wsman.Messages{}, nil) mockRedir.EXPECT().RedirectConnect(gomock.Any(), gomock.Any()).Return(ErrConnectionFailed) }, @@ -382,7 +382,7 @@ func TestRedirectConnectionFlowCoverage(t *testing.T) { }).Times(1) device := &entity.Device{GUID: "test-device", Username: "user", Password: "pass"} - mockRepo.EXPECT().GetByID(gomock.Any(), "test-device", "").Return(device, nil) + mockRepo.EXPECT().GetByGUID(gomock.Any(), "test-device").Return(device, nil) mockRedir.EXPECT().SetupWsmanClient(gomock.Any(), *device, true, true).Return(wsman.Messages{}, nil) // Return error to avoid starting goroutines, but still exercise connection creation mockRedir.EXPECT().RedirectConnect(gomock.Any(), gomock.Any()).Return(ErrTestError) @@ -445,7 +445,7 @@ func TestRedirectAdditionalCoverage(t *testing.T) { mockWSMAN.EXPECT().Worker().Do(func() { defer wg.Done() }).Times(1) - mockRepo.EXPECT().GetByID(gomock.Any(), "missing-guid", "").Return(nil, nil) + mockRepo.EXPECT().GetByGUID(gomock.Any(), "missing-guid").Return(nil, nil) }, guid: "missing-guid", mode: "kvm", @@ -457,7 +457,7 @@ func TestRedirectAdditionalCoverage(t *testing.T) { mockWSMAN.EXPECT().Worker().Do(func() { defer wg.Done() }).Times(1) - mockRepo.EXPECT().GetByID(gomock.Any(), "error-guid", "").Return(nil, ErrDatabaseError) + mockRepo.EXPECT().GetByGUID(gomock.Any(), "error-guid").Return(nil, ErrDatabaseError) }, guid: "error-guid", mode: "kvm", diff --git a/internal/usecase/devices/redirection.go b/internal/usecase/devices/redirection.go index 9d7607ac9..60c7ce51a 100644 --- a/internal/usecase/devices/redirection.go +++ b/internal/usecase/devices/redirection.go @@ -8,7 +8,6 @@ import ( "github.com/device-management-toolkit/go-wsman-messages/v2/pkg/wsman/client" "github.com/device-management-toolkit/console/internal/entity" - "github.com/device-management-toolkit/console/internal/tenant" wsmanAPI "github.com/device-management-toolkit/console/internal/usecase/devices/wsman" ) @@ -16,7 +15,7 @@ type Redirector struct { SafeRequirements security.Cryptor } -func (g *Redirector) SetupWsmanClient(ctx context.Context, device entity.Device, isRedirection, logAMTMessages bool) (wsman.Messages, error) { +func (g *Redirector) SetupWsmanClient(_ context.Context, device entity.Device, isRedirection, logAMTMessages bool) (wsman.Messages, error) { // CIRA device: route redirection through the APF tunnel if isRedirection && device.MPSUsername != "" { connection := wsmanAPI.GetConnectionEntry(device.GUID) @@ -24,12 +23,6 @@ func (g *Redirector) SetupWsmanClient(ctx context.Context, device entity.Device, return wsman.Messages{}, wsmanAPI.ErrCIRADeviceNotConnected } - // The CIRA socket is registered after device authentication. Verify the - // caller owns that socket before forwarding redirection traffic to AMT. - if connection.TenantID != tenant.FromContext(ctx) { - return wsman.Messages{}, wsmanAPI.ErrCIRATenantMismatch - } - return wsman.NewCIRARedirectionMessages(connection), nil } diff --git a/internal/usecase/devices/redirection_test.go b/internal/usecase/devices/redirection_test.go index ef61394c9..8c305edac 100644 --- a/internal/usecase/devices/redirection_test.go +++ b/internal/usecase/devices/redirection_test.go @@ -13,7 +13,6 @@ import ( "github.com/device-management-toolkit/console/internal/entity" "github.com/device-management-toolkit/console/internal/mocks" - "github.com/device-management-toolkit/console/internal/tenant" devices "github.com/device-management-toolkit/console/internal/usecase/devices" wsmanAPI "github.com/device-management-toolkit/console/internal/usecase/devices/wsman" ) @@ -115,27 +114,6 @@ func TestSetupWsmanClient_CIRARedirection(t *testing.T) { require.NotNil(t, msgs.Client) }) - t.Run("rejects a CIRA socket owned by another tenant", func(t *testing.T) { - t.Parallel() - - guid := "cira-device-tenant-mismatch" - wsmanAPI.SetConnectionEntry(guid, &wsmanAPI.ConnectionEntry{ - IsCIRA: true, - TenantID: "tenant-a", - }) - t.Cleanup(func() { wsmanAPI.RemoveConnection(guid) }) - - device := entity.Device{ - GUID: guid, - MPSUsername: "admin", - } - redirector := &devices.Redirector{SafeRequirements: mocks.MockCrypto{}} - ctx := tenant.WithContext(context.Background(), "tenant-b") - - _, err := redirector.SetupWsmanClient(ctx, device, true, false) - require.ErrorIs(t, err, wsmanAPI.ErrCIRATenantMismatch) - }) - t.Run("non-CIRA device skips CIRA path", func(t *testing.T) { t.Parallel() diff --git a/internal/usecase/devices/wsman/message.go b/internal/usecase/devices/wsman/message.go index 88d188816..ad0f4ddc2 100644 --- a/internal/usecase/devices/wsman/message.go +++ b/internal/usecase/devices/wsman/message.go @@ -83,8 +83,6 @@ var ( // ErrCIRADeviceNotConnected is returned when a CIRA device is not connected or not found. ErrCIRADeviceNotConnected = errors.New("CIRA device not connected/not found") - // ErrCIRATenantMismatch is returned when a caller's tenant does not own the CIRA socket. - ErrCIRATenantMismatch = errors.New("CIRA device belongs to a different tenant") // ErrNoWiFiPort is returned when no WiFi interface is found on the device. ErrNoWiFiPort = errors.New("no WiFi interface found (InstanceID == Intel(r) AMT Ethernet Port Settings 1)") @@ -94,11 +92,8 @@ var ( type ConnectionEntry struct { WsmanMessages wsman.Messages IsCIRA bool - // TenantID is learned from the device row during APF auth, so REST callers - // can be checked against the tenant that actually owns the socket. - TenantID string - Conny net.Conn - Timer *time.Timer + Conny net.Conn + Timer *time.Timer // APF channel management for CIRA connections (uses types from go-wsman-messages) APFChannelStore *client.APFChannelStore @@ -174,14 +169,6 @@ func (g GoWSMANMessages) SetupWsmanClient(ctx context.Context, device entity.Dev return } - // Defense in depth behind the repository's tenant filter: the socket is - // owned by the tenant that authenticated it, whatever the caller asked for. - if connection.TenantID != device.TenantID { - errChan <- ErrCIRATenantMismatch - - return - } - cp := client.Parameters{ Target: device.GUID, // Use GUID as Host for CIRA connections IsRedirection: false, diff --git a/internal/usecase/devices/wsman/message_test.go b/internal/usecase/devices/wsman/message_test.go index f0cbc80af..7a90ac67a 100644 --- a/internal/usecase/devices/wsman/message_test.go +++ b/internal/usecase/devices/wsman/message_test.go @@ -173,63 +173,3 @@ func TestDestroyWsmanClient_MissingEntryIsNoop(t *testing.T) { // Should not panic when the entry is absent. g.DestroyWsmanClient(dto.Device{GUID: "destroy-missing-entry"}) } - -// The CIRA socket belongs to the tenant that authenticated it, so a caller from -// another tenant must not be handed the connection. Mirrors MPS ciraMiddleware. -func TestSetupWsmanClient_CIRATenantMismatchIsRejected(t *testing.T) { //nolint:paralleltest // mutates package-level state (requestQueue, queueTickTime, connections) - origTick := queueTickTime - queueTickTime = 1 * time.Millisecond - - t.Cleanup(func() { queueTickTime = origTick }) - - guid := "cira-tenant-mismatch-guid" - - t.Cleanup(func() { RemoveConnection(guid) }) - - SetConnectionEntry(guid, &ConnectionEntry{ - IsCIRA: true, - TenantID: "acme-corp", - Timer: time.AfterFunc(time.Hour, func() {}), - }) - - g := NewGoWSMANMessages(logger.New("error"), passthroughCryptor{}) - - stopWorker := make(chan struct{}) - workerDone := make(chan struct{}) - - go func() { - defer close(workerDone) - - for { - select { - case request := <-requestQueue: - request() - case <-stopWorker: - return - } - } - }() - - t.Cleanup(func() { - close(stopWorker) - <-workerDone - }) - - mismatched := entity.Device{ - GUID: guid, - MPSUsername: "mpsuser", - TenantID: "globex", - } - - _, err := g.SetupWsmanClient(context.Background(), mismatched, false, false) - require.ErrorIs(t, err, ErrCIRATenantMismatch) - - owner := entity.Device{ - GUID: guid, - MPSUsername: "mpsuser", - TenantID: "acme-corp", - } - - _, err = g.SetupWsmanClient(context.Background(), owner, false, false) - require.NoError(t, err) -}