Browse Source

fix: deleting a user crashed the server, and folders ignored their project

The crash first. UserController was handed the access layer four lines
before that layer was constructed, so it bound a reference to a null
pointer. Nothing failed at startup - a reference to nothing looks like a
reference - and the fault waited until the first request that used it.
Deleting a user segfaulted the whole server, which systemd restarted, six
times before the core dump named the frame. Everything that asks the
access layer is now built after it, and the file was checked for any other
controller used before it exists.

Two lessons in that: a null unique_ptr dereferenced into a reference is
undefined behaviour that reports nothing at the site of the mistake, and a
service that restarts itself hides its own crashes - the executions and
the workflows kept running, so from the outside only one endpoint looked
broken.

Then the gap this was found in. Folders carried a projectId that nothing
read, so any account could list, rename, move and delete every folder in
the installation. They are checked now like the workflows in them: the
list shows only reachable projects, an unreachable folder answers 404
rather than 403, and a new folder lands in a project the caller can write
to rather than in none at all.

Credentials gained a project selector, so one can be created where the
people who need it will find it instead of always in the maker's own
project. It says what that means - everybody who can edit that project can
use this credential - because a credential is a password to somebody
else's system and where it lives is who holds it.

Verified with a second account: a member sees none of the owner's folders
and gets 404 for one by id, cannot rename or delete it, and the owner's
folder is untouched. The same member creating a folder sees exactly their
own, filed in their own project - so the filter hides what it should
rather than everything. Deleting that account now succeeds, takes its
empty project with it, and leaves the server standing.
fszontagh 1 month ago
parent
commit
2e2c18f5a3

+ 76 - 14
src/webserver/api/workflow_group_controller.cpp

@@ -7,9 +7,11 @@ namespace smartbotic::webserver::api {
 using namespace common;
 
 WorkflowGroupController::WorkflowGroupController(storage::StorageClient& storage,
+                                                   auth::AccessControl& access,
                                                    auth::AuthMiddleware& middleware,
                                                    WebSocketServer& ws_server)
     : storage_(storage)
+    , access_(access)
     , middleware_(middleware)
     , ws_server_(ws_server) {}
 
@@ -64,6 +66,30 @@ void WorkflowGroupController::registerRoutes(httplib::Server& server) {
     });
 }
 
+
+bool WorkflowGroupController::loadAllowed(httplib::Response& res, const auth::AuthContext& ctx,
+                                          const std::string& id, auth::Action action,
+                                          nlohmann::json& out) {
+    auto stored = storage_.get("workflow_groups", id);
+    if (stored.failed()) {
+        sendError(res, "Folder not found", 404);
+        return false;
+    }
+    const std::string project = auth::AccessControl::projectOf(stored.value());
+    if (!access_.allowed(ctx, project, auth::Action::Read)) {
+        // As with a workflow: whether it exists is only told to people who can
+        // reach it.
+        sendError(res, "Folder not found", 404);
+        return false;
+    }
+    if (action != auth::Action::Read && !access_.allowed(ctx, project, action)) {
+        sendError(res, "You can see this folder but not change it", 403);
+        return false;
+    }
+    out = stored.value();
+    return true;
+}
+
 void WorkflowGroupController::listGroups(const httplib::Request& req, httplib::Response& res,
                                           const auth::AuthContext& ctx) {
     storage::QueryOptions options;
@@ -99,9 +125,17 @@ void WorkflowGroupController::listGroups(const httplib::Request& req, httplib::R
         return;
     }
 
+    // Only the projects this caller can reach. A folder names somebody's work
+    // and shows how it is arranged, which is enough to be worth withholding.
+    const auto reachable = access_.projectsFor(ctx);
+    nlohmann::json visible = nlohmann::json::array();
+    for (const auto& group : result.value().documents) {
+        if (reachable.contains(auth::AccessControl::projectOf(group))) visible.push_back(group);
+    }
+
     nlohmann::json response;
-    response["groups"] = result.value().documents;
-    response["total"] = result.value().total_count;
+    response["groups"] = visible;
+    response["total"] = visible.size();
     response["page"] = page;
     response["pageSize"] = page_size;
     response["hasMore"] = result.value().has_more;
@@ -113,13 +147,10 @@ void WorkflowGroupController::getGroup(const httplib::Request& req, httplib::Res
                                          const auth::AuthContext& ctx) {
     std::string id = req.matches[1];
 
-    auto result = storage_.get("workflow_groups", id);
-    if (result.failed()) {
-        sendError(res, "Group not found", 404);
-        return;
-    }
+    nlohmann::json group;
+    if (!loadAllowed(res, ctx, id, auth::Action::Read, group)) return;
 
-    sendJson(res, result.value());
+    sendJson(res, group);
 }
 
 void WorkflowGroupController::createGroup(const httplib::Request& req, httplib::Response& res,
@@ -133,6 +164,29 @@ void WorkflowGroupController::createGroup(const httplib::Request& req, httplib::
         // Set owner
         body["ownerId"] = ctx.user_id;
 
+        // A folder belongs to a project like the workflows in it. Without one
+        // it would be reachable only by an admin, which is not what somebody
+        // making a folder meant.
+        std::string project = body.value("projectId", "");
+        if (!project.empty() && !access_.allowed(ctx, project, auth::Action::Write)) {
+            sendError(res, "You cannot create a folder in that project", 403);
+            return;
+        }
+        if (project.empty()) {
+            auto personal = access_.personalProjectFor(ctx.user_id);
+            if (personal.ok()) {
+                project = personal.value();
+            } else {
+                auto created = access_.ensurePersonalProject(ctx.user_id, ctx.username);
+                if (created.failed()) {
+                    sendError(res, "You have no project to put this in", 500);
+                    return;
+                }
+                project = created.value().value("_id", "");
+            }
+        }
+        body["projectId"] = project;
+
         // Validate required fields
         if (!body.contains("name") || body["name"].get<std::string>().empty()) {
             sendError(res, "Name is required", 400);
@@ -182,6 +236,10 @@ void WorkflowGroupController::updateGroup(const httplib::Request& req, httplib::
                                            const auth::AuthContext& ctx) {
     try {
         std::string id = req.matches[1];
+
+        nlohmann::json existing;
+        if (!loadAllowed(res, ctx, id, auth::Action::Write, existing)) return;
+
         auto body = nlohmann::json::parse(req.body);
 
         // Prevent updating metadata fields
@@ -219,6 +277,9 @@ void WorkflowGroupController::deleteGroup(const httplib::Request& req, httplib::
                                            const auth::AuthContext& ctx) {
     std::string id = req.matches[1];
 
+    nlohmann::json existing;
+    if (!loadAllowed(res, ctx, id, auth::Action::Write, existing)) return;
+
     // Check force parameter
     bool force = req.has_param("force") && req.get_param_value("force") == "true";
 
@@ -284,12 +345,9 @@ void WorkflowGroupController::getGroupPath(const httplib::Request& req, httplib:
                                             const auth::AuthContext& ctx) {
     std::string id = req.matches[1];
 
-    // Verify group exists
-    auto result = storage_.get("workflow_groups", id);
-    if (result.failed()) {
-        sendError(res, "Group not found", 404);
-        return;
-    }
+    nlohmann::json group;
+    if (!loadAllowed(res, ctx, id, auth::Action::Read, group)) return;
+    auto result = common::Result<nlohmann::json>(group);
 
     // Build path from root to this group
     auto path = buildGroupPath(id);
@@ -303,6 +361,10 @@ void WorkflowGroupController::moveGroup(const httplib::Request& req, httplib::Re
                                          const auth::AuthContext& ctx) {
     try {
         std::string id = req.matches[1];
+
+        nlohmann::json existing;
+        if (!loadAllowed(res, ctx, id, auth::Action::Write, existing)) return;
+
         auto body = nlohmann::json::parse(req.body);
 
         // Get new parent ID - can be null/empty/"root" for top-level

+ 9 - 0
src/webserver/api/workflow_group_controller.hpp

@@ -3,6 +3,7 @@
 #include <httplib.h>
 #include <nlohmann/json.hpp>
 #include "../auth/auth_middleware.hpp"
+#include "../auth/access.hpp"
 #include "storage/storage_client.hpp"
 #include "../websocket_server.hpp"
 
@@ -11,6 +12,7 @@ namespace smartbotic::webserver::api {
 class WorkflowGroupController {
 public:
     WorkflowGroupController(storage::StorageClient& storage,
+                            auth::AccessControl& access,
                            auth::AuthMiddleware& middleware,
                            WebSocketServer& ws_server);
 
@@ -36,6 +38,12 @@ private:
                    const auth::AuthContext& ctx);
 
     // Helpers
+    // Loads a folder and checks the caller may do this to it. A folder is not
+    // secret in itself, but it names somebody's work and says how it is
+    // organised, which is enough.
+    bool loadAllowed(httplib::Response& res, const auth::AuthContext& ctx,
+                     const std::string& id, auth::Action action, nlohmann::json& out);
+
     void sendJson(httplib::Response& res, const nlohmann::json& data, int status = 200);
     void sendError(httplib::Response& res, const std::string& message, int status);
 
@@ -49,6 +57,7 @@ private:
     bool hasChildren(const std::string& group_id);
 
     storage::StorageClient& storage_;
+    auth::AccessControl& access_;
     auth::AuthMiddleware& middleware_;
     WebSocketServer& ws_server_;
 };

+ 9 - 5
src/webserver/webserver_service.cpp

@@ -267,12 +267,16 @@ void WebServerService::setupRoutes() {
     auth_ctrl_ = std::make_unique<api::AuthController>(*auth_store_, *auth_middleware_);
     auth_ctrl_->registerRoutes(server);
 
-    user_ctrl_ = std::make_unique<api::UserController>(*auth_store_, *auth_middleware_, *access_, *storage_);
-    user_ctrl_->registerRoutes(server);
-
-    // Who may do what. Built before the controllers that ask it.
+    // Who may do what. Built before any controller that asks it - dereferencing
+    // this while it was still null bound a reference to nothing, and the first
+    // request that used it took the server down with a segfault rather than
+    // failing anywhere near the mistake.
     access_ = std::make_unique<auth::AccessControl>(*storage_);
 
+    user_ctrl_ = std::make_unique<api::UserController>(
+        *auth_store_, *auth_middleware_, *access_, *storage_);
+    user_ctrl_->registerRoutes(server);
+
     project_ctrl_ = std::make_unique<api::ProjectController>(
         *storage_, *auth_middleware_, *access_, *auth_store_);
     project_ctrl_->registerRoutes(server);
@@ -283,7 +287,7 @@ void WebServerService::setupRoutes() {
     workflow_ctrl_->registerRoutes(server);
 
     workflow_group_ctrl_ = std::make_unique<api::WorkflowGroupController>(
-        *storage_, *auth_middleware_, *ws_server_);
+        *storage_, *access_, *auth_middleware_, *ws_server_);
     workflow_group_ctrl_->registerRoutes(server);
 
     execution_ctrl_ = std::make_unique<api::ExecutionController>(

+ 2 - 0
webui/src/api/credentials.ts

@@ -81,6 +81,8 @@ export interface CreateCredentialRequest {
   type: CredentialType
   data: BasicAuthData | BearerTokenData | ApiKeyData | OAuth2Data | ImapData | MysqlData | PostgresqlData
   allowedWorkflows?: string[]
+  /** Which project it belongs to, and so who can see and use it. */
+  projectId?: string
 }
 
 export interface UpdateCredentialRequest {

+ 34 - 0
webui/src/pages/CredentialsPage.tsx

@@ -1,4 +1,5 @@
 import { useState } from 'react'
+import { projectsApi, Project } from '../api/users'
 import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query'
 import { nodesApi } from '../api/workflows'
 import { format } from 'date-fns'
@@ -382,6 +383,15 @@ function CredentialModal({ credential, onClose, onSuccess }: CredentialModalProp
   const [name, setName] = useState(credential?.name || '')
   const [description, setDescription] = useState(credential?.description || '')
   const [type, setType] = useState<CredentialType>(credential?.type || 'basic')
+  const [projectId, setProjectId] = useState<string>((credential as any)?.projectId || '')
+
+  // Which projects this person can put a credential in. A credential is a
+  // password to somebody else's system, so where it lives decides who can use
+  // it - that should be a choice, not always "wherever mine go".
+  const { data: projects = [] } = useQuery({
+    queryKey: ['projects'],
+    queryFn: () => projectsApi.list(),
+  })
   // A node-registered type such as "sdcpp". It rides alongside the built-in
   // type rather than replacing it - the shape is what gets stored.
   const [declaredType, setDeclaredType] = useState<string>(credential?.declaredType || '')
@@ -563,6 +573,7 @@ function CredentialModal({ credential, onClose, onSuccess }: CredentialModalProp
         declaredType: declaredType || undefined,
         type,
         data,
+        projectId: projectId || undefined,
       })
     }
   }
@@ -778,6 +789,29 @@ function CredentialModal({ credential, onClose, onSuccess }: CredentialModalProp
               </div>
             )}
 
+            {/* Where it lives decides who can use it. Only shown when there
+                is more than one project to choose between. */}
+            {!credential && projects.length > 1 && (
+              <div>
+                <label className="block text-sm font-medium text-gray-700 dark:text-gray-300 mb-1">
+                  Project
+                </label>
+                <select
+                  value={projectId}
+                  onChange={(e) => setProjectId(e.target.value)}
+                  className="w-full px-3 py-2 border border-gray-300 dark:border-slate-600 rounded-lg bg-white dark:bg-slate-700 text-gray-900 dark:text-gray-100"
+                >
+                  <option value="">My own project</option>
+                  {projects.filter((p: Project) => p.myRole === 'admin' || p.myRole === 'editor').map((p: Project) => (
+                    <option key={p._id} value={p._id}>{p.name}</option>
+                  ))}
+                </select>
+                <p className="mt-1 text-xs text-gray-500 dark:text-gray-400">
+                  Everybody who can edit that project can use this credential.
+                </p>
+              </div>
+            )}
+
             {type === 'api_key' && (
               <>
                 <div>