Conversation
Implemented a Model Context Protocol (MCP) server that provides: - Course and lecture search functionality - Review retrieval for courses and lectures - Intelligent course recommendation based on multiple criteria - High rating (overall score) - Easy grading - Low workload - Good teaching quality The server communicates with OTL Plus API endpoints and provides read-only access to course data through MCP tools. Tech stack: - TypeScript - @modelcontextprotocol/sdk - axios for API calls - zod for validation Location: apps/mcp-server/
Enhanced the MCP server with authentication support and user-specific features: Authentication: - Added API Key (JWT access token) support - Implemented setAccessToken/clearAccessToken methods in API client - Support for environment variable OTL_ACCESS_TOKEN New Tools: - set_api_key: Set JWT access token for authenticated requests - get_user_profile: Retrieve current user's profile information - get_my_taken_courses: Get all courses taken by the user - get_taken_lectures_by_semester: Get lectures from a specific semester API Client Updates: - Added authentication headers support - New methods: getUserProfile, getUserTakenCourses, getTakenLecturesBySemester - Authentication state checking Type Definitions: - Added UserProfile, TakenCoursesParams, TakenLecture types Documentation: - Updated README with authentication setup guide - Added API token acquisition instructions - Provided examples for authenticated features - Separated public and authenticated tools in documentation Users can now: 1. Set their access token via environment variable or set_api_key tool 2. View their course history 3. Get semester-specific lecture information 4. Access personalized course data
Implemented a complete API Key authentication system for OTL Plus: Database Schema: - Added session_api_key table with fields: - id, userprofile_id, key (unique), name, created_at - last_used_at, expires_at, is_active - Indexed on key and userprofile_id for performance - Cascade delete when user is deleted Auth Service (auth.service.ts): - generateApiKey(): Generate secure 64-char hex API keys - createApiKey(): Create new API key for user - findUserByApiKey(): Validate and retrieve user by API key - listApiKeys(): Get all API keys for a user - revokeApiKey(): Deactivate an API key - deleteApiKey(): Permanently delete an API key API Endpoints (auth.controller.ts): - POST /session/api-keys - Create new API key - GET /session/api-keys - List all user's API keys - POST /session/api-keys/:keyId/revoke - Revoke an API key - DELETE /session/api-keys/:keyId - Delete an API key Authentication Guard: - Created ApiKeyGuard for API key validation - Supports Authorization: Bearer <key> header - Supports X-API-Key header - Updates last_used_at timestamp on each use - Checks expiration and active status Repository (user.repository.ts): - createApiKey(): Store new API key - findApiKey(): Find key with userprofile relation - updateApiKeyLastUsed(): Track usage - listApiKeys(): Query user's keys - revokeApiKey(): Deactivate key - deleteApiKey(): Remove key MCP Server Documentation: - Added API Key setup guide - Provided curl examples for key generation - Documented API Key management endpoints - Recommended API Keys over JWT tokens for MCP usage Benefits: - Non-expiring tokens suitable for MCP servers - Better security through key rotation - Usage tracking with last_used_at - Easy management and revocation - Multiple keys per user for different applications Next Steps: - Run yarn client:generate to regenerate Prisma client - Run migrations to create session_api_key table
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| ## 라이선스 | ||
|
|
||
| MIT |
There was a problem hiding this comment.
코드 리뷰
잠재적 버그 및 위험 요소
-
인증 절차 관련 취약성:
set_api_key를 통해 JWT 토큰을 설정할 때, 유효하지 않거나 만료된 토큰이 입력될 경우 적절한 오류 처리가 필요합니다. 이 처리가 없으면 API 호출 시 오류가 발생해 사용 경험이 저하될 수 있습니다. -
파라미터 검증: 함수의 파라미터에 대한 유효성 검사가 부족하여 경계값 입력이나 잘못된 형식의 데이터가 들어올 경우 예기치 않은 동작을 일으킬 수 있습니다. 예를 들어,
limit의 경우 정수여야 하며 부정적인 값이 들어올 경우 이를 처리해야 합니다. -
HTTP 요청 실패 처리: API 요청이 실패할 경우 (예: 서버 오류, 네트워크 문제 등) 클라이언트 측에서 오류를 어떻게 처리할지에 대한 로직이 필요합니다. 유저에게 적절한 피드백을 주기 위해 이 부분을 구현해야 합니다.
개선 제안
- 명확한 오류 메시지: 사용자 경험을 향상시키기 위해, API 호출 실패 시 명확한 오류 메시지를 제공해야 합니다. 사용자가 무엇이 잘못되었는지 알 수 있도록 구체적인 정보를 제공하는 것이 중요합니다.
- 매개변수 문서화: 각 API의 매개변수에 대한 설명(허용 값 및 기본값 포함)을 더 확장할 수 있습니다. 예를 들어
limit의 기본값이 20임을 강조하거나,year와semester의 유효 범위를 명확히 하고, optional로 제공되는 파라미터에 대한 설명 추가가 필요합니다. - 보안 강조: API 키 사용 시 보안상의 이유로 사용자에게 키를 노출시키지 않는 것이 중요합니다. 이를 위해 키관리 방법에 대해 더 설명할 필요가 있습니다. 예를 들어, 개발 과정에서 .env 파일 등을 사용하는 방법을 추가하는 것이 도움이 될 수 있습니다.
- 환경 변수 관리:
OTL_BASE_URL을 설정할 수 있도록 하였지만, 다른 환경 변수를 사용하는 방법을 문서의 다른 곳에 명시하는 것이 좋습니다. 예를 들어, 로컬 개발 환경과 프로덕션 환경에서의 설정 방법을 비교해 설명함으로써 사용자에게 혼란을 주지 않도록 합니다.
이러한 사항들을 고려하여 코드를 개선하는 것이 바람직합니다.
| "@types/node": "^22.10.3", | ||
| "typescript": "^5.7.2" | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 패치 리뷰
-
문서화:
author필드가 비어 있습니다. 패키지에 대한 기여자를 명시하는 것은 좋습니다. 관련자 정보를 입력하거나 이 필드를 제거하세요. -
버전 관리: 버전이
1.0.0으로 설정되어 있는데, 이 경우 더 이상 코드가 빠르게 개발된다면 이 번호는 변경될 수 있습니다. 메이저 업데이트를 위해서는 버전 관리 정책을 정립하는 것이 좋습니다. -
스크립트의 일관성:
build와dev스크립트에서 TypeScript 컴파일 명령어가 중복되고 있습니다.dev스크립트는build를 호출하도록 변경하여 중복을 제거할 수 있습니다:"dev": "npm run build && node dist/index.js"
-
종속성 검토:
@modelcontextprotocol/sdk의 최신 버전이 사용되고 있는지 확인하세요. 사용 중인 버전의 안정성과 버그 수정 사항을 검토할 필요가 있습니다. -
라이센스 문제:
license필드는 MIT로 설정되어 있지만, 올바른 라이센스를 보장하기 위해 이 패키지에 포함된 코드가 실제로 MIT 라이센스 기준에 부합하는지를 확인해야 합니다.
위 사항들을 반영하면 코드 품질이 더욱 향상될 수 있습니다.
| const response = await this.client.get<TakenLecture[]>(`/session/${year}/${semester}/taken-lectures`); | ||
| return response.data; | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 리뷰
버그 및 리스크
-
예외 처리 부족: API 요청에서 오류가 발생할 경우(예: 네트워크 오류, 서버 오류 등) 예외를 처리할 로직이 없습니다.
try...catch블록을 추가하여 오류를 캡처하고 적절한 오류 메시지를 반환해야 합니다. -
상태 코드 확인 필요: API 응답 상태 코드에 대한 처리가 없습니다. 예를 들어,
response.status를 확인하여 성공적인 응답이 아닌 경우에 대한 처리가 필요합니다. -
타입 안전성:
getCourseAutocomplete및getLectureAutocomplete메서드는 문자열 또는undefined를 반환할 수 있지만, 클라이언트에서 이를 처리하는 방법에 대한 안내가 없습니다. 결과를 안전하게 처리하도록 명시해야 합니다. -
네트워크 요청 수 제한: 빈번하게 API 요청을 수행할 경우, 서버에 과도한 부하를 줄 수 있습니다. 필요한 경우 요청 수를 제한하거나 페이징 처리를 검토해야 합니다.
개선 제안
-
문서화 강화: 각 메서드는 설명이 존재하지만, 반환값과 예외 상황에 대한 문서화가 부족합니다. 문서화에 반환되는 데이터의 구조와 예외 상황을 추가하는 것이 좋습니다.
-
커스텀 에러 클래스 사용: 공통된 오류 메시지 대신, 커스텀 에러 클래스를 만들어 보다 의미 있는 오류 정보를 제공하는 것이 좋습니다.
-
인증 처리 메서드 개선:
isAuthenticated메서드는 액세스 토큰이 있는지만 체크합니다. 하지만 토큰의 유효성을 확인하지 않고, 만약 잘못된 토큰이 있는 경우에는 추가적인 검증을 포함할 필요가 있습니다. -
상수화: URL 경로는 하드코딩되어 있습니다. API의 엔드포인트를 상수로 정의하여 관리하는 것이 나중에 유지보수에 유리할 수 있습니다.
|
|
||
| export interface TakenLecture extends Lecture { | ||
| // Taken lecture may include additional fields | ||
| } |
There was a problem hiding this comment.
이 코드 패치는 API 응답을 위한 타입 정의를 포함하고 있습니다. 아래는 잠재적인 버그, 리스크, 개선 사항에 대한 피드백입니다:
-
미사용 필드:
Course와Professor인터페이스에professor_name필드가 있으나, 중복성으로 인해 혼란을 초래할 수 있습니다. 이 경우, 적절한 필드 사용을 보장하기 위해서 프레필드에서 선택할 수 있는 방안을 고려할 필요가 있습니다. -
타입 일관성:
department필드는Department타입으로 정의되어 있지만,Course및Lecture인터페이스에서의 사용을 더 명확하게 설명하는 주석이 부족합니다. 이 필드가 필요할 때와 어떻게 사용되는지에 대한 명확한 문서가 필요합니다. -
선택적 필드: 많은 인터페이스에서 다수의 필드가 선택적(
?)로 되어 있습니다. 선택적 필드를 사용할 경우, 이는 잘못된 입력을 유발할 수 있는 잠재적 문제를 가지므로 이 필드들이 어떤 시나리오에서 필수적인지 명확히 하는 것이 좋습니다. -
Search*Params타입들: 이들 타입에서order필드는 잘못된 순서나 비어 있는 배열을 제어하는 로직이 필요합니다. 배열의 유효성 체크가 없으면 의도하지 않은 결과를 초래할 수 있습니다. -
종합적인 문서화: 각 인터페이스가 어떤 역할을 하는지, 그리고 각 필드의 목적과 사용 사례에 대한 자세한 문서화가 필요하며, 이를 통해 팀원들 간의 이해가 높아질 것입니다.
-
타입 정제:
SearchCoursesParams및 유사 구조에서의 필드를 다음과 같이 더 구체적으로 할당하여 이해도를 높일 수 있습니다. 예를 들어,begin과end를timeRange라는 객체로 묶는 방법입니다.
결론적으로, 코드는 기초적으로 괜찮지만, 구조의 일관성 및 클린 코드를 위한 개선 여지가 많습니다. 따라서 이러한 개선 사항을 반영한 후에 머지를 고려하는 것이 좋습니다.
| await this.authService.deleteApiKey(user.id, keyId) | ||
| return { message: 'API key deleted successfully' } | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 리뷰 코멘트
-
API 키 생성 및 삭제 관련 누락된 유효성 검사:
createApiKey및deleteApiKey메서드에 대한 입력 검증이 없습니다.body.name또는keyId의 유효성을 검증하는 로직을 추가해야 합니다. -
오류 처리: 현재 API 키 관련 메서드에서 예외 상황에 대한 처리가 없습니다. 예를 들어,
authService.createApiKey,authService.revokeApiKey,authService.deleteApiKey가 실패할 경우 적절한 오류 메시지를 반환하도록 예외 처리가 필요합니다. -
HTTP 메서드 및 경로의 일관성:
createApiKey,listApiKeys,revokeApiKey,deleteApiKey와 같은 메서드들이 API 리소스에 대한 작업을 수행하지만, 메서드의 역할에 비해 경로가 일반적이지 않을 수 있습니다. RESTful 디자인 원칙을 고려하여 진입 경로를 정리할 필요가 있습니다. -
문서화 부족: 각 API 메서드에 대한 설명이 부족합니다. Swagger와 같은 도구를 사용하여 API 메서드에 대한 문서화를 고려하세요.
-
응답 형식: API 응답 형식이 일관되지 않을 수 있습니다. 모두 같은 구조를 가지도록 변경하세요.
이 모든 사항을 고려할 때, 코드가 병합되기 전에 위의 문제들을 해결할 필요가 있습니다.
| async deleteApiKey(userId: number, keyId: number): Promise<void> { | ||
| await this.userRepository.deleteApiKey(userId, keyId) | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 검토:
-
보안 문제: API 키를 생성하고 사용하며 관리하는 부분에서 모든 키가 32바이트의 랜덤 바이트로부터 생성되어 64글자의 16진수 문자열로 변환됩니다. 이 방식은 안전하지만, 더 나은 보안을 위해 API 키의 복잡성을 증가시키거나 추가적인 보호 메커니즘(예: 키 관리 또는 암호화 저장소 사용)을 고려할 수 있습니다.
-
API 키 만료 처리:
findUserByApiKey메서드는 API 키가 만료되었다고 판단할 때null을 반환합니다. 이것은 사전 예방적인 조치이지만, API 키가 만료되었을 때 관련된 사용자에게 알리는 방법이 필요할 수 있습니다. 사용자를 추적할 수 있는 로그 또는 새로운 프로세스를 도입하여 관리할 수 있습니다. -
예외 처리: 사용자 API 키를 조회하거나 삭제할 때, 예외가 발생할 가능성이 있습니다. 이러한 메서드는 성공과 실패를 명확하게 나타내고, 실패한 경우에 대한 명시적인 처리를 요구합니다. 예외 처리 로직을 추가하는 것을 추천합니다.
-
입력 검증:
createApiKey및revokeApiKey,deleteApiKey와 같은 메서드는 사용자 입력을 받습니다. 이 입력에 대한 유효성 검사가 필요합니다. 숫자 범위 및 사용자 ID의 유효성을 점검하는 논리를 고려하는 것이 좋습니다. -
테스트 부족 가능성: 새롭게 추가된 메서드에 대한 단위 테스트나 통합 테스트가 작성되어 있는지 확인해야 합니다. 또한 모든 CRUD 작업에 대해 이해할 수 있는 에러 핸들링을 테스트해야 합니다.
-
주석 및 문서화: 현재 주석은 기본적인 설명을 제공하지만, 특히 중요한 메서드에 대해서는 더 자세한 설명이 필요할 수 있습니다. 더 자세한 문서화를 통해 향후 유지보수와 코드 이해를 용이하게 할 수 있습니다.
|
|
||
| return null | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 패치를 검토한 결과, 다음과 같은 문제와 개선 사항이 발견되었습니다:
-
타입 안정성:
extractApiKeyFromRequest메서드의request매개변수가any로 선언되어 있습니다. 이를 명확하게Request타입으로 변경하여 코드의 타입 안정성을 높이는 것이 좋습니다. NestJS를 사용하는 경우Request타입을 임포트하고 사용할 수 있습니다. -
에러 메시지: API 키가 누락되거나 잘못된 경우
UnauthorizedException을 던지고 있습니다. 이와 관련된 에러 메시지는 사용자에게 충분한 정보를 제공하지 않을 수 있습니다. 가능하다면, 에러 메시지를 보다 구체적으로 제시하여 디버깅이 용이하도록 하는 것이 좋습니다. -
API 키 유효성 검사: 유효한 API 키를 확인하는 과정이
authService.findUserByApiKey메서드를 통해 이루어지고 있으나, 이 메서드가 반환하는 사용자의 정보가 신뢰할 수 있는지 확인할 필요가 있습니다. 예를 들어, 만약 API 키가 만료되는 경우나 비활성화된 사용자의 경우도 고려해야 합니다. -
테스트 커버리지:
ApiKeyGuard클래스를 단위 테스트로 철저히 검증하는 것이 중요합니다. 올바른 API 키와 잘못된 API 키, 공개 엔드포인트의 시나리오 등에서 적절히 작동하는지 확인해야 합니다. -
불필요한 의존성:
Reflector가 사용되지만, 혹시 이 Guards를 사용하는 곳에서 중복된 속성 검사 로직이 있을 수 있습니다. 의존성을 최소화하고 코드의 가독성을 높이는 방향으로 정리해보는 것이 좋습니다.
이러한 문제들을 해결하고 개선하면 코드의 안정성과 가독성이 향상될 것입니다.
| }, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
다음은 코드 패치에 대한 검토 의견입니다:
-
예외 처리 부족: 데이터베이스 작업을 수행하는 모든 메서드에서 예외 처리를 추가하는 것이 좋습니다. 현재 코드에서는 에러가 발생할 경우에 대한 처리가 없으므로, 예외 발생 시 일반적인 서버 오류가 발생할 수 있습니다.
-
입력 검증 필요:
createApiKey메서드에서data의 속성에 대한 입력 검증이 부족합니다. 예를 들어,key가 빈 문자열이거나userprofile_id가 유효하지 않은 경우를 처리해야 합니다. -
삭제 및 갱신 처리의 안전성:
deleteApiKey및revokeApiKey메서드에서 사용자의 ID와 함께 API 키의 ID를 사용하고 있습니다. 이러한 조합이 적절한 권한 확인 없이 삭제되거나 비활성화될 경우 보안 문제를 일으킬 수 있으므로, 추가적인 권한 검사 로직이 필요합니다. -
타입 안전성:
updateApiKeyLastUsed메서드에서keyId가 유효한 키 ID인지 확인하는 로직이 없어, 잘못된 ID로 인해 예기치 않은 결과가 발생할 수 있습니다. 이 확인을 추가하는 것이 좋습니다. -
타입명시: 메서드의 반환 타입을 명시하는 것이 좋습니다. 어떤 기대하는 데이터 구조가 반환되는지 명확히 하는 것이 가독성을 높이고, 후속 작업 시 오류를 줄일 수 있습니다.
-
주석 추가: 각 메서드의 역할에 대한 주석을 추가하여 코드의 가독성을 높이는 것이 좋습니다. 특히 API 키와 관련된 중요한 작업이 많으므로, 코드 이해도를 높일 수 있습니다.
종합적으로, 이 코드 패치는 기본 기능은 충족하나 보안 및 에러 처리 측면에서 개선이 필요한 상태입니다.
|
|
||
| @@index([userprofile_id], map: "session_api_key_userprofile_id_fkey") | ||
| @@index([key], map: "session_api_key_key_index") | ||
| } |
There was a problem hiding this comment.
다음은 코드 패치에 대한 리뷰입니다:
-
관계 설정:
session_api_key모델에userprofile필드가onDelete: Cascade설정되어 있습니다. 이는 사용자가 삭제될 때 모든 관련 API 키도 삭제된다는 것을 의미합니다. 이 동작이 의도된 것인지 확인이 필요합니다. 사용자 데이터 삭제 시 API 키가 삭제되는 것이 올바른지 검토하십시오. -
키 유효성 검사: 현재
key필드는@unique지시어가 있어 중복된 API 키를 허용하지 않습니다. 그러나 키 생성의 정확성과 안전성을 확인하기 위한 로직이 필요합니다. API 키 생성 시 유효성이 검증되는지 확인하고, 적절한 오류 처리가 필요합니다. -
Nullable 필드:
name과last_used_at은 nullable로 설정되어 있습니다. 이 필드에 대한 무효 상태를 관리할 필요성이 있습니다. 예를 들어, 사용하는 부분에서 null 상태를 충분히 처리하는지 확인해야 합니다. -
인덱스 설정: 현재 두 개의 인덱스가 설정되어 있습니다. 인덱스가 필요하지만 이 두 인덱스가 실제로 성능을 개선하는 데 유용한지 확인하십시오. 데이터베이스 성능을 평가하고 불필요한 인덱스를 피하는 것이 좋습니다.
-
테스트 부족: 해당 모델의 구현이 잘 동작하는지 확인하기 위한 유닛 테스트 및 통합 테스트가 포함되어 있어야 합니다. 특히 API 키 처리와 관련된 로직이 안정적인지 테스트가 필요합니다.
진행하기 전에 위의 사항들을 확인하고 필요한 수정을 진행하십시오.
2 participants
No description provided.