DASSL / ClassDB

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

No functionality available to manage team membership (N) #221

Closed afig closed 6 years ago

afig commented 6 years ago

From an earlier version of M3's Wiki page:

afig commented 6 years ago

In addition to granting the team to the student, we also have to use ALTER DEFAULT PRIVILEGES in order to allow members other than the creator to read and modify items in the team's schema. Likewise, these default privileges have to be removed when removing a student from a team. As a matter of fact, this means that we also have to remove all students before revoking a team (well, their default privileges in relation to the team have to be removed, which is easiest done by removing them from the team).

In terms of additional details for removing a student: if the student was not a initially a member of the team, a NOTICE could be raised. Revoking the team role from the student will prevent them from accessing anything in the team's schema, but does not remove any data from the schema, so doing so should be enough.

afig commented 6 years ago

Thinking about it some more, there is an interesting problem that comes up when removing a student from a team (perhaps as was alluded to by the "details?" in the description that was in the Wiki).

Simply revoking the team's role from the student is sufficient for blocking access while still allowing other team members to continue using objects that the removed student had created in the team's schema. However, an issue comes up when the removed student's objects have their ownership reassigned or when they are dropped (such as during the dropping of a student from ClassDB).

Although the student no longer had access to the objects in the team's schema, they were still the owner of those objects. This means that if a DROP OWNED BY is run, objects in the team's schema that were still in use by other team members may be unexpectedly removed. Additionally, if a REASSIGN OWNED BY is run, this could potentially escalate privileges of any SECURITY DEFINER functions that were in the team's schema (and these functions can still be called by other team members).

One solution is to reassign ownership of objects in the team's schema that were owned by the removed member to the team itself when the member is removed from the team. However, there is no simple command to only reassign objects within a specific schema, so a function would have to be created. Feedback on this idea would be appreciated

KevinKelly25 commented 6 years ago

I like the suggested solution and agree that we will probably have to write a function for removing team members so there is no unwanted side effects. By extending @afig's solution mentioned above I can think of two possible solutions.

1) Create a function that removes a user from a team by reassigning the objects that that user owns to the team then removing the user. Also, we change each of the revokeXYZ/dropXYZ functions so that it checks if that role is part of a team. If true, then it will have to use the function mentioned above to remove that user from each of their teams then finish removing/revoking that role.

2) I am still trying to do more research into the possibility and feasibility of this, but one idea that may make the whole process easier in the long run. I am wondering how possible it would be to make it so whenever a team member creates an object in the team schema it automatically reassigned that object to the team. I think the only way to do this would be with event triggers.

smurthys commented 6 years ago

Thanks @afig for the analysis. This situation is what I had suspected we have to manage with when I wrote details? in the to do list.

@KevinKelly25's option 2 is ideal, but I worry may not be practical because event triggers aren't fired for all necessary DDL statements (should be for both create and alter). Plus, this solution requires considerable dev effort.

I think transferring to the team ownership of object's in the team's schema can work.

BTW, is it presently the case that all team members have full access by default to all objects in the team schema? Just want to confirm.

afig commented 6 years ago

In response to @smurthys's question: yes, all team members have full access to objects created in the team's schema by default. This is done by using ALTER DEFAULT PRIVILEGES on the role being added to the team (only applies within the team's schema). Note that members can still explicitly alter privileges on the objects they create, including restricting other members from accessing them (is this a feature or bug? 😕 Depends on how we document it I suppose 😇).

I also like option 2, but as @smurthys mentioned, we would not be able to check for all necessary operations. Option 1 works. Currently, it would only have to be done in revokeStudent() (only students can be members of a team, and dropStudent() calls revokeStudent() during its process). There are two ways of implementing this (and a combination of the two):

  1. Automatically: Create a function that removes a student from all teams they are a member of (e.g. removeFromAllTeams(studentName)) and call the function during the student revocation process.

  2. Manually: Raise an EXCEPTION during the student revocation process informing the user that the student is still a member of certain teams (and list the team(s) they are still a member of)

  3. Do either 1 or 2 depending on the value of a new optional BOOLEAN parameter to revokeStudent() and dropStudent().

smurthys commented 6 years ago

I think reassigning objects as part of revoking team from a student is simple and suffices (for now).

BTW, function listOwnedObjects returns the objects a specific role owns in all schemas. It is possible that function can be revised to return objects in a specific schema. Or, more easily, the table listOwnedObjects returns can be filtered on the team's schema.

afig commented 6 years ago

Thought I would give an update on the progress of this issue:

Functions for adding and removing members, along with tests for those functions have already been created and are available in the add-teams-membership branch. However, functionality that avoids issues with DROP OWNED BY and REASSIGN OWNED BY is still in development.

The solution outlined above works, and the functions that implement it are in place (not pushed to repo yet). Specifically, two helper functions have been added (their behavior is generally explained by their names) ClassDB.reassignObjectOwnership() and ClassDB.reassignOwnedInSchema(). reassignObjectOwnership() generates a SQL command to change ownership of a specific object. As @smurthys suggested, reassignOwnedInSchema() uses ClassDB.listOwnedObjects() to get a list of objects owned by a role, filters it to a specific schema, and then calls reassignObjectOwnership() with appropriate parameters. Tests for these functions are also in progress

The current issue being dealt with is that, as mentioned in PR #116 , listOwnedObjects() does not list all owned objects, only: Tables, Indexes, Sequences, Views, Materialized Views, Composite Types (listed only as "Types"), TOASTs, Foreign Tables, and Functions. Objects not listed include: Domains, Operators, Non-composite Types (Enumerated, Base, and Array) , and others (to get a general idea of all objects possible, perform a Google Search for "PostgreSQL: Documentation: 10: ALTER" "OWNER TO", including quotes).

In the interest of time, we can draw a line for supported objects at those currently listed by listOwnedObjects(), and open an issue about the remaining object types. Feedback on this would be appreciated.

smurthys commented 6 years ago

Limiting reassignment to only those objects listOwnedObjects identifies suffices, with an issue added that listOwnedObjects does not identify all kinds of objects. (No issue in revoking a team from a student.)