[#13644] Remove GAE Dependency for Cron Jobs#13658
[#13644] Remove GAE Dependency for Cron Jobs#13658DhiraPT merged 16 commits intoTEAMMATES:masterfrom
Conversation
|
Hi @Nsohko, thank you for your interest in contributing to TEAMMATES!
Please address the above before we proceed to review your PR. |
There was a problem hiding this comment.
Pull request overview
Removes Google App Engine–specific cron/queue authentication and replaces it with a shared bearer token so cron schedulers and task queues (e.g., Cloud Scheduler/Cloud Tasks) can invoke /auto/* and /worker/* endpoints outside of GAE.
Changes:
- Introduces
app.cron.and.worker.secretandInternalRequestAuthforAuthorization: Bearer <token>validation. - Updates servlet/auth/filter logic and task-queue clients to use the bearer token instead of GAE headers.
- Adds/updates tests and documentation, including a new external cron setup guide and removal of
cron.yaml.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/teammates/ui/servlets/WebApiServletTest.java | Updates tests to send bearer token instead of GAE queue headers. |
| src/test/java/teammates/ui/servlets/OriginCheckFilterTest.java | Adds a test case asserting bearer-token requests bypass CSRF/origin checks. |
| src/main/resources/build.template.properties | Adds app.cron.and.worker.secret template documentation/value. |
| src/main/resources/build-dev.template.properties | Adds dev template app.cron.and.worker.secret. |
| src/main/java/teammates/ui/webapi/Action.java | Switches “internal request” identification to InternalRequestAuth. |
| src/main/java/teammates/ui/servlets/WebApiServlet.java | Uses InternalRequestAuth to decide 202-vs-error behavior for internal callers. |
| src/main/java/teammates/ui/servlets/RequestTraceFilter.java | Adjusts timeout policy for authenticated worker requests. |
| src/main/java/teammates/ui/servlets/OriginCheckFilter.java | Allows authenticated internal requests to bypass CSRF/origin checks. |
| src/main/java/teammates/logic/external/LocalTaskQueueService.java | Sends bearer token for local task-queue execution. |
| src/main/java/teammates/logic/external/GoogleCloudTasksService.java | Sends bearer token in Cloud Tasks AppEngineHttpRequest headers. |
| src/main/java/teammates/common/util/InternalRequestAuth.java | Adds centralized bearer-token validation helper. |
| src/main/java/teammates/common/util/HttpRequestHelper.java | Avoids logging Authorization header in production. |
| src/main/appengine/cron.yaml | Removes App Engine cron configuration. |
| docs/development.md | Updates config docs to point to external cron setup + bearer secret. |
| docs/design.md | Updates design docs to describe bearer-token-based automated requests. |
| docs/cron-setup.md | Adds external cron schedule/setup guide. |
Comments suppressed due to low confidence (1)
src/main/java/teammates/ui/servlets/WebApiServlet.java:140
throwErrorBasedOnRequesterusesisTrustedCronOrWorkerRequestas a proxy for "Cloud Tasks worker" and returns HTTP 202 to avoid retries. With the new bearer token shared by both cron and worker (and potentially usable on any path), this will also mask real failures for cron endpoints and even non-worker endpoints if called with the token. Tighten the condition to only treat/worker/*requests as non-retriable (e.g., check the request URI prefix in addition to the token).
boolean isRequestFromWorker = InternalRequestAuth.isTrustedCronOrWorkerRequest(req);
if (isRequestFromWorker) {
log.severe(e.getClass().getSimpleName() + " caught by WebApiServlet: " + e.getMessage(), e);
// Response status is not set to 4XX to 5XX to prevent Cloud Tasks retry mechanism because
// if the cause of the exception is improper request URL, no amount of retry is going to help.
// The action will be inaccurately marked as "success", but the severe log can be used
// to trace the origin of the problem.
throwError(resp, HttpStatus.SC_ACCEPTED, e.getMessage());
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
DhiraPT
left a comment
There was a problem hiding this comment.
Thanks for the PR! Please add unit tests for InternalRequestAuth (correct token, wrong token, context path present, /auto/* vs /worker/*), not only static mocking.
…o sai/migrate-cron-from-gae
Hi @DhiraPT , thank u All feedback has been addressed |
DhiraPT
left a comment
There was a problem hiding this comment.
Thanks! A couple of additional edge cases will make this more robust and aligned with auth hardening best practices (tolerate harmless header whitespace, but fail closed on unexpected request state / misconfig, and codify cron-vs-worker scoping). Could you please add the following 5 tests:
-
Bearer token trimming accepted (header tolerance)
Authorization: "Bearer <secret> "should be trusted for a valid/auto/*or/worker/*URI. -
Misconfigured secret with surrounding whitespace should not be silently accepted (fail fast / fail closed)
IfConfig.CRON_AND_WORKER_SECRETcontains leading/trailing whitespace (e.g.," <secret> "), the system should not rely on trimming to “make it work” — assert the desired behavior explicitly (preferably: startup/config validation fails, orisTrustedCronOrWorkerRequestreturns false). -
Root context path behavior
contextPath = ""withrequestUri = "/auto/..."and a correct bearer token should be trusted. -
Null context path should fail closed
contextPath = null(with otherwise valid URI + bearer token) should result inisTrustedCronOrWorkerRequest(req)returning false. -
Cron vs worker separation (supports WebApiServlet’s worker-only behavior)
- trusted cron request:
isTrustedCronOrWorkerRequest(req) == trueandisWorkerRequestPath(req) == false - trusted worker request:
isTrustedCronOrWorkerRequest(req) == trueandisWorkerRequestPath(req) == true
- trusted cron request:
samuelfangjw
left a comment
There was a problem hiding this comment.
Sorry I haven't had time to properly look at this. I did a quick scan and have some comments.
@Nsohko @DhiraPT please look at comments and fix accordingly. Also appreciate if I could be pinged for approval for more significant PRs like this going forward, especially if they are security related or involve high impact changes like these. Thanks!
| if (isTrustedWorkerRequest) { | ||
| log.severe(e.getClass().getSimpleName() + " caught by WebApiServlet: " + e.getMessage(), e); | ||
|
|
||
| // Response status is not set to 4XX to 5XX to prevent Cloud Tasks retry mechanism because | ||
| // if the cause of the exception is improper request URL, no amount of retry is going to help. | ||
| // The action will be inaccurately marked as "success", but the severe log can be used | ||
| // to trace the origin of the problem. | ||
| // For Cloud Tasks worker requests only: response status is not set to 4XX/5XX so the task is not | ||
| // retried when the failure is due to an improper request URL (retries would not help). | ||
| // Cron (/auto/*) requests are not included so schedulers still see real HTTP error status on failure. | ||
| // The action may be inaccurately marked as "success"; the severe log traces the problem. | ||
| throwError(resp, HttpStatus.SC_ACCEPTED, e.getMessage()); |
There was a problem hiding this comment.
This should be removed. Retry policy should be configured on external service instead.
Co-authored-by: Dhira Tengara <dhira.pt@gmail.com>
Co-authored-by: Dhira Tengara <dhira.pt@gmail.com>
Fixes #13644
Remove dependence on GAE for Cron Job and Worker auth.
Now uses a bearer token instead