Skip to content

feat: introduce "protocol" and "method" props for request param in checkSqlAuth #9525

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open
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
11 changes: 10 additions & 1 deletion packages/cubejs-api-gateway/src/sql-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@
gatewayPort?: number,
};

export type SqlAuthServiceAuthenticateRequest = {
protocol: string;
method: string;
};

export class SQLServer {
protected sqlInterfaceInstance: SqlInterfaceInstance | null = null;

Expand Down Expand Up @@ -88,10 +93,14 @@
let { securityContext } = session;

if (request.meta.changeUser && request.meta.changeUser !== session.user) {
const sqlAuthRequest: SqlAuthServiceAuthenticateRequest = {

Check warning on line 96 in packages/cubejs-api-gateway/src/sql-server.ts

View check run for this annotation

Codecov / codecov/patch

packages/cubejs-api-gateway/src/sql-server.ts#L96

Added line #L96 was not covered by tests
protocol: request.meta.protocol,
method: 'password',
};
const canSwitch = session.superuser || await canSwitchSqlUser(session.user, request.meta.changeUser);
if (canSwitch) {
userForContext = request.meta.changeUser;
const current = await checkSqlAuth(request, userForContext, null);
const current = await checkSqlAuth({ ...request, ...sqlAuthRequest }, userForContext, null);

Check warning on line 103 in packages/cubejs-api-gateway/src/sql-server.ts

View check run for this annotation

Codecov / codecov/patch

packages/cubejs-api-gateway/src/sql-server.ts#L103

Added line #L103 was not covered by tests
securityContext = current.securityContext;
} else {
throw new Error(
Expand Down
28 changes: 25 additions & 3 deletions packages/cubejs-backend-native/src/auth.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use async_trait::async_trait;
use cubesql::{
di_service,
sql::{AuthContext, AuthenticateResponse, SqlAuthService},
sql::{AuthContext, AuthenticateResponse, SqlAuthService, SqlAuthServiceAuthenticateRequest},
transport::LoadRequestMeta,
CubeError,
};
Expand Down Expand Up @@ -50,9 +50,28 @@ pub struct TransportRequest {
pub meta: Option<LoadRequestMeta>,
}

#[derive(Debug, Serialize)]
pub struct TransportAuthRequest {
pub id: String,
pub meta: Option<LoadRequestMeta>,
pub protocol: String,
pub method: String,
}

impl From<(TransportRequest, SqlAuthServiceAuthenticateRequest)> for TransportAuthRequest {
fn from((t, a): (TransportRequest, SqlAuthServiceAuthenticateRequest)) -> Self {
Self {
id: t.id,
meta: t.meta,
protocol: a.protocol,
method: a.method,
}
}
}

#[derive(Debug, Serialize)]
struct CheckSQLAuthTransportRequest {
request: TransportRequest,
request: TransportAuthRequest,
user: Option<String>,
password: Option<String>,
}
Expand Down Expand Up @@ -92,6 +111,7 @@ impl AuthContext for NativeSQLAuthContext {
impl SqlAuthService for NodeBridgeAuthService {
async fn authenticate(
&self,
request: SqlAuthServiceAuthenticateRequest,
user: Option<String>,
password: Option<String>,
) -> Result<AuthenticateResponse, CubeError> {
Expand All @@ -100,9 +120,11 @@ impl SqlAuthService for NodeBridgeAuthService {
let request_id = Uuid::new_v4().to_string();

let extra = serde_json::to_string(&CheckSQLAuthTransportRequest {
request: TransportRequest {
request: TransportAuthRequest {
id: format!("{}-span-1", request_id),
meta: None,
protocol: request.protocol,
method: request.method,
},
user: user.clone(),
password: password.clone(),
Expand Down
4 changes: 4 additions & 0 deletions packages/cubejs-backend-native/test/sql.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,8 @@ describe('SQLInterface', () => {
request: {
id: expect.any(String),
meta: null,
method: expect.any(String),
protocol: expect.any(String),
},
user: user || null,
password:
Expand Down Expand Up @@ -257,6 +259,8 @@ describe('SQLInterface', () => {
request: {
id: expect.any(String),
meta: null,
method: expect.any(String),
protocol: expect.any(String),
},
user: 'allowed_user',
password: 'password_for_allowed_user',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,15 @@ module.exports = {
return query;
},
checkSqlAuth: async (req, user, password) => {
if (!req) {
throw new Error('Request is not defined');
}

const missing = ['protocol', 'method'].filter(key => !(key in req));
if (missing.length) {
throw new Error(`Request object is missing required field(s): ${missing.join(', ')}`);
}

if (user === 'admin') {
if (password && password !== 'admin_password') {
throw new Error(`Password doesn't match for ${user}`);
Expand Down
13 changes: 11 additions & 2 deletions rust/cubesql/cubesql/src/compile/router.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
DatabaseVariable, DatabaseVariablesToUpdate,
},
sql::{
auth_service::SqlAuthServiceAuthenticateRequest,
dataframe,
statement::{
ApproximateCountDistinctVisitor, CastReplacer, DateTokenNormalizeReplacer,
Expand Down Expand Up @@ -447,12 +448,16 @@
})?
{
self.state.set_user(Some(to_user.clone()));
let sql_auth_request = SqlAuthServiceAuthenticateRequest {
protocol: "postgres".to_string(),
method: "password".to_string(),
};
let authenticate_response = self
.session_manager
.server
.auth
// TODO do we want to send actual password here?
.authenticate(Some(to_user.clone()), None)
.authenticate(sql_auth_request, Some(to_user.clone()), None)
.await
.map_err(|e| {
CompilationError::internal(format!("Error calling authenticate: {}", e))
Expand Down Expand Up @@ -562,11 +567,15 @@

async fn reauthenticate_if_needed(&self) -> CompilationResult<()> {
if self.state.is_auth_context_expired() {
let sql_auth_request = SqlAuthServiceAuthenticateRequest {
protocol: "postgres".to_string(),
method: "password".to_string(),
};

Check warning on line 573 in rust/cubesql/cubesql/src/compile/router.rs

View check run for this annotation

Codecov / codecov/patch

rust/cubesql/cubesql/src/compile/router.rs#L570-L573

Added lines #L570 - L573 were not covered by tests
let authenticate_response = self
.session_manager
.server
.auth
.authenticate(self.state.user(), None)
.authenticate(sql_auth_request, self.state.user(), None)

Check warning on line 578 in rust/cubesql/cubesql/src/compile/router.rs

View check run for this annotation

Codecov / codecov/patch

rust/cubesql/cubesql/src/compile/router.rs#L578

Added line #L578 was not covered by tests
.await
.map_err(|e| {
CompilationError::fatal(format!(
Expand Down
8 changes: 5 additions & 3 deletions rust/cubesql/cubesql/src/compile/test/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,10 @@ use crate::{
},
config::{ConfigObj, ConfigObjImpl},
sql::{
compiler_cache::CompilerCacheImpl, dataframe::batches_to_dataframe,
pg_auth_service::PostgresAuthServiceDefaultImpl, AuthContextRef, AuthenticateResponse,
HttpAuthContext, ServerManager, Session, SessionManager, SqlAuthService,
auth_service::SqlAuthServiceAuthenticateRequest, compiler_cache::CompilerCacheImpl,
dataframe::batches_to_dataframe, pg_auth_service::PostgresAuthServiceDefaultImpl,
AuthContextRef, AuthenticateResponse, HttpAuthContext, ServerManager, Session,
SessionManager, SqlAuthService,
},
transport::{
CubeMeta, CubeMetaDimension, CubeMetaJoin, CubeMetaMeasure, CubeMetaSegment,
Expand Down Expand Up @@ -750,6 +751,7 @@ pub fn get_test_auth() -> Arc<dyn SqlAuthService> {
impl SqlAuthService for TestSqlAuth {
async fn authenticate(
&self,
_request: SqlAuthServiceAuthenticateRequest,
_user: Option<String>,
password: Option<String>,
) -> Result<AuthenticateResponse, CubeError> {
Expand Down
9 changes: 9 additions & 0 deletions rust/cubesql/cubesql/src/sql/auth_service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use std::{any::Any, env, fmt::Debug, sync::Arc};

use crate::CubeError;
use async_trait::async_trait;
use serde::{Deserialize, Serialize};
use serde_json::Value;

// We cannot use generic here. It's why there is this trait
Expand Down Expand Up @@ -43,10 +44,17 @@ pub struct AuthenticateResponse {
pub skip_password_check: bool,
}

#[derive(Debug, Clone, Serialize, Deserialize)]
pub struct SqlAuthServiceAuthenticateRequest {
pub protocol: String,
pub method: String,
}

#[async_trait]
pub trait SqlAuthService: Send + Sync + Debug {
async fn authenticate(
&self,
request: SqlAuthServiceAuthenticateRequest,
user: Option<String>,
password: Option<String>,
) -> Result<AuthenticateResponse, CubeError>;
Expand All @@ -61,6 +69,7 @@ crate::di_service!(SqlAuthDefaultImpl, [SqlAuthService]);
impl SqlAuthService for SqlAuthDefaultImpl {
async fn authenticate(
&self,
_request: SqlAuthServiceAuthenticateRequest,
_user: Option<String>,
password: Option<String>,
) -> Result<AuthenticateResponse, CubeError> {
Expand Down
2 changes: 1 addition & 1 deletion rust/cubesql/cubesql/src/sql/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ pub(crate) mod types;
// Public API
pub use auth_service::{
AuthContext, AuthContextRef, AuthenticateResponse, HttpAuthContext, SqlAuthDefaultImpl,
SqlAuthService,
SqlAuthService, SqlAuthServiceAuthenticateRequest,
};
pub use database_variables::postgres::session_vars::CUBESQL_PENALIZE_POST_PROCESSING_VAR;
pub use postgres::*;
Expand Down
11 changes: 10 additions & 1 deletion rust/cubesql/cubesql/src/sql/postgres/pg_auth_service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use std::{collections::HashMap, fmt::Debug, sync::Arc};
use async_trait::async_trait;

use crate::{
sql::auth_service::SqlAuthServiceAuthenticateRequest,
sql::{AuthContextRef, SqlAuthService},
CubeError,
};
Expand Down Expand Up @@ -74,8 +75,16 @@ impl PostgresAuthService for PostgresAuthServiceDefaultImpl {
}

let user = parameters.get("user").unwrap().clone();
let sql_auth_request = SqlAuthServiceAuthenticateRequest {
protocol: "postgres".to_string(),
method: "password".to_string(),
};
let authenticate_response = service
.authenticate(Some(user.clone()), Some(password_message.password.clone()))
.authenticate(
sql_auth_request,
Some(user.clone()),
Some(password_message.password.clone()),
)
.await;

let auth_fail = || {
Expand Down
Loading