Skip to content

Commit

Permalink
Added catalog migration fix, fixed existing migration bug, added tests
Browse files Browse the repository at this point in the history 
* Fix to correct the incorrect migration method we were using to migrate from pre 4.0 versions. The incorrect behaviour was adding usernames into the mapd_roles syscat table, when users should not entered there.

* In addition, there is a fix to the existing migration path (it seems it was broken at time, as a new CHECK was added recently to disallow inserts using abstract DBObject keys, which the existing path used to instantiate new permissions).

* Added tests for the migration fix as well as the original migration path.
  • Loading branch information
@misiugodfrey @andrewseidl
misiugodfrey authored and andrewseidl committed Aug 4, 2021
1 parent c3cb1b9 commit fcac163
Show file tree
Hide file tree
Showing 4 changed files with 197 additions and 63 deletions.
10 changes: 10 additions & 0 deletions Catalog/DBObject.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,16 @@ class DBObject {
, objectPrivs_(privs)
, ownerId_(owner){};
DBObject(const DBObject& object);
DBObject(const std::string& name,
DBObjectType type,
DBObjectKey key,
AccessPrivileges privs,
int32_t owner)
: objectName_(name)
, objectType_(type)
, objectKey_(key)
, objectPrivs_(privs)
, ownerId_(owner){};
~DBObject() {}

void setObjectType(const DBObjectType& objectType);
Expand Down
81 changes: 46 additions & 35 deletions Catalog/SysCatalog.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,8 @@ void SysCatalog::initDB() {

void SysCatalog::checkAndExecuteMigrations() {
migratePrivileged_old();
createUserRoles();
createRoles();
fixRolesMigration();
migratePrivileges();
migrateDBAccessPrivileges();
updateUserSchema(); // must come before updatePasswordsToHashes()
Expand Down Expand Up @@ -331,7 +332,7 @@ void SysCatalog::importDataFromOldMapdDB() {
}
}

void SysCatalog::createUserRoles() {
void SysCatalog::createRoles() {
sys_sqlite_lock sqlite_lock(this);
sqliteConnector_->query("BEGIN TRANSACTION");
try {
Expand All @@ -345,20 +346,32 @@ void SysCatalog::createUserRoles() {
sqliteConnector_->query(
"CREATE TABLE mapd_roles(roleName text, userName text, UNIQUE(roleName, "
"userName))");
// need to account for old conversions where we are building and moving
// from pre version 4.0 and 'mapd' was default superuser
sqliteConnector_->query("SELECT name FROM mapd_users WHERE name NOT IN ( \'" +
OMNISCI_ROOT_USER + "\', 'mapd')");
size_t numRows = sqliteConnector_->getNumRows();
vector<string> user_names;
for (size_t i = 0; i < numRows; ++i) {
user_names.push_back(sqliteConnector_->getData<string>(i, 0));
} catch (const std::exception&) {
sqliteConnector_->query("ROLLBACK TRANSACTION");
throw;
}
sqliteConnector_->query("END TRANSACTION");
}

/*
There was an error in how we migrated users from pre-4.0 versions where we would copy
all user names into the mapd_roles table. This table should never have usernames in it
(the correct migration was to copy users into the mapd_object_permissions table instead)
so this migration function prunes such cases out.
*/
void SysCatalog::fixRolesMigration() {
sys_sqlite_lock sqlite_lock(this);
sqliteConnector_->query("BEGIN TRANSACTION");
try {
sqliteConnector_->query("SELECT name FROM mapd_users");
auto num_rows = sqliteConnector_->getNumRows();
std::vector<std::string> user_names;
for (size_t i = 0; i < num_rows; ++i) {
user_names.push_back(sqliteConnector_->getData<std::string>(i, 0));
}
for (const auto& user_name : user_names) {
// for each user, create a fake role with the same name
sqliteConnector_->query_with_text_params(
"INSERT INTO mapd_roles(roleName, userName) VALUES (?, ?)",
vector<string>{user_name, user_name});
sqliteConnector_->query_with_text_param("DELETE FROM mapd_roles WHERE roleName = ?",
user_name);
}
} catch (const std::exception&) {
sqliteConnector_->query("ROLLBACK TRANSACTION");
Expand Down Expand Up @@ -481,34 +494,33 @@ void SysCatalog::migratePrivileges() {
auto dbName = dbs_by_id[grantee.second];
{
// table level permissions
DBObjectKey key;
key.permissionType = DBObjectType::TableDBObjectType;
key.dbId = grantee.second;
DBObject object(key, AccessPrivileges::ALL_TABLE_MIGRATE, OMNISCI_ROOT_USER_ID);
object.setName(dbName);
auto type = DBObjectType::TableDBObjectType;
DBObjectKey key{type, grantee.second};
DBObject object(
dbName, type, key, AccessPrivileges::ALL_TABLE_MIGRATE, OMNISCI_ROOT_USER_ID);
insertOrUpdateObjectPrivileges(
sqliteConnector_, users_by_id[grantee.first], true, object);
}

{
// dashboard level permissions
DBObjectKey key;
key.permissionType = DBObjectType::DashboardDBObjectType;
key.dbId = grantee.second;
DBObject object(
key, AccessPrivileges::ALL_DASHBOARD_MIGRATE, OMNISCI_ROOT_USER_ID);
object.setName(dbName);
auto type = DBObjectType::DashboardDBObjectType;
DBObjectKey key{type, grantee.second};
DBObject object(dbName,
type,
key,
AccessPrivileges::ALL_DASHBOARD_MIGRATE,
OMNISCI_ROOT_USER_ID);
insertOrUpdateObjectPrivileges(
sqliteConnector_, users_by_id[grantee.first], true, object);
}

{
// view level permissions
DBObjectKey key;
key.permissionType = DBObjectType::ViewDBObjectType;
key.dbId = grantee.second;
DBObject object(key, AccessPrivileges::ALL_VIEW_MIGRATE, OMNISCI_ROOT_USER_ID);
object.setName(dbName);
auto type = DBObjectType::ViewDBObjectType;
DBObjectKey key{type, grantee.second};
DBObject object(
dbName, type, key, AccessPrivileges::ALL_VIEW_MIGRATE, OMNISCI_ROOT_USER_ID);
insertOrUpdateObjectPrivileges(
sqliteConnector_, users_by_id[grantee.first], true, object);
}
Expand All @@ -517,11 +529,10 @@ void SysCatalog::migratePrivileges() {
auto dbName = dbs_by_id[0];
if (user.second == false && user.first != OMNISCI_ROOT_USER_ID) {
{
DBObjectKey key;
key.permissionType = DBObjectType::DatabaseDBObjectType;
key.dbId = 0;
DBObject object(key, AccessPrivileges::NONE, OMNISCI_ROOT_USER_ID);
object.setName(dbName);
auto type = DBObjectType::DatabaseDBObjectType;
DBObjectKey key{type, 0};
DBObject object(
dbName, type, key, AccessPrivileges::NONE, OMNISCI_ROOT_USER_ID);
insertOrUpdateObjectPrivileges(
sqliteConnector_, users_by_id[user.first], true, object);
}
Expand Down
3 changes: 2 additions & 1 deletion Catalog/SysCatalog.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,8 @@ class SysCatalog : private CommonFileOperations {
void buildObjectDescriptorMap();
void checkAndExecuteMigrations();
void importDataFromOldMapdDB();
void createUserRoles();
void createRoles();
void fixRolesMigration();
void addAdminUserRole();
void migratePrivileges();
void migratePrivileged_old();
Expand Down
166 changes: 139 additions & 27 deletions Tests/CatalogMigrationTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,31 +35,36 @@
extern bool g_enable_fsi;
extern bool g_enable_s3_fsi;

class FsiSchemaTest : public testing::Test {
namespace BF = boost::filesystem;
using SC = Catalog_Namespace::SysCatalog;

namespace {
bool table_exists(SqliteConnector& conn, const std::string& table_name) {
conn.query("SELECT name FROM sqlite_master WHERE type='table' AND name='" + table_name +
"'");
return conn.getNumRows() > 0;
}

bool has_result(SqliteConnector& conn, const std::string& query) {
conn.query(query);
return conn.getNumRows() > 0;
}
} // namespace

class CatalogTest : public testing::Test {
protected:
FsiSchemaTest()
: sqlite_connector_(
"omnisci",
boost::filesystem::absolute("mapd_catalogs", BASE_PATH).string()) {}
CatalogTest()
: cat_conn_("omnisci", BF::absolute("mapd_catalogs", BASE_PATH).string()) {}

static void SetUpTestSuite() {
g_enable_s3_fsi = true;
Catalog_Namespace::SysCatalog::instance().init(
BASE_PATH, nullptr, {}, nullptr, false, false, {});
SC::instance().init(BASE_PATH, nullptr, {}, nullptr, false, false, {});
}

void SetUp() override {
g_enable_fsi = false;
dropFsiTables();
}

void TearDown() override { dropFsiTables(); }

std::vector<std::string> getTables() {
sqlite_connector_.query("SELECT name FROM sqlite_master WHERE type='table';");
std::vector<std::string> getTables(SqliteConnector& conn) {
conn.query("SELECT name FROM sqlite_master WHERE type='table';");
std::vector<std::string> tables;
for (size_t i = 0; i < sqlite_connector_.getNumRows(); i++) {
tables.emplace_back(sqlite_connector_.getData<std::string>(i, 0));
for (size_t i = 0; i < conn.getNumRows(); i++) {
tables.emplace_back(conn.getData<std::string>(i, 0));
}
return tables;
}
Expand All @@ -72,6 +77,116 @@ class FsiSchemaTest : public testing::Test {
BASE_PATH, db_metadata, nullptr, leaves, nullptr, false);
}

SqliteConnector cat_conn_;
};

class SysCatalogTest : public CatalogTest {
protected:
SysCatalogTest()
: syscat_conn_("omnisci_system_catalog",
BF::absolute("mapd_catalogs", BASE_PATH).string()) {}

void TearDown() override {
if (tableExists("mapd_users")) {
syscat_conn_.query("DELETE FROM mapd_users WHERE name='test_user'");
}
if (tableExists("mapd_object_permissions")) {
syscat_conn_.query(
"DELETE FROM mapd_object_permissions WHERE roleName='test_user'");
}
}

bool hasResult(const std::string& query) { return has_result(syscat_conn_, query); }

bool tableExists(const std::string& table_name) {
return table_exists(syscat_conn_, table_name);
}

void createLegacyTestUser() {
// This creates a test user in mapd_users syscat table, but does not properly add it
// to mapd_object_permissions so it is incomplete by current standards.
ASSERT_TRUE(table_exists(syscat_conn_, "mapd_users"));
syscat_conn_.query("DELETE FROM mapd_users WHERE name='test_user'");
syscat_conn_.query_with_text_params(
"INSERT INTO mapd_users (name, passwd_hash, issuper, can_login) VALUES (?, ?, ?, "
"?)",
{"test_user", "passwd", "true", "true"});
}

static void reinitializeSystemCatalog() {
SC::destroy();
SC::instance().init(BASE_PATH, nullptr, {}, nullptr, false, false, {});
}

SqliteConnector syscat_conn_;
};

// Check that we migrate correctly from pre 4.0 catalog.
TEST_F(SysCatalogTest, MigrateRoles) {
// Make sure the post 4.0 tables do not exist to simulate migration.
syscat_conn_.query("DROP TABLE IF EXISTS mapd_roles");
syscat_conn_.query("DROP TABLE IF EXISTS mapd_object_permissions");
createLegacyTestUser();

// Create the pre 4.0 mapd_privileges table.
syscat_conn_.query(
"CREATE TABLE IF NOT EXISTS mapd_privileges (userid integer references mapd_users, "
"dbid integer references mapd_databases, select_priv boolean, insert_priv boolean, "
"UNIQUE(userid, dbid))");

// Copy users who are not the admin (userid 0) into the pre 4.0 mapd_privileges table.
syscat_conn_.query(
"INSERT INTO mapd_privileges (userid, dbid) SELECT userid, default_db FROM "
"mapd_users WHERE userid <> 0");

// Re-initialization should perform migrations.
reinitializeSystemCatalog();

// Users should be inserted into mapd_object_permissions but not mapd_roles on
// migration.
ASSERT_TRUE(tableExists("mapd_roles"));
ASSERT_FALSE(hasResult("SELECT roleName FROM mapd_roles WHERE roleName='test_user'"));

ASSERT_TRUE(tableExists("mapd_object_permissions"));
ASSERT_TRUE(hasResult(
"SELECT roleName FROM mapd_object_permissions WHERE roleName='test_user'"));
}

TEST_F(SysCatalogTest, FixIncorrectRolesMigration) {
ASSERT_TRUE(tableExists("mapd_roles"));
createLegacyTestUser();

// Setup an incorrect migration situation where we have usernames inserted into
// mapd_roles. This could occur between versions 4.0 and 5.7 and should now be fixed.
ASSERT_TRUE(tableExists("mapd_users"));
syscat_conn_.query("DELETE FROM mapd_roles WHERE roleName='test_user'");
syscat_conn_.query_with_text_params("INSERT INTO mapd_roles VALUES (?, ?)",
{"test_user", "test_user"});

ASSERT_TRUE(hasResult("SELECT name FROM mapd_users WHERE name='test_user'"));
ASSERT_TRUE(hasResult("SELECT roleName FROM mapd_roles WHERE roleName='test_user'"));

// When we re-initialize the SysCatalog we should fix incorrect past migrations.
reinitializeSystemCatalog();

ASSERT_TRUE(hasResult("SELECT name FROM mapd_users WHERE name='test_user'"));
ASSERT_FALSE(hasResult("SELECT roleName FROM mapd_roles WHERE roleName='test_user'"));
}

class FsiSchemaTest : public CatalogTest {
protected:
static void SetUpTestSuite() {
g_enable_s3_fsi = true;
CatalogTest::SetUpTestSuite();
}

void SetUp() override {
g_enable_fsi = false;
dropFsiTables();
}

void TearDown() override { dropFsiTables(); }

void assertExpectedDefaultServer(Catalog_Namespace::Catalog* catalog,
const std::string& server_name,
const std::string& data_wrapper,
Expand Down Expand Up @@ -118,27 +233,25 @@ class FsiSchemaTest : public testing::Test {
}

void assertFsiTablesExist() {
auto tables = getTables();
auto tables = getTables(cat_conn_);
ASSERT_FALSE(std::find(tables.begin(), tables.end(), "omnisci_foreign_servers") ==
tables.end());
ASSERT_FALSE(std::find(tables.begin(), tables.end(), "omnisci_foreign_tables") ==
tables.end());
}

void assertFsiTablesDoNotExist() {
auto tables = getTables();
auto tables = getTables(cat_conn_);
ASSERT_TRUE(std::find(tables.begin(), tables.end(), "omnisci_foreign_servers") ==
tables.end());
ASSERT_TRUE(std::find(tables.begin(), tables.end(), "omnisci_foreign_tables") ==
tables.end());
}

private:
SqliteConnector sqlite_connector_;

void dropFsiTables() {
sqlite_connector_.query("DROP TABLE IF EXISTS omnisci_foreign_servers;");
sqlite_connector_.query("DROP TABLE IF EXISTS omnisci_foreign_tables;");
cat_conn_.query("DROP TABLE IF EXISTS omnisci_foreign_servers;");
cat_conn_.query("DROP TABLE IF EXISTS omnisci_foreign_tables;");
}
};

Expand Down Expand Up @@ -202,8 +315,7 @@ class ForeignTablesTest : public DBHandlerTestFixture {
};

TEST_F(ForeignTablesTest, ForeignTablesAreNotDroppedWhenFsiIsDisabled) {
const auto file_path =
boost::filesystem::canonical("../../Tests/FsiDataFiles/example_1.csv").string();
const auto file_path = BF::canonical("../../Tests/FsiDataFiles/example_1.csv").string();
sql("CREATE FOREIGN TABLE test_foreign_table (c1 int) SERVER omnisci_local_csv "
"WITH (file_path = '" +
file_path + "');");
Expand Down

0 comments on commit fcac163

Please sign in to comment.