Skip to content

Conversation

@smurthys
Copy link
Member

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 function revokeClassDBRole 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.

Sean Murthy added 3 commits August 11, 2019 22:31
Permit db connection when a login role is created; revoke db connection when a role name no longer has any ClassDB role.
DB connection is now managed separately for each role with login. Fixes #278
@smurthys smurthys requested a review from KevinKelly25 August 12, 2019 03:20
@KevinKelly25
Copy link
Contributor

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

@smurthys
Copy link
Member Author

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.

@KevinKelly25
Copy link
Contributor

KevinKelly25 commented Aug 14, 2019

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.

@smurthys
Copy link
Member Author

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.

@KevinKelly25
Copy link
Contributor

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.

@smurthys
Copy link
Member Author

Indeed. RoleBase and pg_roles would need to be joined.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Users known in one ClassDB database are able to log in to all ClassDB databases on the same server (W)

3 participants