Refactor Onboarding Endpoint to Make User Context Optional#335
Refactor Onboarding Endpoint to Make User Context Optional#335AkhileshNegi merged 5 commits intomainfrom
Conversation
WalkthroughOnboardingRequest fields email, password, and user_name became optional; added username validation and a _clean_username helper; a post-validation model_validator sets defaults for missing user_name, email (derived local + short hex @kaapi.org), and password (random token). UserCreate now uses full_name=request.user_name. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant API as Onboarding API
participant M as OnboardingRequest Model
participant S as Onboarding Service
C->>API: POST /onboard { user_name?, email?, password?, project_name? }
API->>M: Construct OnboardingRequest(...)
M-->>M: field_validator(user_name) — validate pattern if provided
alt user_name missing
M-->>M: user_name = _clean_username(project_name)
end
alt email missing
M-->>M: local = _clean_username(user_name)
M-->>M: email = f"{local}.{secrets.token_hex(3)}@kaapi.org"
end
alt password missing
M-->>M: password = secrets.token_urlsafe(12)
end
M-->>API: Validated request (defaults applied)
API->>S: onboard(UserCreate(full_name=request.user_name, email, password, ...))
S-->>API: Result
API-->>C: Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
backend/app/api/routes/onboarding.py (1)
100-110: Avoid reusing an existing user when the email was auto-generated (tenant leakage risk).Current logic reuses any existing user with the same email. If the email was auto-generated, this can attach a new project/org to an unrelated user who happened to get the same synthetic email — a cross-tenant leakage.
Use request.model_fields_set to detect whether the client provided email. Only reuse an existing user when the client explicitly provided the email; otherwise, regenerate a unique fallback email.
Additional code (outside selected lines) showing how to apply this:
def onboard_user(request: OnboardingRequest, session: SessionDep): # ... email_provided_by_client = "email" in request.model_fields_set existing_user = session.query(User).filter(User.email == request.email).first() if existing_user and email_provided_by_client: user = existing_user else: # If email was auto-generated and already exists, regenerate a unique fallback if existing_user and not email_provided_by_client: for _ in range(5): base_local = re.sub(r"[^a-z0-9._]+", ".", request.user_name.strip().lower()).strip(".") if not base_local or not base_local[0].isalpha(): base_local = f"u.{base_local}" if base_local else "u" request.email = f"{base_local}.{secrets.token_hex(4)}@kaapi.org" existing_user = session.query(User).filter(User.email == request.email).first() if not existing_user: break else: raise HTTPException(status_code=500, detail="Failed to generate unique fallback email") user_create = UserCreate( name=request.user_name, email=request.email, password=request.password, ) user = create_user(session=session, user_create=user_create)
🧹 Nitpick comments (4)
backend/app/api/routes/onboarding.py (4)
1-2: Imports look good; minor nit if regex stays.Switch to re/secrets is appropriate for validation and secure defaults. If you keep a strict regex validator, consider compiling the pattern once at module scope and using fullmatch to avoid partial matches.
Example (outside selected lines):
USERNAME_PATTERN = re.compile(r"^[A-Za-z][A-Za-z0-9._]{2,29}$") # ... if not USERNAME_PATTERN.fullmatch(v): ...
75-77: Docstring: clarify that email/password are optional and auto-generated when omitted.Update the docstring to reflect the new behavior to help client developers.
Example (outside selected lines):
""" Handles quick onboarding of a new user. Accepts organization name, project name, user name, and optionally email/password. If email/password are omitted, the system generates a sanitized synthetic email and a random password. Returns an API key for authentication. """
50-59: Consider externalizing the fallback email domain.Hardcoding "@kaapi.org" couples code to a specific domain. Read it from configuration (e.g., settings.AUTO_EMAIL_DOMAIN) with a sensible default.
If acceptable, I can send a follow-up patch to introduce settings.AUTO_EMAIL_DOMAIN and wire it here.
36-37: Add tests for the new optional path (no email/password provided).Ensure we have coverage for:
- Onboarding without email/password returns 200 and creates org/project/user/api key.
- Generated email format is valid and unique.
- Endpoint still supports explicit email/password (existing tests).
I can add parametric tests for both paths and collision handling if you’d like.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
backend/app/api/routes/onboarding.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/api/routes/onboarding.py (2)
backend/app/models/user.py (6)
UserUpdate(27-29)UserUpdateMe(32-34)NewPassword(37-39)UserCreate(16-17)UpdatePassword(42-44)UserRegister(20-23)backend/app/tests/api/routes/test_onboarding.py (1)
test_onboard_user(16-57)
🔇 Additional comments (2)
backend/app/api/routes/onboarding.py (2)
36-37: Email and password made optional — double-check contract and downstream constraints.
- This also makes password optional, which goes beyond the PR title. Confirm this is intended.
- UserCreate enforces password length; your token_urlsafe default satisfies it.
- Since the generated password is not returned in the response, confirm no interactive login is expected for such accounts.
Would you like me to update endpoint/docs to explicitly state that email/password may be omitted and that a non-recoverable random password will be generated?
5-5: Confirm Pydantic v2 dependencyI wasn’t able to locate a Pydantic version constraint in your dependency files. Please verify that you’ve specified Pydantic ≥2.0 so that
model_validatorandfield_validatorare available:• Check pyproject.toml (e.g. under
[tool.poetry.dependencies])
• Check requirements.txt or anyrequirements-*.txt
• Inspect Pipfile/Pipfile.lock or poetry.lock
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
backend/app/api/routes/onboarding.py (1)
101-112: Critical: Existing superusers can be silently demoted.When onboarding with an email that already exists and belongs to a superuser, the code unconditionally sets is_superuser = False on that existing user. That’s a severe privilege downgrade.
Guard this to only apply to newly created users:
@@ - existing_user = session.query(User).filter(User.email == request.email).first() - if existing_user: - user = existing_user - else: - user_create = UserCreate( - name=request.user_name, - email=request.email, - password=request.password, - ) - user = create_user(session=session, user_create=user_create) + existing_user = session.query(User).filter(User.email == request.email).first() + created_new_user = False + if existing_user: + user = existing_user + else: + user_create = UserCreate( + name=request.user_name, + email=request.email, + password=request.password, + ) + user = create_user(session=session, user_create=user_create) + created_new_user = True @@ - user.is_superuser = False - session.add(user) + if created_new_user: + user.is_superuser = False + session.add(user)Also applies to: 131-135
♻️ Duplicate comments (1)
backend/app/api/routes/onboarding.py (1)
40-48: Avoid breaking existing clients: relax username validation; sanitize during email generation instead.Enforcing a strict username regex is a backward-incompatible change (past tests/clients used display names with spaces). Keep display names free-form and shift safety constraints to the auto-generated email path. Also, lowercasing and sanitizing the local-part improves email validity if we relax the username. Extend the random password length slightly for comfort against policies.
Apply:
@@ - @field_validator("user_name") - def validate_username(cls, v): - pattern = r"^[A-Za-z][A-Za-z0-9._]{2,29}$" - if not re.match(pattern, v): - raise ValueError( - "Username must start with a letter, can contain letters, numbers, underscores, and dots, " - "and must be between 3 and 30 characters long." - ) - return v + @field_validator("user_name") + def validate_username(cls, v: str) -> str: + # Preserve display names; we'll slugify during email generation if needed. + return v.strip() @@ - @model_validator(mode="after") - def set_defaults(self): - # Generate email and password if missing - if self.email is None: - suffix = secrets.token_hex(3) - self.email = f"{self.user_name}.{suffix}@kaapi.org" - - if self.password is None: - self.password = secrets.token_urlsafe(8) - return self + @model_validator(mode="after") + def set_defaults(self): + # Generate email and password if missing + if self.email is None: + # Build a safe local-part from user_name + local = self.user_name.strip().lower() + local = re.sub(r"[^a-z0-9._]+", ".", local) # collapse invalid chars to dots + local = re.sub(r"\.{2,}", ".", local).strip(".") # collapse repeats and trim + if not local or not local[0].isalpha(): + local = f"u.{local}" if local else "u" + # Add a short random suffix to reduce collision risk across tenants + suffix = secrets.token_hex(3) # 6 hex chars + self.email = f"{local}.{suffix}@kaapi.org" + + if self.password is None: + # Slightly longer token to comfortably meet length policies + self.password = secrets.token_urlsafe(12) + return selfDo you intend to break the API by disallowing spaces in user_name? If not, please adopt the above to maintain backward compatibility while safely generating emails.
Also applies to: 50-59
🧹 Nitpick comments (4)
backend/app/api/routes/onboarding.py (3)
50-56: Make the auto-email domain configurable, not hardcoded.Hardcoding "kaapi.org" couples behavior to a single tenant. Source from settings with a sensible default.
Example change (outside the current diff range):
# at top-level imports from app.core.config import settings # where building the email domain = getattr(settings, "AUTO_EMAIL_DOMAIN", "kaapi.org") self.email = f"{local}.{suffix}@{domain}"
90-99: Scope project lookup by organization to avoid cross-org collisions.Using only the project name can attach a user to a project under a different organization if names collide. Filter by organization_id too.
- existing_project = ( - session.query(Project).filter(Project.name == request.project_name).first() - ) + existing_project = ( + session.query(Project) + .filter( + Project.name == request.project_name, + Project.organization_id == organization.id, + ) + .first() + )
75-79: Docstring: reflect that email/password are optional and may be auto-generated.Update the docstring to reduce confusion for API consumers.
backend/app/tests/api/routes/test_onboarding.py (1)
22-22: Tests align with strict username validation, but consider preserving display-name support and add coverage for optional fields.If we adopt non-breaking behavior (free-form display names + slugified email), keep one test using a display name with spaces to ensure backward compatibility. Also add tests covering the omission of email/password to assert auto-generation.
Suggested new tests (outside current diff range):
def test_onboard_user_without_email_and_password(client, db: Session, superuser_token_headers: dict[str, str]): data = { "organization_name": "OrgNoCreds", "project_name": "ProjNoCreds", # no email # no password "user_name": "Test User", # display name with space } response = client.post(f"{settings.API_V1_STR}/onboard", json=data, headers=superuser_token_headers) assert response.status_code == 200 body = response.json() user = db.query(User).filter(User.id == body["user_id"]).first() assert user is not None # Auto-generated email present and belongs to the user assert user.email assert user.is_superuser is False def test_onboard_user_display_name_slugified_email(client, db: Session, superuser_token_headers: dict[str, str]): data = { "organization_name": "OrgSlug", "project_name": "ProjSlug", "user_name": "New User", # should be accepted if validator is relaxed } response = client.post(f"{settings.API_V1_STR}/onboard", json=data, headers=superuser_token_headers) assert response.status_code == 200 user_id = response.json()["user_id"] user = db.query(User).filter(User.id == user_id).first() assert user is not None assert user.email.endswith("@kaapi.org") # or configured domainWould you like me to open a follow-up PR to add these tests once you confirm the intended username behavior?
Also applies to: 68-68, 92-92, 115-115
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
backend/app/api/routes/onboarding.py(2 hunks)backend/app/tests/api/routes/test_onboarding.py(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/api/routes/onboarding.py (2)
backend/app/api/routes/private.py (1)
PrivateUserCreate(16-20)backend/app/models/user.py (5)
UserUpdateMe(32-34)UserUpdate(27-29)UserRegister(20-23)NewPassword(37-39)UserCreate(16-17)
⏰ 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). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (1)
backend/app/api/routes/onboarding.py (1)
36-37: LGTM: Making email/password optional aligns with the PR goal.Defaults are provided via the model-level validator, so downstream usage won’t see None. No concerns here.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
backend/app/api/routes/onboarding.py (1)
118-129: Security/data-integrity risk: auto-generated email collision could attach to an existing userIf the generated email collides with an existing user's email, the code currently reuses that user (Line 120-121). This can inadvertently assign the new project and API key to the wrong person. While suffix reduces probability, collisions or deliberate targeting are still possible.
If the email was auto-generated, do not reuse an existing user. Instead, regenerate a unique email a few times before failing.
Apply this diff (relies on the private flag suggested above and the settings import):
- existing_user = session.query(User).filter(User.email == request.email).first() - if existing_user: - user = existing_user - else: - user_create = UserCreate( - full_name=request.user_name, - email=request.email, - password=request.password, - ) - user = create_user(session=session, user_create=user_create) + existing_user = session.query(User).filter(User.email == request.email).first() + if existing_user and not getattr(request, "_email_autogenerated", False): + user = existing_user + else: + # If email was auto-generated and collides, regenerate a fresh unique candidate + if existing_user and getattr(request, "_email_autogenerated", False): + for _ in range(3): + base_local = OnboardingRequest._refactor_username( + request.user_name or request.project_name, max_len=200 + ).strip("._") or "user" + candidate = f"{base_local}.{secrets.token_hex(4)}@{getattr(settings, 'AUTO_EMAIL_DOMAIN', 'kaapi.org')}" + if not session.query(User).filter(User.email == candidate).first(): + request.email = candidate + break + else: + raise HTTPException( + status_code=409, + detail="Could not allocate a unique email for autogenerated user; please retry.", + ) + user_create = UserCreate( + full_name=request.user_name, + email=request.email, + password=request.password, + ) + user = create_user(session=session, user_create=user_create)
♻️ Duplicate comments (1)
backend/app/api/routes/onboarding.py (1)
40-52: Breaking change: strict user_name regex rejects spaces (will break existing clients/tests)The regex
^[A-Za-z][A-Za-z0-9._]{2,199}$disallows spaces and many valid display names. Existing tests/clients use values like "Test User". This change is outside the scope of “email optional” and is backward-incompatible.Recommend preserving user_name as a free-form display name and only slugify for derived email local-part. Trim input; avoid failing validation.
Apply this diff to relax validation while keeping behavior intact:
- @field_validator("user_name") - def validate_username(cls, v): - if v is None: - return v - - pattern = r"^[A-Za-z][A-Za-z0-9._]{2,199}$" - if not re.match(pattern, v): - raise ValueError( - "Username must start with a letter, can contain letters, numbers, underscores, and dots, " - "and must be between 3 and 200 characters long." - ) - return v + @field_validator("user_name") + def normalize_user_name(cls, v: str | None) -> str | None: + # Preserve display names; we only slugify for email generation when needed. + return v.strip() if isinstance(v, str) else v
🧹 Nitpick comments (1)
backend/app/api/routes/onboarding.py (1)
53-62: Username refactor helper is fine; consider basic transliteration (optional)Implementation looks good. If you expect non-ASCII names, consider transliterating to ASCII before regex to avoid empty results for some scripts.
Example (outside this diff):
import unicodedata @staticmethod def _refactor_username(raw: str, max_len: int = 200) -> str: ascii_raw = ( unicodedata.normalize("NFKD", raw) .encode("ascii", "ignore") .decode("ascii") ) username = re.sub(r"[^A-Za-z0-9._]", "_", ascii_raw.strip().lower()) username = re.sub(r"[._]{2,}", "_", username) username = username.strip("._") return username[:max_len]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
backend/app/api/routes/onboarding.py(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/api/routes/onboarding.py (3)
backend/app/models/user.py (5)
UserUpdateMe(32-34)UserRegister(20-23)UserUpdate(27-29)UserCreate(16-17)UserBase(8-12)backend/app/api/routes/private.py (1)
PrivateUserCreate(16-20)backend/app/tests/api/routes/test_onboarding.py (1)
test_onboard_user(16-57)
⏰ 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). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (2)
backend/app/api/routes/onboarding.py (2)
1-5: Pydantic v2 validators import — LGTMGood move to use
model_validatorandfield_validatorfrom Pydantic v2 to handle defaults and field normalization.
124-127: Switch tofull_name— LGTMMatches the current
UserCreateschema usingfull_name; consistent with models.
| email: EmailStr | None = None | ||
| password: str | None = None | ||
| user_name: str | None = None |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Making email/password/user_name optional — LGTM; ensure tests cover no-email/no-password paths
This aligns with the PR objective. Please confirm there are tests that omit each of these fields to exercise default generation.
You can verify coverage with:
🏁 Script executed:
#!/bin/bash
# Search onboarding tests for payloads that omit email or password
rg -n -C2 --type=py 'client\.post\(.+/onboard' backend/app/tests | sed -n '1,200p'
rg -n -C3 --type=py '"email":' backend/app/tests/api/routes/test_onboarding.pyLength of output: 1076
Add tests for missing optional fields
I reviewed test_onboarding.py and all existing payloads include email, password, and user_name. We need explicit coverage for cases where each of these is omitted to exercise default generation.
Please add tests that:
- Omit
emailand assert a generated email is assigned - Omit
passwordand assert a generated password is assigned - Omit
user_nameand assert a generated user name is assigned
File to update:
backend/app/tests/api/routes/test_onboarding.py
🤖 Prompt for AI Agents
In backend/app/api/routes/onboarding.py around lines 36 to 38, the Pydantic
model marks email, password, and user_name as optional, but tests never cover
omitted-field behavior; add three new tests in
backend/app/tests/api/routes/test_onboarding.py that POST to the onboarding
endpoint with payloads that (1) omit email and assert the response contains a
non-empty, valid-looking generated email, (2) omit password and assert the
response contains a non-empty generated password (and not null), and (3) omit
user_name and assert the response contains a non-empty generated user_name; each
test should use the same client fixture and status code assertions as existing
tests and only assert presence/format of the generated field (not exact values).
| @model_validator(mode="after") | ||
| def set_defaults(self): | ||
| if self.user_name is None: | ||
| self.user_name = self._refactor_username(self.project_name) | ||
|
|
||
| if self.email is None: | ||
| local_part = self._refactor_username(self.user_name, max_len=200) | ||
| suffix = secrets.token_hex(3) | ||
| self.email = f"{local_part}.{suffix}@kaapi.org" | ||
|
|
||
| if self.password is None: | ||
| self.password = secrets.token_urlsafe(12) | ||
| return self |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against invalid auto-generated emails and avoid hardcoded domain
Two issues:
- If
project_namenormalizes to an empty string (e.g., all invalid/non-ASCII chars),local_partcan be empty, producing an invalid email like ".@kaapi.org". - The domain is hardcoded. Prefer configuration with a safe default.
Also, store whether the email was auto-generated (private attr) to avoid accidentally attaching projects to an unrelated existing user (see below).
Apply this diff:
-from pydantic import BaseModel, EmailStr, model_validator, field_validator
+from pydantic import BaseModel, EmailStr, model_validator, field_validator, PrivateAttr
+from app.core.config import settings
@@
class OnboardingRequest(BaseModel):
@@
user_name: str | None = None
-
+ # Internal flag: not part of the API schema
+ _email_autogenerated: bool = PrivateAttr(default=False)
@@
@model_validator(mode="after")
def set_defaults(self):
if self.user_name is None:
self.user_name = self._refactor_username(self.project_name)
if self.email is None:
- local_part = self._refactor_username(self.user_name, max_len=200)
- suffix = secrets.token_hex(3)
- self.email = f"{local_part}.{suffix}@kaapi.org"
+ local_part = self._refactor_username(self.user_name or self.project_name, max_len=200).strip("._")
+ if not local_part:
+ local_part = "user"
+ # Larger suffix to further reduce collision probability
+ suffix = secrets.token_hex(4)
+ domain = getattr(settings, "AUTO_EMAIL_DOMAIN", "kaapi.org")
+ self.email = f"{local_part}.{suffix}@{domain}"
+ self._email_autogenerated = True
if self.password is None:
self.password = secrets.token_urlsafe(12)
return self📝 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.
| @model_validator(mode="after") | |
| def set_defaults(self): | |
| if self.user_name is None: | |
| self.user_name = self._refactor_username(self.project_name) | |
| if self.email is None: | |
| local_part = self._refactor_username(self.user_name, max_len=200) | |
| suffix = secrets.token_hex(3) | |
| self.email = f"{local_part}.{suffix}@kaapi.org" | |
| if self.password is None: | |
| self.password = secrets.token_urlsafe(12) | |
| return self | |
| from pydantic import BaseModel, EmailStr, model_validator, field_validator, PrivateAttr | |
| from app.core.config import settings | |
| class OnboardingRequest(BaseModel): | |
| project_name: str | |
| user_name: str | None = None | |
| # Internal flag: not part of the API schema | |
| _email_autogenerated: bool = PrivateAttr(default=False) | |
| email: EmailStr | None = None | |
| password: str | None = None | |
| @model_validator(mode="after") | |
| def set_defaults(self): | |
| if self.user_name is None: | |
| self.user_name = self._refactor_username(self.project_name) | |
| if self.email is None: | |
| local_part = ( | |
| self._refactor_username(self.user_name or self.project_name, max_len=200) | |
| .strip("._") | |
| ) | |
| if not local_part: | |
| local_part = "user" | |
| # Larger suffix to further reduce collision probability | |
| suffix = secrets.token_hex(4) | |
| domain = getattr(settings, "AUTO_EMAIL_DOMAIN", "kaapi.org") | |
| self.email = f"{local_part}.{suffix}@{domain}" | |
| self._email_autogenerated = True | |
| if self.password is None: | |
| self.password = secrets.token_urlsafe(12) | |
| return self |
🤖 Prompt for AI Agents
In backend/app/api/routes/onboarding.py around lines 64 to 76, the auto-email
generation can produce an invalid email when the normalized local_part is empty
and the domain is hardcoded; modify set_defaults to (1) compute local_part from
_refactor_username and if it is empty substitute a safe fallback (e.g., "user"
plus a short random suffix or use the project_name slug fallback) before
appending the token, (2) obtain the domain from configuration/environment with a
sensible default instead of "kaapi.org", and (3) set a private attribute (e.g.,
self._email_was_autogenerated = True) whenever the email was created here so
callers can detect and avoid linking to an existing user; ensure you still
return self.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
backend/app/api/routes/onboarding.py (3)
106-116: Project lookup is not scoped to organization; can link across tenantsQuerying by name alone can attach this onboarding to a project in a different organization if names collide. Scope by organization_id.
- existing_project = ( - session.query(Project).filter(Project.name == request.project_name).first() - ) + existing_project = ( + session.query(Project) + .filter( + Project.name == request.project_name, + Project.organization_id == organization.id, + ) + .first() + )
118-129: Avoid linking to an unrelated existing user when the email was auto-generated; allocate a unique emailIf the generated email happens to collide, current code silently attaches this project to an existing user, which is risky. If the email was auto-generated, generate a new unique email instead of reusing an existing user. Also, track whether a new user was created for downstream logic.
- # Validate user - existing_user = session.query(User).filter(User.email == request.email).first() - if existing_user: - user = existing_user - else: - user_create = UserCreate( - full_name=request.user_name, - email=request.email, - password=request.password, - ) - user = create_user(session=session, user_create=user_create) + # Validate user + created_new_user = False + existing_user = session.query(User).filter(User.email == request.email).first() + if existing_user and not getattr(request, "_email_autogenerated", False): + user = existing_user + else: + # If email was auto-generated and already exists, allocate a unique one + if existing_user and getattr(request, "_email_autogenerated", False): + domain = getattr(settings, "AUTO_EMAIL_DOMAIN", "kaapi.org") + base_local = OnboardingRequest._clean_username( + request.user_name or request.project_name, max_len=200 + ).strip("._") or "user" + for _ in range(5): + candidate = f"{base_local}.{secrets.token_hex(4)}@{domain}" + if session.query(User).filter(User.email == candidate).first() is None: + request.email = candidate + break + else: + raise HTTPException( + status_code=409, + detail="Unable to allocate a unique email; please retry.", + ) + user_create = UserCreate( + full_name=request.user_name, + email=request.email, + password=request.password, + ) + user = create_user(session=session, user_create=user_create) + created_new_user = True
148-151: Don’t demote existing users; commit should still persist API keyUnconditionally setting is_superuser to False can silently demote an existing privileged user. Only set this for newly created users, but still commit so the API key is saved.
- # Set user as non-superuser and save to session - user.is_superuser = False - session.add(user) - session.commit() + # Only demote for newly created users; never change privilege on existing users + if created_new_user: + user.is_superuser = False + session.add(user) + # Always commit to persist the API key and any new records + session.commit()
♻️ Duplicate comments (4)
backend/app/api/routes/onboarding.py (4)
40-51: Breaking change: strict user_name regex rejects spaces and will break existing clients/testsThe new validator disallows spaces and many display-name styles. Existing tests/clients post values like "Test User" and will now fail. Prefer not to hard-enforce a handle format on this API field; normalize/trim only and rely on your email-local-part slug logic where needed.
Apply this diff to avoid breaking the API contract:
- @field_validator("user_name") - def validate_username(cls, v): - if v is None: - return v - - pattern = r"^[A-Za-z][A-Za-z0-9._]{2,199}$" - if not re.match(pattern, v): - raise ValueError( - "Username must start with a letter, can contain letters, numbers, underscores, and dots, " - "and must be between 3 and 200 characters long." - ) - return v + @field_validator("user_name") + def normalize_user_name(cls, v: str | None): + if v is None: + return v + return v.strip()
69-76: Generated email can be invalid and can collide; guard empty local-part, use config domain, set flagIf _clean_username yields an empty string, the current logic produces an invalid address like ".abcd12@kaapi.org". Also, the domain is hardcoded. Set a safe fallback, move the domain to settings, add a longer random suffix, and mark the email as auto-generated.
- if self.email is None: - local_part = self._clean_username(self.user_name, max_len=200) - suffix = secrets.token_hex(3) - self.email = f"{local_part}.{suffix}@kaapi.org" + if self.email is None: + local_part = self._clean_username(self.user_name or self.project_name, max_len=200).strip("._") + if not local_part: + local_part = "user" + suffix = secrets.token_hex(4) # longer suffix to reduce collision probability + domain = getattr(settings, "AUTO_EMAIL_DOMAIN", "kaapi.org") + self.email = f"{local_part}.{suffix}@{domain}" + self._email_autogenerated = TrueNote: Email local-part has practical limits (~64 chars). Optional: truncate local_part accordingly before adding suffix.
5-8: Import PrivateAttr and settings for safer generationTo support tracking whether email was auto-generated and avoid hardcoding the domain, add these imports.
-from pydantic import BaseModel, EmailStr, model_validator, field_validator +from pydantic import BaseModel, EmailStr, model_validator, field_validator, PrivateAttr +from app.core.config import settings
38-41: Track whether the email was auto-generatedAdd a private flag to distinguish explicit vs. generated emails. This enables safe behavior later (don’t attach to an existing user when email was generated).
user_name: str | None = None + + # Internal flag: not exposed in API schema + _email_autogenerated: bool = PrivateAttr(default=False)
🧹 Nitpick comments (1)
backend/app/api/routes/onboarding.py (1)
53-62: Slugification can collapse to empty; consider Unicode transliteration and safer fallbacknon-ASCII names can turn entirely into underscores and then be stripped to empty, which later produces invalid local-parts if not guarded downstream. Consider transliteration via unicodedata.normalize or leave as-is but ensure set_defaults provides a robust fallback when the result is empty (see next comment).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
backend/app/api/routes/onboarding.py(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/api/routes/onboarding.py (3)
backend/app/models/user.py (2)
UserCreate(16-17)UserBase(8-12)backend/app/api/routes/private.py (1)
PrivateUserCreate(16-20)backend/app/tests/api/routes/test_onboarding.py (1)
test_onboard_user(16-57)
⏰ 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). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (3)
backend/app/api/routes/onboarding.py (3)
36-38: Making email/password/user_name optional — LGTMThis aligns with the PR objective and keeps the API payload flexible.
124-124: Using full_name for UserCreate — LGTMThis fixes the earlier issue where name wasn’t set properly on the user record.
1-2: Imports for re/secrets — LGTMsecrets is appropriate for token generation; no concerns here.
| email: EmailStr | None = None | ||
| password: str | None = None | ||
| user_name: str | None = None | ||
|
|
||
| @field_validator("user_name") | ||
| def validate_username(cls, v): | ||
| if v is None: | ||
| return v | ||
|
|
||
| pattern = r"^[A-Za-z][A-Za-z0-9._]{2,199}$" | ||
| if not re.match(pattern, v): | ||
| raise ValueError( | ||
| "Username must start with a letter, can contain letters, numbers, underscores, and dots, " | ||
| "and must be between 3 and 200 characters long." | ||
| ) | ||
| return v | ||
|
|
||
| @staticmethod | ||
| def _clean_username(raw: str, max_len: int = 200) -> str: | ||
| """ | ||
| Normalize a string into a safe username that can also be used | ||
| as the local part of an email address. | ||
| """ | ||
| username = re.sub(r"[^A-Za-z0-9._]", "_", raw.strip().lower()) | ||
| username = re.sub(r"[._]{2,}", "_", username) # collapse repeats | ||
| username = username.strip("._") # remove leading/trailing | ||
| return username[:max_len] | ||
|
|
||
| @model_validator(mode="after") | ||
| def set_defaults(self): | ||
| if self.user_name is None: | ||
| self.user_name = self._clean_username(self.project_name) | ||
|
|
||
| if self.email is None: | ||
| local_part = self._clean_username(self.user_name, max_len=200) | ||
| suffix = secrets.token_hex(3) | ||
| self.email = f"{local_part}.{suffix}@kaapi.org" | ||
|
|
||
| if self.password is None: | ||
| self.password = secrets.token_urlsafe(12) | ||
| return self |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Please add tests that omit each optional field to cover default generation paths
Current tests historically included email/password/user_name; ensure we cover cases where each is omitted to validate defaults, and to verify generated email/password/user_name are non-empty and sane.
You can quickly inspect coverage with this script:
🏁 Script executed:
#!/bin/bash
# Show onboarding test payloads and whether email/password/user_name are present
rg -n -C2 --type=py -P 'client\.post\(.+?/onboard' backend/app/tests
echo '--- email keys ---'
rg -n -C0 --type=py -P '"email"\s*:' backend/app/tests/api/routes/test_onboarding.py || true
echo '--- password keys ---'
rg -n -C0 --type=py -P '"password"\s*:' backend/app/tests/api/routes/test_onboarding.py || true
echo '--- user_name keys ---'
rg -n -C0 --type=py -P '"user_name"\s*:' backend/app/tests/api/routes/test_onboarding.py || trueLength of output: 954
Add tests for default generation when optional fields are omitted
Current onboarding tests always include email, password, and user_name, so we don’t exercise the default‐generation logic. Please add tests that:
- POST
/onboardwithout anemailfield, then assert:response.json()["email"]is a non‐empty string- it matches
^[a-z0-9._]+\\.[0-9a-f]{6}@kaapi\\.org$or equivalent pattern
- POST
/onboardwithout apasswordfield, then assert:response.json()["password"]is a non‐empty string- it meets your minimum length/security requirements (e.g.
len ≥ 12)
- POST
/onboardwithout auser_namefield (but withproject_nameset), then assert:response.json()["user_name"]is a lowercase, cleaned version of the project name- it matches the existing username‐validation regex
Optionally, you can also add a test omitting all three fields together to cover the full “after” model‐validator path.
🤖 Prompt for AI Agents
In backend/app/api/routes/onboarding.py around lines 36 to 76, the model's
after-validator generates defaults for user_name, email and password when
omitted but there are no tests covering that behavior; add tests that POST
/onboard omitting each optional field (and one test omitting all three) and
assert: 1) when email is omitted response.json()["email"] is non-empty and
matches regex /^[a-z0-9._]+\.[0-9a-f]{6}@kaapi\.org$/ (hex suffix length 6 from
token_hex(3)); 2) when password is omitted response.json()["password"] is
non-empty and len >= 12; 3) when user_name is omitted (provide project_name)
response.json()["user_name"] is a lowercase, cleaned version of project_name and
matches the existing username regex ^[A-Za-z][A-Za-z0-9._]{2,199}$ (or adapt to
expect lowercase first char if you enforce that) and 4) optionally a combined
test omitting email, password and user_name to ensure all defaults are
generated; ensure tests use the same POST route, parse JSON, and use
deterministic regex/length assertions rather than exact values.
avirajsingh7
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
backend/app/api/routes/onboarding.py (3)
5-5: Remove unused import flagged by Ruff (F401).
field_validatoris imported but not used.Apply this diff:
-from pydantic import BaseModel, EmailStr, model_validator, field_validator +from pydantic import BaseModel, EmailStr, model_validator
56-60: Optional: Make domain configurable and further reduce collision risk.
- Avoid hardcoding
kaapi.org. Usesettings.AUTO_EMAIL_DOMAINwith a safe default.- Consider a longer suffix (
token_hex(4)) and/or retry on collision to avoid linking a project to an unrelated existing user if the email was generated.If you decide to configure the domain, import settings and adjust generation:
+from app.core.config import settings @@ - self.email = f"{local_part}.{suffix}@kaapi.org" + domain = getattr(settings, "AUTO_EMAIL_DOMAIN", "kaapi.org") + self.email = f"{local_part}.{suffix}@{domain}"To avoid accidental linking, you can mark generated emails and, if
existing_useris found with an auto-generated email, retry with a new suffix instead of attaching to that user.
106-116: Collision handling: avoid attaching to an unrelated user if the email was auto-generated.Right now, if the server generated the email and it happens to match an existing user, we attach the new project to that user and issue an API key — potentially cross-tenant. Low probability due to the suffix, but non-zero.
Consider:
- Marking server-generated emails (e.g., private attr on the request model), and
- If
existing_useris found and the email was generated, re-roll a new email (a few attempts) or return HTTP 409 to have the client retry.If you want, I can draft a small retry loop that re-rolls the suffix up to N times before failing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
backend/app/api/routes/onboarding.py(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/api/routes/onboarding.py (3)
backend/app/api/routes/private.py (1)
PrivateUserCreate(16-20)backend/app/models/user.py (2)
UserCreate(16-17)UserBase(8-12)backend/app/tests/api/routes/test_onboarding.py (1)
test_onboard_user(16-57)
🪛 Ruff (0.12.2)
backend/app/api/routes/onboarding.py
5-5: pydantic.field_validator imported but unused
Remove unused import: pydantic.field_validator
(F401)
⏰ 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). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (3)
backend/app/api/routes/onboarding.py (3)
36-38: LGTM: Made email/password/user_name optional as per PR objectiveThe API contract now aligns with the objective. Default generation logic below covers the missing fields.
61-63: LGTM: Secure default password generation within model constraints.
secrets.token_urlsafe(12)yields ~16 characters (>=8, <=40). Good balance of entropy and compatibility.
110-114: LGTM: Correctly using full_name when creating the user.This fixes the earlier issue where the
namefield ended up null in the DB.
| @model_validator(mode="after") | ||
| def set_defaults(self): | ||
| if self.user_name is None: | ||
| self.user_name = self.project_name + " User" | ||
|
|
||
| if self.email is None: | ||
| local_part = self._clean_username(self.user_name, max_len=200) | ||
| suffix = secrets.token_hex(3) | ||
| self.email = f"{local_part}.{suffix}@kaapi.org" | ||
|
|
||
| if self.password is None: | ||
| self.password = secrets.token_urlsafe(12) | ||
| return self |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Please add tests that omit optional fields to exercise default generation.
Current tests historically always provided email, password, and user_name. We need coverage for:
- Omitted
email→ response user/email created and looks valid - Omitted
password→ non-empty generated password (len >= 12) - Omitted
user_name→ generated fromproject_name
If possible, also add a test omitting all three.
You can use this script to quickly inspect existing onboarding tests for omitted fields:
🏁 Script executed:
#!/bin/bash
# Show onboarding POST payloads and presence of optional keys
rg -n -C2 --type=py -P 'client\.post\(.+?/onboard' backend/app/tests
echo '--- email keys ---'
rg -n -C0 --type=py -P '"email"\s*:' backend/app/tests/api/routes/test_onboarding.py || true
echo '--- password keys ---'
rg -n -C0 --type=py -P '"password"\s*:' backend/app/tests/api/routes/test_onboarding.py || true
echo '--- user_name keys ---'
rg -n -C0 --type=py -P '"user_name"\s*:' backend/app/tests/api/routes/test_onboarding.py || trueLength of output: 954
Add tests for default generation of optional onboarding fields
Current tests in backend/app/tests/api/routes/test_onboarding.py always include "email", "password", and "user_name", so the model_validator branches that set defaults are never exercised. Please add tests that cover:
- Omitting
email
• Verify that the response’suser.emailis generated in the form<clean_username>.<hex>@kaapi.org - Omitting
password
• Verify thatuser.passwordis non-empty, URL-safe, and at least 12 characters long - Omitting
user_name
• Verify thatuser.user_namedefaults to<project_name> User - (Optional) Omitting all three fields in one request to ensure they all default correctly
🤖 Prompt for AI Agents
In backend/app/api/routes/onboarding.py around lines 51 to 63, the
model_validator that generates defaults for user_name, email, and password is
never exercised by tests; add new tests in
backend/app/tests/api/routes/test_onboarding.py that post onboarding requests
omitting each optional field to exercise those branches: (1) omit "email" and
assert response JSON user.email matches
"<cleaned_user_name>.<6-hex-chars>@kaapi.org" (cleaning uses the same rules as
_clean_username), (2) omit "password" and assert response JSON user.password is
non-empty, URL-safe (contains only URL-safe characters), and length >= 12, (3)
omit "user_name" and assert response JSON user.user_name equals "<project_name>
User", and (4) optionally send a request omitting all three and assert all
defaults are set accordingly; ensure tests use the same project_name input and
validate patterns/lengths rather than exact random values.
| if self.email is None: | ||
| local_part = self._clean_username(self.user_name, max_len=200) | ||
| suffix = secrets.token_hex(3) | ||
| self.email = f"{local_part}.{suffix}@kaapi.org" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix: Ensure auto-generated email local-part is valid and within RFC length (64).
Two issues can cause user creation to fail (likely as 500 from UserCreate validation):
- If
_clean_username(...)yields an empty string (e.g., input is all invalid/non-ASCII), you'll generate emails like.abcdef@kaapi.orgwhich are invalid. - The local-part can exceed 64 chars (you currently allow up to 200), which is invalid and will be rejected by
EmailStr.
Harden the generation by providing a fallback base and truncating to fit 64 chars (leaving room for the dot and suffix).
Apply this diff:
- if self.email is None:
- local_part = self._clean_username(self.user_name, max_len=200)
- suffix = secrets.token_hex(3)
- self.email = f"{local_part}.{suffix}@kaapi.org"
+ if self.email is None:
+ suffix = secrets.token_hex(3) # 6 hex chars
+ base = self._clean_username(self.user_name or self.project_name, max_len=200).strip("._")
+ if not base:
+ base = "user"
+ # RFC local-part max length is 64; reserve 1 for the dot and len(suffix)
+ allowed = max(1, 64 - 1 - len(suffix))
+ local_part = base[:allowed]
+ self.email = f"{local_part}.{suffix}@kaapi.org"📝 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 self.email is None: | |
| local_part = self._clean_username(self.user_name, max_len=200) | |
| suffix = secrets.token_hex(3) | |
| self.email = f"{local_part}.{suffix}@kaapi.org" | |
| if self.email is None: | |
| suffix = secrets.token_hex(3) # 6 hex chars | |
| base = self._clean_username(self.user_name or self.project_name, max_len=200).strip("._") | |
| if not base: | |
| base = "user" | |
| # RFC local-part max length is 64; reserve 1 for the dot and len(suffix) | |
| allowed = max(1, 64 - 1 - len(suffix)) | |
| local_part = base[:allowed] | |
| self.email = f"{local_part}.{suffix}@kaapi.org" |
🤖 Prompt for AI Agents
In backend/app/api/routes/onboarding.py around lines 56 to 60, the
auto-generated email local-part can be empty or exceed the 64-character RFC
limit; ensure you provide a safe fallback when _clean_username returns an empty
string and truncate the local-part to fit within 64 characters leaving room for
the dot and suffix. After calling _clean_username, if the result is empty set a
fallback like "user", compute max_local = 64 - 1 - len(suffix) (1 for the dot),
truncate the cleaned local-part to max_local before composing
f"{local}.{suffix}@kaapi.org", and keep the suffix length small enough (e.g.,
token_hex(3)) so the calculation always produces a positive max_local. Ensure no
additional characters are added that could push the local-part over 64.
* email and password optional in onboarding api request * add suffix to email autogenerated * make username optional * rename method * refactor username
2 participants
Summary
As we discussed for the Glific use case, we can make email optional in the onboarding flow.
This change simplifies onboarding and removes the need for clients to always provide an email address.
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Summary by CodeRabbit
New Features
Tests