[JENKINS-55305] Add user creation listener methods to SecurityListener

[davidolorundare]

See JENKINS-55305 and JENKINS-55307.

• Two new methods were added to the jenkins-core SecurityListener class to enable support for listening to Jenkins user-account creation events. The rationale for the additions is discussed in the above ticket.
• Implementations which exercise these SecurityListener methods were made in the user-creation methods of the HudsonPrivateSecurityRealm class.

Proposed changelog entries

• Enhancement: Update to jenkins.security.SecurityListener to support notification of Jenkins user-account creation events.
• Enhancement: Update to hudson.security.HudsonPrivateSecurityRealm adding notification(s) of user-account creation success events to its methods.

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
@daniel-beck

[jvz]

LGTM other than the comments I've left.

[jeffret-b]

I'm not clear on the use for these new methods. Are they only relevant for the internal, private security realm?

[jvz]

@jeffret-b the core use case would be for in HudsonPrivateSecurityRealm, though other authentication plugins can also fire events in this same listener.

[jvz]

You need to add calls to these listeners in the appropriate places in HudsonPrivateSecurityRealm as well to help demonstrate the use case for this. Plus, we need that implemented anyways in order for this listener to be useful.

[jeffret-b]

That makes sense. This operations aren't necessarily meaningful for the other domains.

It would be good to provide the use for these new operations.

[davidolorundare]

@jeffret-b Hi Jeff, as Matt mentioned the use and calls to these new methods are primarily in HudsonPrivateSecurityRealm and are implemented, in that class, in a separate pull-request that I was intending to push after this current one. I sent you some more context in a PM via Gitter.

[jvz]

Updated tasks:

• Add appropriate calls to fireUserCreated() and fireFailedToCreateUser() in HudsonPrivateSecurityRealm
• Add tests to HudsonPrivateSecurityRealmTest exercising these two new methods

[davidolorundare]

Based on the discussion earlier, the use and calls to these new methods, which are implemented in HudsonPrivateSecurityRealm (and the unit tests that exercise them in HudsonPrivateSecurityRealmTest), have now all been added along with this pull request.

[jvz]

Looks good to me!

[jvz]

See also JENKINS-55307 (might want to include that in your changelog in the description).

[jeffret-b]

Looks good.

[jeffret-b]

Kicking the build.

[davidolorundare]

the incremental version still doesn't get generated with this updated PR. I'm going to try running it from a master branch

[davidolorundare]

spoke with @daniel-beck about this earlier and decided to build the Jenkins version of this PR locally (with deploy) , in order to get a Jenkins snapshot that can be referenced in the downstream POM for later tasks of the project

[davidolorundare]

a snapshot of this working Jenkins build is located here- https://repo.jenkins-ci.org/snapshots/org/jenkins-ci/main/jenkins-core/2.158-SNAPSHOT/

[batmat]

@davidolorundare quick note: we would generally use the timestamped version 2.158-20190107.194411-1 in such case.
It's the only simple way we can be 100% sure a downstream PR is using the version we think to check something.

[davidolorundare]

@batmat I tried building my code with 2.158-20190107.194411-1 as the jenkins version number but got the error:

[ERROR] Failed to execute goal on project audit-log: Could not resolve dependencies for project org.jenkins-ci.plugins:audit-log:hpi:1.0-SNAPSHOT: Failure to find org.jenkins-ci.main:jenkins-war:war:2.158-20190107.194411-1 in https://repo.jenkins-ci.org/public/ was cached in the local repository, resolution will not be reattempted until the update interval of repo.jenkins-ci.org has elapsed or updates are forced

any thoughts ?

[Wadeck]

currently the only the HudsonPrivateSecurityRealm class makes use of the new methods

What I meant is about explaining where the different authentication plugins should place the event trigger call. Do I need to wait for the user account to be saved to disk, or only created in memory, etc. All those questions that we do not want to let developer too much freedom, otherwise the use of the event could become really tricky.
It's both meant to help the plugin maintainer that will trigger the event and the developers that will listen to such event, like a contract.

[jeffret-b]

Moved it back into work-in-progress as David is still working on adapting to a merge made yesterday. Closing and re-opening to kick off builds and checks.

[jeffret-b]

Latest updates look good.

[davidolorundare]

cc @Wadeck updates have just been made to the PR

Requested changes have been made.

[Wadeck]

@davidolorundare @jeffret-b

Fired after a new user account has been created and saved to disk.

I am sorry but this is not true. For your both usage of the event the user is created in memory at this point and not yet stored on disk.

Such description is especially useful for the listener implementation, and thinking the user is already saved but actually not really, could be dangerous.

Please either change the description or adapt the code. (The first one is safer)

[jeffret-b]

After looking at it again, I'm not fond of what we changed the JavaDoc to say. I propose changing it to just say, "Fired after a new user account has been created." That seems more universally applicable and sufficiently defined.

If you've got different ideas @Wadeck, could you please clarify them? I'm not entirely sure what you're saying or suggesting.

If this seems like a reasonable approach, we can ask @davidolorundare to submit a minor PR to remove that phrase.

[daniel-beck]

Is the problem here the uncertainty around the concept of 'user existence' when an external security realm is involved? I.e. a user may or may not exist at any time with external security realms (see e.g. legacy token auto creation).

[Wadeck]

@daniel-beck that's exactly my point. The events must be defined more strictly. Look at the loggedIn, it specifies that:

It should be called after the {@link org.acegisecurity.context.SecurityContextHolder#getContext()}'s authentication is set.
So, the consumer knows that in its implementation they will be able to request the authentication from the context. That's not the case for authenticated, but in practice that's mostly true.

As a event consumer, I am more than happy to see such things to avoid troubles. Without putting rules there, all the implementations must have multiple condition checks like does the user exist, do I need to create it, do I need to save it, can I use the session from the request, etc. And if one of the plugin is not firing the event at the correct location, the implementation will perhaps not work.

So, instead of reducing the JavaDoc, I prefer to have a correction there. In this case, we need to say that the user is created but not yet saved.

More generally for events, we need to ensure:

1. Most of the authentication plugin are able to fire that event (are they creating users ? do they save the user to disk themselves ? etc.)
2. The event listening is useful (=helpful) and usable (=can do thing with it) to achieve logic. I don't talk about just logging the username of the newly created account. Imagine if you want to add a property to the user, like the creation date, it will work. Now imagine you want to update the user mapping xml file, as the user is not saved, you need to be aware that this event will not fit your requirement.

Consequences of the two first points, due to the ecosystem:

3. We need to have multiple popular plugins that are using the new mechanisms

4. If there are popular plugins that are listening to those events, so the authentication plugins will fire the events.

It's like a positive feedback cycle. Without plugins listening to the events, nobody will care.

[daniel-beck]

Most of the authentication plugin are able to fire that event

I'm not sure this would actually be useful. There's a huge difference between built in security realm notifications of user creation, and external ones.

[Wadeck]

[...] of user creation, and external ones.

When LDAP plugin received for the first time a user, it will create an account in the internal User database. From my understanding, this event should triggered in such case, no?

Otherwise the audit plugin will be able to follow the user creation only if you are using the embedded security realm, that is a bit restrictive IMHO.

[daniel-beck]

From my understanding, this event should triggered in such case, no?

I'm not sure this is relevant enough. A user might as well already exist in Jenkins from SCM based sources, so it's not even as if this would reliably indicate a 'first ever login'.

Otherwise the audit plugin will be able to follow the user creation only if you are using the embedded security realm, that is a bit restrictive IMHO.

Users in external security realms just exist independent of Jenkins.

[jvz]

External security realms have their own audit logging.

[Wadeck]

As we can see with the discussion here, the JavaDoc must be rewritten. If it's specific for the embedded security realm, it must written explicitely.

[jeffret-b]

In this case, we need to say that the user is created but not yet saved.

Maybe I'm just not understanding what you precisely mean by "saved". In the specific case of HudsonPrivateSecurityRealm a file has been stored onto disk by the time fireUserCreated() is called. What additional conditions do you expect should exist before the user is considered to be "saved"?

(The flow and implementation of user handling has many structural problems. It would be nice to clean many parts of that up and make it sensible but that's difficult.)

We shouldn't expect that all providers have a distinct state of "saved" or that "created" is separate from it. With the HudsonPrivateSecurityRealm there isn't an easily recognized difference between those states. Not all services will persist data to disk to achieve persistence.

2. Now imagine you want to update the user mapping xml file, as the user is not saved, you need to be aware that this event will not fit your requirement.

Any idea that this should be possible or meaningful is part of the structural flaws in handling users. Access to the operations should be via the service or API. Intimate, private knowledge should be discouraged, especially when it runs behind the interface.

do they save the user to disk themselves ?

How is that relevant to Jenkins? We should only be concerned about what is exposed through their interface and not how they implement and store things.

From my understanding, this event should triggered in such case, no?

I don't think so. Maybe the JavaDoc can be improve to clarify that, but I can't quite figure out what it would say. There is already such a mess with account users, SCM users, and ephemeral users and unclear states and connections between them.

(I think you're actually addressing some different points, but I'm not understanding your concerns and intentions. Perhaps if you could propose what the JavaDoc should say or how the implementation should be changed that would help.)

[Wadeck]

Fired after a new user account has been created and saved to disk.

I am sorry but this is not true.

I put the breakpoint on the addProperty instead of putting on fireUserCreated method. So that confusion generated is totally my fault, sorry! The addProperty is triggering the save of the user to the disk.

So I am fine with that part of the JavaDoc.

===========

From my understanding, this event should triggered in such case, no?

I'm not sure this is relevant enough

External security realms have their own audit logging.

I don't think so

As everyone seems to say that an external security realm should not be able to use the fireUserCreated, I think we can either restrict the access (@Restricted(NoExternalUse.class)) and mention in the JavaDoc, that only the embedded security realm is a producer of that event.

IIUC we do not want external security realm to trigger the event because potentially the user was already existing due to SCM, right?

As the changelog says: "Jenkins user-account creation", that could be confusing for authentication plugins to comply. If we do not want them to try, we need to restrict the event trigger and explicitely say it's for internal realm only.

[daniel-beck]

an external security realm should not be able to use

A plugin could easily provide an alternative 'internal' user database with legitimate uses of fireUserCreated.

[Wadeck]

OK let's stop the discussion there, let the JavaDoc as is and we will react in the future only if it's required.