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

Authorization/JAXB-API for Jenkins #55, #57 #60

Merged
merged 26 commits into from
Mar 16, 2023

Conversation

gnl42
Copy link
Contributor

@gnl42 gnl42 commented Feb 23, 2023

Added "crumbissuer" authentication. Do we still need the old authentication code?

Fixed missing JAXB-API complaint
(https://www.vogella.com/blog/eclipse-rcp-java-11-jaxb/)

Copy link
Member

@wimjongman wimjongman left a comment

Choose a reason for hiding this comment

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

Thanks, George!

This looks good. I found a couple of nits.


}

@Deprecated
Copy link
Member

Choose a reason for hiding this comment

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

You can drop it.

@@ -4,14 +4,18 @@ Bundle-Name: %Bundle-Name
Bundle-SymbolicName: org.eclipse.mylyn.hudson.core;singleton:=true
Bundle-Version: 3.26.0.qualifier
Bundle-Vendor: %Bundle-Vendor
Require-Bundle: jakarta.xml.bind;bundle-version="2.3.3",
org.eclipse.core.runtime;bundle-version="0.0.0",
Require-Bundle: org.eclipse.core.runtime;bundle-version="0.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please keep alphabetic order for dependencies

org.eclipse.mylyn-target/mylyn-e4.26.target Show resolved Hide resolved
org.eclipse.mylyn-target/mylyn-e4.26.target Show resolved Hide resolved
org.eclipse.mylyn-target/mylyn-e4.26.target Show resolved Hide resolved
@gnl42
Copy link
Contributor Author

gnl42 commented Feb 24, 2023

Hi Wim,
I made the changes and had to rework the code a bit to fix some issues I came across after some more testing.

The build framework is weird, I don't understand what the intent was in its design.
CommonHttpClient has an "authenticated" flag that is good for a session, yet has a ThreadLocal variable "context" that is used to store the token. Side effect of this is that each thread had to authenticate rather than just doing it once for the session.

One issue I'm unable to solve is how use the JSESSION id to authenticate when running a build using ones password.
It works fine when using a API token, but according to all the documentation I've come across it should be doable in both cases???

I've been testing it against my local Jenkins instance, and against https://ci.eclipse.org/mylyn/job/github-mylyn/.
If someone who is able to login to ci.eclipse.org could test the code I would feel more comfortable.

Also, I haven't seen any sign that the crumb is needed. Could be my jenkins hasn't been set up fully.

@BeckerFrank
Copy link
Contributor

Also, I haven't seen any sign that the crumb is needed. Could be my jenkins hasn't been set up fully.

I have finally remember the password so that I could reactivate the Hudson and Jenkins test instances of the http://mylyn.org domain. Maybe that helps.

@gnl42
Copy link
Contributor Author

gnl42 commented Feb 27, 2023

Also, I haven't seen any sign that the crumb is needed. Could be my jenkins hasn't been set up fully.

I have finally remember the password so that I could reactivate the Hudson and Jenkins test instances of the http://mylyn.org domain. Maybe that helps.

If that is the same instances that the unit tests are using (e.g.: https://mylyn.org/hudson-3.3.3, http://mylyn.org/jenkins-2.32.3/) it doesn't help.

Neither of them accept the ''crumbIssuer/api/json" request

@BeckerFrank
Copy link
Contributor

Maybe the versions are to old to support that or we have never configure this.

@gnl42
Copy link
Contributor Author

gnl42 commented Feb 28, 2023

@BeckerFrank

Maybe the versions are to old to support that or we have never configure this.

Looks like 2.32.3 was released back in 2016 so I would say it's too old.
It also hasn't been configured to use the crumb security (CSRF not enabled)

@gnl42
Copy link
Contributor Author

gnl42 commented Mar 1, 2023

Hi @wimjongman
I think I've done all I can with this at the moment. The unit tests pass (one is disabled).
The only thing I haven't been able to make work is starting a build using username/pw. Docs say this should be doable when passing crumb AND sessionId but I haven't been able to make it work with my Jenkins instance.
Using user/token works

I would suggest merging the code and deal with any issues that crop up with a new ticket

George

@wimjongman
Copy link
Member

I would suggest merging the code and deal with any issues that crop up with a new ticket

Hey George. Cool! Do you want to take a look at the review comments before merging?

Copy link
Contributor

@ruspl-afed ruspl-afed left a comment

Choose a reason for hiding this comment

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

I've removed my comments regarding issues in .target, I need to have closer look later.

@gnl42
Copy link
Contributor Author

gnl42 commented Mar 1, 2023

I would suggest merging the code and deal with any issues that crop up with a new ticket

Hey George. Cool! Do you want to take a look at the review comments before merging?

I think I've addressed the comments to the best of my ability. When I tried to address the comments about the .target I ran into unresolved references that I have no idea how to fix

@gnl42
Copy link
Contributor Author

gnl42 commented Mar 5, 2023

Don't merge yet. Started looking into the session Id problem and found some very not nice behaviour

@gnl42
Copy link
Contributor Author

gnl42 commented Mar 7, 2023

Solved the "sessionId" issue.

user/pw can run build now. Caveat is that if Jenkins is using StrictCrumbIssuer with a timeout set for the crumb one will be unable to run builds once the crumb expires. I can't see any way to have a new crumb created in this situation with how the build framework is designed.
Does not happen with a token, the recommended way, so I'm ok with this behaviour.

@wimjongman
Copy link
Member

George, is the token used in the password field?

@gnl42
Copy link
Contributor Author

gnl42 commented Mar 7, 2023 via email

@wimjongman
Copy link
Member

Yes, it is. To be clear by 'system' do you mean systems like Jenkins or different Jenkins versions? Is the UI still using "Hudson"?

@gnl42
Copy link
Contributor Author

gnl42 commented Mar 7, 2023 via email

@BeckerFrank
Copy link
Contributor

BeckerFrank commented Mar 9, 2023

@BeckerFrank

Maybe the versions are to old to support that or we have never configure this.

Looks like 2.32.3 was released back in 2016 so I would say it's too old. It also hasn't been configured to use the crumb security (CSRF not enabled)

Yes we should use a newer Jenkins version in our new test infrastructure (see Multipass setup supports >= 2_303_3)

@gnl42
Copy link
Contributor Author

gnl42 commented Mar 16, 2023

Could someone merge the PR?

@ruspl-afed ruspl-afed merged commit bddef17 into eclipse-mylyn:main Mar 16, 2023
@wimjongman wimjongman linked an issue Mar 16, 2023 that may be closed by this pull request
@wimjongman wimjongman added this to the 3.26.0 milestone Mar 16, 2023
@wimjongman wimjongman modified the milestones: 3.26.0, 3.26.1 May 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hudson Connector - AuthenticationException: Forbidden
4 participants