korbit-ai[bot] commented on code in PR #33382: URL: https://github.com/apache/superset/pull/33382#discussion_r2077751365
########## superset/security/custom_initializer.py: ########## @@ -0,0 +1,18 @@ +from superset.app import SupersetAppInitializer +from superset.extensions import appbuilder +from superset.security import SupersetSecurityManager + + +class MySupsersetAppInitializer(SupersetAppInitializer): + def init_views(self) -> None: + # Adds api for roles + appbuilder.add_api(SupersetSecurityManager.role_api) Review Comment: ### Missing Error Handling in API Registration <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Multiple sequential API registrations are performed without error handling, which could fail silently if an API is already registered or if registration fails. ###### Why this matters If API registration fails, the application might continue with partially initialized views, leading to inconsistent application state and potential security vulnerabilities. ###### Suggested change ∙ *Feature Preview* ```python def init_views(self) -> None: try: # Adds api for roles appbuilder.add_api(SupersetSecurityManager.role_api) appbuilder.add_api(SupersetSecurityManager.permission_api) appbuilder.add_api(SupersetSecurityManager.user_api) appbuilder.add_api(SupersetSecurityManager.view_menu_api) appbuilder.add_api(SupersetSecurityManager.permission_view_menu_api) super().init_views() except Exception as e: # Log the specific error with context logger.error(f'Failed to initialize Superset views: {str(e)}') raise RuntimeError(f'Failed to initialize Superset views: {str(e)}') from e ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5bd5e4bb-696d-42a5-af90-09990037fa97/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5bd5e4bb-696d-42a5-af90-09990037fa97?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5bd5e4bb-696d-42a5-af90-09990037fa97?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5bd5e4bb-696d-42a5-af90-09990037fa97?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5bd5e4bb-696d-42a5-af90-09990037fa97) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:6baa02b0-587d-437c-8ba7-2ada26bd4d21 --> [](6baa02b0-587d-437c-8ba7-2ada26bd4d21) ########## auth/deploy.sh: ########## @@ -0,0 +1,39 @@ +#!/bin/bash + +# Function to build, push, and deploy a Docker image +build_push_deploy_prod() { + set -e + IMAGE_NAME="559615561845.dkr.ecr.ap-south-1.amazonaws.com/devops-scripts" + IMAGE_VERSION="superset-lambda-v1" + FUNCTION="superset_auth_lambda" + DOCKERFILE="Dockerfile" + BUILD_CONTEXT="." + + #Git PULL + + git pull origin master Review Comment: ### Unverified Git Source Code Pull <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Unvalidated git pull from master branch without verifying source or commit integrity ###### Why this matters Automatically pulling from git without verification could allow injection of malicious code if the repository is compromised ###### Suggested change ∙ *Feature Preview* Add commit verification and specific commit/tag pulling: ```bash git fetch origin master git verify-commit HEAD git reset --hard origin/master ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7f15762e-245e-4c39-b585-b150329b1900/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7f15762e-245e-4c39-b585-b150329b1900?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7f15762e-245e-4c39-b585-b150329b1900?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7f15762e-245e-4c39-b585-b150329b1900?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7f15762e-245e-4c39-b585-b150329b1900) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:014f2b94-756d-4539-9213-fe1c0dc8c8ef --> [](014f2b94-756d-4539-9213-fe1c0dc8c8ef) ########## auth/deploy.sh: ########## @@ -0,0 +1,39 @@ +#!/bin/bash + +# Function to build, push, and deploy a Docker image +build_push_deploy_prod() { + set -e + IMAGE_NAME="559615561845.dkr.ecr.ap-south-1.amazonaws.com/devops-scripts" + IMAGE_VERSION="superset-lambda-v1" + FUNCTION="superset_auth_lambda" + DOCKERFILE="Dockerfile" + BUILD_CONTEXT="." + + #Git PULL + + git pull origin master + # AWS details + AWS_ACCOUNT_ID="559615561845" + AWS_REGION="ap-south-1" + ECR_REPO_NAME="devops-scripts" + + #ECR LOGIN + aws ecr get-login-password --region $AWS_REGION | docker login --username AWS --password-stdin $AWS_ACCOUNT_ID.dkr.ecr.$AWS_REGION.amazonaws.com + echo "ECR Login Succeeded" + + # Build Docker image + docker build -t "${IMAGE_NAME}:${IMAGE_VERSION}" -f "${DOCKERFILE}" "${BUILD_CONTEXT}" + + # Push Docker image + docker push "${IMAGE_NAME}:${IMAGE_VERSION}" + + # Deploy to AWS Lambda + aws lambda update-function-code --function-name "${FUNCTION}" --image-uri "${IMAGE_NAME}:${IMAGE_VERSION}" --region ap-south-1 + # sleep 10 + # aws s3 cp deploy.sh s3://m-prod-devops-s3/lambda_test/deploy.sh Review Comment: ### Dead Code <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Commented-out code blocks that serve no apparent purpose are left in the script. ###### Why this matters Dead code creates confusion about what functionality is actually implemented and increases cognitive load when reading the script. ###### Suggested change ∙ *Feature Preview* Remove the commented-out lines if they are no longer needed, or if they are needed for future reference, document why in a clear comment. ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/710087a2-ee1d-4891-bf53-02947fa7d91e/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/710087a2-ee1d-4891-bf53-02947fa7d91e?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/710087a2-ee1d-4891-bf53-02947fa7d91e?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/710087a2-ee1d-4891-bf53-02947fa7d91e?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/710087a2-ee1d-4891-bf53-02947fa7d91e) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:7d74eb2e-22f0-43e1-b3bd-3e7db950e9fe --> [](7d74eb2e-22f0-43e1-b3bd-3e7db950e9fe) ########## auth/deploy.sh: ########## @@ -0,0 +1,39 @@ +#!/bin/bash + +# Function to build, push, and deploy a Docker image Review Comment: ### Insufficient function documentation context <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Current function description merely states what the function does, which is already evident from the function name and code. ###### Why this matters Missing context about why this deployment script exists and any important prerequisites or assumptions could lead to misuse or deployment issues. ###### Suggested change ∙ *Feature Preview* # Deploys Superset authentication Lambda function to AWS # Prerequisites: # - AWS CLI configured with appropriate permissions # - Docker installed and running # - Git repository access ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/81f4d9af-38c8-477a-8594-677ff4f640eb/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/81f4d9af-38c8-477a-8594-677ff4f640eb?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/81f4d9af-38c8-477a-8594-677ff4f640eb?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/81f4d9af-38c8-477a-8594-677ff4f640eb?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/81f4d9af-38c8-477a-8594-677ff4f640eb) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:253414b1-08bf-47dd-acbe-6ddde835fbe0 --> [](253414b1-08bf-47dd-acbe-6ddde835fbe0) ########## superset/security/api.py: ########## @@ -49,6 +49,10 @@ class UserSchema(PermissiveSchema): last_name = fields.String() +class AddUserSchema(UserSchema): + email = fields.String() Review Comment: ### Missing Email Validation <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? AddUserSchema extends UserSchema to add an email field, but lacks email validation ###### Why this matters Invalid email formats could be accepted, potentially causing issues with user notifications or authentication processes ###### Suggested change ∙ *Feature Preview* Add email validation to ensure proper email format: ```python class AddUserSchema(UserSchema): email = fields.Email(required=True) ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/05dec45e-6158-4618-994f-b377e69ffbad/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/05dec45e-6158-4618-994f-b377e69ffbad?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/05dec45e-6158-4618-994f-b377e69ffbad?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/05dec45e-6158-4618-994f-b377e69ffbad?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/05dec45e-6158-4618-994f-b377e69ffbad) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:357df1ba-ca0d-42bc-ad90-5cb5d7bbe0d4 --> [](357df1ba-ca0d-42bc-ad90-5cb5d7bbe0d4) ########## auth/deploy.sh: ########## @@ -0,0 +1,39 @@ +#!/bin/bash + +# Function to build, push, and deploy a Docker image +build_push_deploy_prod() { + set -e + IMAGE_NAME="559615561845.dkr.ecr.ap-south-1.amazonaws.com/devops-scripts" + IMAGE_VERSION="superset-lambda-v1" + FUNCTION="superset_auth_lambda" + DOCKERFILE="Dockerfile" + BUILD_CONTEXT="." + + #Git PULL + + git pull origin master Review Comment: ### Redundant Comment and Spacing <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Comment 'Git PULL' adds no value as the following command is self-explanatory, and there's unnecessary blank line. ###### Why this matters Redundant comments and unnecessary spacing reduce code readability by adding visual noise. ###### Suggested change ∙ *Feature Preview* ```bash # Update from remote repository git pull origin master ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/21a19a3f-7a77-4b97-82a6-43aa79ecc806/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/21a19a3f-7a77-4b97-82a6-43aa79ecc806?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/21a19a3f-7a77-4b97-82a6-43aa79ecc806?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/21a19a3f-7a77-4b97-82a6-43aa79ecc806?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/21a19a3f-7a77-4b97-82a6-43aa79ecc806) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:83ad99b3-4e28-4282-a4a5-9e8ba1d374e3 --> [](83ad99b3-4e28-4282-a4a5-9e8ba1d374e3) ########## superset/security/custom_initializer.py: ########## @@ -0,0 +1,18 @@ +from superset.app import SupersetAppInitializer +from superset.extensions import appbuilder +from superset.security import SupersetSecurityManager + + +class MySupsersetAppInitializer(SupersetAppInitializer): Review Comment: ### Class name typo <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Class name contains a typo ('Supserset' instead of 'Superset') which affects readability and searchability. ###### Why this matters Typos in class names can make the code harder to find through searches and may cause confusion when referenced in documentation or logs. ###### Suggested change ∙ *Feature Preview* Rename the class to `MySuperset AppInitializer` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1e05c720-37c1-44db-a169-d1f8319444b3/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1e05c720-37c1-44db-a169-d1f8319444b3?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1e05c720-37c1-44db-a169-d1f8319444b3?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1e05c720-37c1-44db-a169-d1f8319444b3?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1e05c720-37c1-44db-a169-d1f8319444b3) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:e8b08625-5360-43a8-829c-ec44efef51f2 --> [](e8b08625-5360-43a8-829c-ec44efef51f2) ########## auth/deploy.sh: ########## @@ -0,0 +1,39 @@ +#!/bin/bash + +# Function to build, push, and deploy a Docker image +build_push_deploy_prod() { + set -e + IMAGE_NAME="559615561845.dkr.ecr.ap-south-1.amazonaws.com/devops-scripts" + IMAGE_VERSION="superset-lambda-v1" + FUNCTION="superset_auth_lambda" + DOCKERFILE="Dockerfile" + BUILD_CONTEXT="." + + #Git PULL + + git pull origin master + # AWS details + AWS_ACCOUNT_ID="559615561845" Review Comment: ### Exposed AWS Account ID <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? AWS Account ID is hardcoded directly in the script ###### Why this matters Exposing AWS Account IDs in code can aid attackers in targeting specific AWS resources and should be treated as sensitive information ###### Suggested change ∙ *Feature Preview* Move the AWS Account ID to a secure environment variable or configuration management system: ```bash AWS_ACCOUNT_ID="${AWS_ACCOUNT_ID:?'AWS_ACCOUNT_ID environment variable is required'}" ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f1e9c816-e1ef-4251-b9ec-786f1152d476/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f1e9c816-e1ef-4251-b9ec-786f1152d476?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f1e9c816-e1ef-4251-b9ec-786f1152d476?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f1e9c816-e1ef-4251-b9ec-786f1152d476?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f1e9c816-e1ef-4251-b9ec-786f1152d476) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:25ae2f95-2ab2-4ff4-b477-1efe326308ac --> [](25ae2f95-2ab2-4ff4-b477-1efe326308ac) ########## superset/security/custom_initializer.py: ########## @@ -0,0 +1,18 @@ +from superset.app import SupersetAppInitializer +from superset.extensions import appbuilder +from superset.security import SupersetSecurityManager + + +class MySupsersetAppInitializer(SupersetAppInitializer): + def init_views(self) -> None: + # Adds api for roles + appbuilder.add_api(SupersetSecurityManager.role_api) + + # Uncomment if you want as well to add api for permissions, users, ect. Review Comment: ### Obsolete comment with typo <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The comment is obsolete since the code below it is already uncommented, and contains a typo ('ect' instead of 'etc'). ###### Why this matters Obsolete comments create confusion and cognitive overhead for developers trying to understand the code's current state. ###### Suggested change ∙ *Feature Preview* Remove the obsolete comment since the APIs are already enabled. ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d1f01190-43bc-4beb-8b15-62ec4c0c6d5e/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d1f01190-43bc-4beb-8b15-62ec4c0c6d5e?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d1f01190-43bc-4beb-8b15-62ec4c0c6d5e?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d1f01190-43bc-4beb-8b15-62ec4c0c6d5e?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d1f01190-43bc-4beb-8b15-62ec4c0c6d5e) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:332d238a-2e62-4edd-85c9-b4576cabeba7 --> [](332d238a-2e62-4edd-85c9-b4576cabeba7) ########## superset/security/custom_initializer.py: ########## @@ -0,0 +1,18 @@ +from superset.app import SupersetAppInitializer +from superset.extensions import appbuilder +from superset.security import SupersetSecurityManager + + +class MySupsersetAppInitializer(SupersetAppInitializer): + def init_views(self) -> None: + # Adds api for roles + appbuilder.add_api(SupersetSecurityManager.role_api) + + # Uncomment if you want as well to add api for permissions, users, ect. + + appbuilder.add_api(SupersetSecurityManager.permission_api) + appbuilder.add_api(SupersetSecurityManager.user_api) + appbuilder.add_api(SupersetSecurityManager.view_menu_api) + appbuilder.add_api(SupersetSecurityManager.permission_view_menu_api) Review Comment: ### Unconditional exposure of sensitive security APIs <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Security-sensitive APIs are unconditionally exposed despite the comment suggesting they should be optional. ###### Why this matters Exposing all security APIs by default could lead to increased attack surface and potential security vulnerabilities when these endpoints are not needed. ###### Suggested change ∙ *Feature Preview* Move the sensitive API endpoints behind a configuration flag or environment variable: ```python from superset.app import SupersetAppInitializer from superset.extensions import appbuilder from superset.security import SupersetSecurityManager class MySupsersetAppInitializer(SupersetAppInitializer): def init_views(self) -> None: # Adds api for roles appbuilder.add_api(SupersetSecurityManager.role_api) # Optional APIs controlled by configuration if app.config.get('ENABLE_SECURITY_APIS', False): appbuilder.add_api(SupersetSecurityManager.permission_api) appbuilder.add_api(SupersetSecurityManager.user_api) appbuilder.add_api(SupersetSecurityManager.view_menu_api) appbuilder.add_api(SupersetSecurityManager.permission_view_menu_api) super().init_views() ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0a040184-fed9-43f4-8c29-35f761ee8c3c/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0a040184-fed9-43f4-8c29-35f761ee8c3c?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0a040184-fed9-43f4-8c29-35f761ee8c3c?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0a040184-fed9-43f4-8c29-35f761ee8c3c?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0a040184-fed9-43f4-8c29-35f761ee8c3c) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:103bb622-4f29-4a1b-b454-996885117dd0 --> [](103bb622-4f29-4a1b-b454-996885117dd0) -- 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]
