Support service-authenticated Experience CS project and asset syncs - #973
Conversation
Test coverage93.41% line coverage reported by SimpleCov. |
abcampo-iry
changed the title
There was a problem hiding this comment.
Pull request overview
Adds a service-authenticated pathway for Experience CS to sync public Scratch projects and upload global Scratch assets, integrating with existing Rails API auth/authorization patterns while keeping user- and school-owned resources restricted.
Changes:
- Introduces
EXPERIENCE_CS_API_KEYconfig +X-Experience-CS-API-Keyheader authentication viaExperienceCsServiceAuthenticator. - Restricts project create/update for the service identity to public Scratch project types, and adds a dedicated global Scratch asset upload endpoint.
- Adds/extends request and feature specs plus updates docs and
.env.example.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
app/services/experience_cs_service_authenticator.rb |
Adds service-key authenticator that returns an Experience CS admin service user. |
app/controllers/concerns/identifiable.rb |
Adds a loader to set current_user from the Experience CS service header. |
app/controllers/api/projects_controller.rb |
Allows service-authenticated create/update while enforcing “public Scratch only” constraints for the service identity. |
app/controllers/api/scratch/assets_controller.rb |
Adds create_global to upload/repair idempotent global assets with checksum conflict detection. |
config/routes.rb |
Adds route for global Scratch asset upload endpoint. |
config/application.rb |
Wires EXPERIENCE_CS_API_KEY into config.x.experience_cs.service_api_key. |
README.md |
Documents Experience CS sync behavior and configuration/header usage. |
.env.example |
Adds EXPERIENCE_CS_API_KEY example value and comment. |
spec/services/experience_cs_service_authenticator_spec.rb |
Adds unit coverage for service authentication success/failure cases. |
spec/requests/projects/update_spec.rb |
Adds request specs for service-authenticated update constraints. |
spec/features/project/creating_a_project_spec.rb |
Adds feature coverage for service-authenticated project creation constraints. |
spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb |
Adds feature coverage for global asset upload behavior (idempotent/conflict/repair/access). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| def global_file_matches?(scratch_asset) | ||
| scratch_asset.file.blob.checksum == request_body_checksum | ||
| end |
There was a problem hiding this comment.
What's the reasoning behind needing to hash the assets?
The asset name should already be the hash of the asset so I'm wondering if it's simpler to use that and skip saving it if the asset already exists.
Since global assets can only be created by trusted systems I think the risk is low (but I might be missing something?)
There was a problem hiding this comment.
For me the key is to compare checksums to avoid silently using the wrong asset if different content is uploaded with the same filename. It's quite inexpensive that check but you are right risk is low
There was a problem hiding this comment.
If it's reliable that ok - a reason to remove it would be if we didn't think it could hash files consistently because some part of them changed even though Scratch think it's the same file.
| def experience_cs_service_project? | ||
| return public_scratch_project_attributes?(experience_cs_service_project_attributes) if action_name == 'create' | ||
|
|
||
| public_scratch_project_attributes?(@project.attributes.symbolize_keys) && | ||
| public_scratch_project_attributes?(experience_cs_service_project_attributes) | ||
| end |
There was a problem hiding this comment.
I found this harder to follow what this was doing, I think it might be due to the naming of the public_scratch_project_attributes?
Could some of this be pushed into another object or the project model? Maybe a .is_public_experience_cs_project? or similar? I'm not sure if it it would work well as validation.
There was a problem hiding this comment.
I moved this logic to Project.public_experience_cs_project?
I don’t think it should be a validation because the project can be valid in other contexts; this rule only limits what Experience CS can sync. Maybe if later we decide to be more global we can move it
| private | ||
|
|
||
| def authorize_experience_cs_service_project | ||
| return unless current_user&.id == ExperienceCsServiceAuthenticator::USER_ID |
There was a problem hiding this comment.
This could be pushed onto the user to remove it from having to check the specific USER id - user.experience_cs_service_account?
There was a problem hiding this comment.
I added User.experience_cs_service_account?
| raise CanCan::AccessDenied | ||
| end | ||
|
|
||
| def experience_cs_service_project? |
There was a problem hiding this comment.
Small naming thing - is the service part important here - might be easier to read if it was just experience_cs_project? unless it's specifically about the service account?
There was a problem hiding this comment.
It applies specifically to Experience CS syncs,
renamed it to experience_cs_service_project_change_permitted?
we can use cs_syncs if you think is more clear
|
I like the approach, and I think creating the 'experience cs user' is a good way to make it work with the existing controllers. I've added suggestions for improvements - let me know if you want to talk through any and I'll re-review after any changes since I want to be able to check the authorization logic again before mergin. |
| authorize! :create, @project_from_header | ||
| raise CanCan::AccessDenied unless experience_cs_public_project? |
There was a problem hiding this comment.
Is this check useful? If we're not saving a asset against a project, what's the reason in authorizing the project and checking that it's an experience CS one?
There was a problem hiding this comment.
Good point. We should authorize the global asset upload, not project creation. We still check the project to ensure the asset is being uploaded for a valid Experience CS project.
There was a problem hiding this comment.
We should authorize the global asset upload
Agreed, I think it's allowed if the user is the experience CS admin using the shared secret?
We still check the project to ensure the asset is being uploaded for a valid Experience CS project.
This I'm still not sure about - if the asset isn't linked to a project, then I don't see any point in sending the project details or checking them. I could see the argument that the asset should be linked to a project, which we could do, we would just need to make sure that assets on public projects were still visible to all.
| authorize! :create_global, ScratchAsset | ||
| raise CanCan::AccessDenied unless @project_from_header.public_experience_cs_project? |
There was a problem hiding this comment.
I talked to @abcampo-iry about this we agreed that there checking the project isn't useful here so can be removed. I was worried it might create a false sense of security, and removing it lets us slightly simplify the api call. This project check would be useful if we were saving the asset on the project.
Status
Points for consideration
Security
Performance
What’s changed?
Steps to perform after deploying to production