Feature/6 GitHub auth and rate limiting#29
Conversation
…b-client - Setup GitHubIntegrationTest class with SpringBoot context configuration. - Implement RedisTemplate mocking using MockitoBean for environment-independent testing. - Configure TestGitHubConfig to enable context loading in sub-module environment.
…b-client - Setup GitHubIntegrationTest class with SpringBoot context configuration. - Implement RedisTemplate mocking using MockitoBean for environment-independent testing. - Configure TestGitHubConfig to enable context loading in sub-module environment.
martian56
left a comment
There was a problem hiding this comment.
Thanks for picking up #6, and the overall shape is right: splitting it into TokenManager (App JWT plus installation token), RateLimiterService, and a GitHubClient facade matches what the issue asked for, and caching tokens in Redis is the correct instinct. But this is the auth core of a security product, and there are several blocking issues. CI being green is misleading here, the tests do not exercise any of the real paths, more on that at the end. I read all of it; grouping by severity.
BLOCKING, correctness:
-
Missing WebClient bean, this will not start. TokenManager injects a WebClient (constructor), but nothing in main defines one, only the test config (TestGitHubConfig) provides a WebClient bean. Boot auto-configures WebClient.Builder, not WebClient. Since these are @service classes and github-client is a dependency of both api and worker, booting either app will fail at context startup with no qualifying bean of type WebClient. That is also why the tests pass and production would not. You need a real @configuration in main that builds a WebClient with baseUrl https://api.github.com, plus the Redis config. Boot the api against the compose stack and you will see the failure.
-
Hardcoded JWT issuer 'YOUR_GITHUB_APP_ID', inline. The App ID is a placeholder, so every JWT GitHub receives is invalid and no token will ever mint. The App ID and the private key location must come from config (application.yml plus env), not literals.
-
Rate limiter dead-locks an installation, inline. updateLimit writes rate_limit: with no TTL, and checkLimit throws as soon as it reads <= 0. Once an installation hits zero it is bricked permanently: checkLimit throws before any call can run, and the only thing that resets the counter is updateLimit, which only runs after a successful call that can never happen. GitHub resets on a window, so this key needs a TTL derived from X-RateLimit-Reset, and the value should be seeded from GitHub's headers, not left to rot at zero.
-
X-RateLimit-Remaining .get(0) is unguarded, inline. Any response without that header (errors, 304s, some endpoints) throws IndexOutOfBounds and takes down the whole call. Guard for missing or empty.
-
Token TTL hardcoded to 3600s and expires_at ignored. You parse expiresAt into the DTO but never use it, and cache for a flat hour. If GitHub's expiry drifts from your fixed hour you will serve a dead token. Derive the TTL from expires_at minus a safety margin (say 60s).
BLOCKING, security (we hold ourselves to the bar we sell):
- Private key loaded from the classpath, inline. Reading private-key.pem off the classpath means the GitHub App private key would live in the repo or be baked into the jar. That key can mint tokens for every installation; it must come from an external secret (env var or mounted file or a secrets manager), never the classpath. Good news, no key is committed today, I checked, so fix the design before one ever is. Also getResource(...) returns null when absent, so this NPEs rather than failing clearly.
QUALITY:
- WebClient.exchange() is deprecated and leak-prone (unconsumed bodies hold connections). Use retrieve() or exchangeToMono.
- Non-English strings in the code. The error messages and several comments are in Azerbaijani. The codebase is English, and these will surface in logs and stack traces; please translate them.
- Raw RuntimeException everywhere. libs/common has an errors home, use typed exceptions so callers can distinguish an auth failure from a rate-limit stop.
- generateJwt reads and parses the .pem on every mint. Parse the PrivateKey once and reuse it.
- build.gradle pulls in both spring-boot-starter-web (servlet) and webflux. For a client lib you only need webflux, which gives you WebClient. Dropping starter-web keeps the servlet stack off consumers. And since every call ends in .block(), consider whether a plain RestClient would be simpler than reactive WebClient used synchronously.
TESTS, why green CI does not mean working:
- TokenManagerTest.getInstallationTokenCallsMintNewTokenWhenNotInCache sets up mocks and asserts nothing, inline, it never calls the method. The entire mint path (JWT, HTTP, caching) is untested.
- GitHubIntegrationTest.testFullFlow only asserts the bean is non-null, inline. It is a context-load check mislabeled as a full flow, and it only loads because the test config supplies the WebClient that prod is missing (see #1).
- RateLimiterServiceTest covers only the throw-at-zero case, not the deadlock, updateLimit, or the happy path.
This needs a real revision pass rather than small patches, so I would flip it back to draft while you work. Happy to pair on the config wiring (#1) and the rate-limit reset (#3) if useful, those two are the trickiest. The bones are good.
|
|
||
| @Test | ||
| void testFullFlow() { | ||
| assertNotNull(gitHubClient); |
There was a problem hiding this comment.
Named testFullFlow but it only checks the bean is non-null, and it only passes because TestGitHubConfig supplies the WebClient that prod lacks. A real flow test would stub the GitHub responses (MockWebServer or a WebClient exchange function) and assert get() sends the bearer token, parses the body, and updates the rate limit from the response headers.
What does this change?
This change implements the necessary dependencies and infrastructure to support the "feature/6-github-auth-and-rate-limiting" task.
jjwt-api,jjwt-impl, andjjwt-jacksonto thegithub-clientlibrary to support GitHub authentication requirements.spring-boot-starter-webfluxandspring-boot-starter-data-redisto facilitate the requested authentication and rate-limiting logic.build.gradle.ktsstructure to comply with thejava-libraryplugin, ensuring the module is correctly configured for the project.Related issue
Closes # 6
Area
Screenshots
If this touches the UI, drop before and after screenshots here.
Checklist
bun run typecheck && bun run buildinapps/web(for frontend changes)