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

Issue 336 - LDAP group to teams dont work in 1.3.2 #140

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@alfsch
Contributor

alfsch commented Feb 10, 2014

I've implemented a synchronization for LDAP groups. Now it's possible to define a period, in which teams are synchronized with the groups from LDAP.
This makes it possible to see all groups which are available in the configured LDAP-(Subtree) in the gitblit user/group management view. It also removes and creates the teams, when they're created/removed in LDAP (after the configured synchronization period).

@gitblit

This comment has been minimized.

Show comment
Hide comment
@gitblit

gitblit Feb 10, 2014

Owner

Hi Alfred,

This looks like a nice addition. I will look at it more closely sometime this week. For fun I have reflected this pr over to my dev server: https://dev.gitblit.com/tickets/gitblit.git/5

Cheers,
-J

Owner

gitblit commented Feb 10, 2014

Hi Alfred,

This looks like a nice addition. I will look at it more closely sometime this week. For fun I have reflected this pr over to my dev server: https://dev.gitblit.com/tickets/gitblit.git/5

Cheers,
-J

@alfsch

This comment has been minimized.

Show comment
Hide comment
@alfsch

alfsch Feb 12, 2014

Contributor

Hi James,

Thank you for reviewing it! Maybe this feature is also useful for the other connectors like Redmine... I didn't took a look at this topic, because we're only using Active Directory with LDAP as Master for User and Role-Management.

BR,

Alfred

PS: Since I've done some work on these LDAP topics, should I take a look on http://code.google.com/p/gitblit/issues/detail?id=364 ?

Contributor

alfsch commented Feb 12, 2014

Hi James,

Thank you for reviewing it! Maybe this feature is also useful for the other connectors like Redmine... I didn't took a look at this topic, because we're only using Active Directory with LDAP as Master for User and Role-Management.

BR,

Alfred

PS: Since I've done some work on these LDAP topics, should I take a look on http://code.google.com/p/gitblit/issues/detail?id=364 ?

@gitblit

This comment has been minimized.

Show comment
Hide comment
@gitblit

gitblit Feb 12, 2014

Owner

Issue 364 is a Wicket problem, not an LDAP one. But you are welcome to give it a shot - I won't hold you back. :)

Owner

gitblit commented Feb 12, 2014

Issue 364 is a Wicket problem, not an LDAP one. But you are welcome to give it a shot - I won't hold you back. :)

@gitblit

This comment has been minimized.

Show comment
Hide comment
@gitblit

gitblit Feb 19, 2014

Owner

Hi Alfred,

I've reviewed this pull request. The 1.3.x and 1.2.x releases sync on-demand with ldap which is why there was a cacheDuration setting... but you had to manually trigger the sync which wasn't obvious. The 1.4.x code was no longer able to sync with ldap due to the massive refactoring in Nov/Dec and the reduction in scope of the ldap class so I am glad you restored this capability as a scheduled task. Much better.

I have made some adjustments, simplified the setting names, rebased your commits on master, and pushed them here:
https://dev.gitblit.com/tickets/gitblit.git/5

I have not yet merged to master. When you have time please review my changes to make sure I didn't miss anything. If you are interested in an account on my dev server, that can be arranged.

Owner

gitblit commented Feb 19, 2014

Hi Alfred,

I've reviewed this pull request. The 1.3.x and 1.2.x releases sync on-demand with ldap which is why there was a cacheDuration setting... but you had to manually trigger the sync which wasn't obvious. The 1.4.x code was no longer able to sync with ldap due to the massive refactoring in Nov/Dec and the reduction in scope of the ldap class so I am glad you restored this capability as a scheduled task. Much better.

I have made some adjustments, simplified the setting names, rebased your commits on master, and pushed them here:
https://dev.gitblit.com/tickets/gitblit.git/5

I have not yet merged to master. When you have time please review my changes to make sure I didn't miss anything. If you are interested in an account on my dev server, that can be arranged.

@alfsch

This comment has been minimized.

Show comment
Hide comment
@alfsch

alfsch Feb 20, 2014

Contributor

Hi James,

I've reviewed your changes and adjustments in my request via browser and all should work as intended. I will try and check this patch set later in my development environment.

Now I understand the cacheDuration setting. I was wondering what it is for and thought it is a cache setting to reduce the communication to the backing ldap system. This was a little bit strange for me, because I've never thought about reducing reading traffic for authentication to ldap, because this is the main purpose of a ldap directory server.

An account on your dev server would be nice.

BR,

Alfred

Contributor

alfsch commented Feb 20, 2014

Hi James,

I've reviewed your changes and adjustments in my request via browser and all should work as intended. I will try and check this patch set later in my development environment.

Now I understand the cacheDuration setting. I was wondering what it is for and thought it is a cache setting to reduce the communication to the backing ldap system. This was a little bit strange for me, because I've never thought about reducing reading traffic for authentication to ldap, because this is the main purpose of a ldap directory server.

An account on your dev server would be nice.

BR,

Alfred

@alfsch

This comment has been minimized.

Show comment
Hide comment
@alfsch

alfsch Feb 21, 2014

Contributor

Hi James,

the review of your changes is done. I've had some findings in the tests, which I've adjusted to the new Keys from config files. I also added a missing Test to the Gitblit TestSuite.

Since I don't have a clue how to add my changes to the Ticket, I've pushed the changes to the branch

https://github.com/alfsch/gitblit/tree/tickets/05/5/2

Have a nice Weekend,

Alfred

Contributor

alfsch commented Feb 21, 2014

Hi James,

the review of your changes is done. I've had some findings in the tests, which I've adjusted to the new Keys from config files. I also added a missing Test to the Gitblit TestSuite.

Since I don't have a clue how to add my changes to the Ticket, I've pushed the changes to the branch

https://github.com/alfsch/gitblit/tree/tickets/05/5/2

Have a nice Weekend,

Alfred

@gitblit

This comment has been minimized.

Show comment
Hide comment
@gitblit

gitblit Feb 21, 2014

Owner

Closed by merging ticket 5

Owner

gitblit commented Feb 21, 2014

Closed by merging ticket 5

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