Skip to content
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

[JENKINS-56005] Add user property-change listener #3901

Closed
wants to merge 14 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 111 additions & 0 deletions core/src/main/java/hudson/model/UserPropertyListener.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
/*
* The MIT License
*
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*/

package hudson.model;

import hudson.ExtensionList;
import hudson.ExtensionPoint;
import hudson.model.UserProperty;
import java.text.MessageFormat;
import java.util.List;
import java.util.logging.Level;
import java.util.logging.Logger;
import javax.annotation.Nonnull;

/**
* Listener interface which all other user property-specific event-listeners make use of.
*/
public interface UserPropertyListener extends ExtensionPoint {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you made this UserPropertyListener<T extends UserProperty> extends ExtensionPoint and then used T instead of UserProperty below, you'd be able to better express that a listener is for a specific property type. Then the static methods can look up the appropriate listener based on the property type. That would work better than the existing parameterization I think.


static final Logger LOGGER = Logger.getLogger(UserPropertyListener.class.getName());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
static final Logger LOGGER = Logger.getLogger(UserPropertyListener.class.getName());
Logger LOGGER = Logger.getLogger(UserPropertyListener.class.getName());

The keywords are implied in interfaces.


/**
* Fired when a new user property has been created.
*
* @param username the user
* @param value property that was newly created.
*
*/
default void onCreated(@Nonnull String username, @Nonnull UserProperty value) {
LOGGER.log(Level.FINE, MessageFormat.format("new {0} property created for user {1}", value.getClass().toString(), username));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My suggestion on syntax when using java.util.logging is to take advantage of the lambda forms and use normal string concatenation. In this line, that would be:

Suggested change
LOGGER.log(Level.FINE, MessageFormat.format("new {0} property created for user {1}", value.getClass().toString(), username));
LOGGER.fine(() -> "new " + value.getClass().toString() + " property created for user " + username));

}

/**
* Fired when a new user property has been created.
*
* @param username the user
* @param value property that was newly created.
*
*/
default <T> void onCreated(@Nonnull String username, @Nonnull T value) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe this method will end up being useful after all. Parameterizing it this way will only affect the call site. In order to make a sort of type safe dispatcher based on T for implementations, you'd need to explicitly check the type at runtime since there's no equivalent syntactical feature for that in Java (yet). Either way, the above method that just uses UserProperty is fine.

The password property listener might not end up being a direct implementation of this listener if there's no good way to pass two Details instances for the onChanged event, though. My suggestion in such a scenario may be to make a more generic EventListener with EventObject instances like in JavaBeans.

LOGGER.log(Level.FINE, MessageFormat.format("new {0} property created for user {1}", value.toString(), username));
}

/**
* Fired when an existing user property has been changed.
*
* @param username the user
* @param oldValue old property of the user
* @param newValue new property of the user
*
*/
default void onChanged(@Nonnull String username, @Nonnull UserProperty oldValue, @Nonnull UserProperty newValue) {
LOGGER.log(Level.FINE, MessageFormat.format("{0} property changed for user {1}", oldValue.getClass().toString(), username));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having the default implementation log will mean that every single listener implementation that doesn't implement the method will spam the log.

}

/**
* Fired when an existing user property has been changed.
*
* @param username the user
* @param oldValue old property of the user
* @param newValue new property of the user
*
*/
default <T> void onChanged(@Nonnull String username, @Nonnull T oldValue, @Nonnull T newValue) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as <T>onCreated here.

LOGGER.log(Level.FINE, MessageFormat.format("{0} property changed for user {1}", oldValue.toString(), username));
}

/**
* Fired when an existing user property has been removed or deleted.
*
* @param username the user
* @param value property that was removed.
*
*/
default void onDeleted(@Nonnull String username, @Nonnull UserProperty value) {
LOGGER.log(Level.FINE, MessageFormat.format("new {0} property created for user {1}", value.getClass().toString(), username));
}

/**
* Fired when an existing user property has been removed or deleted.
*
* @param username the user
* @param value property that was removed
*
*/
default <T> void onDeleted(@Nonnull String username, @Nonnull T value) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as <T>onCreated here.

LOGGER.log(Level.FINE, MessageFormat.format("new {0} property created for user {1}", value.toString(), username));
}

static List<UserPropertyListener> all() { return ExtensionList.lookup(UserPropertyListener.class); }
}
Original file line number Diff line number Diff line change
Expand Up @@ -717,14 +717,21 @@ public Details newInstance(StaplerRequest req, JSONObject formData) throws FormE
if(!Util.fixNull(pwd).equals(Util.fixNull(pwd2)))
throw new FormException("Please confirm the password by typing it twice","user.password2");

User user = Util.getNearestAncestorOfTypeOrThrow(req, User.class);
UserProperty p = user.getProperty(Details.class);
davidolorundare marked this conversation as resolved.
Show resolved Hide resolved
String currentUserHashedPassword = ((Details) p).getPassword();

String data = Protector.unprotect(pwd);
if(data!=null) {
String prefix = Stapler.getCurrentRequest().getSession().getId() + ':';
if(data.startsWith(prefix))
PasswordPropertyListener.fireOnChanged(user.getId(), Util.fixNull(currentUserHashedPassword), Util.fixNull(pwd));
return Details.fromHashedPassword(data.substring(prefix.length()));
}

User user = Util.getNearestAncestorOfTypeOrThrow(req, User.class);
if (p != null) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are checking here if p is not null, but you already called the getPassword() method a few lines before.

PasswordPropertyListener.fireOnChanged(user.getId(), Util.fixNull(currentUserHashedPassword), Util.fixNull(pwd));
}
// the UserSeedProperty is not touched by the configure page
UserSeedProperty userSeedProperty = user.getProperty(UserSeedProperty.class);
if (userSeedProperty != null) {
Expand Down
59 changes: 59 additions & 0 deletions core/src/main/java/hudson/security/PasswordPropertyListener.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
/*
* The MIT License
*
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*/

package hudson.security;

import hudson.Extension;
import hudson.ExtensionList;
import hudson.model.UserPropertyListener;
import java.util.List;
import javax.annotation.Nonnull;

import static hudson.security.HudsonPrivateSecurityRealm.PASSWORD_ENCODER;

/**
* Listener notified of user password change events from the jenkins UI.
*/
@Extension
public class PasswordPropertyListener implements UserPropertyListener {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which is the use case that needs receiving the password (even if hashed)? Detecting the change is fine, but this could allow retrieving the actual values.


/**
* @since TODO
*
* Fired when an existing user password property has been changed.
*
* @param username the user
* @param oldValue old password of the user
* @param newValue new password of the user
*
* **/
static void fireOnChanged(@Nonnull String username, @Nonnull String oldValue, @Nonnull String newValue) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should probably note here somehow that the values are hashed passwords, not the actual password values. Otherwise, I was about to suggest that this method should use char[] instead of String so that you could zero out the arrays after using them, but that's overkill for hashes.

if ((!oldValue.equals(newValue)) && (!PASSWORD_ENCODER.isPasswordValid(oldValue, newValue, null))) {
for (PasswordPropertyListener l : all()) {
l.onChanged(username, oldValue, newValue);
}
}
}

static List<PasswordPropertyListener> all() { return ExtensionList.lookup(PasswordPropertyListener.class); }
}
4 changes: 4 additions & 0 deletions core/src/main/java/jenkins/security/ApiTokenProperty.java
Original file line number Diff line number Diff line change
Expand Up @@ -486,6 +486,9 @@ public HttpResponse doGenerateNewToken(@AncestorInPath User u, @QueryParameter S
}

ApiTokenStore.TokenUuidAndPlainValue tokenUuidAndPlainValue = p.tokenStore.generateNewToken(tokenName);
if ((p != null) && (tokenUuidAndPlainValue != null)) {
ApiTokenPropertyListener.fireOnCreated(u.getId(), p);
}
u.save();

return HttpResponses.okJSON(new HashMap<String, String>() {{
Expand Down Expand Up @@ -549,6 +552,7 @@ public HttpResponse doRevoke(@AncestorInPath User u,
p.apiToken = null;
}
p.tokenStats.removeId(revoked.getUuid());
ApiTokenPropertyListener.fireOnDeleted(u.getId(), p);
}
u.save();

Expand Down
74 changes: 74 additions & 0 deletions core/src/main/java/jenkins/security/ApiTokenPropertyListener.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
/*
* The MIT License
*
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*/

package jenkins.security;

import hudson.Extension;
import hudson.ExtensionList;
import hudson.model.UserProperty;
import hudson.model.UserPropertyListener;
import java.util.List;
import javax.annotation.Nonnull;

/**
* Listener notified of user api-token creation and deletion events from the jenkins UI.
*/
@Extension
public class ApiTokenPropertyListener implements UserPropertyListener {

/**
* @since TODO
*
* Fired when an api token has been created.
*
* @param username the user
* @param value api token property of the user
*
* **/
static void fireOnCreated(@Nonnull String username, @Nonnull UserProperty value) {
if (value instanceof ApiTokenProperty) {
for (ApiTokenPropertyListener l : all()) {
l.onCreated(username, value);
}
}
}

/**
* @since TODO
*
* Fired when an api token has been revoked.
*
* @param username the user
* @param value api token property of the user
*
* **/
static void fireOnDeleted(@Nonnull String username, @Nonnull UserProperty value) {
if (value instanceof ApiTokenProperty) {
for (ApiTokenPropertyListener l : all()) {
l.onDeleted(username, value);
}
}
}

static List<ApiTokenPropertyListener> all() { return ExtensionList.lookup(ApiTokenPropertyListener.class); }
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,19 +26,23 @@

import com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException;
import com.gargoylesoftware.htmlunit.HttpMethod;
import com.gargoylesoftware.htmlunit.Page;
import com.gargoylesoftware.htmlunit.WebRequest;
import com.gargoylesoftware.htmlunit.html.HtmlForm;
import com.gargoylesoftware.htmlunit.html.HtmlPage;
import com.gargoylesoftware.htmlunit.util.NameValuePair;
import com.gargoylesoftware.htmlunit.xml.XmlPage;
import hudson.ExtensionList;
import hudson.model.User;
import hudson.model.UserProperty;
import hudson.remoting.Base64;
import static hudson.security.HudsonPrivateSecurityRealm.CLASSIC;
import static hudson.security.HudsonPrivateSecurityRealm.PASSWORD_ENCODER;
import hudson.security.pages.SignupPage;
import java.io.UnsupportedEncodingException;
import java.net.URL;
import java.net.URLEncoder;
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
Expand Down Expand Up @@ -76,10 +80,12 @@ public class HudsonPrivateSecurityRealmTest {
public JenkinsRule j = new JenkinsRule();

private SpySecurityListenerImpl spySecurityListener;
private SpyPasswordPropertyListenerImpl spyPasswordListener;

@Before
public void linkExtension() throws Exception {
spySecurityListener = ExtensionList.lookup(SecurityListener.class).get(SpySecurityListenerImpl.class);
spyPasswordListener = ExtensionList.lookup(PasswordPropertyListener.class).get(SpyPasswordPropertyListenerImpl.class);
}

@Before
Expand Down Expand Up @@ -342,6 +348,41 @@ public void userCreationWithHashedPasswords() throws Exception {
assertTrue(spySecurityListener.createdUsers.get(0).equals("charlie_hashed"));
}

@Issue("JENKINS-56008")
@Test
public void updateUserPassword() throws Exception {
HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(true, false, null);
davidolorundare marked this conversation as resolved.
Show resolved Hide resolved
j.jenkins.setSecurityRealm(securityRealm);
WebClient wc = j.createWebClient();

spyPasswordListener.usersWithPasswordUpdate.clear();
assertTrue(spyPasswordListener.usersWithPasswordUpdate.isEmpty());

// new user account creation
SignupPage signup = new SignupPage(wc.goTo("signup"));
signup.enterUsername("debbie");
signup.enterPassword("debbie");
signup.enterFullName(StringUtils.capitalize("debbie user"));
signup.enterEmail("debbie" + "@" + "debbie" + ".com");
davidolorundare marked this conversation as resolved.
Show resolved Hide resolved
HtmlPage p = signup.submit(j);

assertEquals(200, p.getWebResponse().getStatusCode());

// execute an http request to change a user's password from their config page
User debbie = User.getById("debbie", false);
URL configPage = wc.createCrumbedUrl(debbie.getUrl() + "/" + "configSubmit");
String formData = "{\"fullName\": \"debbie user\", \"description\": \"\", \"userProperty3\": {\"primaryViewName\": \"\"}, \"userProperty5\": {\"user.password\": \"admin\", \"$redact\": [\"user.password\", \"user.password2\"], \"user.password2\": \"admin\"}, \"userProperty6\": {\"authorizedKeys\": \"\"}, \"userProperty8\": {\"insensitiveSearch\": true}, \"core:apply\": \"true\"}";

WebRequest request = new WebRequest(configPage, HttpMethod.POST);
request.setAdditionalHeader("Content-Type", "application/x-www-form-urlencoded");
request.setRequestBody("json=" + URLEncoder.encode(formData, StandardCharsets.UTF_8.name()));
davidolorundare marked this conversation as resolved.
Show resolved Hide resolved
Page page = wc.getPage(request);

// ensure user whose password was changed was in fact logged
assertEquals(200, page.getWebResponse().getStatusCode());
assertTrue(spyPasswordListener.usersWithPasswordUpdate.get(0).equals("debbie"));
}

private void createFirstAccount(String login) throws Exception {
assertNull(User.getById(login, false));

Expand Down Expand Up @@ -429,6 +470,16 @@ protected void loggedIn(@Nonnull String username) {
protected void userCreated(@Nonnull String username) { createdUsers.add(username); }
}

@TestExtension
public static class SpyPasswordPropertyListenerImpl extends PasswordPropertyListener {
private List<String> usersWithPasswordUpdate = new ArrayList<>();

@Override
public void onChanged(@Nonnull String username, @Nonnull Object oldValue, @Nonnull Object newValue) {
usersWithPasswordUpdate.add(username);
}
}

@Issue("SECURITY-786")
@Test
public void controlCharacterAreNoMoreValid() throws Exception {
Expand Down
Loading