Skip to content

Add default privilege settings for users added via LDAP #492

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

Closed
gitblit opened this issue Aug 12, 2015 · 19 comments
Closed

Add default privilege settings for users added via LDAP #492

gitblit opened this issue Aug 12, 2015 · 19 comments

Comments

@gitblit
Copy link
Collaborator

gitblit commented Aug 12, 2015

Originally reported on Google Code with ID 196

We're happily using the built-in LDAP support for user authentication (thanks!). However,
when a "new" GitBlit user (i.e. a user visiting the GitBlit instance for the first
time) is created then it doesn't have the privilege to create new repositories. This
privilege must be manually enabled by an administrator.

I suggest adding a default privilege setting which is used when new users are created
automatially. For example,

realm.ldap.defaultUserOptions = create fork

would automatically allow new users to create and fork repositories. Similar for "admin".

Reported by torf@torfbold.com on 2013-02-01 13:35:14

@gitblit
Copy link
Collaborator Author

gitblit commented Aug 12, 2015

Sensible idea.

Reported by James.Moger on 2013-02-01 13:56:59

  • Status changed: Accepted
  • Labels added: Milestone-1.3.0

@gitblit
Copy link
Collaborator Author

gitblit commented Aug 12, 2015

Reported by James.Moger on 2013-07-03 14:26:44

  • Status changed: New
  • Labels removed: Milestone-1.3.0

@gitblit
Copy link
Collaborator Author

gitblit commented Aug 12, 2015

Couldn't this also be solved by creating a team with the wanted permissions and either
assign LDAP users to this team in LDAP or define a default team for new users instead
of default options. This way you could also define repository access for this team
and new users would immediately be able to access these repositories.

Reported by f.zschocke on 2013-10-17 18:46:47

@gitblit
Copy link
Collaborator Author

gitblit commented Aug 12, 2015

Guys, I just love Gitblit and its LDAP support OOB. Seriously, it saves me so much time,
so thanks and kudos for that. But regarding to authentication, you gotta keep it clean.
So I boldly disagree with this idea. This is like overriding your LDAP setup to some
degree.

Imagine this scenario: you set this defaultUserOptions to "create fork". Some new users
log in and get ("inherit") these rights. Later on you decide you don't want to allow
new users to fork anymore, so you change this to just "create". Okay, fine, new users
who log in will get just this, but what's with existing users? It will be completely
impossible to track user rights as time goes on and it will all come down to just when
they signed in for the first time (like they got lucky or not). This isn't exactly
a good measure for user rights. And you can't even say, fine, let's remove fork from
all users when you remove it from defaultUserOptions, because they could have gotten
that right either from

a) defaultUserOptions
b) by manually ticking the checkbox for them

Summa summarum: don't mess with multiple authentication mechanisms, focus on just one.
So instead of defaultUserOptions and somehow generating a resultant set of rights from
multiple locations, I propose a much cleaner, much more easier to track as a Gitblit
and LDAP admin, and possibly much easier to implement way of solving this:

Provide options to set all user rights based on LDAP membership. Just like it's already
possible to set the admin right via realm.ldap.admins, it should be pretty straightforward
to add the same mechanism for create, fork, and anything else (if any).

Opinions?

Reported by sundayfunday1234@outlook.com on 2013-11-10 14:39:23

@gitblit
Copy link
Collaborator Author

gitblit commented Aug 12, 2015

One more thing to add: with my proposed solution, it would be probably a good idea to
make the user right checkboxes read-only and greyed-out when the user comes from LDAP
(just like the other fields currently, like username and pw).

Otherwise it can become misleading very quickly: you check the admin box for a user,
then the user logs in later again, he gets his rights overridden from LDAP as they
get recached in the local users.conf, and then you can scratch your head why isn't
he or she getting admin rights.

Reported by sundayfunday1234@outlook.com on 2013-11-10 14:47:40

@gitblit
Copy link
Collaborator Author

gitblit commented Aug 12, 2015

What about that scenario described in comment #5 by sundayfu?
We have exactly that. When I enable a LDAP user to the admin role, it gets overwritten
as soon as he logs in again.
How can I assign the admin role to an LDAP user otherwise?

Reported by jalbersdorfer on 2013-11-12 12:18:00

@gitblit
Copy link
Collaborator Author

gitblit commented Aug 12, 2015

jalbersd, add him/her to the LDAP group you set in realm.ldap.admins?

Reported by sundayfunday1234@outlook.com on 2013-11-12 12:28:19

@gitblit
Copy link
Collaborator Author

gitblit commented Aug 12, 2015

yes, thanks - seen by myself after some reading the .properties file.

Reported by jalbersdorfer on 2013-11-12 12:37:14

@gitblit
Copy link
Collaborator Author

gitblit commented Aug 12, 2015

I think this issue can be closed. Since 1.2.0 there is "Teams can now specify the *admin*,
*create*, and *fork* roles to simplify user administration", which solves the original
issue.

I agree that #5, disabling per-user permissions might enforce a cleaner way of setting
permissions using teams.

Reported by steffen@steffen-gebert.de on 2014-09-05 07:19:50

@gitblit
Copy link
Collaborator Author

gitblit commented Aug 12, 2015

Hmm, I didn't realize that option.

But it doesn't work correctly. If I create an LDAP group and assing it the create right,
then add a user to this group, it will be able to create repos (after singing out,
then in) but if an admin tries to edit his/her permissions, the create checkbox will
not be checked. IMHO not only should it be checked, but even disabled like username
and all the other info that are fetched from LDAP.

Reported by sundayfunday1234@outlook.com on 2014-09-05 08:53:25

@gitblit
Copy link
Collaborator Author

gitblit commented Aug 12, 2015

The checkboxes are now disabled and checked in this scenario.

Reported by James.Moger on 2014-09-11 15:22:44

  • Status changed: Queued
  • Labels added: Milestone-1.6.1

@gitblit
Copy link
Collaborator Author

gitblit commented Aug 12, 2015

Awesome!

Reported by sundayfunday1234@outlook.com on 2014-09-11 15:43:43

@gitblit
Copy link
Collaborator Author

gitblit commented Aug 12, 2015

I can confirm that disabling the checkbox works for create and fork - but not for admin
:D

Reported by sundayfunday1234@outlook.com on 2014-09-19 11:43:52

@gitblit
Copy link
Collaborator Author

gitblit commented Aug 12, 2015

Can you retest the admin checkbox?  It looks ok to me.

If the user does not inherit admin powers from a team then the checkbox should be enabled.
 If the user does inherit admin powers from a team then the checkbox should be disabled.
 Note: this is not a realtime/ajax check - if you add/remove an admin team membership
the checkbox is not updated.

Reported by James.Moger on 2014-09-25 14:28:49

@gitblit
Copy link
Collaborator Author

gitblit commented Aug 12, 2015

It's not inherited from a team, it's from realm.ldap.admins :)

Reported by sundayfunday1234@outlook.com on 2014-09-25 14:30:16

@gitblit
Copy link
Collaborator Author

gitblit commented Aug 12, 2015

So yeah, I wasn't completely consistent, I guess the team inheritence works and this
is a bit different.

Reported by sundayfunday1234@outlook.com on 2014-09-25 14:34:24

@gitblit
Copy link
Collaborator Author

gitblit commented Aug 12, 2015

realm.ldap.admins.  Oy.  I pushed a change to 1.7.0-SNAPSHOT to support this scenario
and other future ones like it.

While I could hack 1.6.x to support this, I won't because the proper fix requires invasive
API changes.  I have already done some invasive API changes for 1.6.1-SNAPSHOT - but
this was to improve security.  You'll have to wait till 1.7.0 to enjoy this specific
enhancement.  Alternatively you could run 1.7.0-SNAPSHOT if your need is great.

Reported by James.Moger on 2014-09-26 14:27:39

@gitblit
Copy link
Collaborator Author

gitblit commented Aug 12, 2015

That's cool, I also hate dirty hacks. Sure, it's not a big deal at all, so it can wait
:) Thanks for the fix already!

Reported by sundayfunday1234@outlook.com on 2014-09-26 14:51:48

@gitblit
Copy link
Collaborator Author

gitblit commented Aug 12, 2015

v1.6.1 released

Reported by James.Moger on 2014-10-20 21:36:02

  • Status changed: Done

@gitblit gitblit closed this as completed Aug 12, 2015
@flaix flaix modified the milestone: 1.6.1 Dec 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants