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

davidolorundare · 2019-02-17T18:11:14Z

See JENKINS-56005 and JENKINS-56008.

Revisions based on the discussions in the comments, an interface; UserPropertyListener was created, with default method all other listener implementations would make use of. Enabling support for listening to Jenkins user property-change events.
Two new listener implementation classes; PasswordPropertyListener and ApiTokenPropertyListener were created and added to jenkins-core. These classes handle firing notifications specific to their respective use cases (i.e. password-change and api-token creation/revocation events respectively). The rationale for the additions is discussed in each above ticket.
The relevant methods, in the corresponding jenkins-core classes, where these events occur; HudsonPrivateSecurityRealm.Details and ApiTokenProperty, were updated with calls to the respective new listener class methods.
The corresponding testsuites for each; HudsonPrivateSecurityRealmTest and ApiTokenPropertyTest were updated with tests that exercise these new listener class methods.

Proposed changelog entries

Enhancement: Addition of new hudson.model.UserPropertyListenerinterface to support notification of current and future Jenkins user-account property-change events (e.g. password changes, api-token key creation and deletion, etc).
Enhancement: Addition of new hudson.security.PasswordPropertyListenerclass to support notification of Jenkins user-account password property-change events.
Enhancement: Addition of new jenkins.security.ApiTokenPropertyListenerclass to support notification of Jenkins user-account api-token creation and revocation events.
Enhancement: Update to hudson.security.HudsonPrivateSecurityRealm.Details adding notification(s) of user-account password property-change events to its method.
Enhancement: Update to jenkins.security.ApiTokenProperty adding notification(s) of user-account api-token creation and revocation events to its method.

Submitter checklist

JIRA issue is well described
Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
* Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
Appropriate autotests or explanation to why this change has no tests
For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@jvz
@jeffret-b
@Wadeck
@daniel-beck

Added a new unit test which evaluates logging of a user property-change (update of user's password).

davidolorundare · 2019-02-18T15:15:57Z

restarting build

jvz · 2019-02-18T15:51:31Z

core/src/main/java/jenkins/security/SecurityListener.java

+     * @param p the user object property
+     * @param username the username
+     */
+    protected void userPropertyUpdated(@Nonnull UserProperty p, @Nonnull String username){}


I kind of wonder if this would be more appropriate in its own UserPropertyListener or similar. I'm thinking in terms that it may eventually be useful to have event listeners for JobProperty, NodeProperty, and others if I'm forgetting.

Also, I'm not sure if there's precedence in this class already, but I think it might make more sense to flip the ordering of the parameters here.

I'm not really convinced one way or another. The userPropertyUpdated() method doesn't quite seem to fit the other things in SecurityListener. But then, some of the other items in SecurityListener don't fit well either. The user property is more about users than specifically about security. But so are some of the other things in this file.

I guess when it comes down to it I have a slight preference for separating out a UserPropertyListener.

hmh.. alright. Question though is for the new UserPropertyListener: it being an abstract-class versus interface ?

@Wadeck Any thoughts on the creation of a new separate UserPropertyListener ?

Separation seems better for the long term, like Matt mentioned, about the other items that could/would require such listeners in the future. And also, it's not directly security related, so not really a reason to be put in SecurityListener, except for the specific case of Details. But I imagine the goal of this listener is to be triggered everytime a property change, in general.

@Wadeck hmh.. abstract versus interface ?

I will bet on abstract being the most natural way as you are providing some static helper methods (for the event trigger). The interfaces should be used for contract definition.

Interfaces allow static methods as of Java 8. See for example the methods added to the collections API. Also default methods.

test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java

jeffret-b

It's looking good.

jeffret-b · 2019-02-19T20:17:35Z

core/src/main/java/jenkins/security/SecurityListener.java

+     * @param p the user object property
+     * @param username the username
+     */
+    protected void userPropertyUpdated(@Nonnull UserProperty p, @Nonnull String username){}


I'm not really convinced one way or another. The userPropertyUpdated() method doesn't quite seem to fit the other things in SecurityListener. But then, some of the other items in SecurityListener don't fit well either. The user property is more about users than specifically about security. But so are some of the other things in this file.

I guess when it comes down to it I have a slight preference for separating out a UserPropertyListener.

Wadeck · 2019-02-20T09:52:24Z

core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java

                String data = Protector.unprotect(pwd);
                if(data!=null) {
                    String prefix = Stapler.getCurrentRequest().getSession().getId() + ':';
                    if(data.startsWith(prefix))
+                        SecurityListener.fireUserPropertyUpdated(p, user.getId());


⚠️ missing {} in the if, so you are breaking the statement there.

That's exactly the main reason why I hate the if without brackets 👿

If that code bug was not discovered by your tests => there is a missing path in your coverage.

do you advise using the if for only one-liners; or to just never use a non-bracketed if at all ?

Like;

if((data!=null) && (p != null)) { String prefix = Stapler.getCurrentRequest().getSession().getId() + ':'; if(data.startsWith(prefix)) return Details.fromHashedPassword(data.substring(prefix.length())); }

versus;

if((data!=null) && (p != null)) { String prefix = Stapler.getCurrentRequest().getSession().getId() + ':'; if(data.startsWith(prefix)) { return Details.fromHashedPassword(data.substring(prefix.length())); } }

I can only advice to never use a if without the associated {}, to avoid situation where someone wants to add a line, but due to the missing brackets will break the code flow like here.

And the one-liner is worse from my PoV due to the poor readability.

perks of using Python over Java :)

I only use one line ifs for things like:

if (foo == null) return null;

or

if (bar) return false;

or

if (baz) return;

Basically, only for control flow type things. Remember, forgetting your brackets can cause security problems (see the old Apple bug). If you auto-format your code before committing, you can see if you missed brackets based on how it indents things.

core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java

Wadeck · 2019-02-20T11:00:17Z

Is such approach really required? I mean to implement event firing specifically per property? I hope there is a better way to do that globally, without modification of plugins, like in UserProperty#reconfigure. Not sure it's feasible but that should be considered first.

Without a global one-time modification, you have to wait for all the plugins to implements that new event trigger, before being able to rely on it. Rough estimation ~50 UserProperty in the ecosystem.

In addition, the current approach is the opposite of DRY.

Another point, what happen if I save a user, without modification? From even listener PoV, the property was not updated, but in reality it will be recreated. That's something to take into account, ideally by not triggering the event if the property is not really changed, otherwise it will be the responsibility of the listener to check such things and that could become really cumbersome.

Finally, the new value is not really sufficient / useful if we do not have the previous value of the property. I imagine more an event with the previous value (potentially null in case of creation of the property), the new value (potentially null in case of removal of that property) and the User as it's not retrievable from the property directly.

jvz · 2019-02-20T17:03:58Z

Is such approach really required? I mean to implement event firing specifically per property? I hope there is a better way to do that globally, without modification of plugins, like in UserProperty#reconfigure. Not sure it's feasible but that should be considered first.

It could be useful to support audit logging changes to your API tokens, credentials (user creds at least; node creds need a separate NodeProperty listener), maybe even some other details. It would be nice for the UserProperty class itself to automatically fire the events rather than consumers.

Another point, what happen if I save a user, without modification? From even listener PoV, the property was not updated, but in reality it will be recreated. That's something to take into account, ideally by not triggering the event if the property is not really changed, otherwise it will be the responsibility of the listener to check such things and that could become really cumbersome.

Agreed. We should track the old and new values. These can be compared with .equals() or whatever appropriate to see if it changed, and then that would fire the event. In that case, it may be useful to include both properties as parameters (old and new).

Updated the HudsonPrivateSecurityRealm newInstance() method to use the PasswordPropertyListener class for notifying subscribers on changes to a user's password.

Updated the ApiTokenProperty doGenerateToken() and doRevoke() methods to use the ApiTokenPropertyListener class for notifying subscribers of api-token creations and revocations from the Jenkins UI, respectively.

Updated HudsonPrivateSecurityRealmTest with a unittest that exercises the added PasswordPropertyListener class for notifying subscribers of user password changes.

Added two unittests to the ApiTokenPropertyTest class for exercising the ApiTokenPropertyListener class methods; for notifying subscribers of api-token creation and revocation events.

davidolorundare · 2019-03-13T14:44:02Z

some updates have been made to the PR based on the earlier discussions.
cc @jeffret-b @jvz @Wadeck @daniel-beck

jvz

I like where this is going, but I'm not clear on the multiple signatures for the same events.

CC: @jenkinsci/code-reviewers for some API feedback.

davidolorundare · 2019-03-25T23:14:58Z

Restarting the build for this pull request.

varyvol · 2019-03-26T10:10:26Z

@davidolorundare did you really intend to close this or was it a mistake?

davidolorundare · 2019-03-26T12:12:19Z

Hi @varyvol, I'm going to reopen it soon; I need it to rebuild again. The PR needs a re-review, however, so it can be merged in.

@davidolorundare did you really intend to close this or was it a mistake?

jvz · 2019-05-07T15:57:09Z

core/src/main/java/hudson/model/UserPropertyListener.java

+ */
+public interface UserPropertyListener extends ExtensionPoint {
+
+    static final Logger LOGGER = Logger.getLogger(UserPropertyListener.class.getName());


Suggested change

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

Logger LOGGER = Logger.getLogger(UserPropertyListener.class.getName());

The keywords are implied in interfaces.

jvz · 2019-05-07T15:59:17Z

core/src/main/java/hudson/model/UserPropertyListener.java

+     * @param value property that was newly created.
+     *
+     */
+    default <T> void onCreated(@Nonnull String username, @Nonnull T value) {


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.

jvz · 2019-05-07T16:01:15Z

core/src/main/java/hudson/model/UserPropertyListener.java

+     *
+     */
+    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));


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));

jvz · 2019-05-07T16:01:54Z

core/src/main/java/hudson/model/UserPropertyListener.java

+     * @param newValue new property of the user
+     *
+     */
+    default <T> void onChanged(@Nonnull String username, @Nonnull T oldValue, @Nonnull T newValue) {


Same issue as <T>onCreated here.

jvz · 2019-05-07T16:02:05Z

core/src/main/java/hudson/model/UserPropertyListener.java

+     * @param value property that was removed
+     *
+     */
+    default <T> void onDeleted(@Nonnull String username, @Nonnull T value) {


Same issue as <T>onCreated here.

jvz · 2019-05-07T16:04:32Z

core/src/main/java/hudson/security/PasswordPropertyListener.java

+     * @param newValue new password of the user
+     *
+     * **/
+    static void fireOnChanged(@Nonnull String username, @Nonnull String oldValue, @Nonnull String newValue) {


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.

jvz · 2019-05-07T16:10:53Z

core/src/main/java/hudson/model/UserPropertyListener.java

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


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.

alecharp · 2019-09-19T09:25:07Z

@davidolorundare Do you think you could revisit this PR to address @Wadeck and @jvz comments? Thanks.

rsandell · 2019-10-17T10:37:45Z

core/src/main/java/hudson/model/UserPropertyListener.java

+     *
+     */
+    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));


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

varyvol

@davidolorundare I made a couple suggestions and I also agree with @jvz 's comments. Do you think you could go through the feedback?

varyvol · 2019-10-17T10:33:34Z

core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java

                        return Details.fromHashedPassword(data.substring(prefix.length()));
                }

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


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

varyvol · 2019-10-17T10:39:33Z

core/src/main/java/hudson/security/PasswordPropertyListener.java

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


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.

jglick · 2019-11-25T19:45:52Z

Closing for now as this seems to have been abandoned.

Normally we would add the stalled-pr label to the corresponding JIRA issue, to make it easier to search for patches that someone could pick up and revive, but I cannot find one in this case—the two linked issues are in the audit-log-plugin component, and marked fixed.

zbynek · 2019-11-25T23:44:28Z

@jglick issues seem to be moved to GitHub jenkinsci/audit-log-plugin#37

davidolorundare added 3 commits February 17, 2019 11:23

[JENKINS-56005] Add user property-change listener

1fe3770

[JENKINS-56008] Update security-realm with listener

f4465d1

[JENKINS-56008] Add user property-change unit test

124b84c

Added a new unit test which evaluates logging of a user property-change (update of user's password).

davidolorundare closed this Feb 18, 2019

davidolorundare reopened this Feb 18, 2019

jvz suggested changes Feb 18, 2019

View reviewed changes

jeffret-b reviewed Feb 19, 2019

View reviewed changes

Wadeck requested changes Feb 20, 2019

View reviewed changes

Merge branch 'master' of https://github.com/jenkinsci/jenkins

43b50ff

daniel-beck self-requested a review March 9, 2019 17:58

davidolorundare added 8 commits March 11, 2019 18:29

Refactor securitylistener methods

49400e5

[JENKINS-56005] Add userpropertylistener interface

13c8864

[JENKINS-56008] Add passwordpropertylistener class

87b44db

[JENKINS-56008] Refactor private-security realm

5de70ac

Updated the HudsonPrivateSecurityRealm newInstance() method to use the PasswordPropertyListener class for notifying subscribers on changes to a user's password.

[JENKINS-56170] Add apitokenpropertylistener class

d0a19f0

[JENKINS-56170] Update apitokenproperty methods

d1382a6

Updated the ApiTokenProperty doGenerateToken() and doRevoke() methods to use the ApiTokenPropertyListener class for notifying subscribers of api-token creations and revocations from the Jenkins UI, respectively.

[JENKINS-56008] Update private-security realm unittest

97bfcc3

Updated HudsonPrivateSecurityRealmTest with a unittest that exercises the added PasswordPropertyListener class for notifying subscribers of user password changes.

[JENKINS-56170] Update apitokenproperty unittests

cc75d2c

Added two unittests to the ApiTokenPropertyTest class for exercising the ApiTokenPropertyListener class methods; for notifying subscribers of api-token creation and revocation events.

davidolorundare changed the title ~~[JENKINS-56005] Add user property-change listener methods to SecurityListener~~ [JENKINS-56005] Add user property-change listener Mar 12, 2019

davidolorundare mentioned this pull request Mar 12, 2019

[JENKINS-55694] Log audit events for user property changes jenkinsci/audit-log-plugin#19

Open

jvz reviewed Mar 15, 2019

View reviewed changes

davidolorundare added 2 commits March 20, 2019 09:07

Merge branch 'master' of https://github.com/jenkinsci/jenkins

4466180

Refactor userpropertylistener methods

4e1c33f

davidolorundare closed this Mar 25, 2019

davidolorundare reopened this Mar 25, 2019

davidolorundare closed this Mar 25, 2019

davidolorundare reopened this Mar 26, 2019

daniel-beck added the needs-more-reviews Complex change, which would benefit from more eyes label Apr 10, 2019

oleg-nenashev requested a review from jvz May 7, 2019 10:19

jvz suggested changes May 7, 2019

View reviewed changes

daniel-beck added the plugin-api-changes Changes the API of Jenkins available for use in plugins. label Oct 11, 2019

rsandell reviewed Oct 17, 2019

View reviewed changes

varyvol reviewed Oct 17, 2019

View reviewed changes

rsandell added the needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging label Nov 7, 2019

jglick closed this Nov 25, 2019

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

	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));

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

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

Conversation

davidolorundare commented Feb 17, 2019 • edited Loading

Proposed changelog entries

Submitter checklist

Desired reviewers

davidolorundare commented Feb 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidolorundare Feb 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffret-b left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidolorundare Feb 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wadeck commented Feb 20, 2019 • edited Loading

jvz commented Feb 20, 2019

davidolorundare commented Mar 13, 2019

jvz left a comment

Choose a reason for hiding this comment

davidolorundare commented Mar 25, 2019

varyvol commented Mar 26, 2019

davidolorundare commented Mar 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecharp commented Sep 19, 2019

Choose a reason for hiding this comment

varyvol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick commented Nov 25, 2019

zbynek commented Nov 25, 2019

davidolorundare commented Feb 17, 2019 •

edited

Loading

davidolorundare Feb 20, 2019 •

edited

Loading

davidolorundare Feb 20, 2019 •

edited

Loading

Wadeck commented Feb 20, 2019 •

edited

Loading

davidolorundare commented Mar 26, 2019 •

edited

Loading