First implementation of a user service based on Apache htpasswd files. #97
Conversation
| public HtpasswdUserService() | ||
| { | ||
| super(); | ||
| this.htpasswdFile = GitBlit.getFileOrFolder(KEY_HTPASSWD_FILE, DEFAULT_HTPASSWD_FILE); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
👍 |
1 similar comment
|
👍 |
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
Use the new AccountType.EXTERNAL which is available since v1.3.0.
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.
Add option to overrride and overwrite an existing local account if the account is found as an external account, too.
Fix a problem where specifying a wrong password for a user that has his password stored as Apache MD5 would result in an IllegalArgumentException.
…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 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.
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.
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 and use account type HTPASSWD for Apache htpasswd user service.
|
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. |
|
Looks great. Closed by commit a0c34e3. I rebased & squashed your commits onto master. I also updated all appropriate documentation for your contribution. |
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.