Fix and test cases for #2930

[kevloral]

Two test cases added to class PasswordProtectedP2RepositoryTest for trying to reproduce the bug:

• testMirror: the test server is accessed over a mirror without authentication.
• testAuthMirror: the test server is accessed over a mirror with authentication. This the one that should fail.

As things stand, they both pass, so they need improving.

[laeubi]

@kevloral thnaks for working on this, if I understand correctly from your example you have the case that there is one (not authenticated) p2 site that is mirrored but the mirror is password protected.

I would suggest that your copy the PasswordProtectedP2RepositoryTest and maybe resources (for example as PasswordProtectedMirrorP2RepositoryTest) and replicate it as much as possible, e.g. you can use the latest eclipse as the not password protected site as in your real example, then add a setting to mirror that to the local test server and then assert that the test server was accessed.

[kevloral]

Not exactly: the p2-repo that is accessed through the mirror is always password protected (test-user/test-password). It is the mirror that may be protected or not:

• testMirror: it is trying to access a password protected p2-repo (test-user/test-password) through a mirror. The mirror is not password protected.
• testAuthMirror: it is trying to access a password protected p2-repo (test-user/test-password) through a mirror. The mirror is password protected (mirror-user/mirror-password). Note that, even though both are password protected, the username and passwords are different (just in case).

Anyway, these test cases pass even though I expected them not to. Therefore, I am going to add some logging to the MavenAuthenticator class to see if the problem is actually in there (and, also, to try to find out why those tests pass). Once I know more, I will change those tests if needed or add others that manage to reproduce the bug.

[kevloral]

Two more test cases added. The second one (testTargetDefinitionAuthMirror) successfully reproduces the bug:

• testTargetDefinitionMirror: this one tries to resolve a target definition from a p2 repo accessed over a mirror with no authentication. It passes.

• testTargetDefinitionAuthMirror: this one tries to resolve a target definition from a p2 repo accessed over an authenticated mirror. It fails, as this is exactly the case that is being reported on the associated bug.

[kevloral]

Fixed.

When MavenAuthenticator tried to get the credentials for a given URI (method getServerCredentials), it asked DefaultRepositoryIdManager for the list of known maven locations (method getKnownMavenRepositoryLocations) so it could compare the URIs of those locations with the one credentials are being requested for (method uriPrefixMatch).

That list of known locations did not include mirrors, even though methods getEffectiveLocation and getEffectiveLocationAndPrepareLoad do return mirrors when needed. Therefore, credentials were never returned by method method getServerCredentials when the URI belonged to a mirror because the URIs never matched.

Now, whenever a new mapping is added to DefaultRepositoryIdManager (method addMapping), the corresponding mirror, if any, is stored. When the list of known locations is requested, method getKnownMavenRepositoryLocations returns both repos and mirrors and, therefore, method getServerCredentials now return the credentials because the URIs do match.

[laeubi]

Many thanks for the test-case and working on a fix, it looks good in general but could maybe be improved a bit.

Beside that it would be good if you

1. target the main branch, the change can then be backported automatically to tycho-4 branch
2. squash your commits into one with a combined commit message that includes what was changed.

[github-actions]

Test Results

  576 files  ± 0    576 suites  ±0   3h 38m 41s ⏱️ - 14m 15s
  384 tests + 4    376 ✅ +2   7 💤 +1  1 ❌ +1 
1 152 runs  +12  1 129 ✅ +8  22 💤 +3  1 ❌ +1 

For more details on these failures, see this check.

Results for commit 48c008c. ± Comparison against base commit 64b3ccf.

This pull request skips 1 test.

org.eclipse.tycho.test.TYCHO321deployableFeature.DeployableFeatureTest ‑ testDeployableFeature

♻️ This comment has been updated with latest results.

[kevloral]

I will follow your suggestions and prepare a new PR targeting main instead. Thanks a lot for your comments.

[laeubi]

I will follow your suggestions and prepare a new PR targeting main instead. Thanks a lot for your comments.

If you squash all your commits into one and then rebase on main you can also just edit this PR I think.

[laeubi]

By the way, I don't know if you find tests testMirror, testAuthMirror and testTargetDefinitionMirror worth merging or if you think I should remove them altogether.

We should just keep them, even though they do not show the bug behavior they prove these cases already work.

[laeubi]

@kevloral can you rebase and adjust it to use the MavenRepositorySettings#getMirrors() aproach? I want to make a new release this month and it would be good to include your changes there!

[kevloral]

@kevloral can you rebase and adjust it to use the MavenRepositorySettings#getMirrors() aproach? I want to make a new release this month and it would be good to include your changes there!

Of course. I will make that change as well as the others you suggested in the next few days.

[laeubi]

@kevloral thanks for the changes, just one more adjustment and I think its fine, beside that, can you please squash + rebase the changes into one commit so we can easier backport the changes?

[kevloral]

Sure. I will replace that Iterable with a Stream and squash the commits into one. By the way, it is already rebased on main even though my branch is still called kevloral:tycho-4.0.x.

[laeubi]

@kevloral I plan to do a bugfix release soon and your change seems important enough to include to you think you can finish it?

[kevloral]

@kevloral I plan to do a bugfix release soon and your change seems important enough to include to you think you can finish it?

Sorry for the delay. Other commitments got in the way. As you requested, I have replaced the Iterable with a Stream and squashed the commits into one. I guess it is ready to be merged now.

Thanks a lot for your help.

[laeubi]

@kevloral thanks for the testcase and bugfix!

[github-actions]

💚 All backports created successfully

Status	Branch	Result
✅	tycho-4.0.x

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details