Conversation
EjPlatzer
left a comment
EjPlatzer
left a comment
There was a problem hiding this comment.
Thanks for working on this! I see a few issues with your changes, which is not really your fault because you're working around logic that is wrong in the backend.
src/components/Profile/index.tsx
Outdated
| )} | ||
|
|
||
| {(myProf || !profileIsStudent || canReadStudentSchedules) && ( | ||
| {(myProf || (!profileIsStudent && !profileIsStaff) || canReadStudentSchedules) && ( // is it only faculty that have schedule? could we say if faculty instead here? |
There was a problem hiding this comment.
I think this is fixing a poor backend authorization strategy with a workaround in the frontend. The better fix would be to improve the auth strategy in the backend. See, for example, my changes to StateYourBusiness.cs in my schedule-quad branch for the API. That is a good start, but probably still not sufficient.
There was a problem hiding this comment.
Thank you. Is this the branch that was fixing the quad issue? should we continue this solution?
| const viewerIsPolice = useAuthGroups(AuthGroup.Police); | ||
| const [canReadStudentSchedules, setCanReadStudentSchedules] = useState<boolean>(); | ||
| const profileIsStudent = profile.PersonType?.includes('stu'); | ||
| const profileIsStaff = profile.Type == 'Staff'; // should we create an dict enum of the possible values? |
There was a problem hiding this comment.
The profile.Type field is not a reliable indicator of a person's status at Gordon, sadly. For one thing, the same person can at the same time be both staff and student (or equally faculty and student). Also, people who are officially staff can teach courses, in which case their course schedule should be visible to students.
I think, rather than trying to work around this complexity in the frontend, we should clean up the logic in the backend. I think the ideal logic would be something like:
- Anyone can view their own course schedule without limitations
- Faculty (and potentially a couple special categories of staff such as Staff Advisors) can view students' course schedules
- Any (authenticated) user can view a person's instructor schedule - i.e. a schedule that only shows the courses where a person is an instructor, and not a student. This handles the case where a person is both a student and an instructor, since their student course schedule shouldn't be visible to other students, but their instructor schedule should be.
There was a problem hiding this comment.
Would you suggest working on the authentication issue in its own branch or continuing the work you had in the schedule-quad branch?
There was a problem hiding this comment.
This is a good question. I think if you want to implement the third point in my original comment, you should probably continue work in my schedule-quad branch (and the corresponding schedule-quad-fixes branch for the UI.
There was a problem hiding this comment.
Looking into the branch, it is 513 commits behind develop, which would make it challenging to perform a clean merge (will definitely be easier later with more familiarity with the code). Maybe a rework of the solution from develop could be easier?
There was a problem hiding this comment.
Yeah, it will probably be easier to start fresh on the latest develop. Normally, that wouldn't be such an issue since you can simply rebase the commits, but there have been some really big formatting and whitespace changes in the interim, which git struggles with.
There was a problem hiding this comment.
For option 3 in the authorization logic, the way the process is currently set up--checking for authorization for the getAllCourses action-- makes it a little complicated to only give access to Instructor courses.
I wonder if it would be a good solution to handle this in the controller--to check who is requesting all the courses and if it is a student to only return instructor courses for that profile. otherwise return all of them.
maybe a getAllInstructorCourses could be added in the service.
There was a problem hiding this comment.
Yes, that would work. Except I would say, if the requestor is a faculty/staff member, return all courses, otherwise return only instructor courses.
There was a problem hiding this comment.
okay. sounds good! Is it an issue if all faculty can see classes that a faculty or staff is taking?
There was a problem hiding this comment.
No, I don't think so. It's part of faculty's job to see student's course schedules, and just because someone is a faculty or staff member doesn't mean they're not also a student.
There was a problem hiding this comment.
Hi,
We are trying to recover the updates we made in Terms last week from the VM. Could we merge this current solution to the permisions and quad issues? I can work on the Terms vs sessions in a different pull Request.
| setLoading(false); | ||
| console.log(profile); | ||
| }) | ||
| .catch((reason: AuthError) => { |
There was a problem hiding this comment.
This AuthError comes from Microsoft's Authentication library, and is unrelated to authorization errors thrown by the backend.
| allSessionSchedules[0]; | ||
| setSelectedSchedule(defaultSchedule); | ||
| setLoading(false); | ||
| console.log(profile); |
There was a problem hiding this comment.
For future reference, pull requests should not have any leftover console.logs.
4 participants
Fix: #2656
The implemented fix in this pull request is to check whether the person is a staff member (not faculty) as a condition to building the schedule section.
I also added a catch for the initial network error that was causing the constant spinning. The server sends a 401 authorization error if the profile being viewed isn't faculty or student. The catch will set the loading to false.