Open smurthys opened 5 years ago
All unit and privilege tests pass for me. I also tested this change with role in multiple ClassDB instances and each role was only able to connect to the databases it was added to (as expected). The only small issue which we already discussed in our meeting is that there is a misspelling in a comment on line 314 of addRoleBaseMgmtCore.sql
.
Thank you for implementing this temporary solution @smurthys
Thanks for the review and for testing @KevinKelly25.
I see one issue in initializeDBCore.sql
: In addition to not granting database connection to ClassDB group roles, it is also necessary to revoke database connection from those roles. The revocation is necessary if an existing ClassDB installation is updated with this revision. The downside to the revocation is that existing users immediately lose the ability to connect, but that can be addressed by explicitly granting db connection to existing users. I am thinking about a simple way to do that, and I welcome suggestions.
One possible approach is that we can run the following if the RoleBase
table exists already:
FOR User IN
SELECT RoleName FROM ClassDB.RoleBase
LOOP
EXECUTE FORMAT('GRANT CONNECT ON DATABASE %I TO %s', current_database(), User);
END LOOP;
The problem with this approach is that this will also be run if user already can connect to the database. I do not believe this causes an error (worth checking though) so it is more of an efficiency problem.
Thanks for the tip @KevinKelly25. I played around with a similar solution. The query needs to be over pg_roles table if executed in initializeDBCore.sql
. which is where I think the solution should be. I will think more about over the next couple of days.
I agree it logically should be in initializeDBCore.sql
. However, I do not believe it is possible to use pg_roles
for this operation as it shows roles for the whole server, not just the database it is currently in. Since role information is stored at the server level rather then the database you would have to relate the role to a specific database. I am not sure that is possible with the system tables since they don't have database specific roles in the current setup.
Indeed. RoleBase
and pg_roles
would need to be joined.
The commits in this PR remove automatic DB connection to ClassDB group roles (at DB initialization) and instead manage that permission separately for each login role: grants permission in function
createRole
; revokes permission in functionrevokeClassDBRole
if the role has no more ClassDB roles.These commits fix #278.
This fix can be removed when Issue #277 is fixed, assuming that issue is addressed by creating database-specific names for ClassDB group roles as proposed in a comment at that issue.
The changes are tested manually. Privilege tests need to be updated.