Conversation
📝 WalkthroughWalkthroughThe pull request introduces a new Locator module for tracking faculty location and availability, extends profile update functionality in dashboard and users routers with new data models and endpoints, and adds utility scripts for credential generation and authentication debugging. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/main.py (1)
45-51: Security:allow_origins=["*"]withallow_credentials=Trueis problematic.This CORS configuration is both insecure and functionally broken. Browsers reject
Access-Control-Allow-Credentials: truewhen origin is*. For development, use explicit origins (e.g.,["http://localhost:3000"]); for production, use a whitelist.🔎 Proposed fix
app.add_middleware( CORSMiddleware, - allow_origins=["*"], + allow_origins=["http://localhost:3000", "http://127.0.0.1:3000"], # Add your frontend origins allow_credentials=True, allow_methods=["*"], allow_headers=["*"], )
🧹 Nitpick comments (10)
app/scripts/set_faculty_creds.py (2)
46-60: Add a safeguard to prevent infinite loop in email generation.The
while Trueloop could run indefinitely if email generation consistently fails. Add a maximum iteration limit.🔎 Proposed fix
counter = 1 +MAX_ATTEMPTS = 100 while True: + if counter > MAX_ATTEMPTS: + print(f"⚠️ Could not generate unique email for {name}") + break check_query = """ MATCH (u:User {email: $email}) WHERE u.user_id <> $uid RETURN count(u) as exists """ exists = session.run(check_query, email=email, uid=uid).single()["exists"] if exists == 0: break counter += 1 email = f"{clean_name}{counter}@gurusetu.edu"
78-81: Consider more specific exception handling.Catching bare
Exceptioncan mask unexpected errors. Consider catching more specific exceptions (e.g.,neo4j.exceptions.ServiceUnavailable) while letting others propagate.app/routers/dashboard.py (3)
56-59: Avoid bareexceptclause.Catching all exceptions silently can hide bugs. Specify the expected exception types.
🔎 Proposed fix
def safe_date(date_obj): """Safely converts Neo4j/Python date objects to ISO string""" if not date_obj: return "N/A" try: if hasattr(date_obj, 'isoformat'): return date_obj.isoformat().split('T')[0] return str(date_obj) - except: + except (AttributeError, ValueError, TypeError): return "N/A"
459-462: Use specific exception type for JSON parsing.Replace bare
exceptwithjson.JSONDecodeErrorto avoid masking unrelated errors.🔎 Proposed fix
if r["coordinates"]: try: coords = json.loads(r["coordinates"]) - except: + except json.JSONDecodeError: coords = None
725-727: Chain the exception for proper debugging.Re-raising without chaining loses the original traceback, making debugging harder.
🔎 Proposed fix
except Exception as e: print(f"Update Error: {str(e)}") - raise HTTPException(status_code=500, detail="Failed to update profile") + raise HTTPException(status_code=500, detail="Failed to update profile") from eapp/routers/locator.py (3)
122-123: Chain exception for proper debugging.🔎 Proposed fix
except Exception as e: - raise HTTPException(status_code=500, detail=str(e)) + raise HTTPException(status_code=500, detail=str(e)) from e
238-238: Remove unnecessary f-string prefix.The string has no placeholders, so the
fprefix is redundant.🔎 Proposed fix
- f"⚠️ 3+ Students are at your cabin requesting an update.", + "⚠️ 3+ Students are at your cabin requesting an update.",
299-301: Chain ValueError exception.🔎 Proposed fix
except ValueError as e: print(f"Date Error: {e}") - raise HTTPException(status_code=400, detail="Invalid Date Format. Use ISO (YYYY-MM-DDTHH:MM:SS)") + raise HTTPException(status_code=400, detail="Invalid Date Format. Use ISO (YYYY-MM-DDTHH:MM:SS)") from eapp/routers/users.py (2)
254-257: Use specific exception type for JSON parsing.🔎 Proposed fix
if data['coordinates']: try: data['coordinates'] = json.loads(data['coordinates']) - except: + except json.JSONDecodeError: data['coordinates'] = None
218-220: Chain exception for proper stack trace.🔎 Proposed fix
except Exception as e: print(f"Update Error: {e}") - raise HTTPException(status_code=500, detail=str(e)) + raise HTTPException(status_code=500, detail=str(e)) from e
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/main.pyapp/routers/dashboard.pyapp/routers/locator.pyapp/routers/users.pyapp/scripts/set_faculty_creds.pydebug.py
🧰 Additional context used
🧬 Code graph analysis (5)
app/routers/locator.py (2)
app/core/security.py (1)
get_current_user(58-77)app/core/database.py (2)
get_session(35-49)close(30-33)
debug.py (1)
app/core/database.py (3)
get_session(35-49)close(30-33)connect(11-28)
app/scripts/set_faculty_creds.py (1)
app/core/database.py (3)
get_session(35-49)close(30-33)connect(11-28)
app/routers/dashboard.py (4)
app/routers/users.py (2)
WorkItem(19-24)FacultyProfileUpdate(55-76)app/models/user.py (2)
WorkItem(5-10)FacultyProfileUpdate(49-67)app/core/security.py (1)
get_current_user(58-77)app/core/database.py (2)
get_session(35-49)close(30-33)
app/routers/users.py (4)
app/core/security.py (1)
get_current_user(58-77)app/routers/dashboard.py (3)
WorkItem(19-24)FacultyProfileUpdate(27-44)get_all_faculty(417-479)app/models/user.py (5)
WorkItem(5-10)ProjectCreate(18-25)PublicationItem(12-16)StudentProfileUpdate(28-46)FacultyProfileUpdate(49-67)app/core/database.py (2)
close(30-33)get_session(35-49)
🪛 Ruff (0.14.10)
app/routers/locator.py
121-121: Consider moving this statement to an else block
(TRY300)
122-122: Do not catch blind exception: Exception
(BLE001)
123-123: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
183-183: Unused function argument: current_user
(ARG001)
183-183: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
207-207: Unused function argument: current_user
(ARG001)
207-207: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
238-238: f-string without any placeholders
Remove extraneous f prefix
(F541)
257-257: Unused function argument: faculty_id
(ARG001)
293-297: Consider moving this statement to an else block
(TRY300)
301-301: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
debug.py
106-106: Possible hardcoded password assigned to: "password"
(S105)
132-132: Probable use of requests call without timeout
(S113)
145-145: Do not catch blind exception: Exception
(BLE001)
app/scripts/set_faculty_creds.py
63-63: Possible hardcoded password assigned to: "plain_password"
(S105)
78-78: Do not catch blind exception: Exception
(BLE001)
app/routers/dashboard.py
461-461: Do not use bare except
(E722)
659-659: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
723-723: Consider moving this statement to an else block
(TRY300)
725-725: Do not catch blind exception: Exception
(BLE001)
726-726: Use explicit conversion flag
Replace with conversion flag
(RUF010)
727-727: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
app/routers/users.py
93-93: Consider moving this statement to an else block
(TRY300)
220-220: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
256-256: Do not use bare except
(E722)
🔇 Additional comments (2)
app/main.py (1)
67-68: LGTM!The locator router is correctly imported and registered following the existing pattern.
app/routers/dashboard.py (1)
657-659: No action needed. The endpoints exist at different API paths due to distinct router prefixes (/usersand/dashboard), so they do not conflict or shadow each other.
| class WorkItem(BaseModel): | ||
| title: str | ||
| type: str | ||
| year: str | ||
| outcome: Optional[str] = None | ||
| collaborators: Optional[str] = None | ||
|
|
||
| # --- NEW: Model for Profile Updates --- | ||
| class FacultyProfileUpdate(BaseModel): | ||
| name: Optional[str] = None | ||
| profile_picture: Optional[str] = None | ||
| email: Optional[str] = None | ||
| phone: Optional[str] = None | ||
| designation: Optional[str] = None | ||
| department: Optional[str] = None | ||
| office_hours: Optional[str] = None | ||
| cabin_block: Optional[str] = None | ||
| cabin_floor: Optional[str] = None | ||
| cabin_number: Optional[str] = None | ||
| ug_details: List[str] = [] | ||
| pg_details: List[str] = [] | ||
| phd_details: List[str] = [] | ||
| domain_interests: List[str] = [] | ||
| previous_work: List[WorkItem] = [] | ||
| current_status: Optional[str] = None | ||
| status_source: Optional[str] = None |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Code duplication: Models defined in multiple places.
WorkItem and FacultyProfileUpdate are duplicated in:
app/routers/dashboard.py(here)app/routers/users.pyapp/models/user.py
Import from a single source (app/models/user.py) to avoid inconsistencies.
🔎 Proposed fix
+from app.models.user import WorkItem, FacultyProfileUpdate
+
-class WorkItem(BaseModel):
- title: str
- type: str
- year: str
- outcome: Optional[str] = None
- collaborators: Optional[str] = None
-
-# --- NEW: Model for Profile Updates ---
-class FacultyProfileUpdate(BaseModel):
- # ... all fields ...Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/routers/dashboard.py around lines 19 to 44, the WorkItem and
FacultyProfileUpdate Pydantic models are duplicated here while canonical
definitions live in app/models/user.py; remove these duplicate class definitions
and import the models from app.models.user instead, updating the import block to
pull WorkItem and FacultyProfileUpdate (and any needed typing/BaseModel symbols
only if they are no longer used locally) so all routers use the single source of
truth and avoid divergence.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch CodeRabbit configuration schema and documentation
curl -s https://coderabbit.ai/integrations/schema.v2.json > /tmp/schema.json
curl -s https://docs.coderabbit.ai/guides/review-instructions > /tmp/instructions.html
curl -s https://docs.coderabbit.ai/faq/ > /tmp/faq.html
echo "=== Configuration Schema Keys ==="
jq -r '.properties | keys[]' /tmp/schema.json 2>/dev/null | head -20
echo -e "\n=== Review Instructions Documentation ==="
grep -A 5 "path-based\|AST-based\|configuration" /tmp/instructions.html | head -30
echo -e "\n=== FAQ about Configuration ==="
grep -A 3 "configure\|YAML\|settings" /tmp/faq.html | head -30
Length of output: 137449
@Ajey95, could you please clarify what configuration help you need? Here are the options:
If you want to configure CodeRabbit:
- YAML Configuration: Configure review behavior via
.coderabbit.yamlin your repository (docs) - Path Filters: Exclude specific files/paths from review using
path_filtersconfiguration (docs) - Review Instructions: Add custom path-based or AST-based review instructions
- Settings Dashboard: Adjust organization and repository preferences through the UI
However, the code duplication issue I flagged is a legitimate code quality concern. Having duplicate model definitions in multiple files (app/routers/dashboard.py, app/routers/users.py, and potentially app/models/user.py) can lead to:
- Inconsistencies when models evolve
- Maintenance difficulties
- Potential bugs if one copy is updated but others aren't
Recommended approach: Rather than configuring CodeRabbit to ignore this, I'd suggest refactoring to import the models from a single source (likely app/models/user.py) as originally suggested.
If you have a specific reason to keep the current structure, please let me know and I can help find the best solution!
| # 2. Update Domain Interests | ||
| if update.domain_interests is not None: | ||
| session.run("MATCH (f:Faculty {user_id: $uid})-[r:INTERESTED_IN]->() DELETE r", uid=user_id) | ||
| for domain in update.domain_interests: | ||
| session.run("MATCH (f:Faculty {user_id: $uid}) MERGE (c:Concept {name: $domain}) MERGE (f)-[:INTERESTED_IN]->(c)", uid=user_id, domain=domain) | ||
|
|
||
| # 3. Update Previous Work | ||
| if update.previous_work: | ||
| session.run("MATCH (f:Faculty {user_id: $uid})-[r:WORKED_ON]->() DELETE r", uid=user_id) | ||
| for item in work_data: | ||
| session.run(""" | ||
| MATCH (f:Faculty {user_id: $uid}) | ||
| CREATE (w:Work {id: randomUUID(), title: $title, type: $type, year: $year, outcome: $outcome, collaborators: $collab, created_at: datetime()}) | ||
| CREATE (f)-[:WORKED_ON]->(w) | ||
| """, uid=user_id, title=item['title'], type=item['type'], year=item['year'], outcome=item['outcome'], collab=item['collaborators']) | ||
|
|
||
| # 4. CRITICAL: Update Map Connection if Cabin Number changed | ||
| # This fixes the issue where map pins wouldn't move! | ||
| if update.cabin_number: | ||
| map_query = """ | ||
| MATCH (f:Faculty {user_id: $uid}) | ||
| OPTIONAL MATCH (f)-[r:LOCATED_AT]->(:Cabin) | ||
| DELETE r | ||
| WITH f | ||
| MATCH (c:Cabin {code: $code}) | ||
| MERGE (f)-[:LOCATED_AT]->(c) | ||
| """ | ||
| session.run(map_query, uid=user_id, code=update.cabin_number) |
There was a problem hiding this comment.
Non-atomic database operations risk data inconsistency.
Multiple separate session.run() calls for domains, work, and map updates aren't wrapped in a transaction. If one fails, the profile could be left in an inconsistent state.
🔎 Consider using a transaction
def do_profile_update(tx, user_id, update, work_data):
# Run all queries within the transaction
tx.run(basic_query, ...)
tx.run(domain_delete_query, ...)
# ... etc
# Then:
session.write_transaction(do_profile_update, user_id, update, work_data)🤖 Prompt for AI Agents
In app/routers/dashboard.py around lines 694 to 721, the profile update uses
multiple separate session.run() calls (domains, previous work, map updates)
which are not executed inside a single transaction, risking partial updates on
failure; refactor by wrapping all database operations in a single write
transaction function that accepts tx, user_id, update, work_data and performs
the delete/merge/create statements via tx.run(...) so they commit or roll back
together, then call session.write_transaction(...) (or
session.execute_write(...) depending on driver) to run that function, and
propagate errors so failures rollback and maintain consistency.
| @router.post("/seed") | ||
| def seed_locator_data(): |
There was a problem hiding this comment.
Security: Seed endpoint has no authentication.
POST /seed can be called by anyone to reset all faculty location data. This should require admin authentication or be disabled in production.
🔎 Proposed fix - add admin check
@router.post("/seed")
-def seed_locator_data():
+def seed_locator_data(current_user: dict = Depends(get_current_user)):
+ # Add admin role check
+ if current_user.get("role", "").lower() != "admin":
+ raise HTTPException(status_code=403, detail="Admin access required")
session = db.get_session()📝 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.
| @router.post("/seed") | |
| def seed_locator_data(): | |
| @router.post("/seed") | |
| def seed_locator_data(current_user: dict = Depends(get_current_user)): | |
| # Add admin role check | |
| if current_user.get("role", "").lower() != "admin": | |
| raise HTTPException(status_code=403, detail="Admin access required") |
🤖 Prompt for AI Agents
In app/routers/locator.py around lines 51-52, the POST /seed endpoint is
unprotected and allows anyone to reset faculty location data; require admin
authentication or disable in production. Add an authentication dependency (e.g.,
current_user via your auth dependency) to the endpoint signature and check a
user.is_admin/is_staff flag before performing the seed, returning 403 if not
admin; alternatively gate the route so it is only registered when running in a
non-production environment (check an ENV var) and document/remove it for
production builds.
| @router.put("/faculty/{faculty_id}/status") | ||
| def update_status(faculty_id: str, update: StatusUpdate, current_user: dict = Depends(get_current_user)): | ||
| session = db.get_session() | ||
| try: | ||
| # Check permissions handled by `get_current_user` mostly, but logic: | ||
| # If Manual -> Must be Faculty themselves | ||
| # If Student-QR -> Must be Student at location | ||
|
|
||
| query = """ | ||
| MATCH (f:Faculty {user_id: $fid}) | ||
| SET f.current_status = $status, | ||
| f.status_source = $source, | ||
| f.last_status_updated = datetime() | ||
| RETURN f.name as name | ||
| """ | ||
| result = session.run(query, fid=faculty_id, status=update.status, source=update.source).single() | ||
|
|
||
| if not result: | ||
| raise HTTPException(status_code=404, detail="Faculty not found") | ||
|
|
||
| return {"message": f"Status updated to {update.status}"} |
There was a problem hiding this comment.
Missing authorization: Any authenticated user can update any faculty's status.
update_status accepts any current_user but doesn't verify they're authorized to update the target faculty's status. The comment mentions permission checks but they're not implemented.
🔎 Proposed fix
@router.put("/faculty/{faculty_id}/status")
def update_status(faculty_id: str, update: StatusUpdate, current_user: dict = Depends(get_current_user)):
session = db.get_session()
try:
- # Check permissions handled by `get_current_user` mostly, but logic:
- # If Manual -> Must be Faculty themselves
- # If Student-QR -> Must be Student at location
+ # Verify authorization
+ if update.source == "Manual":
+ # Only the faculty themselves can manually update
+ if current_user["user_id"] != faculty_id or current_user["role"].lower() != "faculty":
+ raise HTTPException(status_code=403, detail="Only the faculty member can update their own status")
+ elif update.source == "Student-QR":
+ # Students can update via QR
+ if current_user["role"].lower() != "student":
+ raise HTTPException(status_code=403, detail="Only students can update via QR")
query = """🧰 Tools
🪛 Ruff (0.14.10)
183-183: Unused function argument: current_user
(ARG001)
183-183: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
| @router.post("/faculty/{faculty_id}/future") | ||
| def check_future_availability(faculty_id: str, check: FutureCheck): | ||
| try: | ||
| # 1. Edge Case Fix: Remove 'Z' (UTC marker) to prevent Python crash | ||
| clean_date = check.datetime.replace("Z", "") | ||
|
|
||
| dt = datetime.fromisoformat(clean_date) | ||
| day_name = dt.strftime("%A") # e.g., 'Monday' | ||
| time_str = dt.strftime("%H:%M") # e.g., '14:30' | ||
|
|
||
| status = "Available" | ||
| message = "Free according to timetable" | ||
| found_conflict = False | ||
|
|
||
| # 2. Priority 1: Check against Mock Timetable (Specific Events) | ||
| for slot in MOCK_TIMETABLE: | ||
| if slot['day'] == day_name: | ||
| # String comparison works for ISO times: "09:00" <= "14:30" < "16:00" | ||
| if slot['start'] <= time_str < slot['end']: | ||
| status = "In Class" # Or "Busy" | ||
| message = slot['activity'] | ||
| found_conflict = True | ||
| break | ||
|
|
||
| # 3. Priority 2: General Availability (Only if NO Class was found) | ||
| # If they are free from class, check if they are actually at home (Weekend/After Hours) | ||
| if not found_conflict: | ||
| # Fix: Use 'in list' instead of 'or string' | ||
| if day_name in ['Saturday', 'Sunday']: | ||
| status = "Busy" | ||
| message = "At Home (Weekend)" | ||
|
|
||
| # Fix: Check office hours (e.g., 9 AM to 4 PM) | ||
| elif time_str < "09:00" or time_str > "16:00": | ||
| status = "Busy" | ||
| message = "At Home (After Hours)" | ||
|
|
||
| return { | ||
| "query_time": f"{day_name}, {time_str}", | ||
| "status": status, | ||
| "message": message | ||
| } | ||
|
|
||
| except ValueError as e: | ||
| print(f"Date Error: {e}") | ||
| raise HTTPException(status_code=400, detail="Invalid Date Format. Use ISO (YYYY-MM-DDTHH:MM:SS)") No newline at end of file |
There was a problem hiding this comment.
faculty_id parameter is unused - same mock timetable for all faculty.
The check_future_availability endpoint ignores faculty_id and returns the same mock timetable data regardless of which faculty is queried. This makes the endpoint misleading.
🔎 Proposed fix - fetch faculty-specific timetable
@router.post("/faculty/{faculty_id}/future")
def check_future_availability(faculty_id: str, check: FutureCheck):
+ session = db.get_session()
try:
+ # Fetch faculty-specific timetable from DB
+ query = """
+ MATCH (f:Faculty {user_id: $fid})-[:HAS_SCHEDULE]->(s:Schedule)
+ RETURN s.day as day, s.start as start, s.end as end, s.activity as activity
+ """
+ result = session.run(query, fid=faculty_id)
+ timetable = [dict(r) for r in result]
+
+ # Fall back to mock if no schedule found
+ if not timetable:
+ timetable = MOCK_TIMETABLE
+
clean_date = check.datetime.replace("Z", "")
# ... rest of logic using timetable instead of MOCK_TIMETABLECommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.10)
257-257: Unused function argument: faculty_id
(ARG001)
293-297: Consider moving this statement to an else block
(TRY300)
301-301: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
| # 2. CRITICAL: Update Map Relationship (The "Req") | ||
| # This moves the pin on the map when you save the profile | ||
| if data.cabin_number: | ||
| map_query = """ | ||
| MATCH (f:Faculty {user_id: $uid}) | ||
|
|
||
| // Remove old map link | ||
| OPTIONAL MATCH (f)-[r:LOCATED_AT]->(:Cabin) | ||
| DELETE r | ||
|
|
||
| // Find new cabin by code and link it | ||
| WITH f | ||
| MATCH (c:Cabin {code: $code}) | ||
| MERGE (f)-[:LOCATED_AT]->(c) | ||
| """ | ||
| session.run(map_query, uid=user_id, code=data.cabin_number) |
There was a problem hiding this comment.
Non-atomic map update after main profile update.
The map relationship update runs in a separate query after the main profile update. If it fails, the profile data is committed but map data is stale. Consider wrapping in a transaction.
🤖 Prompt for AI Agents
In app/routers/users.py around lines 356 to 371, the map relationship update is
executed as a separate query after the main profile update which can leave the
database in an inconsistent state if the second query fails; wrap the profile
update and the LOCATED_AT delete/merge steps in a single write transaction so
both succeed or both roll back, by opening a transaction (e.g.,
session.begin_transaction()/write_transaction) and running the profile update
and the map_query within that transaction, committing only on success and
handling/propagating errors to ensure rollback on failure.
| plain_password = "123456" | ||
| hashed_password = pwd_context.hash(plain_password) | ||
|
|
||
| # 4. Update the DB | ||
| session.run(""" | ||
| MATCH (f:Faculty {user_id: $uid}) | ||
| SET f.email = $email, | ||
| f.password = $password | ||
| """, uid=uid, email=email, password=hashed_password) | ||
|
|
||
| print(f"{name:<30} | {email:<35} | {plain_password}") |
There was a problem hiding this comment.
Security: Hardcoded weak password and logging credentials is a compliance risk.
Using "123456" as a default password is extremely weak and printing passwords to stdout could leak to logs. Even for a setup script, this is problematic if the password isn't immediately changed.
🔎 Proposed fix - generate secure passwords
+import secrets
+import string
+
+def generate_password(length=12):
+ alphabet = string.ascii_letters + string.digits
+ return ''.join(secrets.choice(alphabet) for _ in range(length))
+
for fac in faculty_list:
# ...
- plain_password = "123456"
+ plain_password = generate_password()
hashed_password = pwd_context.hash(plain_password)
# ...
- print(f"{name:<30} | {email:<35} | {plain_password}")
+ # Consider writing to a secure file instead of stdout
+ print(f"{name:<30} | {email:<35} | [password generated]")Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.10)
63-63: Possible hardcoded password assigned to: "plain_password"
(S105)
🤖 Prompt for AI Agents
In app/scripts/set_faculty_creds.py around lines 63 to 73, the script currently
uses a hardcoded weak password ("123456") and prints plaintext passwords to
stdout; replace the hardcoded password with a securely generated random password
for each user (use the secrets module to generate a sufficiently long, URL-safe
or base64 string), immediately hash and store only the hashed password in the
DB, and stop printing plaintext passwords to logs. Instead of printing
credentials, either write the plaintext passwords to a securely permissioned
file (owner-only) if needed for one-time delivery, or better, trigger a
password-reset workflow/email or print only a non-sensitive indicator (e.g.,
"password set" or a one-time token ID) so no plaintext secrets are logged.
Ensure generated passwords meet length/complexity requirements and remove any
remaining hardcoded defaults.
| # # import requests | ||
| # # import json | ||
|
|
||
| # # # URL of your backend login | ||
| # # url = "http://127.0.0.1:8000/auth/login" | ||
|
|
||
| # # # Test Credentials (Faculty) | ||
| # # payload = { | ||
| # # "email": "deepika.t@gurusetu.edu", | ||
| # # "password": "123456", | ||
| # # "role": "faculty" # We are sending lowercase 'faculty' | ||
| # # } | ||
|
|
||
| # # print(f"🔄 Attempting Login for: {payload['email']}...") | ||
|
|
||
| # # try: | ||
| # # response = requests.post(url, json=payload) | ||
|
|
||
| # # print(f"\n🔢 Status Code: {response.status_code}") | ||
| # # print(f"📄 Response Body: {response.text}") | ||
|
|
||
| # # if response.status_code == 200: | ||
| # # print("\n✅ LOGIN SUCCESS! The backend is working fine.") | ||
| # # print(" If you still can't login from the browser, the Frontend is sending the wrong data.") | ||
| # # else: | ||
| # # print("\n❌ LOGIN FAILED.") | ||
| # # print(" Look at the 'Response Body' above to see the missing field or error.") | ||
|
|
||
| # # except Exception as e: | ||
| # # print(f"\n💥 Connection Error: {e}") | ||
| # # print(" Make sure your backend (uvicorn) is running!") | ||
|
|
||
| # import sys | ||
| # import os | ||
| # from passlib.context import CryptContext | ||
|
|
||
| # # Setup path | ||
| # sys.path.append(os.path.join(os.path.dirname(__file__), ".")) | ||
|
|
||
| # from app.core.database import db | ||
|
|
||
| # # Setup the same hasher | ||
| # pwd_context = CryptContext(schemes=["bcrypt"], deprecated="auto") | ||
|
|
||
| # def check_new_user(): | ||
| # session = db.get_session() | ||
|
|
||
| # # --- ENTER YOUR NEW EMAIL HERE --- | ||
| # email = "j.uma@gurusetu.edu" | ||
| # # --------------------------------- | ||
|
|
||
| # try: | ||
| # print(f"🔍 Searching for: {email}...") | ||
|
|
||
| # # 1. Check if node exists and get labels/properties | ||
| # query = "MATCH (u:User {email: $email}) RETURN u, labels(u) as labels" | ||
| # result = session.run(query, email=email).single() | ||
|
|
||
| # if not result: | ||
| # print("❌ ERROR: User NOT FOUND in Database!") | ||
| # print(" Did the signup actually succeed? Check the 'User' table.") | ||
| # return | ||
|
|
||
| # node = result["u"] | ||
| # labels = result["labels"] | ||
|
|
||
| # print("\n✅ USER FOUND!") | ||
| # print(f" Labels: {labels} <-- Should contain 'User' AND 'Faculty'") | ||
| # print(f" Name: {node.get('name')}") | ||
| # print(f" Role: {node.get('role')} <-- Should be 'faculty' (lowercase)") | ||
| # print(f" Stored Password: {node.get('password')}") | ||
|
|
||
| # # 2. Check Role Logic | ||
| # if "Faculty" not in labels and node.get('role') == 'faculty': | ||
| # print("⚠️ WARNING: Node has 'faculty' role but missing :Faculty label.") | ||
| # print(" This might prevent you from appearing in the directory.") | ||
|
|
||
| # if node.get('role') == 'Faculty': | ||
| # print("⚠️ WARNING: Role is 'Faculty' (Capitalized). Login might expect 'faculty'.") | ||
|
|
||
| # except Exception as e: | ||
| # print(f"💥 Error: {e}") | ||
| # finally: | ||
| # session.close() | ||
|
|
||
| # if __name__ == "__main__": | ||
| # db.connect() | ||
| # check_new_user() | ||
| # db.close() |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove commented-out code.
Large blocks of commented code add noise and should be removed. If this is meant for reference, consider keeping it in documentation or a separate debug examples file.
🤖 Prompt for AI Agents
In debug.py around lines 1 to 89 there is a large block of commented-out code
that clutters the file; remove these commented sections entirely (or move them
into a dedicated docs/debug_examples.md or a separate debug_examples.py) so the
file contains only active, relevant code; ensure any useful snippets are
preserved in the chosen external file and then run linters/tests to confirm no
imports or references were accidentally removed.
| def fix_and_test(): | ||
| session = db.get_session() | ||
| email = "j.uma@gurusetu.edu" | ||
| password = "123456" | ||
|
|
||
| try: | ||
| print(f"🔧 RESETTING password for {email}...") | ||
|
|
||
| # 1. Force Reset Password in DB | ||
| hashed_pw = pwd_context.hash(password) | ||
| query = """ | ||
| MATCH (u:User {email: $email}) | ||
| SET u.password = $pw, u.role = 'faculty' | ||
| RETURN u.name, u.role | ||
| """ | ||
| # Note: We also force role to lowercase 'faculty' to match your output | ||
| session.run(query, email=email, pw=hashed_pw) | ||
| print("✅ Password reset in Neo4j.") | ||
|
|
||
| # 2. Test Login Endpoint | ||
| print(f"\n🚀 TESTING Login Endpoint: {URL}") | ||
|
|
||
| payload = { | ||
| "email": email, | ||
| "password": password, | ||
| "role": "faculty" # Trying lowercase first | ||
| } | ||
|
|
||
| print(f" Sending: {payload}") | ||
| response = requests.post(URL, json=payload) | ||
|
|
||
| print(f"\n Status: {response.status_code}") | ||
| print(f" Response: {response.text}") | ||
|
|
||
| if response.status_code == 200: | ||
| print("\n🎉 SUCCESS! Login is working.") | ||
| print(" Go to your browser and use these exact credentials.") | ||
| else: | ||
| print("\n❌ FAILED via Python too.") | ||
| print(" This means the bug is inside your 'app/routers/auth.py' file.") | ||
| print(" Please paste your 'auth.py' code here so I can fix the logic.") | ||
|
|
||
| except Exception as e: | ||
| print(f"💥 Error: {e}") | ||
| finally: | ||
| session.close() |
There was a problem hiding this comment.
Debug script with hardcoded credentials should not be committed to main branch.
This script modifies production database state (resets passwords) and contains hardcoded credentials. Consider:
- Moving to a
scripts/debug/directory with.gitignore - Using environment variables for credentials
- Adding prominent warnings about production use
🧰 Tools
🪛 Ruff (0.14.10)
106-106: Possible hardcoded password assigned to: "password"
(S105)
132-132: Probable use of requests call without timeout
(S113)
145-145: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In debug.py around lines 103 to 148 the helper function fix_and_test contains
hardcoded credentials and performs destructive changes to the DB (resets
passwords), which must not be committed to main; move this script out of the app
root into a non-deployed folder such as scripts/debug/, add scripts/debug/ to
.gitignore, replace hardcoded email/password/URL with environment variables (and
fail fast if they are not set), add a prominent runtime confirmation and a
safety check to refuse execution against production (e.g., require an explicit
DEBUG_ALLOW_PROD=true env or check NODE_ENV/ENVIRONMENT), and add a clear
comment/warning header describing the destructive nature so the file can be kept
for local troubleshooting without risking production CI or accidental runs.
| } | ||
|
|
||
| print(f" Sending: {payload}") | ||
| response = requests.post(URL, json=payload) |
There was a problem hiding this comment.
Add timeout to HTTP request to prevent hanging.
The requests.post call lacks a timeout, which could cause the script to hang indefinitely if the server is unresponsive.
🔎 Proposed fix
-response = requests.post(URL, json=payload)
+response = requests.post(URL, json=payload, timeout=10)📝 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.
| response = requests.post(URL, json=payload) | |
| response = requests.post(URL, json=payload, timeout=10) |
🧰 Tools
🪛 Ruff (0.14.10)
132-132: Probable use of requests call without timeout
(S113)
🤖 Prompt for AI Agents
In debug.py around line 132, the requests.post call has no timeout which can
cause the script to hang; add a timeout argument (e.g. timeout=10) to the call
and wrap the request in a try/except that catches requests.exceptions.Timeout
and requests.exceptions.RequestException so you can log/handle timeouts and
other network errors gracefully.
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
1 participant
Check the main.py and other merge issue files.
Thank you
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.