From 2ecdfefcb8b39fdc565ab0cab580cb1632d6bc13 Mon Sep 17 00:00:00 2001 From: Vasiliy Date: Fri, 24 Aug 2018 14:37:35 +0300 Subject: [PATCH] issue 459.2: ensure view retrieval is denied of detailed definition is denied (#6237) * issue 459.2: ensure view retrieval is denied of detailed definition is denied * address test failures --- arangod/RestHandler/RestViewHandler.cpp | 32 ++++++++ arangod/V8Server/v8-views.cpp | 31 +++++++- ...ight-task-update-view-arangosearch-spec.js | 4 +- ...ess-right-update-view-arangosearch-spec.js | 2 + tests/RestHandler/RestViewHandler-test.cpp | 70 ++++++++++++++++++ tests/V8Server/v8-views-test.cpp | 73 +++++++++++++++++++ 6 files changed, 209 insertions(+), 3 deletions(-) diff --git a/arangod/RestHandler/RestViewHandler.cpp b/arangod/RestHandler/RestViewHandler.cpp index ed0a9bad38..aa6f1aeed5 100644 --- a/arangod/RestHandler/RestViewHandler.cpp +++ b/arangod/RestHandler/RestViewHandler.cpp @@ -83,6 +83,25 @@ void RestViewHandler::getView(std::string const& nameOrId, bool detailed) { return; } + // skip views for which the full view definition cannot be generated, as per https://github.com/arangodb/backlog/issues/459 + try { + arangodb::velocypack::Builder viewBuilder; + + viewBuilder.openObject(); + + auto res = view->toVelocyPack(viewBuilder, true, false); + + if (!res.ok()) { + generateError(res); + + return; // skip view + } + } catch(...) { + generateError(arangodb::Result(TRI_ERROR_INTERNAL)); + + return; // skip view + } + arangodb::velocypack::Builder builder; builder.openObject(); @@ -436,6 +455,19 @@ void RestViewHandler::getViews() { continue; // skip views that are not authorised to be read } + // skip views for which the full view definition cannot be generated, as per https://github.com/arangodb/backlog/issues/459 + try { + arangodb::velocypack::Builder viewBuilder; + + viewBuilder.openObject(); + + if (!view->toVelocyPack(viewBuilder, true, false).ok()) { + continue; // skip view + } + } catch(...) { + continue; // skip view + } + arangodb::velocypack::Builder viewBuilder; viewBuilder.openObject(); diff --git a/arangod/V8Server/v8-views.cpp b/arangod/V8Server/v8-views.cpp index 32718dd8da..5c209707f2 100644 --- a/arangod/V8Server/v8-views.cpp +++ b/arangod/V8Server/v8-views.cpp @@ -384,6 +384,21 @@ static void JS_ViewVocbase(v8::FunctionCallbackInfo const& args) { TRI_V8_THROW_EXCEPTION_MESSAGE(TRI_ERROR_FORBIDDEN, "insufficient rights to get view"); } + // skip views for which the full view definition cannot be generated, as per https://github.com/arangodb/backlog/issues/459 + try { + arangodb::velocypack::Builder viewBuilder; + + viewBuilder.openObject(); + + auto res = view->toVelocyPack(viewBuilder, true, false); + + if (!res.ok()) { + TRI_V8_THROW_EXCEPTION(res); // skip view + } + } catch(...) { + TRI_V8_THROW_EXCEPTION(TRI_ERROR_INTERNAL); // skip view + } + v8::Handle result = WrapView(isolate, view); if (result.IsEmpty()) { @@ -425,6 +440,7 @@ static void JS_ViewsVocbase(v8::FunctionCallbackInfo const& args) { // already create an array of the correct size v8::Handle result = v8::Array::New(isolate); + uint32_t entry = 0; size_t const n = views.size(); for (size_t i = 0; i < n; ++i) { @@ -435,6 +451,19 @@ static void JS_ViewsVocbase(v8::FunctionCallbackInfo const& args) { continue; // skip views that are not authorised to be read } + // skip views for which the full view definition cannot be generated, as per https://github.com/arangodb/backlog/issues/459 + try { + arangodb::velocypack::Builder viewBuilder; + + viewBuilder.openObject(); + + if (!view->toVelocyPack(viewBuilder, true, false).ok()) { + continue; // skip view + } + } catch(...) { + continue; // skip view + } + v8::Handle c = WrapView(isolate, view); if (c.IsEmpty()) { @@ -442,7 +471,7 @@ static void JS_ViewsVocbase(v8::FunctionCallbackInfo const& args) { break; } - result->Set(static_cast(i), c); + result->Set(entry++, c); } if (error) { diff --git a/js/client/tests/authentication/user-access-right-task-update-view-arangosearch-spec.js b/js/client/tests/authentication/user-access-right-task-update-view-arangosearch-spec.js index 4e3c46dddf..a790d5728c 100644 --- a/js/client/tests/authentication/user-access-right-task-update-view-arangosearch-spec.js +++ b/js/client/tests/authentication/user-access-right-task-update-view-arangosearch-spec.js @@ -308,8 +308,8 @@ function hasIResearch (db) { if (dbLevel['rw'].has(name)) { tasks.register(task); wait(keySpaceId, name); - expect(getKey(keySpaceId, `${name}_status`)).to.equal(true, `${name} could not update the view with sufficient rights`); - expect(rootTestView(testViewRename)).to.equal(true, 'View renaming reported success, but updated view was not found afterwards'); + expect(getKey(keySpaceId, `${name}_status`)).to.equal(colLevel['ro'].has(name) || colLevel['rw'].has(name), `${name} could not update the view with sufficient rights`); + expect(rootTestView(testViewRename)).to.equal(colLevel['ro'].has(name) || colLevel['rw'].has(name), 'View renaming reported success, but updated view was not found afterwards'); } else { try { tasks.register(task); diff --git a/js/client/tests/authentication/user-access-right-update-view-arangosearch-spec.js b/js/client/tests/authentication/user-access-right-update-view-arangosearch-spec.js index 8d8880e85f..cc70a14ac8 100644 --- a/js/client/tests/authentication/user-access-right-update-view-arangosearch-spec.js +++ b/js/client/tests/authentication/user-access-right-update-view-arangosearch-spec.js @@ -249,6 +249,8 @@ function hasIResearch (db) { //FIXME: remove try/catch block after renaming will work in cluster if (e.code === 404 && (e.errorNum === 1203 || e.errorNum === 1470)) { return; + } else if (e.code === 403) { + return; // not authorised is valid if a non-read collection is present in the view } else { throw e; } diff --git a/tests/RestHandler/RestViewHandler-test.cpp b/tests/RestHandler/RestViewHandler-test.cpp index adf99f1247..217bb31bb5 100644 --- a/tests/RestHandler/RestViewHandler-test.cpp +++ b/tests/RestHandler/RestViewHandler-test.cpp @@ -712,6 +712,27 @@ SECTION("test_auth") { CHECK((slice.hasKey(arangodb::StaticStrings::ErrorNum) && slice.get(arangodb::StaticStrings::ErrorNum).isNumber() && TRI_ERROR_FORBIDDEN == slice.get(arangodb::StaticStrings::ErrorNum).getNumber())); } + // not authorized (failed detailed toVelocyPack(...)) as per https://github.com/arangodb/backlog/issues/459 + { + arangodb::auth::UserMap userMap; + auto& user = userMap.emplace("", arangodb::auth::User::newUser("", "", arangodb::auth::Source::LDAP)).first->second; + user.grantDatabase(vocbase.name(), arangodb::auth::Level::RO); + user.grantCollection(vocbase.name(), "testView", arangodb::auth::Level::NONE); // for missing collections User::collectionAuthLevel(...) returns database auth::Level + userManager->setAuthInfo(userMap); // set user map to avoid loading configuration from system database + auto* testView = arangodb::LogicalView::cast(logicalView.get()); + testView->_appendVelocyPackResult = arangodb::Result(TRI_ERROR_FORBIDDEN); + auto resetAppendVelocyPackResult = std::shared_ptr(testView, [](TestView* p)->void { p->_appendVelocyPackResult = arangodb::Result(); }); + + auto status = handler.execute(); + CHECK((arangodb::RestStatus::DONE == status)); + CHECK((arangodb::rest::ResponseCode::FORBIDDEN == responce.responseCode())); + auto slice = responce._payload.slice(); + CHECK((slice.isObject())); + CHECK((slice.hasKey(arangodb::StaticStrings::Code) && slice.get(arangodb::StaticStrings::Code).isNumber() && size_t(arangodb::rest::ResponseCode::FORBIDDEN) == slice.get(arangodb::StaticStrings::Code).getNumber())); + CHECK((slice.hasKey(arangodb::StaticStrings::Error) && slice.get(arangodb::StaticStrings::Error).isBoolean() && true == slice.get(arangodb::StaticStrings::Error).getBoolean())); + CHECK((slice.hasKey(arangodb::StaticStrings::ErrorNum) && slice.get(arangodb::StaticStrings::ErrorNum).isNumber() && TRI_ERROR_FORBIDDEN == slice.get(arangodb::StaticStrings::ErrorNum).getNumber())); + } + // authorized (NONE view) as per https://github.com/arangodb/backlog/issues/459 { arangodb::auth::UserMap userMap; @@ -806,6 +827,27 @@ SECTION("test_auth") { CHECK((slice.hasKey(arangodb::StaticStrings::ErrorNum) && slice.get(arangodb::StaticStrings::ErrorNum).isNumber() && TRI_ERROR_FORBIDDEN == slice.get(arangodb::StaticStrings::ErrorNum).getNumber())); } + // not authorized (failed detailed toVelocyPack(...)) + { + arangodb::auth::UserMap userMap; + auto& user = userMap.emplace("", arangodb::auth::User::newUser("", "", arangodb::auth::Source::LDAP)).first->second; + user.grantDatabase(vocbase.name(), arangodb::auth::Level::RO); + user.grantCollection(vocbase.name(), "testView", arangodb::auth::Level::NONE); // for missing collections User::collectionAuthLevel(...) returns database auth::Level + userManager->setAuthInfo(userMap); // set user map to avoid loading configuration from system database + auto* testView = arangodb::LogicalView::cast(logicalView.get()); + testView->_appendVelocyPackResult = arangodb::Result(TRI_ERROR_FORBIDDEN); + auto resetAppendVelocyPackResult = std::shared_ptr(testView, [](TestView* p)->void { p->_appendVelocyPackResult = arangodb::Result(); }); + + auto status = handler.execute(); + CHECK((arangodb::RestStatus::DONE == status)); + CHECK((arangodb::rest::ResponseCode::FORBIDDEN == responce.responseCode())); + auto slice = responce._payload.slice(); + CHECK((slice.isObject())); + CHECK((slice.hasKey(arangodb::StaticStrings::Code) && slice.get(arangodb::StaticStrings::Code).isNumber() && size_t(arangodb::rest::ResponseCode::FORBIDDEN) == slice.get(arangodb::StaticStrings::Code).getNumber())); + CHECK((slice.hasKey(arangodb::StaticStrings::Error) && slice.get(arangodb::StaticStrings::Error).isBoolean() && true == slice.get(arangodb::StaticStrings::Error).getBoolean())); + CHECK((slice.hasKey(arangodb::StaticStrings::ErrorNum) && slice.get(arangodb::StaticStrings::ErrorNum).isNumber() && TRI_ERROR_FORBIDDEN == slice.get(arangodb::StaticStrings::ErrorNum).getNumber())); + } + // authorized (NONE view) as per https://github.com/arangodb/backlog/issues/459 { arangodb::auth::UserMap userMap; @@ -901,6 +943,34 @@ SECTION("test_auth") { CHECK((slice.hasKey(arangodb::StaticStrings::ErrorNum) && slice.get(arangodb::StaticStrings::ErrorNum).isNumber() && TRI_ERROR_FORBIDDEN == slice.get(arangodb::StaticStrings::ErrorNum).getNumber())); } + // not authorized (failed detailed toVelocyPack(...)) as per https://github.com/arangodb/backlog/issues/459 + { + arangodb::auth::UserMap userMap; + auto& user = userMap.emplace("", arangodb::auth::User::newUser("", "", arangodb::auth::Source::LDAP)).first->second; + user.grantDatabase(vocbase.name(), arangodb::auth::Level::RO); + user.grantCollection(vocbase.name(), "testView1", arangodb::auth::Level::NONE); // for missing collections User::collectionAuthLevel(...) returns database auth::Level + user.grantCollection(vocbase.name(), "testView2", arangodb::auth::Level::NONE); // for missing collections User::collectionAuthLevel(...) returns database auth::Level + userManager->setAuthInfo(userMap); // set user map to avoid loading configuration from system database + auto* testView = arangodb::LogicalView::cast(logicalView2.get()); + testView->_appendVelocyPackResult = arangodb::Result(TRI_ERROR_FORBIDDEN); + auto resetAppendVelocyPackResult = std::shared_ptr(testView, [](TestView* p)->void { p->_appendVelocyPackResult = arangodb::Result(); }); + + auto status = handler.execute(); + CHECK((arangodb::RestStatus::DONE == status)); + CHECK((arangodb::rest::ResponseCode::OK == responce.responseCode())); + auto slice = responce._payload.slice(); + CHECK((slice.isObject())); + CHECK((slice.hasKey(arangodb::StaticStrings::Code) && slice.get(arangodb::StaticStrings::Code).isNumber() && size_t(arangodb::rest::ResponseCode::OK) == slice.get(arangodb::StaticStrings::Code).getNumber())); + CHECK((slice.hasKey(arangodb::StaticStrings::Error) && slice.get(arangodb::StaticStrings::Error).isBoolean() && false == slice.get(arangodb::StaticStrings::Error).getBoolean())); + CHECK((slice.hasKey("result"))); + slice = slice.get("result"); + CHECK((slice.isArray())); + CHECK((1U == slice.length())); + slice = slice.at(0); + CHECK((slice.isObject())); + CHECK((slice.hasKey(arangodb::StaticStrings::DataSourceName) && slice.get(arangodb::StaticStrings::DataSourceName).isString() && std::string("testView1") == slice.get(arangodb::StaticStrings::DataSourceName).copyString())); + } + // authorized (NONE view) as per https://github.com/arangodb/backlog/issues/459 { arangodb::auth::UserMap userMap; diff --git a/tests/V8Server/v8-views-test.cpp b/tests/V8Server/v8-views-test.cpp index 611c3a04f4..acac32ae1c 100644 --- a/tests/V8Server/v8-views-test.cpp +++ b/tests/V8Server/v8-views-test.cpp @@ -977,6 +977,30 @@ SECTION("test_auth") { CHECK((false == !view)); } + // not authorized (failed detailed toVelocyPack(...)) as per https://github.com/arangodb/backlog/issues/459 + { + arangodb::auth::UserMap userMap; + auto& user = userMap.emplace("", arangodb::auth::User::newUser("", "", arangodb::auth::Source::LDAP)).first->second; + user.grantDatabase(vocbase.name(), arangodb::auth::Level::RO); + user.grantCollection(vocbase.name(), "testView", arangodb::auth::Level::NONE); // for missing collections User::collectionAuthLevel(...) returns database auth::Level + userManager->setAuthInfo(userMap); // set user map to avoid loading configuration from system database + auto* testView = arangodb::LogicalView::cast(logicalView.get()); + testView->_appendVelocyPackResult = arangodb::Result(TRI_ERROR_FORBIDDEN); + auto resetAppendVelocyPackResult = std::shared_ptr(testView, [](TestView* p)->void { p->_appendVelocyPackResult = arangodb::Result(); }); + + arangodb::velocypack::Builder responce; + v8::TryCatch tryCatch(isolate.get()); + auto result = v8::Function::Cast(*fn_view)->CallAsFunction(context, fn_view, args.size(), args.data()); + CHECK((result.IsEmpty())); + CHECK((tryCatch.HasCaught())); + CHECK((TRI_ERROR_NO_ERROR == TRI_V8ToVPack(isolate.get(), responce, tryCatch.Exception(), false))); + auto slice = responce.slice(); + CHECK((slice.isObject())); + CHECK((slice.hasKey(arangodb::StaticStrings::ErrorNum) && slice.get(arangodb::StaticStrings::ErrorNum).isNumber() && TRI_ERROR_FORBIDDEN == slice.get(arangodb::StaticStrings::ErrorNum).getNumber())); + auto view = vocbase.lookupView("testView"); + CHECK((false == !view)); + } + // authorized (NONE view) as per https://github.com/arangodb/backlog/issues/459 { arangodb::auth::UserMap userMap; @@ -1103,6 +1127,30 @@ SECTION("test_auth") { CHECK((false == !view)); } + // not authorized (failed detailed toVelocyPack(...)) + { + arangodb::auth::UserMap userMap; + auto& user = userMap.emplace("", arangodb::auth::User::newUser("", "", arangodb::auth::Source::LDAP)).first->second; + user.grantDatabase(vocbase.name(), arangodb::auth::Level::RO); + user.grantCollection(vocbase.name(), "testView", arangodb::auth::Level::NONE); // for missing collections User::collectionAuthLevel(...) returns database auth::Level + userManager->setAuthInfo(userMap); // set user map to avoid loading configuration from system database + auto* testView = arangodb::LogicalView::cast(logicalView.get()); + testView->_appendVelocyPackResult = arangodb::Result(TRI_ERROR_FORBIDDEN); + auto resetAppendVelocyPackResult = std::shared_ptr(testView, [](TestView* p)->void { p->_appendVelocyPackResult = arangodb::Result(); }); + + arangodb::velocypack::Builder responce; + v8::TryCatch tryCatch(isolate.get()); + auto result = v8::Function::Cast(*fn_properties)->CallAsFunction(context, arangoView, args.size(), args.data()); + CHECK((result.IsEmpty())); + CHECK((tryCatch.HasCaught())); + CHECK((TRI_ERROR_NO_ERROR == TRI_V8ToVPack(isolate.get(), responce, tryCatch.Exception(), false))); + auto slice = responce.slice(); + CHECK((slice.isObject())); + CHECK((slice.hasKey(arangodb::StaticStrings::ErrorNum) && slice.get(arangodb::StaticStrings::ErrorNum).isNumber() && TRI_ERROR_FORBIDDEN == slice.get(arangodb::StaticStrings::ErrorNum).getNumber())); + auto view = vocbase.lookupView("testView"); + CHECK((false == !view)); + } + // authorized (NONE view) as per https://github.com/arangodb/backlog/issues/459 { arangodb::auth::UserMap userMap; @@ -1229,6 +1277,31 @@ SECTION("test_auth") { CHECK((false == !view2)); } + // not authorized (failed detailed toVelocyPack(...)) as per https://github.com/arangodb/backlog/issues/459 + { + arangodb::auth::UserMap userMap; + auto& user = userMap.emplace("", arangodb::auth::User::newUser("", "", arangodb::auth::Source::LDAP)).first->second; + user.grantDatabase(vocbase.name(), arangodb::auth::Level::RO); + user.grantCollection(vocbase.name(), "testView1", arangodb::auth::Level::NONE); // for missing collections User::collectionAuthLevel(...) returns database auth::Level + user.grantCollection(vocbase.name(), "testView2", arangodb::auth::Level::NONE); // for missing collections User::collectionAuthLevel(...) returns database auth::Level + userManager->setAuthInfo(userMap); // set user map to avoid loading configuration from system database + auto* testView = arangodb::LogicalView::cast(logicalView2.get()); + testView->_appendVelocyPackResult = arangodb::Result(TRI_ERROR_FORBIDDEN); + auto resetAppendVelocyPackResult = std::shared_ptr(testView, [](TestView* p)->void { p->_appendVelocyPackResult = arangodb::Result(); }); + + auto result = v8::Function::Cast(*fn_views)->CallAsFunction(context, fn_views, args.size(), args.data()); + CHECK((!result.IsEmpty())); + CHECK((result.ToLocalChecked()->IsArray())); + auto* resultArray = v8::Array::Cast(*result.ToLocalChecked()); + CHECK((1U == resultArray->Length())); + auto v8View = *TRI_UnwrapClass>(resultArray->Get(0).As(), WRP_VOCBASE_VIEW_TYPE); + CHECK((false == !v8View)); + CHECK((std::string("testView1") == v8View->name())); + CHECK((std::string("testViewType") == v8View->type().name())); + auto view1 = vocbase.lookupView("testView1"); + CHECK((false == !view1)); + } + // authorized (NONE view) as per https://github.com/arangodb/backlog/issues/459 { arangodb::auth::UserMap userMap;