Skip to content

Conversation

@meiswjn
Copy link
Contributor

@meiswjn meiswjn commented Mar 25, 2022

This change will allow tracking Credentials usage much easier, independent from Fingerprints. So far it was necessary to use the SaveableListener provided by the Fingerprints to subscribe to Credentials usage. Because certain credential consumers do not create fingerprints, this approach is flawed.

Creating the listener gives a lightweight method of tracking Credentials usage, improving overall security.

The listener is introduced in jenkinsci/credentials-plugin#295

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@meiswjn meiswjn force-pushed the feature/add-credentials-listener branch from 204f7a1 to 65d73b8 Compare March 30, 2022 10:58
@meiswjn meiswjn marked this pull request as ready for review March 30, 2022 11:11
@meiswjn
Copy link
Contributor Author

meiswjn commented Mar 30, 2022

Sorry, that was a bit too early. Obviously requires jenkinsci/credentials-plugin#295 before it is ready for review / merge. Sadly, I cannot mark it as draft again..

Copy link
Contributor

@PierreBtz PierreBtz left a comment

Choose a reason for hiding this comment

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

That looks like a very valuable addition to the plugin. Thanks for the contribution!

I understand you need to incrementalize the plugin and I'm ok with it, but I don't think this should be done in this PR. I'm happy to do it for you, or feel free to create another PR dedicated to this change.

@PierreBtz PierreBtz marked this pull request as draft March 30, 2022 13:02
@PierreBtz
Copy link
Contributor

Sorry, that was a bit too early. Obviously requires jenkinsci/credentials-plugin#295 before it is ready for review / merge. Sadly, I cannot mark it as draft again..

No worries, did it for you

@meiswjn
Copy link
Contributor Author

meiswjn commented Apr 12, 2022

Hey @PierreBtz,
Just wondering if there are any news on this :) Thanks!

@PierreBtz
Copy link
Contributor

@meiswjn sorry I didn't have time to come back to this PR :/
I'm ok on the principle, you fixed my main concern that I considered blocking, the rest is merely nitpicking. I guess the next step is to merge the upstream PR to have it available in a weekly version, then we'll be able to progress on this PR.

Regarding our discussion about the core version to depend on, I agree with you it will be a pain to wait 3 LTS as I usually do. My suggestion would be to bump to the first weekly containing the upstream dev, it will cut the latest version of the plugin from the majority of the user base but since I expect I won't have time to make other changes in the near future I guess it's ok.

@meiswjn
Copy link
Contributor Author

meiswjn commented Apr 19, 2022

@PierreBtz Thanks for your reply and your willingness to merge this!

Let's see what version the upstream change will get merged to, then I will adapt the PR and put it to Ready for Review.

@meiswjn meiswjn marked this pull request as ready for review April 20, 2022 09:51
@meiswjn
Copy link
Contributor Author

meiswjn commented Apr 20, 2022

Depends on #69

@meiswjn
Copy link
Contributor Author

meiswjn commented May 2, 2022

Hey @PierreBtz, not to rush you, but would you be able to give me a timeline for this PR? Thanks!

@PierreBtz
Copy link
Contributor

@meiswjn sorry about the delay. I'm away this week, I commit to make this move next week. I had a quick look earlier this month, the prerequisite PR (bump of core version) needs adaptation since an API used in the tests changed and broke the test compilation.

PierreBtz added 3 commits May 12, 2022 17:05
…ials-listener

# Conflicts:
#	pom.xml
#	src/test/java/hudson/plugins/audit_trail/AuditTrailFilterTest.java
@PierreBtz
Copy link
Contributor

@meiswjn I took the liberty of:

  • refreshing the branch with the latest changes
  • fixing the broken casc test
  • adding documentation for the feature you developped
  • adding an additional log entry to ease debug

If you agree with those changes, I'll go ahead, merge this and cut a new release.

@meiswjn
Copy link
Contributor Author

meiswjn commented May 13, 2022

Sure! Thank you!

@PierreBtz PierreBtz merged commit 39bd2b4 into jenkinsci:master May 13, 2022
@PierreBtz
Copy link
Contributor

@meiswjn the artifactory of the project seems to have some trouble, I'll have to wait to complete the release, I'll try to do it this week-end, beginning of the next week at the latest.

@meiswjn meiswjn deleted the feature/add-credentials-listener branch May 13, 2022 08:59
@PierreBtz
Copy link
Contributor

@meiswjn this is now released as part of 3.11. Give it a few hours and it should be visible in your update center.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants