refactor: add auth service and refactor token endpoints#115
refactor: add auth service and refactor token endpoints#115TimilsinaBimal merged 1 commit intodevfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello, 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 significantly refactors the authentication and token management system by introducing a new 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. Changelog
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 significantly improves code organization by introducing a dedicated AuthService and refactoring token-related endpoints, leading to clearer separation of concerns. However, a critical security flaw persists where the Stremio user_id is used as the authentication token for personalized manifests, enabling easy enumeration and unauthorized access to user data. It is highly recommended to use a randomly generated, non-predictable token (e.g., a UUID) instead of the user_id for manifest access. Additionally, I've provided specific comments addressing potential efficiency gains in the new AuthService, a breaking change in an API response, and clarifications for model field details.
| user_id, resolved_email, stremio_auth_key = await self.get_stremio_user_data(payload) | ||
|
|
||
| # 2. Check if user already exists | ||
| token = token_store.get_token_from_user_id(user_id) |
There was a problem hiding this comment.
The application uses the Stremio user_id as the authentication token for accessing personalized manifests. Since Stremio user IDs are not secret and can be obtained or guessed, an attacker can access any user's personalized manifest by knowing their user_id. The manifest contains sensitive information such as personalized recommendations based on the user's private library and watch history. This is an Insecure Direct Object Reference (IDOR) vulnerability.
| logger.info(f"[{redact_token(token)}] Token deleted for user {user_id}") | ||
| return {"detail": "Settings deleted successfully"} | ||
| await auth_service.delete_user_account(payload) | ||
| return JSONResponse(status_code=200, content="Settings deleted successfully") |
There was a problem hiding this comment.
The response for this delete endpoint has changed from a JSON object to a raw string. Previously, it returned {"detail": "Settings deleted successfully"}. The new implementation returns the string "Settings deleted successfully". This is a breaking change for any client expecting a JSON object with keys.
To maintain API consistency and avoid breaking clients, it's better to return a JSON object.
| return JSONResponse(status_code=200, content="Settings deleted successfully") | |
| return {"message": "Settings deleted successfully"} |
| default="balanced", description="Popularity for TMDB API" | ||
| ) | ||
| year_min: int = Field(default=2010, description="Minimum release year for TMDB API") | ||
| year_max: int = Field(default=2026, description="Maximum release year for TMDB API") |
There was a problem hiding this comment.
The default value for year_max has been changed from 2025 to 2026. This is a silent change in behavior. If this is intentional, it's fine, but it's worth noting as it affects the default filtering logic.
| ) | ||
| simkl_api_key: str | None = Field(default=None, description="Simkl API Key for the user") | ||
| gemini_api_key: str | None = Field(default=None, description="Gemini API Key for AI features") | ||
| tmdb_api_key: str | None = Field(default=None, description="TMDB API Key") |
There was a problem hiding this comment.
The description for tmdb_api_key has lost some useful context. The previous description, TMDB API Key (required for new clients if server has none), was more informative for developers using the API. It would be beneficial to restore it.
| tmdb_api_key: str | None = Field(default=None, description="TMDB API Key") | |
| tmdb_api_key: str | None = Field(default=None, description="TMDB API Key (required for new clients if server has none)") |
| async def resolve_auth_key(self, credentials: dict, token: str | None = None) -> str | None: | ||
| """Validate auth key. If expired, try email+password login. Update store on refresh.""" | ||
| auth_key = (credentials.get("authKey") or "").strip() or None | ||
| email = (credentials.get("email") or "").strip() or None | ||
| password = (credentials.get("password") or "").strip() or None | ||
|
|
||
| if auth_key and auth_key.startswith('"') and auth_key.endswith('"'): | ||
| auth_key = auth_key[1:-1].strip() | ||
|
|
||
| bundle = StremioBundle() | ||
| try: | ||
| # 1. Try existing auth key | ||
| if auth_key: | ||
| try: | ||
| await bundle.auth.get_user_info(auth_key) | ||
| return auth_key | ||
| except Exception: | ||
| logger.info("Stremio auth key expired or invalid, attempting refresh with credentials") | ||
|
|
||
| # 2. Try login if auth key failed or wasn't provided | ||
| if email and password: | ||
| try: | ||
| new_key = await bundle.auth.login(email, password) | ||
| if token and new_key != auth_key: | ||
| existing_data = await self.get_credentials(token) | ||
| if existing_data: | ||
| existing_data["authKey"] = new_key | ||
| await token_store.update_user_data(token, existing_data) | ||
| return new_key | ||
| except Exception as e: | ||
| logger.error(f"Stremio login failed: {e}") | ||
| return None | ||
| finally: | ||
| await bundle.close() | ||
|
|
||
| return None | ||
|
|
||
| async def get_credentials(self, token: str) -> dict | None: | ||
| """Get user credentials from token store.""" | ||
| return await token_store.get_user_data(token) | ||
|
|
||
| async def store_credentials(self, user_id: str, payload: dict) -> str: | ||
| """Store credentials, return token.""" | ||
| # Ensure last_updated is present if it's a new user | ||
| if "last_updated" not in payload: | ||
| token = token_store.get_token_from_user_id(user_id) | ||
| existing = await self.get_credentials(token) | ||
| if existing: | ||
| payload["last_updated"] = existing.get("last_updated") | ||
| else: | ||
| payload["last_updated"] = datetime.now(timezone.utc).isoformat() | ||
|
|
||
| return await token_store.store_user_data(user_id, payload) | ||
|
|
||
| async def get_stremio_user_data(self, payload: TokenRequest) -> tuple[str, str, str]: | ||
| """ | ||
| Authenticates with Stremio and returns (user_id, email, auth_key). | ||
| """ | ||
| creds = payload.model_dump() | ||
| auth_key = await self.resolve_auth_key(creds) | ||
|
|
||
| if not auth_key: | ||
| raise HTTPException(status_code=400, detail="Failed to verify Stremio identity. Provide valid credentials.") | ||
|
|
||
| bundle = StremioBundle() | ||
| try: | ||
| user_info = await bundle.auth.get_user_info(auth_key) | ||
| user_id = user_info["user_id"] | ||
| resolved_email = user_info.get("email", payload.email or "") | ||
| return user_id, resolved_email, auth_key | ||
| except Exception as e: | ||
| logger.error(f"Stremio identity verification failed: {e}") | ||
| raise HTTPException(status_code=400, detail=f"Failed to verify Stremio identity: {e}") | ||
| finally: | ||
| await bundle.close() |
There was a problem hiding this comment.
The current implementation creates multiple StremioBundle instances within a single authentication flow, which is inefficient. Specifically, get_stremio_user_data calls resolve_auth_key (which creates and closes a bundle), and then get_stremio_user_data creates a second bundle to perform a redundant get_user_info call. This leads to unnecessary HTTP client setup/teardown and extra API calls.
To improve efficiency, you can merge the logic of resolve_auth_key into get_stremio_user_data so that only one StremioBundle is created and used. This makes the process more efficient and the code easier to follow.
async def get_stremio_user_data(self, payload: TokenRequest) -> tuple[str, str, str]:
"""
Authenticates with Stremio and returns (user_id, email, auth_key).
"""
creds = payload.model_dump()
auth_key = (creds.get("authKey") or "").strip() or None
email = (creds.get("email") or "").strip() or None
password = (creds.get("password") or "").strip() or None
if auth_key and auth_key.startswith('"') and auth_key.endswith('"'):
auth_key = auth_key[1:-1].strip()
bundle = StremioBundle()
try:
user_info = None
# 1. Try existing auth key
if auth_key:
try:
user_info = await bundle.auth.get_user_info(auth_key)
except Exception:
logger.info("Stremio auth key expired or invalid, attempting refresh with credentials")
auth_key = None # Invalidate key
# 2. Try login if auth key failed or wasn't provided
if not auth_key and email and password:
try:
auth_key = await bundle.auth.login(email, password)
user_info = await bundle.auth.get_user_info(auth_key)
except Exception as e:
logger.error(f"Stremio login failed: {e}")
auth_key = None
if not auth_key or not user_info:
raise HTTPException(status_code=400, detail="Failed to verify Stremio identity. Provide valid credentials.")
user_id = user_info["user_id"]
resolved_email = user_info.get("email", payload.email or "")
return user_id, resolved_email, auth_key
except Exception as e:
logger.error(f"Stremio identity verification failed: {e}")
raise HTTPException(status_code=400, detail=f"Failed to verify Stremio identity: {e}")
finally:
await bundle.close()1 participant
No description provided.