Skip to content

Commit

Permalink
TRUNK-6187 Protect admin credentials with runtime property
Browse files Browse the repository at this point in the history
  • Loading branch information
rkorytkowski committed Aug 16, 2023
1 parent 31f49fc commit 6bd3351
Show file tree
Hide file tree
Showing 9 changed files with 52 additions and 4 deletions.
2 changes: 1 addition & 1 deletion Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ RUN mvn $MVN_SETTINGS -B dependency:go-offline -P !default-tools.jar,!mac-tools.
COPY . .

# Append --build-arg MVN_ARGS='clean install' to change default maven arguments
ARG MVN_ARGS='clean install'
ARG MVN_ARGS='clean install -DskipTests'

# Build the project
RUN mvn $MVN_SETTINGS $MVN_ARGS
Expand Down
4 changes: 4 additions & 0 deletions api/src/main/java/org/openmrs/api/UserService.java
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@
* @see org.openmrs.api.context.Context
*/
public interface UserService extends OpenmrsService {

public static final String ADMIN_PASSWORD_LOCKED_PROPERTY = "admin_password_locked";

/**
* Create user with given password.
Expand Down Expand Up @@ -325,6 +327,8 @@ public interface UserService extends OpenmrsService {
* <strong>Should</strong> match on incorrectly hashed sha1 stored password
* <strong>Should</strong> match on sha512 hashed password
* <strong>Should</strong> be able to update password multiple times
* <strong>Should</strong> respect locking via runtime properties
* <strong>Should</strong> respect locking via runtime properties except for startup
*/
@Logging(ignoredArgumentIndexes = { 0, 1 })
public void changePassword(String pw, String pw2) throws APIException;
Expand Down
7 changes: 6 additions & 1 deletion api/src/main/java/org/openmrs/api/impl/UserServiceImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,6 @@ public Role saveRole(Role role) throws APIException {
@Override
public void changePassword(String pw, String pw2) throws APIException {
User user = Context.getAuthenticatedUser();

changePassword(user, pw, pw2);
}

Expand Down Expand Up @@ -652,6 +651,12 @@ public void changePassword(User user, String oldPassword, String newPassword) th
}

private void updatePassword(User user, String newPassword) {
if (user.isSuperUser() && Boolean.valueOf(Context.getRuntimeProperties()
.getProperty(ADMIN_PASSWORD_LOCKED_PROPERTY, "false")) &&
!Context.hasPrivilege(PrivilegeConstants.EDIT_ADMIN_USER_PASSWORD)) {
throw new APIException("admin.password.is.locked");
}

OpenmrsUtil.validatePassword(user.getUsername(), newPassword, user.getSystemId());
dao.changePassword(user, newPassword);
}
Expand Down
3 changes: 3 additions & 0 deletions api/src/main/java/org/openmrs/util/PrivilegeConstants.java
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,9 @@ private PrivilegeConstants() {

@AddOnStartup(description = "Able to change the passwords of users in OpenMRS")
public static final String EDIT_USER_PASSWORDS = "Edit User Passwords";

@AddOnStartup(description = "Able to change the admin user password even if locked (for startup use only)")
public static final String EDIT_ADMIN_USER_PASSWORD = "Edit Admin User Password";

@AddOnStartup(description = "Able to add patient encounters")
public static final String ADD_ENCOUNTERS = "Add Encounters";
Expand Down
1 change: 1 addition & 0 deletions api/src/main/resources/messages.properties
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,7 @@ searchWidget.noResultsFoundFor=No results found for: <b>{0}</b>, here are partia

admin.title.short=Admin
admin.title=Administration
admin.password.locked=Admin password is locked and can only be changed via runtime property.

auth.logged.out=You are now logged out
auth.session.expired=Your session has expired.
Expand Down
30 changes: 30 additions & 0 deletions api/src/test/java/org/openmrs/api/UserServiceTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,36 @@ public void changePassword_shouldBeAbleToUpdatePasswordMultipleTimes() {
userService.changePassword("test", "Tester12");
userService.changePassword("Tester12", "Tester13");
}

@Test
public void changePassword_shouldRespectLockingViaRuntimeProperty() {
User u = userService.getUserByUsername(ADMIN_USERNAME);

Context.getRuntimeProperties().setProperty(UserService.ADMIN_PASSWORD_LOCKED_PROPERTY, "true");

assertThrows(APIException.class, () -> userService.changePassword("admin", "SuperAdmin123"));

Context.getRuntimeProperties().setProperty(UserService.ADMIN_PASSWORD_LOCKED_PROPERTY, "True");

assertThrows(APIException.class, () -> userService.changePassword("admin", "SuperAdmin123"));

Context.getRuntimeProperties().remove(UserService.ADMIN_PASSWORD_LOCKED_PROPERTY);

userService.changePassword(u,"test", "SuperAdmin123");
}

@Test
public void changePassword_shouldRespectLockingViaRuntimePropertyExceptForStartup() {
User u = userService.getUserByUsername(ADMIN_USERNAME);

Context.getRuntimeProperties().setProperty(UserService.ADMIN_PASSWORD_LOCKED_PROPERTY, "true");

Context.addProxyPrivilege(PrivilegeConstants.EDIT_ADMIN_USER_PASSWORD);

userService.changePassword(u,"test", "SuperAdmin123");

Context.removeProxyPrivilege(PrivilegeConstants.EDIT_ADMIN_USER_PASSWORD);
}

@Test
public void saveUser_shouldGrantNewRolesInRolesListToUser() {
Expand Down
5 changes: 3 additions & 2 deletions docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ services:
cache_from:
- openmrs/openmrs-core:${TAG:-dev}
- openmrs/openmrs-core:${TAG:-nightly}
depends_on:
links:
- db
ports:
- "8080:8080"
Expand All @@ -41,7 +41,8 @@ services:
OMRS_DB_NAME: ${OMRS_DB_NAME:-openmrs}
OMRS_DB_USERNAME: ${OMRS_DB_USERNAME:-openmrs}
OMRS_DB_PASSWORD: ${OMRS_DB_PASSWORD:-openmrs}
OMRS_ADMIN_USER_PASSWORD: ${OMRS_ADMIN_USER_PASSWORD-Admin123}
OMRS_ADMIN_USER_PASSWORD: ${OMRS_ADMIN_USER_PASSWORD:-Admin123}
OMRS_ADMIN_PASSWORD_LOCKED: ${OMRS_ADMIN_PASSWORD_LOCKED:-true}
healthcheck:
test: ["CMD", "curl", "-f", "http://localhost:8080/openmrs"]
interval: 3s
Expand Down
2 changes: 2 additions & 0 deletions startup-init.sh
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
# Configurable environment variables
OMRS_ADD_DEMO_DATA=${OMRS_ADD_DEMO_DATA:-false}
OMRS_ADMIN_USER_PASSWORD=${OMRS_ADMIN_USER_PASSWORD:-Admin123}
OMRS_ADMIN_PASSWORD_LOCKED=${OMRS_ADMIN_PASSWORD_LOCKED:-false}
# When set to 'true' all DB updates are applied automatically upon startup
OMRS_AUTO_UPDATE_DATABASE=${OMRS_AUTO_UPDATE_DATABASE:-true}
OMRS_CREATE_DATABASE_USER=${OMRS_CREATE_DATABASE_USER:-false}
Expand Down Expand Up @@ -123,6 +124,7 @@ echo "Writing out $OMRS_SERVER_PROPERTIES_FILE"
cat > $OMRS_SERVER_PROPERTIES_FILE << EOF
add_demo_data=${OMRS_ADD_DEMO_DATA}
admin_user_password=${OMRS_ADMIN_USER_PASSWORD}
admin_password_locked=${OMRS_ADMIN_PASSWORD_LOCKED}
auto_update_database=${OMRS_AUTO_UPDATE_DATABASE}
connection.driver_class=${OMRS_DB_DRIVER_CLASS}
connection.username=${OMRS_DB_USERNAME}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1782,7 +1782,9 @@ && getRuntimePropertiesFile().setReadable(true)) {
if (wizardModel.createTables) {
try {
Context.authenticate("admin", "test");
Context.addProxyPrivilege(PrivilegeConstants.EDIT_ADMIN_USER_PASSWORD);
Context.getUserService().changePassword("test", wizardModel.adminUserPassword);
Context.removeProxyPrivilege(PrivilegeConstants.EDIT_ADMIN_USER_PASSWORD);
Context.logout();
}
catch (ContextAuthenticationException ex) {
Expand Down

0 comments on commit 6bd3351

Please sign in to comment.