Repository navigation
App/idrs creds to executor - #128
Merged
Merged
Conversation
Runway will select how student identities are resolved per partner, and the executor needs a value that can't change mid-flight. A partner-level setting alone would let an edit between job creation and a re-run silently change how an existing job behaves, so the job carries a snapshot taken at creation. The column defaults are migration safety only — existing rows stay id_based without a backfill. Job creation will write the snapshot explicitly. idMatchingMode joins GetJobDto's intentionally-not-exposed block: the type requires every Job field, but there is no matching-mode UI in this rollout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The executor needs a partner-scoped IDRS bearer token, but the job payload is fetched once at container start and can outlive any token we could put in it — and a token sitting in that payload is one more copy of a credential in flight. So the payload carries only a callback URL, and the executor exchanges its run-scoped bearer token for credentials immediately before it needs them. That run-scoped token is the whole trust boundary: it names the run, which names the partner whose connection info may be returned. The callback adds no matching-mode, run-status, or one-call restriction on top — background fuzzy work legitimately runs after the authoritative run reports done, and an id_based executor calling the unadvertised URL just gets a 500 and carries on. Token minting is bounded so it fits inside the executor's 20s request timeout: 5s for the secret lookup (SDK retries included, via a per-request abort signal so no other secret getter inherits the bound) and 5s per OAuth attempt with one jittered sub-second retry for transient failures only. Tokens are cached per partner per app instance, with concurrent misses coalesced onto one lookup so a cold start doesn't stampede Secrets Manager. The ten-minute buffer is a reuse threshold, not a minimum lifetime — a token too short to clear it is still returned, just not cached, since caching it would only make the next caller mint another one. Errors collapse to three broad responses; diagnosis comes from the structured logs, which carry cause categories and safe upstream metadata but never credentials, tokens, OAuth bodies, or the callback response. Pure fuzzy does no roster matching, so it gets neither appUrls.roster nor rosterFilePath. Background mode still runs the authoritative ID-based pass and keeps both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Integration tests cover what the executor actually sees: the mode on the payload, whether the callback URL is advertised, which roster sources survive each mode, that a run follows its job's snapshot after the partner setting moves, and that the callback resolves the right partner, stays open after done, and maps failures to the three-way contract. The callback tests stub only the AWS and OAuth boundaries so routing, auth, partner resolution and error mapping all run for real. Unit tests carry the parts integration can't reach: the exact token request, the cache reuse window, the short-lived and missing-expiration branches, per-partner isolation, miss coalescing and its failure cleanup, and which OAuth failures retry. Both layers assert the logs don't leak credentials or tokens, using distinctive values so a leak can't hide in ordinary log words. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done |
Contributor
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![amazon-inspector-ohio[bot]](https://avatars.githubusercontent.com/u/214039647?s=60&v=4){kind=small}
|
✅ I finished the code review, and left comments with the issues I found. |
typeof null === 'object', so a secret or token body of JSON null reached the destructuring and threw a raw TypeError. IdentityServiceTokenService only translates its own typed errors, so that surfaced as an unclassified 500 with no cause-category log — the two things the error contract exists to prevent. Arrays slipped through the same check. Reject both at each boundary. Secret field values are validated as non-empty strings rather than merely truthy. Secrets Manager content is untrusted at runtime whatever the type says, and a numeric clientID would be coerced by URLSearchParams and come back as an OAuth rejection — pointing diagnosis at the authorization server for what is actually a provisioning error. Tests cover both null bodies (verified red against the previous code), arrays, and wrongly-typed secret fields, plus the two latency bounds the callback depends on: a five-second abort signal per OAuth attempt and a retry backoff capped at one second under maximum jitter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A reader comparing IDRS_OAUTH_TOKEN_URL against OAUTH2_ISSUER or UM_CONFIG_SECRET would reasonably conclude the CloudFormation wiring was simply forgotten, since cloudformation/ lives in this repo. It wasn't — the cloud engineering team owns that stack and provisions both the token endpoint and the per-partner secrets out of band. Say so where the rollout steps are described, so the next person doesn't "fix" it by adding a stack parameter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The error handling had outgrown what anyone acts on. Two cause unions, two AWS error-name allowlists and a translation layer between services existed to pick between 500, 502 and 503 — but the executor only distinguishes 200 from non-200, so none of it changed a decision. Collapse to one response and one log line. The AWS error name we were bucketing is more informative raw than mapped, and the allowlists were guesses about SDK internals that would have silently mis-bucketed anything unanticipated. getIdrsConnectionInfo now follows getEduConnectionInfo right above it: null for missing or malformed config, real AWS failures thrown. That convention has run in prod for two years. The OAuth retry is gone. One request, bounded at 5s. A transient blip now fails the run, which is a fine thing to learn about from a real failure rather than pre-solve — and it takes the worst-case dependency budget from ~16s to ~10s inside the executor's 20s timeout, so the jitter arithmetic goes away too. In-flight coalescing is gone. One callback per IDRS phase backed by a 24-hour token means the concurrency it guarded against is rare and harmless when it happens: two tokens instead of one. Token-lifetime handling is one rule — cache it if expires_in leaves more than the buffer, otherwise hand it back uncached with a warning — replacing separate missing/short/zero/negative/non-numeric branches. An odd expires_in no longer fails a run over a token that works. Tests follow the requirements down: 67 → 39 unit, and the integration matrix collapses to one parameterized case per distinct upstream failure. What's left asserts observable behavior. Net: app-config +201 → +94 lines against development, token service 260 → 145. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two narrow holes from review, both about credential material rather than correctness. IDRS_OAUTH_TOKEN_URL was returned unvalidated and handed to fetch with the client secret in the body, so a misconfigured http endpoint would transmit the credential in plaintext. That is worse than a failed job, and it is the one IDRS URL that had no scheme check — the per-partner base url already had one. Refuse non-https outside local development, in the getter, alongside the existing check. The not-cacheable warning interpolated the raw expires_in, which is whatever the OAuth server put in the response body. Log the lifetime we derived instead: 0 already distinguishes unusable from short-but-valid, so nothing diagnostic is lost. The partner-isolation test started A and B concurrently against an empty cache, so both read it before either populated it — returning any cached entry to any partner would have passed. Sequential A/B/A/B over the reuse path fails that mutation at partner B. Refresh is driven by controlled time rather than reading the private cache, fetch is a restorable spy instead of a direct assignment, the timeout tests assert the signal reaches the fetch/SDK boundary rather than only that it was constructed, and the snapshot test now changes the partner setting to something it wasn't already. Also: drop the redundant database resets the shared beforeEach already does, collapse one duplicate integration failure case that the unit layer covers, and trim comments that retold history or narrated obvious checks. AGENTS.md now makes an EDFIAL-481 executor a hard prerequisite for enabling either fuzzy mode, not just an ordering preference. An older executor ignores both new payload fields, and pure fuzzy omits the roster sources it reads unconditionally. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The logging test claimed to prove a foreign upstream message stays out of the log, but requestToken already replaces a transport error's message with one of ours before it can reach any logger, and the test only captured this service's logger anyway. The assertion could not fail. Review caught it. Split it in two. The logging test keeps the assertions that can fail — the token and the raw expires_in — and says in its name and a comment that its scope is this service's logger, with the callback boundary covered in the integration spec. The stripping itself moves to the network-error case, asserted where it is enforced: our message, the error name as upstream, and the transport message absent from the error. Inverting that behavior now fails the test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Merged
The executor uses the url from the identity-service callback as-is, so the app has to hand over the route it will actually call rather than the base recorded in the partner secret. The base is still stored and sent verbatim as the OAuth audience, which has to match what IDRS was configured with — the route is only appended at the callback boundary, where the run's partner and tenant are both in hand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Keep shared failure coverage at the integration boundary and fold warning redaction into the existing lifetime cases. Preserve no-retry assertions without repeating the unit setup. No production behavior or nearby documentation changes are needed.
Mode and run-status restrictions would reflect a deliberate scope change. Remove the two tests that only preserve their absence; production behavior and its documentation are unchanged.
The creation tests already verify that the partner mode is snapshotted. Retroactively changing existing jobs would require an explicit product decision, so do not preserve its absence with another integration case.
Remove tests that pin deliberate non-behavior, duplicate coverage at another boundary, or repeat equivalent invalid-input cases. Keep coverage for auth, roster modes, caching, timeouts, secret rotation, malformed responses, and credential redaction.
Define the per-partner secret with the same clientId spelling used by the application. This removes an unnecessary boundary mapping from clientID.
The afterEach reset was dead: per-test-file-setup runs refreshSeed() in a global beforeEach, which deletes and recreates the partner row from the fixture, so the mutation never outlived the test. The sibling cross-year test at the bottom of this describe already relies on that. Asserting the seeded id_based value up front says the same thing the reset was trying to say, and says it where it matters — it is what makes the later fuzzy assertion evidence that creation copied the setting. With the reset gone the wrapping describe held a single test, so it folds into the parent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
usesIdBasedMatching describes what is being tested — the job runs the authoritative ID-based pass — rather than one of the two things that follow from it, and reads correctly for id_based_fuzzy_background, which does run that pass. Trims the comments alongside. The dropped detail about the callback having no mode check lives in the controller and in AGENTS.md, where it belongs; repeating it here was the third copy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The spy asserted that building a payload doesn't call getCredentials, which nothing in the payload path could do — the callback URL is a string. The res.body.token check was the same idea from the other side, and the response DTO has no token field to expose. What the test is for is that the two fuzzy modes advertise the callback, which the remaining assertions cover. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The field existed so the controller could append a sanitized fragment to its log line, which meant the controller had to know the error type to read a value it only concatenated into a string. Nothing consumed it programmatically. Sanitizing at construction is what actually made it safe, and that has not moved: the transport error contributes its name, the rejected request its status, and neither carries foreign text. Saying so in the message puts it where every other reason already lives, and the controller goes back to logging any error the same way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Once the controller stopped reading the type, IdentityServiceTokenError was a marker with no production consumer — only the tests distinguished it, and asserting the message says more than asserting the class. The failure tests now name the reason they expect, so a throw that moves to a different branch fails them instead of passing on a shared type. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The field said tokens and the method handed back credentials — a token plus the partner's base url, which the return type already called IdentityServiceCredentials. The class was named after one of the two things it produces, and the field pluralized that into a collection. Renamed to IdrsCredentialsService, matching the IDRS_* wording the config layer already uses, and injected as `idrs` so the call site reads as what it does: get credentials for this partner. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three readability bumps, no behavior change. The expected url repeated the partner id because the stubbed IDRS base embeds it; naming that base separately makes the two occurrences distinct things, and searchUrl now takes the tenant fixture rather than two fields pulled off it. `arrange` came from the AAA vocabulary and said nothing about what the case does — it induces the failure the row is named for. The cross-run token test was really a cross-partner one: tenantX sits under a different partner, which is the leak the run-scoped token exists to prevent. Title says so, and it now asserts we never reached for the other partner's connection info. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Putting the partner id in the host rather than a path segment leaves the one in the search route reading as the separate thing it is, so the expected url no longer looks like it says the same thing twice. Name the parameterized row's setup for the mock it installs. induceFailure described the intent and left the reader to work out the mechanics, which is the wrong way round in an it.each — the rows differ only by what they do to getIdrsConnectionInfo. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The constant was referenced once, and its test asserted it equalled the literal it was named for. The bound is clearer stated where it applies, with the reason beside it. Also notes the local-development fallback in the getter's doc comment, and has the missing-local-values test assert we never reached for a secret. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four negated predicates joined by or is a hard way to say "all three are non-empty strings and the url is https". Asserting that directly and returning from inside the guard leaves the malformed case falling through to the warning, which is where it belongs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
edandylytics
commented
Sep 18, 2026
Collaborator
Author
There was a problem hiding this comment.
This is a new unit test file for app config services. The logic in config can be hard to check with integrated tests, so I like using unit tests here. Just tests what this PR touches, not the existing methods.
Collaborator
There was a problem hiding this comment.
One thing I'm not seeing a test for here that does exist in the idrs-creds service spec is a test explicitly checking the secret value isn't logged. Should there bee one?
Both asserted that AbortSignal.timeout was called with 5000 and that the resulting signal was passed along. Neither could show an abort cancelling anything, because the thing that would be cancelled is mocked — whether a signal actually stops an in-flight request belongs to the SDK and to fetch. That leaves OAUTH_TIMEOUT_MS used once, so it is inlined beside the call like the secret-lookup bound. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The shape reads as an odd choice next to every other request in the app, which sends JSON. It is what RFC 6749 requires of a token endpoint, so cite it rather than leave the next reader to wonder. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
"Enters the reuse window" reads as the opposite of what happens: a reuse window would be when the token is reused, not when it is replaced. The condition is a comparison, so state it — the token is served while more life remains than the buffer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The case read as though it checked that a non-JSON body throws, which is
fetch's behavior and not something a mocked fetch can show. What is ours
is replacing that error: a real SyntaxError quotes the text it choked on,
so without the catch a piece of the token endpoint's response body ends up
in our logs. The mock's message now looks like one and the test asserts it
does not survive.
The sibling network-error case made the same claim through
JSON.stringify(err), which is "{}" for an Error — message and stack are not
enumerable, so it would have passed while carrying the sentinel. String(err)
actually reads the message.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There is one IDRS service per deployment and each partner is addressed
within it by request path — that is why the search route carries
/partners/{partnerId} at all. So the base url does not vary per partner,
and keeping it in {ENVLABEL}-idrs-connection-info-{partnerId} meant
provisioning N copies of one value, each an opportunity to typo a
partner into a failure that only shows up when they first run a fuzzy
job.
The split was also backwards against the rollout we expect: the token
endpoint, the one url we already anticipate varying (a future TX-specific
endpoint), was deployment-level, while the base url, with no identified
reason to vary, was per-partner.
IDRS_URL now applies in every environment rather than standing in for
the secret only in local development, behind an idrsUrl() getter that
mirrors idrsOauthTokenUrl(): lazy, null when unset, https required
outside dev. It is still the OAuth audience and is still returned
verbatim. The secret holds { clientId, clientSecret } and nothing else,
so cloud engineering provisions two fields per partner.
The token cache drops its url field as a consequence. It only earned a
place there when the url arrived alongside rotating per-partner
credentials; a single env-var read cannot go stale against itself.
The per-partner IDRS host in the integration test was what proved the
callback minted a token for the right partner — wrong credentials would
have surfaced in the returned url. One shared base removes that
evidence, and the path segments come from the run lookup rather than the
credential path, so they do not substitute for it. The success case now
asserts the OAuth request carries that partner's client_id, scope and
the configured audience.
Nothing is provisioned yet, so this costs no out-of-band rework.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rtavernaea
reviewed
Sep 18, 2026
rtavernaea
left a comment
rtavernaea
left a comment
Collaborator
There was a problem hiding this comment.
Got about halfway through review, only had one comment so far but wanted to submit that now and can finish up review Monday
| runStatus?: RunStatus; | ||
| // Snapshotted onto the job at creation in real life; set it directly here | ||
| // so a test can exercise a fuzzy mode without changing global fixtures. | ||
| idMatchingMode?: IdMatchingMode; |
Collaborator
There was a problem hiding this comment.
Does this need ? here? Isn't it always guaranteed?
Collaborator
Author
There was a problem hiding this comment.
That ? allows callers of seedJob to omit it idMatchingMode, and then it'll just default to id_based on line 31 below. So the job object itself will always have idMatchingMode set -- callers only need to be explicit about it if they want to be.
rtavernaea
reviewed
Sep 21, 2026
rtavernaea
left a comment
rtavernaea
left a comment
Collaborator
There was a problem hiding this comment.
Just a couple small questions
| }); | ||
| } catch (err) { | ||
| if (err instanceof Error && err.name === 'ResourceNotFoundException') { | ||
| return null; |
Collaborator
There was a problem hiding this comment.
do we care to log anything here?
Collaborator
Author
There was a problem hiding this comment.
We don't log here, but the route handler does log when creds are requested but not present, which I think is a good spot for it. In general, I'd like to have the app config service just return config or null, as appropriate (logging for other errors) and then the callers can decide whether missing config is worth raising a flag about.
| it.each([ | ||
| ['null', 'null'], | ||
| ['a numeric clientId', JSON.stringify({ clientId: 1, clientSecret: 's' })], | ||
| ['missing a clientSecret', JSON.stringify({ clientId: 'a' })], |
Collaborator
There was a problem hiding this comment.
Could add a test for an invalid JSON value
Collaborator
There was a problem hiding this comment.
One thing I'm not seeing a test for here that does exist in the idrs-creds service spec is a test explicitly checking the secret value isn't logged. Should there bee one?
Two gaps Rachel found in the config spec. A secret body that isn't JSON takes a path the existing matrix misses: fetchAWSSecret treats it as a plain-text secret and returns the raw string, so there are no fields to read. And the malformed branch is the only one that logs, which makes it the one place a future edit would be tempted to quote the body to explain what was wrong -- so pin that only the secret name appears. Capturing logger.warn instead of discarding it is what lets the second test see the line at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The ResourceNotFoundException catch returns before the "missing or malformed" warn, so that is never the line an id_based executor sees when it calls the unadvertised callback. It gets the error the credentials service throws instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rtavernaea
approved these changes
Sep 21, 2026
rtavernaea
left a comment
rtavernaea
left a comment
Collaborator
There was a problem hiding this comment.
changes look good & are covered by tests
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR addresses EDFIAL-602: the app side of handing IDRS credentials to the executor.
Design
id_matching_modecolumn with these values:id_based- what we've used up until nowfuzzy- new fuzzy matchingid_based_fuzzy_background- do ID-based matching first, then process again using fuzzy matching "in the background" which means processing it again as a fuzzy matching job, after the first pass has completed, and without sending the output anywhere. This allows us to exercise the new code and gain confidence that it works.appUrls.identityServiceendpoint that allows the Executor to request connection info to the partner's identity service.fuzzyandid_based_fuzzy_backgroundmodes, which indicate (a) there is an IDRS for this partner and (b) it's ready for usecrossYearMatchAvailablechanges meaning. For ID-based processing, it continues to mean that the executor should query the app for a cross-year roster to do ID-based matching. For fuzzy modes, it means that the Executor should accept matches from school years other than the API year because the partner is set up to sideload those matches.Rollout
We'll need to set:
IDRS_OAUTH_TOKEN_URL-- this is for the IDRS's auth server (where to request IDRS tokensEach partner will need a secret with creds for the IDRS auth server, with the secret named:
{ENVLABEL}-idrs-connection-info-{partnerId}The PR is safe to merge before Executor changes and before any of this config ☝️ is done.
Review suggestions
There are a number of moving parts in this PR. Here's one way to chunk up review into manageable portions:
earthbeamInputForRuninEarthbeamApiServiceconstructs the input correctly. Tests describe what's expected.Testing
I've tested locally with #131 . You can set up the env vars listed here to point to the dev IDRS server. You'll need to be in an ea/ea tenant for the dev IDRS to accept the request. Depending on where Executor/bundle dev is at, you might need to have an AI shim certain functionality (e.g. updating the observables output by the bundle to match the shape the IDRS API expects)