Skip to content

Feat/auth migration#240

Merged
locnguyen1986 merged 8 commits intomainfrom
feat/auth-migration
Nov 13, 2025
Merged

Feat/auth migration#240
locnguyen1986 merged 8 commits intomainfrom
feat/auth-migration

Conversation

@locnguyen1986
Copy link
Collaborator

Fix auth
Fix project and conversations

Copilot AI review requested due to automatic review settings November 13, 2025 18:10
@locnguyen1986 locnguyen1986 merged commit 7f99adb into main Nov 13, 2025
3 checks passed
@github-project-automation github-project-automation bot moved this to QA in Jan Nov 13, 2025
@locnguyen1986 locnguyen1986 deleted the feat/auth-migration branch November 13, 2025 18:10
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enhances the authentication flow and adds project public ID tracking to conversations. The changes improve the OAuth callback mechanism by storing redirect URLs in the authorization request state, and denormalize the project's public ID in the conversations table for more efficient API responses.

  • Fixed OAuth redirect URL handling to store and reuse the client-specified redirect URL across the authorization flow
  • Added project_public_id column to conversations table with proper indexing for performance
  • Updated conversation domain models, handlers, and responses to track and expose project public IDs

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
services/llm-api/migrations/000004_add_project_public_id_to_conversations.up.sql Adds project_public_id VARCHAR(64) column and index to conversations table
services/llm-api/migrations/000004_add_project_public_id_to_conversations.down.sql Rollback migration to remove project_public_id column and index
services/llm-api/internal/interfaces/httpserver/responses/conversation/conversation.go Adds ProjectID field to conversation response and maps from domain model
services/llm-api/internal/interfaces/httpserver/handlers/conversationhandler/conversation_handler.go Extracts and passes project public ID when creating conversations
services/llm-api/internal/interfaces/httpserver/handlers/authhandler/keycloak_oauth.go Stores redirect URL in AuthRequest and uses it during OAuth callback instead of query parameter
services/llm-api/internal/infrastructure/database/dbschema/conversation.go Adds ProjectPublicID field to conversation schema with GORM tags
services/llm-api/internal/domain/conversation/conversation_service.go Adds ProjectPublicID to CreateConversationInput and sets it on conversation entity
services/llm-api/internal/domain/conversation/conversation.go Adds ProjectPublicID field to Conversation domain model

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +2 to +4
DROP INDEX IF EXISTS llm_api.idx_conversations_project_public_id;

ALTER TABLE llm_api.conversations
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The schema prefix llm_api is used inconsistently. This down migration includes the schema prefix, but migration 000002's down migration doesn't use it (e.g., ALTER TABLE conversations without the schema prefix). Consider removing the llm_api prefix to maintain consistency with the existing migration pattern.

Suggested change
DROP INDEX IF EXISTS llm_api.idx_conversations_project_public_id;
ALTER TABLE llm_api.conversations
DROP INDEX IF EXISTS idx_conversations_project_public_id;
ALTER TABLE conversations

Copilot uses AI. Check for mistakes.
Comment on lines +178 to +181
if redirectURL == "" {
// Default redirect URL if not provided
redirectURL = "https://chat-dev.jan.ai/auth/keycloak/callback"
}
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hardcoded fallback URL https://chat-dev.jan.ai/auth/keycloak/callback is environment-specific and may not work correctly in different deployment environments (production, staging, local development). Consider using a configured default redirect URL from the handler's configuration (e.g., add a defaultRedirectURL field to the handler struct initialized from environment variables) instead of hardcoding a dev environment URL.

Copilot uses AI. Check for mistakes.
redirectURL := authRequest.RedirectURL
if redirectURL == "" {
// Default redirect URL if not provided
redirectURL = "https://chat-dev.jan.ai/auth/keycloak/callback"
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hardcoded fallback URL https://chat-dev.jan.ai/auth/keycloak/callback is environment-specific and may not work correctly in different deployment environments. Consider using a configured default redirect URL instead of hardcoding a dev environment URL. This should match the default used in InitiateLogin for consistency.

Suggested change
redirectURL = "https://chat-dev.jan.ai/auth/keycloak/callback"
redirectURL = config.Get().Keycloak.RedirectURL

Copilot uses AI. Check for mistakes.
Comment on lines +177 to +181
redirectURL := c.Query("redirect_url")
if redirectURL == "" {
// Default redirect URL if not provided
redirectURL = "https://chat-dev.jan.ai/auth/keycloak/callback"
}
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The redirect_url query parameter is accepted without validation, which creates an open redirect vulnerability. An attacker could craft a malicious authorization URL with a redirect to an external domain. Consider validating that the redirect URL matches an allowlist of allowed domains/patterns before storing it in the AuthRequest.

Copilot uses AI. Check for mistakes.
Comment on lines +2 to +9
ALTER TABLE llm_api.conversations
ADD COLUMN IF NOT EXISTS project_public_id VARCHAR(64);

-- Add index for project_public_id
CREATE INDEX IF NOT EXISTS idx_conversations_project_public_id
ON llm_api.conversations(project_public_id);

COMMENT ON COLUMN llm_api.conversations.project_public_id IS 'Public ID of the project this conversation belongs to';
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The schema prefix llm_api is used inconsistently across migration files. Migration 000002 and 000003 don't use the schema prefix (e.g., ALTER TABLE conversations instead of ALTER TABLE llm_api.conversations), but this migration does. Consider removing the llm_api prefix to maintain consistency with the existing migration pattern, or update all previous migrations to use the schema prefix consistently.

Suggested change
ALTER TABLE llm_api.conversations
ADD COLUMN IF NOT EXISTS project_public_id VARCHAR(64);
-- Add index for project_public_id
CREATE INDEX IF NOT EXISTS idx_conversations_project_public_id
ON llm_api.conversations(project_public_id);
COMMENT ON COLUMN llm_api.conversations.project_public_id IS 'Public ID of the project this conversation belongs to';
ALTER TABLE conversations
ADD COLUMN IF NOT EXISTS project_public_id VARCHAR(64);
-- Add index for project_public_id
CREATE INDEX IF NOT EXISTS idx_conversations_project_public_id
ON conversations(project_public_id);
COMMENT ON COLUMN conversations.project_public_id IS 'Public ID of the project this conversation belongs to';

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: QA

Development

Successfully merging this pull request may close these issues.

2 participants