pg-ldap-sync icon indicating copy to clipboard operation
pg-ldap-sync copied to clipboard

Added the ability to have users and groups with the same names.

Open LloydAlbin opened this issue 8 years ago • 7 comments

Added the ability to have users and groups with the same names. Added support for case insensitive dn's within LDAP. LDAP is case aware but case insensitive.

Lloyd Albin Database Administrator Statistical Center for HIV/AIDS Research and Prevention (SCHARP) Vaccine and Infectious Disease Division (VIDD) Fred Hutchinson Cancer Research Center (FHCRC) www.fredhutch.org

LloydAlbin avatar Oct 18 '16 22:10 LloydAlbin

I saw tsykes did a fork to allows allow skipping over failed created while I was writing this patch. My patch takes care of the create user works then create group fails situation, but does not handle when you are doing a first time sync to an existing Postgres server where the the user already exists but is not part of the group "ldap_users".

I am going to go back into the code and make it so that if the user already exists it will add them to the appropriate group(s), "ldap_users" and/or "ldap_groups". To do so I am going to add them as options in config file and also create those groups if they do not exist.

LloydAlbin avatar Oct 19 '16 17:10 LloydAlbin

I have added the following:

  • Script will warn instead of die if role still owns objects in the database and can't be deleted from the database
  • Script will warn about duplicate role. You should only run into this the first time you run this script against an existing server and you are using the "ldap_users" and "ldap_groups".
  • Added testing for usage of "ldap_users" and "ldap_groups" group name
  • Added "grant_this_group" to the pg_users and pg_groups YAML config file to support moving a user to become group to become user and to auto create these groups if they do not exist
  • Added the ability to run more than one test with different YAML files for config and ldap.
  • Added the ability to test user who belongs to more than one group by changing the query to return the groups in lower case alpha order.

It now passes the following tests:

  1. Your original test
  2. Your original test using ldap_users & ldap_groups
  3. Your original test + Users & Groups with the same names
  4. Your original test + Users & Groups with the same names and using ldap_users & ldap_groups

The only issue that I see is that I am getting some warnings about not being able to revoke/grant when using ldap_users and ldap_groups and your are switching from user to group to user.

Lloyd Albin

LloydAlbin avatar Oct 21 '16 00:10 LloydAlbin

I have fixed the last issue that I knew about in my code. All tests pass.

LloydAlbin avatar Oct 21 '16 21:10 LloydAlbin

I just added capability of adding the user group 'ldap_users' and group group 'ldap_groups' names on a system that already has users and groups without any errors or warnings.

LloydAlbin avatar Oct 26 '16 23:10 LloydAlbin

Thanks @LloydAlbin for working on this! I'll mostly merge it.

There are some minor things I would like to change. Since you added proper test cases I'll probably not break your additions.

The new config option "grant_this_group" is redundant with the "create_options" option. I think it's better to automatically add the "grant_this_group" to the CREATE statement.

larskanis avatar Nov 09 '16 16:11 larskanis

Lars,

I am happy that you liked what I did.

I agree with you about the "grant_this_group" being somewhat redundant with the "create_options" but I was also trying not to break backwards compatibility so that people could use the new version with an old config file and have it work out of the box. I figures that breaking backwards compatibility would be an automatic no for merging back into the project. That is also why I made sure your original test cases would still work after I updated the code.

I am also not surprised that you would have some changes to the code as this was my first time ever working with Ruby code.

One thing that I would change, is that I re-scan after putting existing users into the "grant_this_group". A flag could be set so that if any grant was performed and then and only then do the re-scan, so that the re-scan is not done every time. Also because I am not a Ruby developer, there may have been a much better way to do this with adding the grant_this_group to the normal grants process if the the grant did not already exist.

My boss at work is happy that you are looking at merging these capabilities back into your code. This is because next year we want to look at the ability to merge LDAP groups and write them to the same server. An example of this would be for a development server, where we would want all the group's from the production server plus the extra grants for the development server. This would make the LDAP groups cleaner and easier to maintain. Since you are open to merging code, he would be willing to let me add this to your code, instead of writing a whole new sync process from scratch. Let me know what you think of the merging groups idea.

Lloyd Albin

LloydAlbin avatar Nov 16 '16 22:11 LloydAlbin

Any updates on pulling in this PR?

bradyclifford avatar Nov 14 '17 17:11 bradyclifford