[WEB-5225]feat: enhance authentication logging with detailed error and info message #7998
Conversation
…sages - Added logging for various authentication events in the Adapter and its subclasses, including email validation, user existence checks, and password strength validation. - Implemented error handling for GitHub OAuth email retrieval, ensuring proper logging of unexpected responses and missing primary emails. - Updated logging configuration in local and production settings to include a dedicated logger for authentication events.
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on November 20. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
WalkthroughThe changes introduce comprehensive logging throughout the authentication module across adapter and provider components, with new logger configuration entries added to both local and production settings. Logging is added to validation paths, exception handlers, and provider-specific events without altering core logic or public API signatures. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes The changes follow a consistent, repetitive pattern of adding logging statements and logger configuration. The scope is contained within the authentication module with no logic alterations or signature changes. Most edits are straightforward logging additions that require minimal reasoning per file. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances authentication logging by adding detailed error and informational messages throughout the authentication flow. The changes focus on improving observability and debugging capabilities by logging key authentication events such as email validation failures, user existence checks, password strength validation, and OAuth-related errors.
Key changes:
- Added dedicated logger configuration for authentication events in both local and production settings
- Implemented comprehensive logging in authentication adapters and providers to capture error conditions and important events
- Enhanced GitHub OAuth email retrieval with proper error handling and validation
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/api/plane/settings/production.py | Added logger configuration for plane.authentication |
| apps/api/plane/settings/local.py | Added logger configuration for plane.authentication |
| apps/api/plane/authentication/provider/oauth/github.py | Added error handling and logging for GitHub OAuth email retrieval and organization validation |
| apps/api/plane/authentication/provider/credentials/email.py | Added logging for user existence checks and authentication failures |
| apps/api/plane/authentication/adapter/oauth.py | Added logging for OAuth token and user response retrieval errors |
| apps/api/plane/authentication/adapter/base.py | Added logger initialization and logging for email validation, password strength checks, and signup validation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| self.logger.warning("Error getting email from GitHub", extra={ | ||
| "headers": headers, | ||
| "emails_response": emails_response, | ||
| }) |
There was a problem hiding this comment.
The variable emails_response may not be defined in this exception handler if the error occurs during the request itself (before line 110 is executed). This will cause a NameError. Move the logging inside the conditions where emails_response is guaranteed to be defined, or handle the case where it doesn't exist.
| def sanitize_email(self, email): | ||
| # Check if email is present | ||
| if not email: | ||
| self.logger.error(f"Email is not present: {email}") |
There was a problem hiding this comment.
Logging the value of email when it's not present (i.e., None or empty) provides no useful information. Consider changing the message to just 'Email is not present' without the variable interpolation.
| self.logger.error(f"Email is not present: {email}") | |
| self.logger.error("Email is not present") |
| """Validate password strength""" | ||
| results = zxcvbn(self.code) | ||
| if results["score"] < 3: | ||
| self.logger.warning(f"Password is not strong enough: {email}") |
There was a problem hiding this comment.
This log message includes the email parameter but references password strength validation. Never log password values. While this appears to only log the email, the message could be misleading. Consider changing to 'Password is not strong enough for user: {email}' to be more explicit about what's being logged.
| self.logger.warning(f"Password is not strong enough: {email}") | |
| self.logger.warning(f"Password is not strong enough for user: {email}") |
| self.logger.warning("Error getting user token", extra={ | ||
| "data": data, | ||
| "headers": headers, | ||
| }) |
There was a problem hiding this comment.
Logging request data and headers in OAuth flows may expose sensitive information such as client secrets, authorization codes, or access tokens. Sanitize or exclude sensitive fields before logging.
| ) | ||
|
|
||
| email = self.__get_email(headers=headers) | ||
| self.logger.info("Email found", extra={ |
There was a problem hiding this comment.
[nitpick] Logging email addresses at INFO level in production may have privacy implications depending on your data handling policies. Consider whether this should be logged at DEBUG level or if PII should be redacted/hashed.
| self.logger.info("Email found", extra={ | |
| self.logger.debug("Email found", extra={ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/api/plane/authentication/adapter/base.py(7 hunks)apps/api/plane/authentication/adapter/oauth.py(2 hunks)apps/api/plane/authentication/provider/credentials/email.py(3 hunks)apps/api/plane/authentication/provider/oauth/github.py(2 hunks)apps/api/plane/settings/local.py(1 hunks)apps/api/plane/settings/production.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/api/plane/authentication/provider/oauth/github.py (2)
packages/logger/src/config.ts (1)
logger(14-14)apps/api/plane/authentication/adapter/error.py (1)
AuthenticationException(70-85)
apps/api/plane/authentication/provider/credentials/email.py (1)
packages/logger/src/config.ts (1)
logger(14-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Lint API
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
apps/api/plane/settings/local.py (1)
79-83: LGTM! Logger configuration is correct.The new
plane.authenticationlogger is properly configured with INFO level and console handler, consistent with other loggers in the file.apps/api/plane/settings/production.py (1)
89-93: LGTM! Production logger configuration is correct.The
plane.authenticationlogger configuration is consistent with the local settings and appropriate for production use.apps/api/plane/authentication/adapter/base.py (1)
4-4: LGTM! Logger initialization is correct.The logging module import and logger initialization using
plane.authenticationare properly implemented.Also applies to: 33-33
apps/api/plane/authentication/provider/oauth/github.py (1)
153-156: Consider whether user_login qualifies as PII for your use case.Depending on your organization's privacy policy, GitHub usernames (
user_login) might be considered PII. If so, this should not be logged.If
user_loginis considered PII in your context, apply this diff:if not self.is_user_in_organization(user_info_response.get("login")): - self.logger.warning("User is not in organization", extra={ - "organization_id": self.organization_id, - "user_login": user_info_response.get("login"), - }) + self.logger.warning("User is not in required organization", extra={ + "organization_id": self.organization_id, + }) raise AuthenticationException(
| def sanitize_email(self, email): | ||
| # Check if email is present | ||
| if not email: | ||
| self.logger.error(f"Email is not present: {email}") |
There was a problem hiding this comment.
Avoid logging email addresses (PII) in error messages.
Logging email addresses directly can violate privacy regulations (GDPR/CCPA). Consider logging a hashed version or omitting the email entirely from logs.
Apply this diff to remove PII from logs:
if not email:
- self.logger.error(f"Email is not present: {email}")
+ self.logger.error("Email is not present")
raise AuthenticationException(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.logger.error(f"Email is not present: {email}") | |
| if not email: | |
| self.logger.error("Email is not present") | |
| raise AuthenticationException( |
🤖 Prompt for AI Agents
In apps/api/plane/authentication/adapter/base.py around line 56, the logger
currently prints the raw email (PII) in the error message; replace that with a
non-identifying value by either omitting the email entirely (e.g., "Email is not
present") or log a deterministic hash of the email (e.g., compute SHA-256 and
log the hex digest) so the log cannot be used to reconstruct the address; ensure
any new hashing uses a standard library hash function and that the message keeps
useful context without exposing the raw email.
| try: | ||
| validate_email(email) | ||
| except ValidationError: | ||
| self.logger.warning(f"Email is not valid: {email}") |
There was a problem hiding this comment.
Avoid logging email addresses (PII) in validation messages.
Email addresses are personally identifiable information (PII). Logging them can create compliance issues with GDPR/CCPA.
Apply this diff:
except ValidationError:
- self.logger.warning(f"Email is not valid: {email}")
+ self.logger.warning("Email validation failed")
raise AuthenticationException(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.logger.warning(f"Email is not valid: {email}") | |
| except ValidationError: | |
| self.logger.warning("Email validation failed") | |
| raise AuthenticationException( |
🤖 Prompt for AI Agents
In apps/api/plane/authentication/adapter/base.py around line 70, the logger
currently includes the full email address which is PII; change the message to
avoid logging the email (either drop the address entirely or log a masked/hashed
identifier). Replace the f-string that emits the raw email with a generic
warning like "Email validation failed" or with a masked form that retains no
identifiable information, and ensure any tests or call sites expecting the old
message are updated accordingly.
| """Validate password strength""" | ||
| results = zxcvbn(self.code) | ||
| if results["score"] < 3: | ||
| self.logger.warning(f"Password is not strong enough: {email}") |
There was a problem hiding this comment.
Avoid logging email addresses (PII) during password validation.
Logging email addresses alongside password validation events can create privacy concerns and compliance risks.
Apply this diff:
if results["score"] < 3:
- self.logger.warning(f"Password is not strong enough: {email}")
+ self.logger.warning("Password strength insufficient")
raise AuthenticationException(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.logger.warning(f"Password is not strong enough: {email}") | |
| if results["score"] < 3: | |
| self.logger.warning("Password strength insufficient") | |
| raise AuthenticationException( |
🤖 Prompt for AI Agents
In apps/api/plane/authentication/adapter/base.py around line 83, the warning
currently logs the user's email (PII) during password validation; remove the raw
email from the log and either omit the identifier entirely or replace it with a
non-PII token (e.g., a masked username, a user ID, or a one-way hash of the
email) so the warning reads generically (e.g., "Password is not strong enough
for user" or "Password is not strong enough for user id=<masked>"); update the
logger call accordingly and ensure any masking/hashing is deterministic and
irreversible to avoid exposing PII.
|
|
||
| # Check if sign up is disabled and invite is present or not | ||
| if ENABLE_SIGNUP == "0" and not WorkspaceMemberInvite.objects.filter(email=email).exists(): | ||
| self.logger.warning(f"Sign up is disabled and invite is not present: {email}") |
There was a problem hiding this comment.
Avoid logging email addresses (PII) in signup checks.
Email addresses should not be logged to maintain user privacy and comply with data protection regulations.
Apply this diff:
if ENABLE_SIGNUP == "0" and not WorkspaceMemberInvite.objects.filter(email=email).exists():
- self.logger.warning(f"Sign up is disabled and invite is not present: {email}")
+ self.logger.warning("Sign up disabled without valid invite")
# Raise exception📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.logger.warning(f"Sign up is disabled and invite is not present: {email}") | |
| if ENABLE_SIGNUP == "0" and not WorkspaceMemberInvite.objects.filter(email=email).exists(): | |
| self.logger.warning("Sign up disabled without valid invite") | |
| # Raise exception |
🤖 Prompt for AI Agents
In apps/api/plane/authentication/adapter/base.py around line 101, the warning
currently logs the user's email (PII); remove the raw email from log output and
replace it with a non-identifying value. Change the message to not include the
email (e.g., "Sign up is disabled and invite is not present") or, if you need
traceability, log a deterministic non-PII fingerprint such as a hashed or masked
form of the email (compute a one-way hash or mask local-part) instead of the
cleartext address; ensure no plaintext email is ever passed to logger.
| self.logger.warning("Error getting user token", extra={ | ||
| "data": data, | ||
| "headers": headers, | ||
| }) |
There was a problem hiding this comment.
Avoid logging authorization headers containing tokens.
The headers parameter may contain sensitive authorization tokens (e.g., Bearer tokens). Logging these creates a security risk as tokens could be exposed in log files or monitoring systems.
Consider sanitizing headers before logging:
except requests.RequestException:
- self.logger.warning("Error getting user token", extra={
- "data": data,
- "headers": headers,
- })
+ sanitized_headers = {k: v for k, v in (headers or {}).items() if k.lower() not in ['authorization', 'cookie']}
+ self.logger.warning("Error getting user token", extra={
+ "data": data,
+ "headers": sanitized_headers,
+ })
code = self.authentication_error_code()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.logger.warning("Error getting user token", extra={ | |
| "data": data, | |
| "headers": headers, | |
| }) | |
| sanitized_headers = {k: v for k, v in (headers or {}).items() if k.lower() not in ['authorization', 'cookie']} | |
| self.logger.warning("Error getting user token", extra={ | |
| "data": data, | |
| "headers": sanitized_headers, | |
| }) |
🤖 Prompt for AI Agents
In apps/api/plane/authentication/adapter/oauth.py around lines 75 to 78, the
code logs the full headers which can contain sensitive authorization tokens;
sanitize headers before logging by removing or masking the Authorization (and
other sensitive) header values, e.g., copy headers, delete or replace the
Authorization value with a fixed masked string or boolean flag indicating its
presence, and then log that sanitized copy (or only non-sensitive headers) along
with data instead of the raw headers.
| self.logger.warning("Authentication failed", extra={ | ||
| "email": self.key, | ||
| }) |
There was a problem hiding this comment.
Avoid logging email addresses (PII) during authentication failures.
Logging email addresses in authentication failure events can expose user information and violate privacy regulations.
Apply this diff:
if not user.check_password(self.code):
- self.logger.warning("Authentication failed", extra={
- "email": self.key,
- })
+ self.logger.warning("Authentication failed - invalid credentials")
raise AuthenticationException(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.logger.warning("Authentication failed", extra={ | |
| "email": self.key, | |
| }) | |
| self.logger.warning("Authentication failed - invalid credentials") |
🤖 Prompt for AI Agents
In apps/api/plane/authentication/provider/credentials/email.py around lines 79
to 81, remove the plain email from the authentication failure log to avoid
logging PII; instead either omit the "email" field entirely or log a
non-reversible identifier such as a SHA-256 hex digest of the email (compute
hash server-side before logging) or a redacted version (e.g., keep only domain
or first char with mask) and replace the extra field with that value so the
logger no longer contains the raw email address.
| if not isinstance(emails_response, list): | ||
| self.logger.error(f"Unexpected response format from GitHub emails API: {emails_response}") | ||
| raise AuthenticationException( | ||
| error_code=AUTHENTICATION_ERROR_CODES["GITHUB_OAUTH_PROVIDER_ERROR"], | ||
| error_message="GITHUB_OAUTH_PROVIDER_ERROR", | ||
| ) |
There was a problem hiding this comment.
Avoid logging API response containing email addresses (PII).
The emails_response variable contains a list of email addresses, which are PII. Logging this data can violate privacy regulations.
Apply this diff:
# Ensure the response is a list before iterating
if not isinstance(emails_response, list):
- self.logger.error(f"Unexpected response format from GitHub emails API: {emails_response}")
+ self.logger.error(f"Unexpected response format from GitHub emails API: expected list, got {type(emails_response).__name__}")
raise AuthenticationException(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if not isinstance(emails_response, list): | |
| self.logger.error(f"Unexpected response format from GitHub emails API: {emails_response}") | |
| raise AuthenticationException( | |
| error_code=AUTHENTICATION_ERROR_CODES["GITHUB_OAUTH_PROVIDER_ERROR"], | |
| error_message="GITHUB_OAUTH_PROVIDER_ERROR", | |
| ) | |
| if not isinstance(emails_response, list): | |
| self.logger.error(f"Unexpected response format from GitHub emails API: expected list, got {type(emails_response).__name__}") | |
| raise AuthenticationException( | |
| error_code=AUTHENTICATION_ERROR_CODES["GITHUB_OAUTH_PROVIDER_ERROR"], | |
| error_message="GITHUB_OAUTH_PROVIDER_ERROR", | |
| ) |
🤖 Prompt for AI Agents
In apps/api/plane/authentication/provider/oauth/github.py around lines 112 to
117, the code currently logs the full emails_response (PII) on error; remove the
direct logging of emails_response and instead log a non-PII message such as the
unexpected response type/structure or the number of items (if safe) without
including actual email values, then raise the same AuthenticationException;
ensure no PII is written to logs (use type(...) or len(...) or a simple static
message) and keep the existing error_code/error_message behavior.
| if not email: | ||
| self.logger.error(f"No primary email found for user: {emails_response}") | ||
| raise AuthenticationException( | ||
| error_code=AUTHENTICATION_ERROR_CODES["GITHUB_OAUTH_PROVIDER_ERROR"], | ||
| error_message="GITHUB_OAUTH_PROVIDER_ERROR", | ||
| ) |
There was a problem hiding this comment.
Avoid logging email addresses (PII) from API response.
Logging emails_response exposes user email addresses in logs, creating privacy and compliance risks.
Apply this diff:
email = next((email["email"] for email in emails_response if email["primary"]), None)
if not email:
- self.logger.error(f"No primary email found for user: {emails_response}")
+ self.logger.error("No primary email found in GitHub response")
raise AuthenticationException(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if not email: | |
| self.logger.error(f"No primary email found for user: {emails_response}") | |
| raise AuthenticationException( | |
| error_code=AUTHENTICATION_ERROR_CODES["GITHUB_OAUTH_PROVIDER_ERROR"], | |
| error_message="GITHUB_OAUTH_PROVIDER_ERROR", | |
| ) | |
| if not email: | |
| self.logger.error("No primary email found in GitHub response") | |
| raise AuthenticationException( | |
| error_code=AUTHENTICATION_ERROR_CODES["GITHUB_OAUTH_PROVIDER_ERROR"], | |
| error_message="GITHUB_OAUTH_PROVIDER_ERROR", | |
| ) |
🤖 Prompt for AI Agents
In apps/api/plane/authentication/provider/oauth/github.py around lines 119 to
124, the code logs the full emails_response which may contain user email
addresses (PII); remove the emails_response from the log and instead log only
non-PII context (e.g., a generic message, the number of emails returned, or the
GitHub user id if available), then raise the same AuthenticationException;
ensure no email values or raw response bodies are written to logs or error
messages.
| self.logger.warning("Error getting email from GitHub", extra={ | ||
| "headers": headers, | ||
| "emails_response": emails_response, | ||
| }) |
There was a problem hiding this comment.
Fix undefined variable reference and avoid logging sensitive data.
There are two critical issues here:
emails_responsemay not be defined if the exception occurs before line 110, causing a runtime error.- Logging
headersexposes authorization tokens, andemails_responsecontains PII.
Apply this diff:
except requests.RequestException:
- self.logger.warning("Error getting email from GitHub", extra={
- "headers": headers,
- "emails_response": emails_response,
- })
+ self.logger.warning("Error getting email from GitHub")
raise AuthenticationException(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.logger.warning("Error getting email from GitHub", extra={ | |
| "headers": headers, | |
| "emails_response": emails_response, | |
| }) | |
| self.logger.warning("Error getting email from GitHub") |
🤖 Prompt for AI Agents
In apps/api/plane/authentication/provider/oauth/github.py around lines 127 to
130, fix the undefined variable and sensitive logging: ensure you only reference
emails_response if it exists (e.g., check for a local variable or use a safe
default like None or include it in the log only when set), and stop logging raw
headers (remove headers from extra) — instead log non-sensitive context such as
the request URL, HTTP status code, and the caught exception message/traceback;
also avoid logging full email data/PII by redacting or logging only a count or
masked identifier.
| self.logger.info("Email found", extra={ | ||
| "email": email, | ||
| }) |
There was a problem hiding this comment.
Avoid logging email addresses (PII).
Logging the retrieved email address exposes PII. Consider using a generic success message instead.
Apply this diff:
email = self.__get_email(headers=headers)
- self.logger.info("Email found", extra={
- "email": email,
- })
+ self.logger.info("Email successfully retrieved from GitHub")
super().set_user_data(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.logger.info("Email found", extra={ | |
| "email": email, | |
| }) | |
| self.logger.info("Email successfully retrieved from GitHub") |
🤖 Prompt for AI Agents
In apps/api/plane/authentication/provider/oauth/github.py around lines 163 to
165, the logger currently records the user's email (PII); remove the email from
the log payload and replace the message with a generic success message (e.g.,
"Email found for GitHub user") or, if diagnostic context is required, log a
non-PII token such as a masked or hashed identifier instead of the full email;
ensure extra no longer contains the "email" field.
Description
Type of Change
Test Scenarios
References
WEB-5225
Summary by CodeRabbit
Release Notes