[Buganizer ID: 470285380] move spyCloud to third party repo #400
Conversation
Summary of ChangesHello @haggit-eliyahu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request undertakes a significant refactoring of the SpyCloud integration by migrating its entire codebase to a new third-party repository. This strategic move aims to streamline maintenance and enhance modularity. Concurrently, the integration has been updated to ensure full compatibility with Python 3.11, aligning with modern development standards. The changes also introduce three new core functionalities: the ability to list available SpyCloud catalogs, retrieve entity-specific breach information, and perform a connectivity test, all supported by robust new data models and interactive UI widgets. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the SpyCloud integration. While it's a good starting point, there are several critical issues and style guide violations that need to be addressed. Most importantly, the pull request is missing unit tests, which are mandatory for new integrations in content/response_integrations/** according to the repository style guide. I've also identified a critical configuration bug that would prevent the integration from working, a security issue in password handling, and numerous violations of the style guide regarding type hinting, docstrings, and the use of synchronous libraries. Please review the detailed comments for suggestions on how to bring this integration up to the repository's standards for production-readiness.
| name: SpyCloud | ||
| parameters: | ||
| - name: API Root | ||
| default_value: https:/{{api root}} |
There was a problem hiding this comment.
There appears to be a typo in the default_value for the API Root. It is missing a slash in https:/. This will cause connection errors for users configuring the integration.
default_value: https://{{api root}}| def to_json(self): | ||
| obfuscated_password = ( | ||
| self.obfuscate_password(self.password) if self.password else self.password | ||
| ) | ||
| self.raw_data["password"] = obfuscated_password | ||
| self.raw_data["password_plaintext"] = obfuscated_password | ||
| return self.raw_data |
There was a problem hiding this comment.
This method writes the obfuscated password to both password and password_plaintext. Exposing a field named password_plaintext is misleading and a potential security risk, even if it contains obfuscated data. According to the style guide (line 23-24, 60-61), secrets should not persist in telemetry. It's best to remove the password_plaintext key entirely.
| def to_json(self): | |
| obfuscated_password = ( | |
| self.obfuscate_password(self.password) if self.password else self.password | |
| ) | |
| self.raw_data["password"] = obfuscated_password | |
| self.raw_data["password_plaintext"] = obfuscated_password | |
| return self.raw_data | |
| def to_json(self): | |
| if self.password: | |
| self.raw_data["password"] = self.obfuscate_password(self.password) | |
| # The 'password_plaintext' key should not be exposed in JSON results. | |
| self.raw_data.pop("password_plaintext", None) | |
| return self.raw_data |
| def obfuscate_password(self, password): | ||
| obfuscated = "" | ||
| for index in range(1, len(password) - 2): | ||
| obfuscated += password[index].replace(password[index], "*") | ||
| return password[0] + obfuscated + password[-2:] |
There was a problem hiding this comment.
This password obfuscation logic is buggy for passwords with a length of 3 or less, and the implementation is inefficient. For example, a 2-character password ab would become aab. This version is more robust, handles edge cases correctly, and is more Pythonic. It also includes the required type hints.
| def obfuscate_password(self, password): | |
| obfuscated = "" | |
| for index in range(1, len(password) - 2): | |
| obfuscated += password[index].replace(password[index], "*") | |
| return password[0] + obfuscated + password[-2:] | |
| def obfuscate_password(self, password: str) -> str: | |
| """Obfuscates a password for safe display, handling edge cases.""" | |
| if not password: | |
| return "" | |
| length = len(password) | |
| if length <= 2: | |
| return password[0] + "*" * (length - 1) | |
| if length == 3: | |
| return f"{password[0]}*{password[-1]}" | |
| # For passwords with 4 or more characters, keep the first and last two. | |
| return f"{password[0]}{'*' * (length - 3)}{password[-2:]}" |
| ) | ||
| params = {"source_id": catalog_id} if catalog_id else {} | ||
|
|
||
| results = self.session.get(url, params=params) |
There was a problem hiding this comment.
The use of the synchronous requests library for network calls can lead to performance bottlenecks. The repository style guide (lines 69-72) recommends using an asynchronous library like httpx or aiohttp to avoid blocking operations. Consider refactoring this manager to use async and await for all network requests to improve performance.
| def is_valid_email(user_name): | ||
| """ | ||
| Check if the user_name is valid email. | ||
| :param user_name: {str} User name | ||
| :return: {bool} True if valid email, else False | ||
| """ |
There was a problem hiding this comment.
This function, and others in the file, are missing type hints and use a docstring format that doesn't align with the repository style guide (lines 80, 88). Please update all functions in this file to include type annotations and Google-style docstrings.
For example:
def is_valid_email(user_name: str) -> bool:
"""Checks if a string is a valid email address.
Args:
user_name: The string to validate.
Returns:
True if the string is a valid email, False otherwise.
"""|
|
||
| class Catalog(BaseModel): | ||
| def __init__(self, raw_data, title, type, num_records, site, id): | ||
| super(Catalog, self).__init__(raw_data) |
There was a problem hiding this comment.
The use of super(ClassName, self) is an older style. Modern Python 3 allows for a cleaner, argument-less super() call, which is less prone to errors.
| super(Catalog, self).__init__(raw_data) | |
| super().__init__(raw_data) |
|
|
||
| class Breach(BaseModel): | ||
| def __init__(self, raw_data, target_url, email, infected_time, sighting, severity, password): | ||
| super(Breach, self).__init__(raw_data) |
There was a problem hiding this comment.
The use of super(ClassName, self) is an older style. Modern Python 3 allows for a cleaner, argument-less super() call, which is less prone to errors.
| super(Breach, self).__init__(raw_data) | |
| super().__init__(raw_data) |
|
|
||
|
|
||
| @output_handler | ||
| def main(): |
There was a problem hiding this comment.
The main function is missing type hints and a docstring, which violates the repository style guide (lines 80, 88-90). All functions should have type annotations and a Google-style docstring.
| def main(): | |
| def main() -> None: | |
| """Return information about breaches related to entities.""" |
| description: 'Specify how many catalogs to return. Default: 50.' | ||
| is_mandatory: false | ||
| dynamic_results_metadata: | ||
| - result_example_path: resources/list_catalogs_JsonResult_example.json |
There was a problem hiding this comment.
The result_example_path does not follow the naming convention specified in the repository style guide (line 158). For an action file named ListCatalogs.py, the example file should be named ListCatalogs_json_example.json.
Please update this path and rename the corresponding file in the resources/ directory.
- result_example_path: resources/ListCatalogs_json_example.json|
|
||
| try: | ||
| if limit < 0: | ||
| raise Exception('"Max Catalogs To Return" should be a positive number.') |
There was a problem hiding this comment.
Raising a generic Exception is not recommended. For invalid parameter values, it's better to use a more specific exception like ValueError to provide clearer error information.
| raise Exception('"Max Catalogs To Return" should be a positive number.') | |
| raise ValueError('"Max Catalogs To Return" should be a positive number.') |
content/response_integrations/third_party/partner/spycloud/pyproject.toml
Show resolved
Hide resolved
|
❌ Marketplace Validation Failed Click to view the full reportValidation Report🧩 IntegrationsPre-Build Stagespycloud
|
2 participants
Description
Migrate spy cloud integration to third-party repo and add missing EnvCommon version to packages.
Checklist:
Please ensure you have completed the following items before submitting your PR.
This helps us review your contribution faster and more efficiently.
General Checks:
Open-Source Specific Checks:
For Google Team Members and Reviewers Only:
Screenshots (If Applicable)
If your changes involve UI or visual elements, please include screenshots or GIFs here.
Ensure any sensitive data is redacted or generalized.
Further Comments / Questions
Any additional comments, questions, or areas where you'd like specific feedback.