-
Couldn't load subscription status.
- Fork 1.6k
feat: Zoho Mail #3214
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
base: master
Are you sure you want to change the base?
feat: Zoho Mail #3214
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| scope=scopes, | ||
| ) | ||
| print("\nToken response:") | ||
| print(json.dumps(token_resp, indent=2)) |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
sensitive data (secret)
This expression logs
sensitive data (secret)
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To fix this issue, we must avoid logging cleartext sensitive data such as OAuth tokens, the client secret, and refresh tokens or secrets that could be included in either a successful response or within error information.
The correct way is to:
- Never print the entire
token_respout to the logs. - Instead, explicitly print only non-sensitive subfields as needed (e.g., print e.g.
"Access token received"or the keys but not values, or simply suppress printing altogether unless debugging). - If details from the response must be shown, redact any sensitive fields like
access_token,refresh_token, and anyclient_secretorrequest_params/request_jsonobjects. - Specifically for the snippet, remove or replace the line
print(json.dumps(token_resp, indent=2))with a safe message, or print only a fixed set of non-sensitive keys (e.g. the keys present in the response).
Only make these changes in examples/toolkits/zoho_toolkit.py, as shown.
-
Copy modified lines R121-R128
| @@ -118,7 +118,14 @@ | ||
| scope=scopes, | ||
| ) | ||
| print("\nToken response:") | ||
| print(json.dumps(token_resp, indent=2)) | ||
| # Do not print entire response as it may contain sensitive information. | ||
| # Only indicate success or reason for failure (if available). | ||
| if "access_token" in token_resp: | ||
| print("Access token received.") | ||
| elif "error" in token_resp: | ||
| print(f"Token request failed: {token_resp.get('error')}") | ||
| else: | ||
| print("Token response received, but access_token not found.") | ||
| access_token = token_resp.get("access_token") or "" | ||
| if not access_token: | ||
| print("Failed to obtain access token.") |
| on_subtask_completed (Optional[Callable[[Task], None]], optional): | ||
| Callback function to be called when a subtask is completed. | ||
| (default: :obj:`None`) | ||
| on_subtask_failed (Optional[Callable[[Task], None]], optional): | ||
| Callback function to be called when a subtask fails. | ||
| (default: :obj:`None`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why we need to add these 2 argument in this PR, could you describe more for this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was working on this issue
, but when I saw that coolbeevip was already working on it, I decided to drop it. I’m not sure how I ended up committing, but I will remove it.
|
@Saedbhati while testing, I provided: $env:ZOHO_CLIENT_ID
$env:ZOHO_CLIENT_SECRET
$env:ZOHO_REDIRECT_URI
$env:ZOHO_TO_ADDRESSWhich I got from the api console (Workspace account): But I am facing this issue, the outh generation seems fine though. Is this the correct way to get started with the api?
|
|
@a7m-1st |
|
Oh all right, didn't realize that. I will try again later. Thanks @Saedbhati |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Saed good work!
I left some comments, most are quick fixes, let me know if you need any clarification!
| "Content-Type": "application/json", | ||
| } | ||
| try: | ||
| response = requests.request( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other toolkits include a timeout parameter here just in case, may be useful if you have time
| url = f"{self.base_url}/api/accounts/{self.account_id}/messages/search" | ||
| query = f"label:{tag_id}" | ||
| params: Dict[str, Any] = { | ||
| "query": query, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In line 758 you use searchKey, just wan't to double check this difference is intentional
| @staticmethod | ||
| def build_authorize_url( | ||
| *, | ||
| client_type: str, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding some input validation for client_type client_id and scope
| } | ||
| if scope: | ||
| params["scope"] = scope | ||
| return self._request_json("POST", url, params=params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OAUTH 2.0 specification (RFC 6749) violation here may cause exposed client secrets susceptible to attack.
params is sent in URL query string as opposed to request body
| } | ||
| if scope: | ||
| params["scope"] = scope | ||
| return self._request_json("POST", url, params=params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is the same violation as above
| description: Optional[str] = None, | ||
| due_in_epoch_ms: Optional[int] = None, | ||
| ) -> Dict[str, Any]: | ||
| r"""Action: Create Task (OAuth Scope: ZohoMail.tasks ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docstring
| description: Optional[str] = None, | ||
| due_in_epoch_ms: Optional[int] = None, | ||
| ) -> Dict[str, Any]: | ||
| r"""Action: Create Task (OAuth Scope: ZohoMail.tasks ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| r"""Action: Create Task (OAuth Scope: ZohoMail.tasks ) | |
| r"""Action: Create Task (OAuth Scope: ZohoMail.tasks) |
| folder_id (str): Folder ID. | ||
| since_epoch_seconds (Optional[int]): Since epoch seconds. | ||
| limit (int): Limit. | ||
| sort_order (str): Sort order. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly a more detailed description to help the agent understand what the Args mean, also missing default tags, see CONTRIBUTING.md
| def trigger_new_email_matching_search_poll( | ||
| self, | ||
| query: str, | ||
| folder_id: Optional[str] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
never used
| params["includeto"] = True | ||
| if received_time_ms is not None: | ||
| params["receivedTime"] = int(received_time_ms) | ||
| # Note: folderId is not supported in search API; omit to avoid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the next method uses folderId in the search API request so not sure whether this note is correct or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @Saedbhati , all in all my comments is that it requires an existing server side zoho api which is configured correctly. Also requires a local oauth2 server for redirection for some reason and have to manage the auth & api requests manually i.e. no sdk's or anything. Have to use Zoho's mail and set the region server manually too.
Too much hussle for a niche use case, but as a reviewer, it works for me (perhaps just the fixes in mentioned)
| subject: str, | ||
| content: str, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps can rename this to body for consistency with imap_toolkit
| "encoding": encoding, | ||
| } | ||
|
|
||
| self._add_recipients(payload, to=to, cc=cc, bcc=bcc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, I like this idea of modular add recipients.
| ) -> Dict[str, Any]: | ||
| r"""Trigger: New Tagged Email (Polling) | ||
| Retrieves emails that have the specified tag/label. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this we can retrieve emails based on tags, is there a way to send emails with a tag as in this example from Resend toolkit:
def send_email(
self,
to: List[str],
subject: str,
from_email: str,
html: Optional[str] = None,
text: Optional[str] = None,
cc: Optional[List[str]] = None,
bcc: Optional[List[str]] = None,
reply_to: Optional[str] = None,
tags: Optional[List[Dict[str, str]]] = None,
headers: Optional[Dict[str, str]] = None,
) -> str:| from_email: Optional[str] = None, | ||
| cc: Optional[Union[List[str], List[Dict[str, Any]]]] = None, | ||
| bcc: Optional[Union[List[str], List[Dict[str, Any]]]] = None, | ||
| is_html: bool = True, | ||
| encoding: str = "UTF-8", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think would be nice to have reply_to param with the documentation:
https://www.zoho.com/mail/help/api/post-reply-to-an-email.html
I guess in your case it will be a boolean value & just add those to your api request
"askReceipt" : "yes",
"action":"reply"
|
|
||
| temp = ZohoMailToolkit(access_token="", account_id="", datacenter="in") | ||
| token_resp = temp.exchange_code_for_token( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
datacenter is hardcoded here ++
| identifier = os.getenv("ZOHO_ACCOUNT_ID", "").strip() | ||
| if not identifier: | ||
| identifier = input( | ||
| "Enter ZOHO_ACCOUNT_ID (numeric or email, leave blank to auto): " | ||
| ).strip() | ||
| if identifier.isdigit(): | ||
| account_id, default_from_email = identifier, "" | ||
| else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # Action: Create Task | ||
| task_resp = toolkit.create_task( | ||
| title="Follow up", description="Call customer", due_in_epoch_ms=None | ||
| ) | ||
| print("\nCreate Task response:") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think need to update the scope , as api returns 404: URL_RULE_NOT_CONFIGURED.
Need to add ZohoMail.tasks to the scope above
| Creates a task in Zoho Mail Tasks. | ||
| """ | ||
| url = f"{self.base_url}/api/accounts/{self.account_id}/tasks" | ||
| payload: Dict[str, Any] = {"title": title} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This endpoint doesn't seem to match the doc, unless you are using a different one such as Zoho Mail360?
https://www.zoho.com/mail/help/api/task-api.html
| continue | ||
| fid = f.get("folderId") or f.get("id") or f.get("folder_id") | ||
| names = [] | ||
| for key in ( | ||
| "folderName", | ||
| "displayName", | ||
| "name", | ||
| "systemFolder", | ||
| "folderType", | ||
| "type", | ||
| ): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to check for id, type, or display name. Here is what each item in the loop looks like:
{'path': '/Outbox',
'VW': True,
'previousFolderId': '1026610003021',
'HIDE': False,
'isArchived': 0,
'folderName': 'Outbox',
'imapAccess': True,
'folderType': 'Outbox',
'URI': 'https:mail.zoho.in/api/accounts/1026610002002/folders/1026610002028',
'folderId': '102660002028'}| print("\nTrigger: New Email Matching Search (Polling)") | ||
| search_resp = toolkit.trigger_new_email_matching_search_poll( | ||
| query=f'from:"{to_email}"', folder_id="inbox", limit=10 | ||
| ) | ||
| print(json.dumps(search_resp, indent=2)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can I ask what the polling is supposed to do? Just fetch latest results?





Description
Describe your changes in detail (optional if the linked issue already contains a detailed description of the changes).
fixes #3203
Checklist
Go over all the following points, and put an
xin all the boxes that apply.Fixes #issue-numberin the PR description (required)pyproject.tomlanduv lockIf you are unsure about any of these, don't hesitate to ask. We are here to help!