Kolibri: Class coaches can access class pages even when they are not assigned to the class

[mrpau-richard]

Observed behavior

~I can access the class groups rename form as coach but I have 403 error after I submit it.~

Class Coaches can access a classroom page even if they are not assigned to the classroom.

Expected behavior

~The class groups rename form most not accessible in coach.~

Pages from classrooms not assigned to the coach should not be accessible to the coach.

It should be redirected in the auth-message refer at the screenshot below

Steps to reproduce

• Log in as admin, then navigate at class groups page then copy the link.
• Log out as admin then Log in as coach then navigate at the class groups page using the link.
• Try to rename a class groups then submit it.

Context

Tell us about your environment, including:

• Kolibri 0.10.0.dev1.dev+git-33-g268dd8a
• macOS 10.13.3
• Chome Version 65.0.3325.181 (Official Build) (64-bit)

…

[jonboiser]

@mrpau-richard Could you provide more details on the Coach account? Is it a Class Coach or Facility Coach account?

I think it _may_ be possible for a Class Coach to view Classes they are not assigned to, but cannot edit (e.g. rename a Learner Group) any part of them.

[mrpau-richard]

@jonboiser yeah, It's Class Coach. I have this screenshot showing that there is an option button enabled and it can access to rename group form.

But after I submitted or save it will show a 403 forbidden error.

[jonboiser]

@mrpau-richard My theory is that the Class Coach has not been assigned to the Classroom you're looking at.

So the underlying issue is that we do not handle this case (coaches going directly to class pages they should not access) properly at the moment.

Compare these screenshots:

Coach is assigned to classroom

The text above "Class groups" is correct

Coach is not assigned to classroom

The text above "Class groups" is not correct (like in your screenshot). Adding or renaming groups will not work.

[jonboiser]

The reason we don't see the 'auth-message' component in this scenario is because Coaches can _view_ any class, even if they are not assigned to it.

[mrpau-richard]

@jonboiser I think we should disable the options button then, if the Class Coach can't rename and delete the Class group.

[indirectlylit]

Coaches can view any class, even if they are not assigned to it

My understanding is that this is incorrect. I believe classes should be private and only visible to assigned coaches, facility-level coaches, and admins

[jonboiser]

@ralphiee22 This came up when we were implementing class coaches. Should we change the permissions for Classrooms so not-assigned-to-this-class Coaches get a 40X error when going to these pages?

[indirectlylit]

@ralphiee22 mind fixing this in develop?

[ralphiee22]

Regarding this, current permissions allow anybody from the same facility to READ any collection. In the backend, we do not prevent even a learner from being able to read a group. cc: @jamalex
A possible solution is attaching the collection id to the roles in the session object (to see if a coach has access to certain classes), but I am unsure how many places in the frontend depend on that being an array as opposed to a dictionary.

[indirectlylit]

I am unsure how many places in the frontend depend on that being an array as opposed to a dictionary

These should be straightforward to track down, and for 0.12 we have plenty of time for regression testing

[rtibbles]

I think @ralphiee22's solution is the right one, so that we can make role checks on the frontend more granular, but we should bump this to 0.12.

[jonboiser]

As of 0.12.2 (Demo Server), Class Coaches can actually access any class's coach page, even if they are not assigned to the class (as long as they have the URL).

On the demo server, I've reproduced this using the 'jbs' account, which is a Class Coach account for only one classroom (also checked to make sure it didn't have superuser permissions).

Class coaches should only be able to view coach pages for the classes they have been assigned.

I've updated the issue to explain this.

[MisRob]

 Class coaches should only be able to view coach pages for the classes they have been assigned.

This was implemented in https://github.com/learningequality/kolibri/pull/5308 (there's also a screen before and after). I've just checked develop one more time to be sure and following scenario works for me:

1. I created two class coaches - coach1, coach2 and assigned only coach1 to the classroom
2. visited the classroom as coach1, saved the URL (e.g. /coach/#/685397f1ae40e96cbdd77d9d7261d6a0/home)
3. logged in as coach2 (who is not assigned to the classroom)
4. visited the saved URL of the classroom

Then I see this:

and in console, all classroom related endpoints seem to return 403.

 As of 0.12.2 (Demo Server), Class Coaches can actually access any class's coach page, even if they are not assigned to the class (as long as they have the URL).

I checked 0.12.2 codes and there is still previous version of classroom permissions that allowed this behavior.

[MisRob]

Though I guess it would be better to display something more informative, this message doesn't make much sense in this situation. I suppose that this depends on those more granular role FE checks that Richard mentioned already above, right?

[jonboiser]

Okay, so to fix this for 0.12.3, we will need to cherry pick #5308 into release-v0.12.x

[MisRob]

@jonboiser PR is open #5356

[indirectlylit]

Though I guess it would be better to display something more informative, this message doesn't make much sense in this situation.

Good idea. Would you mind opening a new issue for this?

I suppose that this depends on those more granular role FE checks that Richard mentioned already above, right?

Maybe a simpler hack could work?

notAuthorizedMessage(isCoach) {
  if (isCoach) return "You are not an assigned coach for this class";
  return "You must be signed in as an admin or coach to view this page";
}

[MisRob]

Sounds good to me! Another solution might be returning some sort of internal error code/type from API and only defining a message for this error code on FE side.

We already have an issue for these things - #5305. I've just checked it and can see that @jonboiser has already added this scenario to a list (last point).

[rtibbles]

Given that we can tell if someone is signed in, and we can tell if someone is a coach, all in the frontend, I don't think we need to return a custom status, because we can work it out.

[MisRob]

The idea behind this was that BE usually knows exactly where was the problem and why it sends (any) error status to FE. Sometimes there can be more problematic scenarios than one or two and I'm not sure how reliable will be to "guess" what happened in general. But I've seen just a tiny portion of error handling so don't know if this is needed, was just an idea. If there are not many situations to differ between, I guess no problem then!

[rtibbles]

I assume that the backend permission check is just seeing if the user has coach permission for the particular classroom, so without adding extra logic it has no more information than the frontend, and the frontend already knows the extra context (this user is logged in and has a coach role).

I think in general the BE actually tends to have less contextual information - although one place I can think of is the case where a user has been automatically signed out, here we could return a 401 rather than a 403, which would be helpful to distinguish some cases.

[MisRob]

I assume that the backend permission check is just seeing if the user has coach permission for the particular classroom

Yeah if I can remember right, in this case, it is so indeed.

[jonboiser]

This issue should be fixed for 0.12.3 and beyond by cherry picking the fixes into the v0.12 release branch in #5356.