Skip to content

Commit ab7c1e6

Browse files
authored
refactor!: remove permission check hook and auth manager from core path (#258)
* refactor: drop table level permission control * refactor!: remove permission hook and AuthManager from core path * chore: update some comments * chore: remove integraion tests
1 parent 28c6637 commit ab7c1e6

File tree

7 files changed

+18
-235
lines changed

7 files changed

+18
-235
lines changed

datafusion-postgres-cli/src/main.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -198,8 +198,9 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
198198

199199
let session_config = SessionConfig::new().with_information_schema(true);
200200
let session_context = SessionContext::new_with_config(session_config);
201-
let auth_manager = Arc::new(AuthManager::new());
202201

202+
// TODO: remove or replace AuthManager for pg_catalog
203+
let auth_manager = Arc::new(AuthManager::new());
203204
setup_session_context(&session_context, &opts, Arc::clone(&auth_manager)).await?;
204205

205206
let server_options = ServerOptions::new()
@@ -208,7 +209,7 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
208209
.with_tls_cert_path(opts.tls_cert)
209210
.with_tls_key_path(opts.tls_key);
210211

211-
serve(Arc::new(session_context), &server_options, auth_manager)
212+
serve(Arc::new(session_context), &server_options)
212213
.await
213214
.map_err(|e| format!("Failed to run server: {e}"))?;
214215

datafusion-postgres/src/handlers.rs

Lines changed: 5 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,7 @@ use pgwire::api::{ClientInfo, ErrorHandler, PgWireServerHandlers, Type};
2222
use pgwire::error::{PgWireError, PgWireResult};
2323
use pgwire::types::format::FormatOptions;
2424

25-
use crate::auth::AuthManager;
2625
use crate::client;
27-
use crate::hooks::permissions::PermissionsHook;
2826
use crate::hooks::set_show::SetShowHook;
2927
use crate::hooks::transactions::TransactionStatementHook;
3028
use crate::hooks::QueryHook;
@@ -33,7 +31,6 @@ use arrow_pg::datatypes::{arrow_schema_to_pg_fields, into_pg_type};
3331
use datafusion_pg_catalog::sql::PostgresCompatibilityParser;
3432

3533
/// Simple startup handler that does no authentication
36-
/// For production, use DfAuthSource with proper pgwire authentication handlers
3734
pub struct SimpleStartupHandler;
3835

3936
#[async_trait::async_trait]
@@ -44,9 +41,8 @@ pub struct HandlerFactory {
4441
}
4542

4643
impl HandlerFactory {
47-
pub fn new(session_context: Arc<SessionContext>, auth_manager: Arc<AuthManager>) -> Self {
48-
let session_service =
49-
Arc::new(DfSessionService::new(session_context, auth_manager.clone()));
44+
pub fn new(session_context: Arc<SessionContext>) -> Self {
45+
let session_service = Arc::new(DfSessionService::new(session_context));
5046
HandlerFactory { session_service }
5147
}
5248

@@ -99,15 +95,9 @@ pub struct DfSessionService {
9995
}
10096

10197
impl DfSessionService {
102-
pub fn new(
103-
session_context: Arc<SessionContext>,
104-
auth_manager: Arc<AuthManager>,
105-
) -> DfSessionService {
106-
let hooks: Vec<Arc<dyn QueryHook>> = vec![
107-
Arc::new(PermissionsHook::new(auth_manager)),
108-
Arc::new(SetShowHook),
109-
Arc::new(TransactionStatementHook),
110-
];
98+
pub fn new(session_context: Arc<SessionContext>) -> DfSessionService {
99+
let hooks: Vec<Arc<dyn QueryHook>> =
100+
vec![Arc::new(SetShowHook), Arc::new(TransactionStatementHook)];
111101
Self::new_with_hooks(session_context, hooks)
112102
}
113103

datafusion-postgres/src/hooks/permissions.rs

Lines changed: 6 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -40,21 +40,21 @@ impl PermissionsHook {
4040
let query_trimmed = query_lower.trim();
4141

4242
let (required_permission, resource) = if query_trimmed.starts_with("select") {
43-
(Permission::Select, self.extract_table_from_query(query))
43+
(Permission::Select, ResourceType::All)
4444
} else if query_trimmed.starts_with("insert") {
45-
(Permission::Insert, self.extract_table_from_query(query))
45+
(Permission::Insert, ResourceType::All)
4646
} else if query_trimmed.starts_with("update") {
47-
(Permission::Update, self.extract_table_from_query(query))
47+
(Permission::Update, ResourceType::All)
4848
} else if query_trimmed.starts_with("delete") {
49-
(Permission::Delete, self.extract_table_from_query(query))
49+
(Permission::Delete, ResourceType::All)
5050
} else if query_trimmed.starts_with("create table")
5151
|| query_trimmed.starts_with("create view")
5252
{
5353
(Permission::Create, ResourceType::All)
5454
} else if query_trimmed.starts_with("drop") {
55-
(Permission::Drop, self.extract_table_from_query(query))
55+
(Permission::Drop, ResourceType::All)
5656
} else if query_trimmed.starts_with("alter") {
57-
(Permission::Alter, self.extract_table_from_query(query))
57+
(Permission::Alter, ResourceType::All)
5858
} else {
5959
// For other queries (SHOW, EXPLAIN, etc.), allow all users
6060
return Ok(());
@@ -78,25 +78,6 @@ impl PermissionsHook {
7878

7979
Ok(())
8080
}
81-
82-
/// Extract table name from query (simplified parsing)
83-
fn extract_table_from_query(&self, query: &str) -> ResourceType {
84-
let words: Vec<&str> = query.split_whitespace().collect();
85-
86-
// Simple heuristic to find table names
87-
for (i, word) in words.iter().enumerate() {
88-
let word_lower = word.to_lowercase();
89-
if (word_lower == "from" || word_lower == "into" || word_lower == "table")
90-
&& i + 1 < words.len()
91-
{
92-
let table_name = words[i + 1].trim_matches(|c| c == '(' || c == ')' || c == ';');
93-
return ResourceType::Table(table_name.to_string());
94-
}
95-
}
96-
97-
// If we can't determine the table, default to All
98-
ResourceType::All
99-
}
10081
}
10182

10283
#[async_trait]

datafusion-postgres/src/lib.rs

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ use tokio::sync::Semaphore;
2121
use tokio_rustls::rustls::{self, ServerConfig};
2222
use tokio_rustls::TlsAcceptor;
2323

24-
use crate::auth::AuthManager;
2524
use handlers::HandlerFactory;
2625
pub use handlers::{DfSessionService, Parser};
2726
pub use hooks::QueryHook;
@@ -86,10 +85,9 @@ fn setup_tls(cert_path: &str, key_path: &str) -> Result<TlsAcceptor, IOError> {
8685
pub async fn serve(
8786
session_context: Arc<SessionContext>,
8887
opts: &ServerOptions,
89-
auth_manager: Arc<AuthManager>,
9088
) -> Result<(), std::io::Error> {
9189
// Create the handler factory with authentication
92-
let factory = Arc::new(HandlerFactory::new(session_context, auth_manager));
90+
let factory = Arc::new(HandlerFactory::new(session_context));
9391

9492
serve_with_handlers(factory, opts).await
9593
}

datafusion-postgres/src/testing.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ pub fn setup_handlers() -> DfSessionService {
2121
)
2222
.expect("Failed to setup sesession context");
2323

24-
DfSessionService::new(Arc::new(session_context), Arc::new(AuthManager::new()))
24+
DfSessionService::new(Arc::new(session_context))
2525
}
2626

2727
#[derive(Debug, Default)]

tests-integration/test.sh

Lines changed: 2 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ wait_for_port() {
2020
local port=$1
2121
local timeout=30
2222
local count=0
23-
23+
2424
# Use netstat as fallback if lsof is not available
2525
while (lsof -Pi :$port -sTCP:LISTEN -t >/dev/null 2>&1) || (netstat -ln 2>/dev/null | grep ":$port " >/dev/null 2>&1); do
2626
if [ $count -ge $timeout ]; then
@@ -120,32 +120,6 @@ fi
120120
kill -9 $PARQUET_PID 2>/dev/null || true
121121
sleep 3
122122

123-
# Test 4: Role-Based Access Control
124-
echo ""
125-
echo "🔐 Test 4: Role-Based Access Control (RBAC)"
126-
echo "--------------------------------------------"
127-
wait_for_port 5435
128-
../target/debug/datafusion-postgres-cli -p 5435 --csv delhi:delhiclimate.csv &
129-
RBAC_PID=$!
130-
sleep 5
131-
132-
# Check if server is actually running
133-
if ! ps -p $RBAC_PID > /dev/null 2>&1; then
134-
echo "❌ RBAC server failed to start"
135-
exit 1
136-
fi
137-
138-
if python3 test_rbac.py; then
139-
echo "✅ RBAC test passed"
140-
else
141-
echo "❌ RBAC test failed"
142-
kill -9 $RBAC_PID 2>/dev/null || true
143-
exit 1
144-
fi
145-
146-
kill -9 $RBAC_PID 2>/dev/null || true
147-
sleep 3
148-
149123
# Test 5: SSL/TLS Security
150124
echo ""
151125
echo "🔒 Test 5: SSL/TLS Security Features"
@@ -177,12 +151,10 @@ echo "=========================================="
177151
echo ""
178152
echo "📈 Test Summary:"
179153
echo " ✅ Enhanced CSV data loading with PostgreSQL compatibility"
180-
echo " ✅ Complete transaction support (BEGIN/COMMIT/ROLLBACK)"
154+
echo " ✅ Complete transaction support (BEGIN/COMMIT/ROLLBACK)"
181155
echo " ✅ Enhanced Parquet data loading with advanced data types"
182156
echo " ✅ Array types and complex data type support"
183157
echo " ✅ Improved pg_catalog system tables"
184158
echo " ✅ PostgreSQL function compatibility"
185-
echo " ✅ Role-based access control (RBAC)"
186159
echo " ✅ SSL/TLS encryption support"
187160
echo ""
188-
echo "🚀 Ready for secure production PostgreSQL workloads!"

tests-integration/test_rbac.py

Lines changed: 0 additions & 159 deletions
This file was deleted.

0 commit comments

Comments
 (0)