Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement roles.delete, databases.drop & databases.configured.disconnect endpoints #3858

Merged
merged 8 commits into from
Sep 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions db/databases/operations/drop.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
from db.connection import exec_msar_func
from psycopg import sql


def drop_database(database_oid, conn):
cursor = conn.cursor()
conn.autocommit = True
drop_database_query = exec_msar_func(
conn,
'drop_database_query',
database_oid
).fetchone()[0]
cursor.execute(sql.SQL(drop_database_query))
cursor.close()
5 changes: 5 additions & 0 deletions db/roles/operations/drop.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
from db.connection import exec_msar_func


def drop_role(role_oid, conn):
exec_msar_func(conn, 'drop_role', role_oid)
80 changes: 78 additions & 2 deletions db/sql/00_msar.sql
Original file line number Diff line number Diff line change
Expand Up @@ -516,6 +516,28 @@ END;
$$ LANGUAGE plpgsql RETURNS NULL ON NULL INPUT;


CREATE OR REPLACE FUNCTION msar.get_database_name(dat_id oid) RETURNS TEXT AS $$/*
Return the UNQUOTED name of a given database.

If the database does not exist, an exception will be raised.

Args:
dat_id: The OID of the role.
*/
DECLARE dat_name text;
BEGIN
SELECT datname INTO dat_name FROM pg_catalog.pg_database WHERE oid=dat_id;

IF dat_name IS NULL THEN
RAISE EXCEPTION 'Database with OID % does not exist', dat_id
USING ERRCODE = '42704'; -- undefined_object
END IF;

RETURN dat_name;
END;
$$ LANGUAGE plpgsql STABLE RETURNS NULL ON NULL INPUT;


CREATE OR REPLACE FUNCTION msar.get_role_name(rol_oid oid) RETURNS TEXT AS $$/*
Return the UNQUOTED name of a given role.

Expand All @@ -526,7 +548,7 @@ Args:
*/
DECLARE rol_name text;
BEGIN
SELECT rolname INTO rol_name FROM pg_roles WHERE oid=rol_oid;
SELECT rolname INTO rol_name FROM pg_catalog.pg_roles WHERE oid=rol_oid;

IF rol_name IS NULL THEN
RAISE EXCEPTION 'Role with OID % does not exist', rol_oid
Expand All @@ -535,7 +557,7 @@ BEGIN

RETURN rol_name;
END;
$$ LANGUAGE plpgsql RETURNS NULL ON NULL INPUT;
$$ LANGUAGE plpgsql STABLE RETURNS NULL ON NULL INPUT;


CREATE OR REPLACE FUNCTION msar.get_constraint_type_api_code(contype char) RETURNS TEXT AS $$/*
Expand Down Expand Up @@ -1326,6 +1348,23 @@ END;
$$ LANGUAGE plpgsql;


CREATE OR REPLACE FUNCTION
msar.drop_role(rol_id regrole) RETURNS void AS $$/*
Drop a role.

Note:
- To drop a superuser role, you must be a superuser yourself.
- To drop non-superuser roles, you must have CREATEROLE privilege and have been granted ADMIN OPTION on the role.

Args:
rol_id: The OID of the role to drop on the database.
*/
BEGIN
EXECUTE format('DROP ROLE %I', msar.get_role_name(rol_id));
END;
$$ LANGUAGE plpgsql;


CREATE OR REPLACE FUNCTION
msar.build_database_privilege_replace_expr(rol_id regrole, privileges_ jsonb) RETURNS TEXT AS $$
SELECT string_agg(
Expand Down Expand Up @@ -1680,6 +1719,43 @@ END;
$$ LANGUAGE plpgsql;


----------------------------------------------------------------------------------------------------
----------------------------------------------------------------------------------------------------
-- DROP DATABASE FUNCTIONS
--
-- Drop a database.
----------------------------------------------------------------------------------------------------
----------------------------------------------------------------------------------------------------


CREATE OR REPLACE FUNCTION
msar.drop_database_query(dat_id oid) RETURNS text AS $$/*
Return the SQL query to drop a database.

If no database exists with the given oid, an exception will be raised.

Args:
dat_id: The OID of the role to drop.
*/
BEGIN
RETURN format('DROP DATABASE %I', msar.get_database_name(dat_id));
END;
$$ LANGUAGE plpgsql RETURNS NULL ON NULL INPUT;


CREATE OR REPLACE FUNCTION
msar.drop_database_query(dat_name text) RETURNS text AS $$/*
Return the SQL query to drop a database.

Args:
dat_id: An unqoted name of the database to be dropped.
*/
BEGIN
RETURN format('DROP DATABASE %I', dat_name);
END;
$$ LANGUAGE plpgsql RETURNS NULL ON NULL INPUT;


----------------------------------------------------------------------------------------------------
----------------------------------------------------------------------------------------------------
-- DROP SCHEMA FUNCTIONS
Expand Down
3 changes: 3 additions & 0 deletions docs/docs/api/rpc.md
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ To use an RPC function:
options:
members:
- get
- delete
- DatabaseInfo

## Configured Databases
Expand All @@ -72,6 +73,7 @@ To use an RPC function:
options:
members:
- list_
- disconnect
- ConfiguredDatabaseInfo

## Database Privileges
Expand Down Expand Up @@ -252,6 +254,7 @@ To use an RPC function:
members:
- list_
- add
- delete
- get_current_role
- RoleInfo
- RoleMember
Expand Down
17 changes: 17 additions & 0 deletions mathesar/rpc/databases/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

from mathesar.rpc.utils import connect
from db.databases.operations.select import get_database
from db.databases.operations.drop import drop_database
from mathesar.rpc.exceptions.handlers import handle_rpc_exceptions


Expand Down Expand Up @@ -53,3 +54,19 @@ def get(*, database_id: int, **kwargs) -> DatabaseInfo:
with connect(database_id, user) as conn:
db_info = get_database(conn)
return DatabaseInfo.from_dict(db_info)


@rpc_method(name="databases.delete")
@http_basic_auth_login_required
@handle_rpc_exceptions
def delete(*, database_oid: int, database_id: int, **kwargs) -> None:
"""
Drop a database from the server.

Args:
database_oid: The OID of the database to delete on the database.
database_id: The Django id of the database to connect to.
"""
user = kwargs.get(REQUEST_KEY).user
with connect(database_id, user) as conn:
drop_database(database_oid, conn)
Comment on lines +63 to +72
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been thinking more about this and we should only be providing delete for databases created in the internal server. Only Mathesar admins should be able to perform this.

When the user requests a delete, these conditions should match:

  1. The user should be a Mathesar admin
  2. The database should be present within the internal db server
  3. The collaborator must use a role that owns the database

And then, in a single transaction:

  • connect to the database that's about to be deleted
  • change ownership to the internal db role that we use in mathesar_django
  • disconnect
  • connect to the internal db server with the internal db role
  • drop the database

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing needed in this PR, we can do this for beta.

14 changes: 14 additions & 0 deletions mathesar/rpc/databases/configured.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,3 +50,17 @@ def list_(*, server_id: int = None, **kwargs) -> list[ConfiguredDatabaseInfo]:
database_qs = Database.objects.all()

return [ConfiguredDatabaseInfo.from_model(db_model) for db_model in database_qs]


@rpc_method(name="databases.configured.disconnect")
@http_basic_auth_login_required
@handle_rpc_exceptions
def disconnect(*, database_id: int, **kwargs) -> None:
"""
Disconnect a configured database.

Args:
database_id: The Django id of the database.
"""
database_qs = Database.objects.get(id=database_id)
database_qs.delete()
Comment on lines +65 to +66
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to discuss on how to remove the Mathesar schemas. This requires a broader discussion on how we'll retain/request the user for the role & password for the role that owns the schemas.

We can get back to this before beta.

22 changes: 22 additions & 0 deletions mathesar/rpc/roles/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from mathesar.rpc.utils import connect
from db.roles.operations.select import list_roles, get_current_role_from_db
from db.roles.operations.create import create_role
from db.roles.operations.drop import drop_role


class RoleMember(TypedDict):
Expand Down Expand Up @@ -118,6 +119,27 @@ def add(
return RoleInfo.from_dict(role)


@rpc_method(name="roles.delete")
@http_basic_auth_login_required
@handle_rpc_exceptions
def delete(
*,
role_oid: int,
database_id: int,
**kwargs
) -> None:
"""
Drop a role on a database server.

Args:
role_oid: The OID of the role to drop on the database.
database_id: The Django id of the database.
"""
user = kwargs.get(REQUEST_KEY).user
with connect(database_id, user) as conn:
drop_role(role_oid, conn)


@rpc_method(name="roles.get_current_role")
@http_basic_auth_login_required
@handle_rpc_exceptions
Expand Down
15 changes: 15 additions & 0 deletions mathesar/tests/rpc/test_endpoints.py
Original file line number Diff line number Diff line change
Expand Up @@ -144,12 +144,22 @@
"databases.get",
[user_is_authenticated]
),
(
databases.delete,
"databases.delete",
[user_is_authenticated]
),

(
databases.configured.list_,
"databases.configured.list",
[user_is_authenticated]
),
(
databases.configured.disconnect,
"databases.configured.disconnect",
[user_is_authenticated]
),

(
databases.privileges.list_direct,
Expand Down Expand Up @@ -255,6 +265,11 @@
"roles.add",
[user_is_authenticated]
),
(
roles.delete,
"roles.delete",
[user_is_authenticated]
),
(
roles.get_current_role,
"roles.get_current_role",
Expand Down
30 changes: 30 additions & 0 deletions mathesar/tests/rpc/test_roles.py
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,36 @@ def mock_create_role(rolename, password, login, conn):
roles.add(rolename=_username, database_id=_database_id, password=_password, login=True, request=request)


def test_roles_delete(rf, monkeypatch):
_username = 'alice'
_password = 'pass1234'
_database_id = 2
_role_oid = 10
request = rf.post('/api/rpc/v0/', data={})
request.user = User(username=_username, password=_password)

@contextmanager
def mock_connect(database_id, user):
if database_id == _database_id and user.username == _username:
try:
yield True
finally:
pass
else:
raise AssertionError('incorrect parameters passed')

def mock_drop_role(role_oid, conn):
if (
role_oid != _role_oid
):
raise AssertionError('incorrect parameters passed')
return None

monkeypatch.setattr(roles.base, 'connect', mock_connect)
monkeypatch.setattr(roles.base, 'drop_role', mock_drop_role)
roles.delete(role_oid=_role_oid, database_id=_database_id, request=request)


def test_get_current_role(rf, monkeypatch):
_username = 'alice'
_password = 'pass1234'
Expand Down
Loading