DASSL / ClassDB

An open-source system to let students experiment with relational data
https://dassl.github.io/ClassDB/
Other
8 stars 2 forks source link

Dropping a user as student from a DB removes the user from student role in all DBs on the server #277

Open KevinKelly25 opened 6 years ago

KevinKelly25 commented 6 years ago

If you drop a student from one ClassDB database it drops that student from all ClassDB databases. It makes sense why it does that looking at dropStudent function but it seems inconsistent with createStudent. For example:

If I have two databases cs305 and cs205. If I add a student to cs205 that student will not show up in cs305. However, if I add a student to both cs205 and cs305 then drop the student from either of the classes it will remove the student from both classes. At the very least not letting them connect to either and also removing them from student activity table of both.

afig commented 6 years ago

If I understand the situation correctly, in the example/scenario provided, the student should still be in the ClassDB.RoleBase table of the other class (but will not show up in the ClassDB.Student view, since that view checks for the ClassDB_Student group role, which the student no longer has).

The main issue is that the ClassDB_Student group role is revoked even if that user is still a student in another database within the same Postgres server cluster.

I believe related behavior has been informally discussed in-person before, but not in the context of being an issue.

KevinKelly25 commented 6 years ago

I believe you are right about what is happening, especially after doing a lot of tests surrounding the issue.

Maybe we could add a function to remove a student from just a single database? I think it would be relatively simple, the only problem I see with that is testing that functionality in the test scripts, since you would have to be testing in multiple databases.

smurthys commented 6 years ago

Ideally function dropXYZ we should revoke a dropped user's membership from a ClassDB role only if no the user does not have that role in any other DB. However, that is not (easily) possible in Postgres due to limitations on cross-DB queries.

An easy solution is to add an optional parameter such as dropServerRole to dropXYZ functions which defaults to FALSE. We can drop the server role only if the new parameter is TRUE. This approach has the disadvantage of leaving zombie server roles, but I don't see that being too much of an issue. It is possible a future createXYZ fails due the role remaining, but that too does not seem much of an issue. I can expand on these concerns later as we develop the solution.

Testing with multiple DBs should not be an issue because a psql script can change connection using the \connect meta-command.

EDIT: Finishing the apparently unfinished sentence from many moon ago. Apologies for the delay in noticing it.

Long term however, this problem can be addressed by using db-specific names for ClassDB roles instead of the fixed names ClassDB_Student and ClassDB_Instructor. For example, the role names can be ClassDB_Student_014 and ClassDB_Instructor_014 for database 014, but ClassDB_Student_8523 and ClassDB_Instructor_8523 for database 8523. Here 014 and 8523 are immutable IDs generated for each database at ClassDB installation.

The approach I suggest has some interesting consequences and implementation alternatives which need to be carefully evaluated, but I feel confident it can work.

afig commented 6 years ago

I think the solution that @smurthys provides will work well for our needs. While the described behavior could be implemented with a separate function, there is no need to since all we need to do is guard one action in the current drop scripts, which is easiest done with a parameter.

I agree that testing with multiple DBs is possible, with the test script creating, connecting to, and dropping a test database (only after asserting that the database did not previously exist).

As an aside, it seems like @smurthys' comment has an unfinished idea at the end. It would be interesting to hear about long-term solutions and considerations about this Issue. I can see how a future change to the way ClassDB manages roles could change the necessity of such a parameter.

KevinKelly25 commented 6 years ago

After some testing I have noticed a problem related to this issue. It most likely will be solved along with the above issue but I wanted to make the problem known.

If there is a user in two ClassDB databases and the user is dropped from one database but not the other the above problem is encountered. However, another related problem also occurs if the user is dropped from database 1 and their objects remain owned by them. As it would happen if you called the drop function like: classdb.dropstudent('teststu2',false, false, 'as_is');

This would make the objects remain in that database 1, as expected. However, if an admin/teacher in database 2 wants to drop the same student from the server using classdb.dropstudent('teststu2',true, false, 'drop_c'); it would fail with the error:

NOTICE:  Role "teststu2" is not a member of ClassDB role "classdb_student"
ERROR:  role "teststu2" cannot be dropped because some objects depend on it
DETAIL:  1 object in database testing1_1lvc01hojllf1r02
CONTEXT:  SQL statement "DROP ROLE teststu2"
PL/pgSQL function droprole(idnamedomain,boolean,boolean,character varying,idnamedomain) line 106 at EXECUTE
SQL statement "SELECT ClassDB.dropRole($1, $2, $3, $4, $5)"
PL/pgSQL function dropstudent(idnamedomain,boolean,boolean,character varying,idnamedomain) line 7 at PERFORM

This error is caused by the objects that were left in database 1 still owned by teststu2 which makes dropping the role not possible with the current function in database 1.

This can be easily fixed by a superuser by going into every database that still has teststu2 objects and running DROP OWNED BY teststu2. However, not all teachers may have access to do this. An alternative is to find and run a crossdb query with one of the two extensions dblink or postgres_fdw. However, these also require a user with access to all DBs which need to be deleted. Another possible "fix" would be to disallow the as_is option from dropstudent(). However, this is a regression and possibly not the best way to go about this either.

In any case, we need to keep this problem in mind while trying to solve this issue.

afig commented 6 years ago

Thanks for the comprehensive report @KevinKelly25. I do not believe that it will be solved alongside the above issue, since they are relatively distinct as one involves permissions while the other involves user management. In Postgres, these end up using the same DBMS feature, (roles) but the two uses serve different purposes.

Overall, the scenario described could be considered intended behavior. I do not think we should blindly drop data that is contained in other databases, since, in general, we do not want to drop data that was not created through the current instance of ClassDB. This is important in situations where an instance of ClassDB is running on a server cluster shared with other applications (or other instances of ClassDB that are operated by different instructors). If we were to implement the fix provided, then in such situations, an instructor or DB manager may accidentally (or even maliciously) drop data in other databases that they were not supposed to drop.

In the description provided, it is correctly said

However, not all teachers may have access to do this.

This is true, and ClassDB should not provide a way to circumvent those restrictions.

I agree that this behavior is important to keep in mind when managing related ClassDB functionality. Thanks again @KevinKelly25 for bringing this up.


Although tangential from the original issue, I wish to provide some possible solutions to this secondary issue:

One way to mitigate problems caused by this is by updating the documentation on the Removing Users page to better explain the consequences of each object disposition option. We can also give a better error message than the one provided by default.

Another (somewhat complex) solution is to add a "smart drop" option for dropFromServer, where the resulting action depends on whether the user being dropped owns objects in other databases. I can see 3 possible situations:

  1. There are no objects owned by the user in the other databases, and so the role is dropped from the server without issue. (Current behavior)
  2. The user has objects in other databases, but they are all ClassDB instances where the executing user is also an instructor/DB manager. The objects in the other instances will also be dropped, and the role is then dropped from the server. A notice may be shown to the user. (New behavior)
  3. The user has objects in other non-ClassDB databases, or in a ClassDB instance where the executing user is not an instructor or DB manager. An exception is thrown. (Current behavior)