First implementation of a user service based on Apache htpasswd files. #97

Closed
wants to merge 12 commits into
from

Conversation

Projects
None yet
4 participants
@fzs
Collaborator

fzs commented Jul 17, 2013

This is a first implementation of a user service using Apache htpasswd files as the store for users and passwords. This version is read-only, i.e. it only can read credentials but not update them or create new entries in the htpasswd file.

+ public HtpasswdUserService()
+ {
+ super();
+ this.htpasswdFile = GitBlit.getFileOrFolder(KEY_HTPASSWD_FILE, DEFAULT_HTPASSWD_FILE);

This comment has been minimized.

@gitblit

gitblit Jul 17, 2013

Owner

I would move this to setup() since it is a setup-related action and definitely related to IStoredSettings, even though we're using a Gitblit static method here.

@gitblit

gitblit Jul 17, 2013

Owner

I would move this to setup() since it is a setup-related action and definitely related to IStoredSettings, even though we're using a Gitblit static method here.

This comment has been minimized.

@fzs

fzs Jul 18, 2013

Collaborator

I had moved this into the constructor to be able to the make htpasswdFile final. Which is a minor issue and I certainly see your argument that it is IStoreSetting related. I am kind of relying on a certain moment of getting instantiated, true.

@fzs

fzs Jul 18, 2013

Collaborator

I had moved this into the constructor to be able to the make htpasswdFile final. Which is a minor issue and I certainly see your argument that it is IStoreSetting related. I am kind of relying on a certain moment of getting instantiated, true.

This comment has been minimized.

@fzs

fzs Jul 18, 2013

Collaborator

Thinking further about this, I wonder if it should be in the constructor or setup at all. Both require a restart in order to switch to a different file.
If I move it to the read() method, then the htpasswdFile could be changed to a different one without a restart.Are thee any reasons not to allow this?

@fzs

fzs Jul 18, 2013

Collaborator

Thinking further about this, I wonder if it should be in the constructor or setup at all. Both require a restart in order to switch to a different file.
If I move it to the read() method, then the htpasswdFile could be changed to a different one without a restart.Are thee any reasons not to allow this?

This comment has been minimized.

@gitblit

gitblit Jul 18, 2013

Owner

If I move it to the read() method, then the htpasswdFile could be
changed to a different one without a restart.Are thee any reasons
not to allow this?

That would be preferable, actually. I try to make most settings
hot-reloadable without a restart.

@gitblit

gitblit Jul 18, 2013

Owner

If I move it to the read() method, then the htpasswdFile could be
changed to a different one without a restart.Are thee any reasons
not to allow this?

That would be preferable, actually. I try to make most settings
hot-reloadable without a restart.

@simonharrer

This comment has been minimized.

Show comment
Hide comment
@simonharrer

simonharrer Jul 22, 2013

Contributor

👍

Contributor

simonharrer commented Jul 22, 2013

👍

@lenhard

This comment has been minimized.

Show comment
Hide comment

lenhard commented Jul 24, 2013

👍

fzs added some commits Jul 9, 2013

Add a custom user service using an Apache htpasswd file for
authentication.

Add a new class HtpasswdUserService which implements custom
authorization against a text file created with the Apache 'htpasswd'
program.
Moved HtpasswdUserService.java to new source directory according to new
layout after direcory reorganisation since v1.2.1.
Adjusted to Constants.EXTERNAL_ACCOUNT.
Added dependency on commons-codec:1.7
Adjust account type to use general AccountType.EXTERNAL.
Use the new AccountType.EXTERNAL which is available since v1.3.0.
Use Keys file, added keys for htpasswd US.
Changed the htpasswd key from .file to .userfile.
Added keys to gitblit.properties file, relying on the build to include them in the Keys class. Swicthed to using Keys class in code.
Implement overriding local accounts with htpassd file.
Add option to overrride and overwrite an existing local account if the account is found as an external account, too.
Fix problem with invalid MD5 passwords.
Fix a problem where specifying a wrong password for a user that has his password stored as Apache MD5 would result in an IllegalArgumentException.
User IStoredSettings instead of Gitblit. Changed realmFile variable n…
…ame.

Implemented two suggestions:
Renamed the variable realmFile to htpasswdFile.
Store the IStoredSettings passed into setup() and use that instead of Gitblit in most places.
Add unit test for HtpasswdUserService.
Add unit test class for Apache htpasswd user service.
Add method getNumberUsers() to HtpasswdUserService which is only for use by
the unit test.
Change access to GitBlit.getFileOrFilder(String, String) to
GitBlit.getFileOrFolder(string) because the former would result in a
segementation fault during unit testing as the settings are not present in
GitBlit at that time.
Move setting of this.htpasswdFile from constructor to setup() for the same
reason as above.
Stop supporting crypt() and plain text passwords at the same time.
Disable support of plain text and crypt() encrypted passwords at the same
time. This caused problems because they both don't have an identifying
prefix in the htpasswd file. This means that when someone gleams the
crypt() hashes from the file she could log in using the hash as it would
successfully test against the stored hash using plain text comparison.

Now only either crypt() is supported or plain text but not both. Following
the Apache server, plain text is only supported on Windows and NetWare.
Otherwise crypt() is supported.

An undocumented setting key "realm.htpasswd.supportPlaintextPasswords" is
used for unit testing and could potentially be used to override the
OS-specific behaviour.
Make realm.htpasswd.userfile changeable without restart.
Move reading of realm.htpasswd.userfile key to read() from setup(). This
means it can be changed during runtime without requiring a restart, at the
cost of checking the setting on every read(), which currently is every
authentication.

Changing the setting will clear the current htpasswd user base. That means
that setting it to a non-existing file will also disallow logins from any
user in the old file. This is a conscious decision since we allow the case
of not having a htpasswd file at all and only reling on the backing user
service.
Add account type HTPASSWD.
Add and use account type HTPASSWD for Apache htpasswd user service.
@fzs

This comment has been minimized.

Show comment
Hide comment
@fzs

fzs Aug 12, 2013

Collaborator

I have added unit tests now, have fixed one problem with authentication and added another change to make the user file configuration more dynamic. While I see more improvements possible, I would consider this a first version ready for inclusion, with the prospect of adding more features later on.

Collaborator

fzs commented Aug 12, 2013

I have added unit tests now, have fixed one problem with authentication and added another change to make the user file configuration more dynamic. While I see more improvements possible, I would consider this a first version ready for inclusion, with the prospect of adding more features later on.

@gitblit

This comment has been minimized.

Show comment
Hide comment
@gitblit

gitblit Aug 12, 2013

Owner

Looks great.

Closed by commit a0c34e3.

I rebased & squashed your commits onto master. I also updated all appropriate documentation for your contribution.

Owner

gitblit commented Aug 12, 2013

Looks great.

Closed by commit a0c34e3.

I rebased & squashed your commits onto master. I also updated all appropriate documentation for your contribution.

@gitblit gitblit closed this Aug 12, 2013

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