Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

LloydAlbin
Copy link

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

Added support for case insentive dn's within LDAP.

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
Copy link
Author

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.

unknown added 2 commits October 19, 2016 10:52
…atabase 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
@LloydAlbin
Copy link
Author

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

…sers and ldap_groups and your are switching from user to group to user.
@LloydAlbin
Copy link
Author

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

…ers and groups without any errors or warnings.
@LloydAlbin
Copy link
Author

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.

@larskanis
Copy link
Owner

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.

@LloydAlbin
Copy link
Author

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

@bradyclifford
Copy link

Any updates on pulling in this PR?

Lloyd Albin and others added 4 commits August 3, 2018 14:28
* Fix compatibility with PostgreSQL-10
* Don't abort on SQL errors, but print ERROR notice
* Run sync within a SQL transaction, so that no partial sync happens
* Todo: Add Larskanis updates to the test suite and add the automated
tests.
@larskanis larskanis force-pushed the master branch 3 times, most recently from d0a97d7 to bf1137f Compare January 17, 2022 13:04
@larskanis larskanis force-pushed the master branch 5 times, most recently from 8435637 to f4d85e4 Compare December 2, 2022 11:02
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.

3 participants