Skip to content
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

Bug: Unauthorized notification sending causes missing user IDs #1186

Open
ObiFaith opened this issue Mar 1, 2025 · 2 comments
Open

Bug: Unauthorized notification sending causes missing user IDs #1186

ObiFaith opened this issue Mar 1, 2025 · 2 comments
Labels

Comments

@ObiFaith
Copy link
Contributor

ObiFaith commented Mar 1, 2025

Issue Overview

This issue addresses a security issue where the /api/v1/notifications/send endpoint lacks authorization, allowing anyone to send notifications. As a result, the user_id in the notification table is set to NULL, preventing users from retrieving their notifications correctly.

🔴 Unauthorized Notification Sending

  • The send_notification endpoint does not require authentication, allowing unauthenticated users to send notifications.

  • This results in notifications being stored with a NULL user_id in the database.

  • Consequently, when /api/v1/notifications/current-user tries to retrieve their notifications, no notifications are attached to them, regardless of how many have been sent.

🛑 Data Integrity Issue

  • Notifications should be associated with a valid user_id.

  • Allowing notifications without authentication leads to incorrect or missing records when users try to fetch their notifications.

  • This affects the integrity and reliability of the notification system.

Motivation & Context

🚀 Security Fix: Ensures that only authenticated users can send notifications.

🔍 Data Consistency: Prevents NULL values in the user_id field.

🛠 User Experience: Ensures that users see the correct notifications associated with their accounts.

Proposed Fix

Before:

@notification.post(
    "/send",
    summary="Send a notification",
    description="This endpoint sends a notification",
    status_code=status.HTTP_201_CREATED,
)
def send_notification(
    notification_data: NotificationCreate,
    db: Session = Depends(get_db)
):
    notification = notification_service.`send_notification`(
        title=notification_data.title,
        message=notification_data.message,
        db=db,
    )
    return success_response(
        status_code=201, message="Notification sent successfully", data=notification
    )

After (Fix - Require Authentication & Associate User ID):

@notification.post(
    "/send",
    summary="Send a notification",
    description="This endpoint sends a notification",
    status_code=status.HTTP_201_CREATED,
)
def send_notification(
    notification_data: NotificationCreate,
    user: User = Depends(user_service.get_current_user),
    db: Session = Depends(get_db)
):
    notification = notification_service.`send_notification`(
        title=notification_data.title,
        message=notification_data.message,
        user=user,
        db=db,
    )
    return success_response(
        status_code=201, message="Notification sent successfully", data=notification
    )

Alternative Fix (If Current Authentication Method Needs Updates)

  • Ensure get_current_user correctly retrieves the authenticated user.

  • Validate that user_id is correctly attached before committing to the database.

How Has This Been Tested?

  • Attempted to send notifications without authentication and verified the request is rejected.

  • Verified that authenticated users can send notifications successfully.

  • Checked that notifications are correctly associated with the current_user.

  • Tested retrieval of notifications to ensure they are correctly linked to the user.

Types of Changes

🐞 Bug Fix: Secures the send_notification endpoint by enforcing authentication.

🔐 Security Enhancement: Prevents unauthorized access and data inconsistencies.

🛠 Data Integrity Fix: Ensures notifications are correctly stored and retrieved.

Checklist

  • My code follows the project’s coding style.

  • I have updated the documentation where necessary.

  • I have read the CONTRIBUTING guidelines.

  • All new and existing tests passed.

Key Notes for Reviewers

  • The send_notification endpoint now requires authentication.

  • user_id is correctly associated with the authenticated user before storing notifications.

  • If get_current_user needs improvements, ensure it correctly retrieves and validates the user.

Expected Outcomes

  • Prevents unauthorized users from sending notifications.
  • Ensures notifications are properly linked to users.
  • Improves overall data consistency and security.
@ObiFaith
Copy link
Contributor Author

ObiFaith commented Mar 1, 2025

@ObiFaith will be working on this

@ObiFaith ObiFaith changed the title [FIX]: Send Notification Endpoint Bug: Unauthorized users can send notifications, causing missing user associations Mar 1, 2025
@ObiFaith ObiFaith changed the title Bug: Unauthorized users can send notifications, causing missing user associations Bug: Unauthorized notification sending causes missing user IDs Mar 1, 2025
@ObiFaith
Copy link
Contributor Author

ObiFaith commented Mar 1, 2025

@joboy-dev Please approve this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants