korbit-ai[bot] commented on code in PR #31909:
URL: https://github.com/apache/superset/pull/31909#discussion_r1920676103
##########
upmetrics/superset_confug.py:
##########
@@ -0,0 +1,151 @@
+# superset_config.py
+
+# ---------------------------------------------------------
+# This is a superset in inframe auth implementation prototype
+# --------------------------------------------------------
+from flask import current_app, request, jsonify
+from flask_wtf.csrf import generate_csrf
+from superset.security import SupersetSecurityManager
+from flask import current_app, request, jsonify
+from flask_login import login_user
+import jwt
+import datetime
+from flask_cors import CORS
+
+class CustomSecurityManager(SupersetSecurityManager):
+ def __init__(self, appbuilder):
+ super().__init__(appbuilder)
+
+ def create_org_role(self, org_id):
+ """Create a role for an organization if it doesn't exist"""
+ role_name = f"org_{org_id}"
+ role = self.find_role(role_name)
+ if not role:
+ role = self.add_role(role_name)
+ # Inherit Gamma permissions
+ gamma_role = self.find_role("Gamma")
+ for perm in gamma_role.permissions:
+ self.add_permission_role(role, perm)
+ return role
+
+ def create_org_user(self, username, org_id, email=None):
+ """Create a user with organization-specific role"""
+ user = self.find_user(username=username)
+ if not user:
+ org_role = self.create_org_role(org_id)
+ user = self.add_user(
+ username=username,
+ email=username,
+ first_name="",
+ last_name="",
+ role=org_role,
+ )
+ return user
+
+
+def custom_init_app(app):
+ CORS(app,
+ resources={r"/api/v1/security/*": {"origins":
["http://localhost:7777","https://next-staging.upmetrics.com";],
+ "supports_credentials": True}},
+ allow_headers=["Content-Type", "Authorization"],
+ expose_headers=["Content-Type", "Authorization"])
+
+ def security_endpoints():
+
+ @app.route('/api/v1/security/guest_token/<username>/<org_id>',
methods=['GET'])
+ def get_guest_token(username, org_id):
+ sm = current_app.appbuilder.sm
+ user = sm.create_org_user(username, org_id)
+
+ token = jwt.encode(
+ {
+ 'username': username,
+ 'org_id': org_id,
+ 'exp': datetime.datetime.now(datetime.timezone.utc) +
datetime.timedelta(hours=24)
+ },
+ current_app.config['SECRET_KEY'],
+ algorithm='HS256'
+ )
Review Comment:
### Switch to an asymmetric signing algorithm for JWT tokens.
<sub></sub>
<details>
<summary>Tell me more</summary>
​
The get_guest_token endpoint signs the JWT token using the HS256 algorithm
and the app's SECRET_KEY as the signing key. This is vulnerable to token
forgery if the SECRET_KEY is compromised. Consider using an asymmetric signing
algorithm like RS256 and a dedicated secret for signing tokens.
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help
Korbit improve your reviews.
<!--- korbi internal id:57a90f38-5010-4d4c-bb18-850c12ce047a -->
##########
upmetrics/superset_confug.py:
##########
@@ -0,0 +1,151 @@
+# superset_config.py
+
+# ---------------------------------------------------------
+# This is a superset in inframe auth implementation prototype
+# --------------------------------------------------------
+from flask import current_app, request, jsonify
+from flask_wtf.csrf import generate_csrf
+from superset.security import SupersetSecurityManager
+from flask import current_app, request, jsonify
+from flask_login import login_user
+import jwt
+import datetime
+from flask_cors import CORS
+
+class CustomSecurityManager(SupersetSecurityManager):
+ def __init__(self, appbuilder):
+ super().__init__(appbuilder)
+
+ def create_org_role(self, org_id):
+ """Create a role for an organization if it doesn't exist"""
+ role_name = f"org_{org_id}"
+ role = self.find_role(role_name)
+ if not role:
+ role = self.add_role(role_name)
+ # Inherit Gamma permissions
+ gamma_role = self.find_role("Gamma")
+ for perm in gamma_role.permissions:
+ self.add_permission_role(role, perm)
+ return role
+
+ def create_org_user(self, username, org_id, email=None):
+ """Create a user with organization-specific role"""
+ user = self.find_user(username=username)
+ if not user:
+ org_role = self.create_org_role(org_id)
+ user = self.add_user(
+ username=username,
+ email=username,
+ first_name="",
+ last_name="",
+ role=org_role,
+ )
+ return user
+
+
+def custom_init_app(app):
+ CORS(app,
+ resources={r"/api/v1/security/*": {"origins":
["http://localhost:7777","https://next-staging.upmetrics.com";],
+ "supports_credentials": True}},
+ allow_headers=["Content-Type", "Authorization"],
+ expose_headers=["Content-Type", "Authorization"])
+
+ def security_endpoints():
+
+ @app.route('/api/v1/security/guest_token/<username>/<org_id>',
methods=['GET'])
+ def get_guest_token(username, org_id):
+ sm = current_app.appbuilder.sm
+ user = sm.create_org_user(username, org_id)
+
+ token = jwt.encode(
+ {
+ 'username': username,
+ 'org_id': org_id,
+ 'exp': datetime.datetime.now(datetime.timezone.utc) +
datetime.timedelta(hours=24)
Review Comment:
### Consider reducing the guest JWT token expiration time. <sub></sub>
<details>
<summary>Tell me more</summary>
​
The guest JWT token is set to expire after 24 hours. This long expiration
time increases the risk if a token is stolen. Consider reducing the expiration
to a few minutes and using refresh tokens for longer sessions.
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help
Korbit improve your reviews.
<!--- korbi internal id:abdb0fdb-b003-45df-b26e-5f24c2dee7fc -->
##########
upmetrics/superset_confug.py:
##########
@@ -0,0 +1,151 @@
+# superset_config.py
+
+# ---------------------------------------------------------
+# This is a superset in inframe auth implementation prototype
+# --------------------------------------------------------
+from flask import current_app, request, jsonify
+from flask_wtf.csrf import generate_csrf
+from superset.security import SupersetSecurityManager
+from flask import current_app, request, jsonify
+from flask_login import login_user
+import jwt
+import datetime
+from flask_cors import CORS
+
+class CustomSecurityManager(SupersetSecurityManager):
+ def __init__(self, appbuilder):
+ super().__init__(appbuilder)
+
+ def create_org_role(self, org_id):
+ """Create a role for an organization if it doesn't exist"""
+ role_name = f"org_{org_id}"
+ role = self.find_role(role_name)
+ if not role:
+ role = self.add_role(role_name)
+ # Inherit Gamma permissions
+ gamma_role = self.find_role("Gamma")
+ for perm in gamma_role.permissions:
+ self.add_permission_role(role, perm)
+ return role
+
+ def create_org_user(self, username, org_id, email=None):
+ """Create a user with organization-specific role"""
+ user = self.find_user(username=username)
+ if not user:
+ org_role = self.create_org_role(org_id)
+ user = self.add_user(
+ username=username,
+ email=username,
+ first_name="",
+ last_name="",
+ role=org_role,
+ )
+ return user
+
+
+def custom_init_app(app):
+ CORS(app,
+ resources={r"/api/v1/security/*": {"origins":
["http://localhost:7777","https://next-staging.upmetrics.com";],
+ "supports_credentials": True}},
+ allow_headers=["Content-Type", "Authorization"],
+ expose_headers=["Content-Type", "Authorization"])
+
+ def security_endpoints():
+
+ @app.route('/api/v1/security/guest_token/<username>/<org_id>',
methods=['GET'])
+ def get_guest_token(username, org_id):
+ sm = current_app.appbuilder.sm
+ user = sm.create_org_user(username, org_id)
+
+ token = jwt.encode(
+ {
+ 'username': username,
+ 'org_id': org_id,
+ 'exp': datetime.datetime.now(datetime.timezone.utc) +
datetime.timedelta(hours=24)
+ },
+ current_app.config['SECRET_KEY'],
+ algorithm='HS256'
+ )
+ return jsonify({'token': token})
+
+ @app.route('/api/v1/security/authenticate', methods=['GET'])
+ def authenticate_token():
+ token = request.args.get('token')
+ if not token:
+ return jsonify({'error': 'Token is required'}), 401
+
+ try:
+ payload = jwt.decode(token, current_app.config['SECRET_KEY'],
algorithms=['HS256'])
+ username = payload['username']
+
+ sm = current_app.appbuilder.sm
+ user = sm.find_user(username=username)
+
+ if user:
+ login_user(user)
+ return jsonify({'message': 'Logged in successfully',
'username': username})
+
+ except jwt.ExpiredSignatureError:
+ return jsonify({'error': 'Token has expired'}), 401
+ except jwt.InvalidTokenError:
+ return jsonify({'error': 'Invalid token'}), 401
+
+ return jsonify({'error': 'User not found'}), 404
+
+ # @app.route('/api/v1/security/csrf_token', methods=['GET'])
+ # def csrf_token():
+ # token = generate_csrf()
+ # return jsonify({
+ # 'result': token
+ # })
+
+ @app.route('/api/v1/security/create_user', methods=['POST'])
+ def create_user():
+ if not request.is_json:
+ return jsonify({'error': 'Request must be JSON'}), 400
+
+ data = request.json
+ sm = current_app.appbuilder.sm
+
+ user = sm.find_user(username=data.get('username'))
+ if user:
+ return jsonify({'error': 'User already exists'}), 400
+
+ try:
+ user = sm.add_user(
+ username=data.get('username'),
+ first_name=data.get('first_name', ''),
+ last_name=data.get('last_name', ''),
+ email=data.get('email', ''),
+ role=sm.find_role('Gamma')
+ )
Review Comment:
### Add validation for username in create_user endpoint. <sub></sub>
<details>
<summary>Tell me more</summary>
​
The create_user endpoint does not validate that a username is provided.
Passing an empty or missing username could result in an invalid user being
created. Add validation to ensure all required fields are provided.
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help
Korbit improve your reviews.
<!--- korbi internal id:36e707a5-c19a-48b9-9c2a-cf72ca934879 -->
##########
upmetrics/superset_confug.py:
##########
@@ -0,0 +1,151 @@
+# superset_config.py
+
+# ---------------------------------------------------------
+# This is a superset in inframe auth implementation prototype
+# --------------------------------------------------------
+from flask import current_app, request, jsonify
+from flask_wtf.csrf import generate_csrf
+from superset.security import SupersetSecurityManager
+from flask import current_app, request, jsonify
+from flask_login import login_user
+import jwt
+import datetime
+from flask_cors import CORS
+
+class CustomSecurityManager(SupersetSecurityManager):
+ def __init__(self, appbuilder):
+ super().__init__(appbuilder)
+
+ def create_org_role(self, org_id):
+ """Create a role for an organization if it doesn't exist"""
+ role_name = f"org_{org_id}"
+ role = self.find_role(role_name)
+ if not role:
+ role = self.add_role(role_name)
+ # Inherit Gamma permissions
+ gamma_role = self.find_role("Gamma")
+ for perm in gamma_role.permissions:
+ self.add_permission_role(role, perm)
+ return role
+
+ def create_org_user(self, username, org_id, email=None):
+ """Create a user with organization-specific role"""
+ user = self.find_user(username=username)
+ if not user:
+ org_role = self.create_org_role(org_id)
+ user = self.add_user(
+ username=username,
+ email=username,
+ first_name="",
+ last_name="",
+ role=org_role,
+ )
+ return user
+
+
+def custom_init_app(app):
+ CORS(app,
+ resources={r"/api/v1/security/*": {"origins":
["http://localhost:7777","https://next-staging.upmetrics.com";],
+ "supports_credentials": True}},
+ allow_headers=["Content-Type", "Authorization"],
+ expose_headers=["Content-Type", "Authorization"])
+
+ def security_endpoints():
+
+ @app.route('/api/v1/security/guest_token/<username>/<org_id>',
methods=['GET'])
+ def get_guest_token(username, org_id):
+ sm = current_app.appbuilder.sm
+ user = sm.create_org_user(username, org_id)
+
+ token = jwt.encode(
+ {
+ 'username': username,
+ 'org_id': org_id,
+ 'exp': datetime.datetime.now(datetime.timezone.utc) +
datetime.timedelta(hours=24)
+ },
+ current_app.config['SECRET_KEY'],
+ algorithm='HS256'
+ )
+ return jsonify({'token': token})
+
+ @app.route('/api/v1/security/authenticate', methods=['GET'])
+ def authenticate_token():
+ token = request.args.get('token')
+ if not token:
+ return jsonify({'error': 'Token is required'}), 401
+
+ try:
+ payload = jwt.decode(token, current_app.config['SECRET_KEY'],
algorithms=['HS256'])
+ username = payload['username']
+
+ sm = current_app.appbuilder.sm
+ user = sm.find_user(username=username)
+
+ if user:
+ login_user(user)
+ return jsonify({'message': 'Logged in successfully',
'username': username})
+
+ except jwt.ExpiredSignatureError:
+ return jsonify({'error': 'Token has expired'}), 401
+ except jwt.InvalidTokenError:
+ return jsonify({'error': 'Invalid token'}), 401
+
+ return jsonify({'error': 'User not found'}), 404
+
+ # @app.route('/api/v1/security/csrf_token', methods=['GET'])
+ # def csrf_token():
+ # token = generate_csrf()
+ # return jsonify({
+ # 'result': token
+ # })
+
+ @app.route('/api/v1/security/create_user', methods=['POST'])
+ def create_user():
+ if not request.is_json:
+ return jsonify({'error': 'Request must be JSON'}), 400
+
+ data = request.json
+ sm = current_app.appbuilder.sm
+
+ user = sm.find_user(username=data.get('username'))
+ if user:
+ return jsonify({'error': 'User already exists'}), 400
+
+ try:
+ user = sm.add_user(
+ username=data.get('username'),
+ first_name=data.get('first_name', ''),
+ last_name=data.get('last_name', ''),
+ email=data.get('email', ''),
+ role=sm.find_role('Gamma')
+ )
+ return jsonify({
+ 'message': 'User created successfully',
+ 'username': user.username
+ })
+ except Exception as e:
+ return jsonify({'error': str(e)}), 500
Review Comment:
### Overly broad exception handling with exposed details <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Using a bare Exception catch and exposing the raw error message to the
client in create_user endpoint.
###### Why this matters
Exposing internal error details to clients could reveal sensitive system
information and pose security risks. Additionally, catching all exceptions
makes it harder to handle specific error cases appropriately.
###### Suggested change ∙ *Feature Preview*
Catch specific exceptions and return sanitized error messages:
except (ValueError, AttributeError) as e:
current_app.logger.error(f'User creation failed: {str(e)}')
return jsonify({'error': 'Failed to create user'}), 500
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help
Korbit improve your reviews.
<!--- korbi internal id:0a0a9b8d-7094-49d5-866d-258d26e0e072 -->
##########
upmetrics/superset_confug.py:
##########
@@ -0,0 +1,151 @@
+# superset_config.py
+
+# ---------------------------------------------------------
+# This is a superset in inframe auth implementation prototype
+# --------------------------------------------------------
+from flask import current_app, request, jsonify
+from flask_wtf.csrf import generate_csrf
+from superset.security import SupersetSecurityManager
+from flask import current_app, request, jsonify
+from flask_login import login_user
+import jwt
+import datetime
+from flask_cors import CORS
+
+class CustomSecurityManager(SupersetSecurityManager):
+ def __init__(self, appbuilder):
+ super().__init__(appbuilder)
+
+ def create_org_role(self, org_id):
+ """Create a role for an organization if it doesn't exist"""
+ role_name = f"org_{org_id}"
+ role = self.find_role(role_name)
+ if not role:
+ role = self.add_role(role_name)
+ # Inherit Gamma permissions
+ gamma_role = self.find_role("Gamma")
+ for perm in gamma_role.permissions:
+ self.add_permission_role(role, perm)
+ return role
+
+ def create_org_user(self, username, org_id, email=None):
+ """Create a user with organization-specific role"""
+ user = self.find_user(username=username)
+ if not user:
+ org_role = self.create_org_role(org_id)
+ user = self.add_user(
+ username=username,
+ email=username,
+ first_name="",
+ last_name="",
+ role=org_role,
+ )
+ return user
Review Comment:
### Invalid Email Assignment <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The create_org_user function uses username as email without validation,
which could create invalid user records.
###### Why this matters
Invalid email addresses could prevent user notifications, break
email-dependent features, and cause system integration issues.
###### Suggested change ∙ *Feature Preview*
Use the provided email parameter or validate username as email:
```python
def create_org_user(self, username, org_id, email=None):
"""Create a user with organization-specific role"""
user = self.find_user(username=username)
if not user:
org_role = self.create_org_role(org_id)
user_email = email if email else f"{username}@upmetrics.com" # Or
proper email validation
user = self.add_user(
username=username,
email=user_email,
first_name="",
last_name="",
role=org_role,
)
return user
```
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help
Korbit improve your reviews.
<!--- korbi internal id:b15d51c8-fe54-4f82-a0e3-402a0b497fea -->
##########
upmetrics/superset_confug.py:
##########
@@ -0,0 +1,151 @@
+# superset_config.py
+
+# ---------------------------------------------------------
+# This is a superset in inframe auth implementation prototype
+# --------------------------------------------------------
+from flask import current_app, request, jsonify
+from flask_wtf.csrf import generate_csrf
+from superset.security import SupersetSecurityManager
+from flask import current_app, request, jsonify
+from flask_login import login_user
+import jwt
+import datetime
+from flask_cors import CORS
+
+class CustomSecurityManager(SupersetSecurityManager):
+ def __init__(self, appbuilder):
+ super().__init__(appbuilder)
+
+ def create_org_role(self, org_id):
+ """Create a role for an organization if it doesn't exist"""
+ role_name = f"org_{org_id}"
+ role = self.find_role(role_name)
+ if not role:
+ role = self.add_role(role_name)
+ # Inherit Gamma permissions
+ gamma_role = self.find_role("Gamma")
+ for perm in gamma_role.permissions:
+ self.add_permission_role(role, perm)
+ return role
+
+ def create_org_user(self, username, org_id, email=None):
+ """Create a user with organization-specific role"""
+ user = self.find_user(username=username)
+ if not user:
+ org_role = self.create_org_role(org_id)
+ user = self.add_user(
+ username=username,
+ email=username,
+ first_name="",
+ last_name="",
+ role=org_role,
+ )
+ return user
+
+
+def custom_init_app(app):
+ CORS(app,
+ resources={r"/api/v1/security/*": {"origins":
["http://localhost:7777","https://next-staging.upmetrics.com";],
+ "supports_credentials": True}},
+ allow_headers=["Content-Type", "Authorization"],
+ expose_headers=["Content-Type", "Authorization"])
+
+ def security_endpoints():
+
+ @app.route('/api/v1/security/guest_token/<username>/<org_id>',
methods=['GET'])
+ def get_guest_token(username, org_id):
+ sm = current_app.appbuilder.sm
+ user = sm.create_org_user(username, org_id)
+
+ token = jwt.encode(
+ {
+ 'username': username,
+ 'org_id': org_id,
+ 'exp': datetime.datetime.now(datetime.timezone.utc) +
datetime.timedelta(hours=24)
+ },
+ current_app.config['SECRET_KEY'],
+ algorithm='HS256'
+ )
+ return jsonify({'token': token})
+
+ @app.route('/api/v1/security/authenticate', methods=['GET'])
+ def authenticate_token():
+ token = request.args.get('token')
+ if not token:
+ return jsonify({'error': 'Token is required'}), 401
Review Comment:
### Token Exposure in URL Parameters <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The authenticate_token endpoint accepts tokens via query parameters, which
exposes them in URLs and server logs.
###### Why this matters
Sensitive authentication tokens in URLs can be leaked through browser
history, server logs, and referrer headers, creating security vulnerabilities.
###### Suggested change ∙ *Feature Preview*
Use Authorization header instead of query parameters:
```python
def authenticate_token():
auth_header = request.headers.get('Authorization')
if not auth_header or not auth_header.startswith('Bearer '):
return jsonify({'error': 'Bearer token required'}), 401
token = auth_header.split(' ')[1]
```
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help
Korbit improve your reviews.
<!--- korbi internal id:a1a52b8b-826f-41c0-b89b-25fa97c3121e -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
