Create roles for stranded students - #1016
Conversation
b3b4d4e to
3824744
Compare
Test coverage93.57% line coverage reported by SimpleCov. |
This check isn't needed as it's done in profile when creating a student account. In the case a student is logged in, we know if they are part of a school or not (now by checking role, and later the school_id claim as part of their token) so don't need to check their domain. If a student is logged out, we don't know which domain they will try to sign up with so can't check This check might be needed if we were allowing the provisioning of teacher accounts but we're not doing this.
3824744 to
a580862
Compare
|
To decide: how backwards compatible does this change need to be - i.e. how longer after the profile change before tokens are all updated and this can be used |
Previously a student account whose token carried a school claim but who had no Role record was stranded: they could authenticate but appeared to belong to no school, so school-scoped endpoints returned nothing for them. This could happen for the new SSO student flow - most of the time students will join an class and get a roll as part of that, but this is needed in case a student uses SSO without a class link. This change reads the school_id claim from the user profile and, on each authenticated request, has StudentRoleService create the missing student role for that school. The service is a no-op for non-student accounts and for students who already have a role, so it is safe to run on every request. I've added the e2e spec coverage into the school index route as that's the first request made after login and a good way to test that the user has access to the school.
Now we're creating roles automatically in the the student roles service this code can be simplified. Note that the request specs fail if the StudentRoleService action was commented out. I've also changed the code that created class students as this was more complex that it needed to be.
a580862 to
1db9208
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Address the two critical issues in role creation and class enrollment validation.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR creates student roles from Profile’s school_id claim and removes redundant domain-mismatch handling for school joins.
Changes:
- Provisions missing student roles during authentication.
- Simplifies join and enrollment behavior.
- Updates models, factories, mocks, and related specs.
File summaries
| File | Summary |
|---|---|
spec/support/user_profile_mock.rb |
Adds school_id to profile mocks. |
spec/services/student_role_service_spec.rb |
Tests student role provisioning. |
spec/services/join_status_service_spec.rb |
Updates join-status coverage. |
spec/requests/join_controller_spec.rb |
Updates join scenarios. |
spec/models/school_spec.rb |
Removes obsolete helper coverage. |
spec/lib/profile_api_client_spec.rb |
Updates persisted school setup. |
spec/features/school/listing_schools_spec.rb |
Tests role creation from tokens. |
spec/factories/user.rb |
Updates student school and role setup. |
app/services/student_role_service.rb |
Creates roles from token school claims; blank claims can cause invalid role creation and 500 responses. |
app/services/join_status_service.rb |
Removes domain-mismatch handling. |
app/models/user.rb |
Exposes the student school ID. |
app/models/school.rb |
Removes obsolete domain helper. |
app/controllers/concerns/identifiable.rb |
Invokes role provisioning during authentication. |
app/controllers/api/join_controller.rb |
Enrolls students in classes; currently bypasses school-role validation by not assigning the current user. |
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
While eventually this will be set in student tokens, there might be a period of time when deploying where it isn't. Make sure we don't error in this case
There was a problem hiding this comment.
Looks neat, I like that we're making Profile authoritative for this.
One drawback of using JWTs for claims about authorisation is that we can't revoke them when things change. I wonder if we might have a situation where the student's roles have changed from Profile and the API's perspective, but an old token would mean they get the roles again with this implementation. Did you give that any thought?
(Very much a potential edge case, and honestly I'm not sure the existing behaviour works properly either)
I hadn't and it is a concern. I think this would mean that teachers couldn't remove students from a school as they would be added again after. I think the best way to solve this is to ensure a user is still a student by making an API call to loading the current user info from profile. I'll look at making this change and do it so it only gets called when a student doesn't have the role. |
I've checked this and we're already doing this so no changes needed (see |
Warning
Dependent on https://github.com/RaspberryPiFoundation/profile/pull/2099
Status
What's changed?
Before it was possible for students to use the new SSO feature with a school code and get 'stuck' without a role. This automatically creates roles for students that have been authenticated with Profile.
school_idclaim from token to identify students and create roles for them in their school.See commits for more details
Screenshots
After this change students will see the same thing as they would if they joined the school without being added to any classes: