docs: B2B learner records spec and provider-access decision - #55
Open
blarghmatey wants to merge 6 commits into
Open
docs: B2B learner records spec and provider-access decision#55blarghmatey wants to merge 6 commits into
blarghmatey wants to merge 6 commits into
Conversation
The learner records design had only ever existed as untracked files, so nobody could review it or link to it. This commits the one-pager, the data-backing/tenancy doc and the draft OpenAPI contract as they stood, plus a proposal for the blocking open question: who authorizes a training provider to read an organization's learner data. The proposal separates the provider's credential (a Keycloak client, Pulumi-owned per the B2B onboarding substrate/tenant split) from the per-organization grant (a mitxonline record). The organization authorizes, MIT records and enforces it, and phase 1 lets the organization see and revoke grants before self-service authorization exists. It is marked pending sign-off. Nothing is implemented. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBTqmr3ki2pAAP1EeWH7wg
Access to the M2M API is settled when the contract is signed, not mediated by mitxonline at request time, and the partner handles per-user authorization in its own LMS. That replaces the grant-record proposal: the per-contract Keycloak client is the whole record, with its organizations as a hardcoded claim and identity as the read-pii scope. The OpenAPI file was wrapped in markdown code fences, so it was not parseable YAML as committed. Stripping them makes it pass check-yaml and openapi-spec-validator. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBTqmr3ki2pAAP1EeWH7wg
blarghmatey
changed the title
blarghmatey
marked this pull request as ready for review
September 10, 2026 19:09
…gn facts contract_id is mitxonline's ContractPage id, exposed by dim_contract as b2b_contract_id: an integer. The spec typed it as a string with a UUID example, the same mismatch #39 fixed in b2b_dashboard after production rejected input_value=695. A partner generating a client from this contract would have expected UUID strings. Also corrects three stale statements in the design doc: §4 still titled the provider-authorization question "Open" above its decision, the b2b_analytics MV count is eight rather than six, and the roster's manager column is userorganization_is_manager. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBTqmr3ki2pAAP1EeWH7wg
dim_contract.contract_id and the mitxonline source column b2b_contractpage.page_ptr_id are both BIGINT in the production warehouse, so the contract states the width a partner's client should use rather than leaving it to generator defaults. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBTqmr3ki2pAAP1EeWH7wg
There was a problem hiding this comment.
🟡 Changes recommended
The export design can bypass PII scope restrictions, and several contract semantics remain inconsistent or ambiguous.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Documents the proposed B2B learner-records API and its provider-authorization model.
Changes:
- Adds the draft OpenAPI contract and data design.
- Defines consent, tenancy, and export behavior.
- Records contract-based Keycloak client authorization.
File summaries
| File | Description |
|---|---|
docs/openapi/b2b-learner-records-v1.yaml |
Defines the draft API contract. |
docs/b2b-learner-records-provider-authorization.md |
Records the provider-access decision. |
docs/b2b-learner-records-onepager.md |
Summarizes scope and open questions. |
docs/b2b-learner-records-design.md |
Describes data backing and tenancy. |
Review details
Suppressed comments (1)
docs/openapi/b2b-learner-records-v1.yaml:549
- The prose promises that PII fields return
nullwithout the PII scope and that every field afteroutcomes_sharedis nulled when outcomes are withheld, but therequiredlist permits all of those fields to be omitted. Require the stable record fields and use null only as the documented unavailable/withheld value.
required:
- Files reviewed: 4/4 changed files
- Comments generated: 9
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Drops the read/read-pii scope split. The organization already holds its learners' identity because they are its employees or students, so redacting it protects nothing. A single per-organization export could not have honoured the split anyway. Records and exports always carry identity, and one scope remains. Pins down contract semantics the review found ambiguous: - Enrollment grain includes the contract, matching the backing model. No org/run pair in production links to more than one contract today (0 of 2468), so this adds no duplicates. - Every list endpoint has a unique order, so offset paging is stable. - /learners?contract_id= recomputes rollups from that contract's enrollments only. - Fixed-shape nullable fields are required, so omission is not a second way to say null. - The x-data-readiness claim is limited to the record schemas, and the security scheme says "per contracted integration", matching the decision doc. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBTqmr3ki2pAAP1EeWH7wg
… scope The organization can see which learners declined by diffing its roster, and no response shape changes that. How it may use that knowledge is a contract term owned by legal and contracting, and issuing a client presumes those terms exist, so the spec drops the options list and keeps the per-learner record shape. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014zeAAspVVGJDPn944Gt9vz
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.
What are the relevant tickets?
N/A
Description (What does it do?)
Commits the B2B learner records spec, which until now existed only as untracked local files, and records the decision on its blocking open question: who authorizes a training provider to read an organization's learner data.
b2b-learner-records-onepager.md: high-level design and open questions.b2b-learner-records-design.md: data backing, tenancy rationale, scope, consent.openapi/b2b-learner-records-v1.yaml: draft OpenAPI 3.1 contract.b2b-learner-records-provider-authorization.md(new): the access decision.HardcodedClaimProtocolMapper) and the singlelearner-records:readscope.email/full_name, and the draft'sread-piiscope is dropped.Changes to the draft OpenAPI:
contract_idisinteger(int64) everywhere. It's mitxonline's ContractPage id (dim_contract.b2b_contract_id, BIGINT in the production warehouse), the same identifier fix(b2b_dashboard): type contract_id as the integer it actually is #39 retyped inb2b_dashboard.(learner, contract, course run), matching the backing model. No organization/course-run pair in production links to more than one contract today (0 of 2468)./learners?contract_id=recomputes rollups within that contract.required.How can this be tested?
Docs only.
openapi-spec-validator docs/openapi/b2b-learner-records-v1.yamlreports OK, and the repo'scheck-yamlhook passes. The code references in the decision doc were grepped against current ol-infrastructure and ol-analytics-api. The design doc's warehouse claims and the contract figures above were checked against the ol-data-platform dbt models and the production warehouse through local DuckDB.HardcodedClaimProtocolMapperwas confirmed present in pulumi-keycloak 6.13.0, within ol-infrastructure's>=6.0.0,<7pin.Additional Context
Three things in the decision doc to verify on QA before building: which claim carries the client ID in a Keycloak 26.7 client-credentials token, whether APISIX passes the hardcoded claim and scope through
X-Userinfoon a bearer-only route, and the access-token lifespan (the revocation window).🤖 Generated with Claude Code
https://claude.ai/code/session_01EBTqmr3ki2pAAP1EeWH7wg