github.pull_request_review

{
  "$raw": {},
  "action": "submitted",
  "author": "developer123",
  "changes": [
    {
      "additions": 25,
      "changes": 35,
      "deletions": 10,
      "filename": "src/services/auth.ts",
      "patch": "@@ -15,10 +15,25 @@ export class AuthService {\n   private async validateToken(token: string) {\n-    // Basic auth validation\n-    const [username, password] = Buffer.from(token, 'base64')\n-      .toString()\n-      .split(':');\n-\n-    return this.validateCredentials(username, password);\n+    try {\n+      const decoded = jwt.verify(token, this.config.jwt_secret) as JwtPayload;\n+      \n+      if (!decoded.sub || !decoded.exp) {\n+        throw new Error('Invalid token structure');\n+      }\n+      \n+      if (decoded.exp < Date.now() / 1000) {\n+        throw new Error('Token has expired');\n+      }\n+      \n+      const user = await this.userService.findById(decoded.sub);\n+      if (!user) {\n+        throw new Error('User not found');\n+      }\n+      \n+      if (!user.active) {\n+        throw new Error('User account is disabled');\n+      }\n+      \n+      return user;\n+    } catch (error) {\n+      throw new AuthenticationError('Invalid or expired token');\n+    }\n   }",
      "status": "modified"
    },
    {
      "additions": 50,
      "changes": 50,
      "deletions": 0,
      "filename": "tests/auth.test.ts",
      "patch": "@@ -0,0 +1,50 @@\n+import { describe, it, expect, jest } from 'jest';\n+import { AuthService } from '../src/services/auth';\n+import { UserService } from '../src/services/user';\n+import { AuthenticationError } from '../src/errors';\n+\n+describe('AuthService', () => {\n+  let authService: AuthService;\n+  let userService: jest.Mocked<UserService>;\n+\n+  beforeEach(() => {\n+    userService = {\n+      findById: jest.fn(),\n+    } as any;\n+\n+    authService = new AuthService({\n+      jwt_secret: 'test-secret',\n+      token_expiry: '1h'\n+    }, userService);\n+  });\n+\n+  describe('validateToken', () => {\n+    it('should successfully validate a valid token', async () => {\n+      const mockUser = {\n+        id: '123',\n+        active: true,\n+        name: 'Test User'\n+      };\n+\n+      userService.findById.mockResolvedValue(mockUser);\n+\n+      const token = authService.generateToken(mockUser);\n+      const result = await authService.validateToken(token);\n+\n+      expect(result).toEqual(mockUser);\n+      expect(userService.findById).toHaveBeenCalledWith('123');\n+    });\n+\n+    it('should throw on expired token', async () => {\n+      const mockUser = {\n+        id: '123',\n+        active: true,\n+        name: 'Test User'\n+      };\n+\n+      // Generate token that's already expired\n+      const token = authService.generateToken(mockUser, '-1h');\n+\n+      await expect(authService.validateToken(token))\n+        .rejects\n+        .toThrow(AuthenticationError);\n+    });\n+  });\n+});",
      "status": "added"
    },
    {
      "additions": 15,
      "changes": 20,
      "deletions": 5,
      "filename": "README.md",
      "patch": "@@ -10,8 +10,18 @@ ## Authentication\n-### Basic Authentication\n-To authenticate requests, provide your credentials in the Authorization header:\n-\n-```\n-Authorization: Basic base64(username:password)\n-```\n+### OAuth2 Authentication\n+\n+This API uses OAuth2 for authentication. To authenticate requests, include a Bearer token in the Authorization header:\n+\n+```\n+Authorization: Bearer <your_token>\n+```\n+\n+#### Obtaining a Token\n+\n+1. Redirect users to `/oauth/authorize`\n+2. Users approve the application\n+3. Exchange the authorization code for a token at `/oauth/token`\n+\n+Tokens expire after 1 hour. Use the refresh token to obtain a new access token.\n+\n+For more details, see our [OAuth2 Implementation Guide](./docs/oauth2.md).",
      "status": "modified"
    }
  ],
  "commit_message": "feat: implement OAuth2 authentication",
  "commit_sha": "abc123def456789ghijklmnop",
  "files_changed": [
    "src/services/auth.ts",
    "tests/auth.test.ts",
    "README.md"
  ],
  "owner": "octocat",
  "pull_request": {
    "author": "developer123",
    "body": "This PR adds OAuth2 authentication support and removes the basic auth implementation.\n\nChanges:\n- Replaces basic auth with JWT-based OAuth2\n- Adds comprehensive tests\n- Updates documentation\n- Adds token validation and expiry handling\n\nPlease review the token expiry duration and error messages.",
    "comments": [
      {
        "author": "reviewer456",
        "body": "Could you add some documentation about token refresh flows? Particularly around handling expired refresh tokens.",
        "created_at": "2024-03-21T11:30:00Z",
        "diff_hunk": "@@ -15,7 +15,12 @@ export class AuthService {\n   private async validateToken(token: string) {\n-    // Basic auth validation\n-    const [username, password] = Buffer.from(token, 'base64')\n-      .toString()\n-      .split(':');\n-\n-    return this.validateCredentials(username, password);\n+    try {\n+      const decoded = jwt.verify(token, this.config.jwt_secret) as JwtPayload;",
        "filename": "src/services/auth.ts",
        "id": 444555666,
        "line": 17,
        "node_id": "PRRC_kwDOA3333333333",
        "original_line": 2,
        "original_position": 2,
        "original_start_line": 15,
        "start_line": 17,
        "start_side": "RIGHT",
        "subject_type": "line",
        "url": "https://api.github.com/repos/octocat/Hello-World/pulls/comments/444555666"
      },
      {
        "author": "security-reviewer",
        "body": "We should add a check for token blacklisting here in case we need to revoke access.",
        "created_at": "2024-03-21T12:15:00Z",
        "diff_hunk": "@@ -20,6 +20,11 @@ export class AuthService {\n       if (!decoded.sub || !decoded.exp) {\n         throw new Error('Invalid token structure');\n       }\n+      \n+      if (decoded.exp < Date.now() / 1000) {\n+        throw new Error('Token has expired');\n+      }\n+      \n+      const user = await this.userService.findById(decoded.sub);",
        "filename": "src/services/auth.ts",
        "id": 444555667,
        "line": 25,
        "node_id": "PRRC_kwDOA3333333334",
        "original_line": null,
        "original_position": 5,
        "original_start_line": null,
        "start_line": 25,
        "start_side": "RIGHT",
        "subject_type": "line",
        "url": "https://api.github.com/repos/octocat/Hello-World/pulls/comments/444555667"
      },
      {
        "author": "developer123",
        "body": "Good catch! I'll add the blacklist check in the next commit.",
        "created_at": "2024-03-21T12:30:00Z",
        "diff_hunk": "@@ -25,6 +25,10 @@ export class AuthService {\n       if (decoded.exp < Date.now() / 1000) {\n         throw new Error('Token has expired');\n       }\n+      \n+      const user = await this.userService.findById(decoded.sub);\n+      if (!user) {\n+        throw new Error('User not found');\n+      }",
        "filename": "src/services/auth.ts",
        "id": 444555668,
        "line": 28,
        "node_id": "PRRC_kwDOA3333333335",
        "original_line": null,
        "original_position": 8,
        "original_start_line": null,
        "start_line": 28,
        "start_side": "RIGHT",
        "subject_type": "line",
        "url": "https://api.github.com/repos/octocat/Hello-World/pulls/comments/444555668"
      }
    ],
    "created_at": "2024-03-21T10:00:00Z",
    "id": 987654321,
    "labels": [
      {
        "color": "84b6eb",
        "default": true,
        "description": "New feature or request",
        "id": 111222,
        "name": "enhancement",
        "node_id": "LA_kwDOA1111111111"
      },
      {
        "color": "d93f0b",
        "default": false,
        "description": "Security related changes",
        "id": 333444,
        "name": "security",
        "node_id": "LA_kwDOA2222222222"
      }
    ],
    "node_id": "PR_kwDOA1234567890",
    "number": 123,
    "state": "open",
    "title": "Implement OAuth2 Authentication",
    "url": "https://api.github.com/repos/octocat/Hello-World/pulls/123"
  },
  "ref": "feature/oauth-auth",
  "repo": "Hello-World",
  "review": {
    "author": "senior-dev",
    "body": "I've reviewed the OAuth2 implementation and have some thoughts:\n\n1. The token validation looks solid, good error handling.\n2. The test coverage is comprehensive.\n3. The README updates are clear and helpful.\n\nHowever, I think we should:\n1. Add rate limiting for token refresh\n2. Consider implementing token blacklisting for revoked tokens\n3. Add more logging around token validation failures\n\nOverall, this is a great improvement over basic auth. Please address the points above and we can merge this.",
    "commit_id": "abc123def456789ghijklmnop",
    "created_at": "2024-03-21T14:30:00Z",
    "id": 888999000,
    "node_id": "PRR_kwDOA4444444444",
    "submitted_at": "2024-03-21T15:45:00Z",
    "url": "https://api.github.com/repos/octocat/Hello-World/pulls/123/reviews/888999000"
  }
}