Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Authorizer: Update homeroom method to be built from student methods #2584

merged 11 commits into from Sep 4, 2019


Copy link

@kevinrobinson kevinrobinson commented Sep 4, 2019

Who is this PR for?

students, families, educators

What problem does this PR fix?

This fixes an authorization gap between the methods answering "what homerooms can an educator access" and "what students can an educator access"? For users with access to the homeroom page, this can result in two problems. This has come up at the start of this school year for this first time.

First, the homeroom dropdown shows homerooms where they wouldn't have access to view full profiles for any students.

Screen Shot 2019-09-04 at 12 28 44 PM

Second, if using the dropdown to visit other homeroom pages, those pages would show name and other information about those students, but the student authorization check would prevent access to the profile or to the student photo. This does result in authenticated educators being able to view enrollment information, and other information including disability status.

Screen Shot 2019-09-04 at 12 28 36 PM

Student-level authorization rules blocked photos from loading, or from full profiles from being accessed. Monitoring on this is how we came to learn about the problem (there have been no reports from educators).

The core difference in semantics between the previous homeroom-level authorization method and student-level authorization is that the previous homeroom method would include homerooms at the same grade level in the same school as the educator's homeroom, if it was assigned. This would also result in older homerooms (with no students listed) being incorrectly included in the list.

What does this PR do?

  • Move previous homeroom authorization method from Educator to Authorizer, rename it as deprecated and require callers to pass acknowledge_deprecated:true.
  • Audit that there are no calls to previous authorization method outside of tests.
  • Add new method, built off of student-level authorization.
  • Add new tests to verify expected behavior of new method, including that it no longer includes homerooms at the same grade level and school, and that it excludes homerooms with no active students. Add parity tests showing the behavior is the same as the previous method, and describe all differences in test cases. Add option to describe differences with "no students" homerooms in test cases.
  • Fix old spec setup bugs where grade: 'K' was set on homeroom directly, instead of KF on student model. Add explicit validation of grade on Homeroom model as well.
  • Add PerfTest to test for performances regressions, verify no issues across districts.
  • Run manual data quality checks in production.
  • Add more test cases verifying current behavior to HomeroomsController

This does not cut over the behavior in HomeroomsController, so nothing should change by shipping this, and I'll do a final check on this before cutting over in a separate PR.


Which features or pages does this PR touch?
This won't change anything as-is, but when cut over, it will influence:

  • Homeroom
  • Core, authorization

Does this PR use tests to help verify we can deploy these changes quickly and confidently?

  • Included specs for changes
  • Improved specs for existing code in need of better test coverage
  • Manual testing made more sense here

Copy link
Contributor Author

@kevinrobinson kevinrobinson commented Sep 4, 2019


@kevinrobinson kevinrobinson merged commit d92ba73 into master Sep 4, 2019
1 check passed
@kevinrobinson kevinrobinson deleted the feature/homeroom-authorizations branch Sep 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet

Successfully merging this pull request may close these issues.

None yet

1 participant