Bugzilla – Full Text Bug Listing
| Summary: | [Proxy] org.eclipse.core.net bundle should not require org.eclipse.core.runtime | ||
|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | DJ Houghton <dj.houghton> |
| Component: | Team | Assignee: | Tomasz Zarna <tomasz.zarna> |
| Status: | VERIFIED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | CC: | jeffmcaffer, john.arthorne, ob1.eclipse, Szymon.Brandys |
| Version: | 3.4 | ||
| Target Milestone: | 3.4 M7 | ||
| Hardware: | PC | ||
| OS: | Windows XP | ||
| Whiteboard: | |||
| Bug Depends on: | 227112 | ||
| Bug Blocks: | 213381 | ||
| Attachments: | |||
DJ, I'm sorry I haven't take a look at the patch earlier. I would like to run some tests against the new net plug-in before releasing it. I will try to do this asap, but I'm afraid I won't make it for todays I-build. Tests located in org.eclipse.core.tests.net plug-in don't indicate any problems. Patch released to HEAD. Thanks DJ! Hi Tomasz, sorry but my changes for this bug weren't 100% correct. Is it possible to back out of this change for M5? My patch included references to internal classes which I thought were ok for now, but it turns out there are problems if the core.net bundle is activated before the core.runtime bundle. (see bug 218058) So my recommendation is to back out of this patch for M5, and then post-M5 move to the new security APIs. They will be graduated from the incubator to the regular repository in the next couple of weeks. Sorry for the inconvenience. Thanks. (In reply to comment #3) > Sorry for the inconvenience. Thanks. No problem, patch has been reverted. Szymon will send a request for a rebuild in a minute. Shouldn't we mark this bug as invalid now? Thanks Tomasz. Perhaps we should leave this open and mark as M6 as a reminder to switch to the new security APIs post-M5? what's that status on this? the latest core.net still required core.runtime. Still waiting for the new security APIs... The new security bundles are now in the build. We should work on releasing a combination of the previous patch plus using the new security APIs. Oleg, can you post a link here to point Tomasz to how to convert his code? Thanks. Oleg? I would be very happy to see the link DJ mentioned in order to have at least a chance to make it for M6. Created attachment 93623 [details]
Sample patch for the security part - straight line-by-line replacement
This is a simple patch that essentiually does line-by-line replacement of the Authorization engine with the new Secure storage.
While trying this patch I found that there are lots of exessive calls made - whle there seems to be only one place in UI to specify login/password it is saved separately for all 3 types (HTTP, HTTPS, SOCKS).
This information is loaded about 12 times on startup and saved about 6 times whenevr it is modified.
I'll attach another patch that does read this information in a somwehat lazy way moving user/password caching to the ProxyData.
Created attachment 93624 [details]
Sample patch for the security part - with lazy caching of login credentials
This is the approach that adds a degree of laziness to persisting login crdentials to proxies.
Note however that core.net code will still try to read it on the very start of the Workbench - possibly resulting in the password promt from the user. It seems that it does so to set Java system variables to this information (see ProxyType#updateHttpSystemProperties for example). Not sure what can be done to avoid this.
On testing:
- Please test with both SDK supplied as-is (on Windows platform your login credentials will be used, no password prompt)
- Delete secure storage (located in your home directory, such as C:\Documents and Settings\obesedin.000\secure_preferences.equinox)
- Test again with removed/disabled fragment org.eclipse.equinox.security.win32.x86 - this will use UI promt
(In reply to comment #11) > This is the approach that adds a degree of laziness to persisting login > crdentials to proxies. > > Note however that core.net code will still try to read it on the very start of > the Workbench - possibly resulting in the password promt from the user. It seems > that it does so to set Java system variables to this information (see > ProxyType#updateHttpSystemProperties for example). Not sure what can be done to > avoid this. btw, not setting Java system variables results in core.tests.net tests failures. At least what I get with the second patch applied. Created attachment 94562 [details]
Exception
An exception I was hit by when playing with proxy prefs.
Steps to reproduce:
1. Enable proxy authentication
2. Enter a username and a pass
3. Disable proxy auth (ie. uncheck the checkbox)
4. Click Apply/Ok
=> Take a look at your Error Log, the exception should be there.
The NPE is fixed in the bug 225623, thank you for finding it. I tried JUnits with the first patch, There were two problems in secure preferences: - It converted null values to empty string in put()/get() cicles - It was bringing up UI prompt for password recovery on the first run Those are fixed under bug 225843 (note that the following bundles were modified: org.eclipse.equinox.security org.eclipse.equinox.security.tests org.eclipse.equinox.security.ui For the second patch, the problem is two-fold as well. First, the ProxyData#disable() method needs to reset values in the secure preferences: public void disable() { ... setUserid(null); // instead of user = null; setPassword(null); // instead of password = null; ... authenticationLoaded = true; // this one I am not sure about - depends on where ProxyData could be reused after it was disabled() } The second part is rather wide-spread problem with the core.net tests - they routinely construct multiple ProxyData instances and used them to compare information at different stages of testing. Problem is, all of those instances are backed up by the same data (ISecurePreferences for the userID/password). For non-lazy patch it still works fine, but for the "lazy" patch the values are different from expected as they are populated when first requested. That is rather widespread pattern and would take a bit of effort to change. It might be more prudent to go with the "straight line-by-line" replacement patch for 3.4 given the time in the release cicle. Unless the userID/password is actually specified, performance impact on startup should be minimal; and if the userID/password are specified, they would have to be decoded on startup anyway to populate system properties. (The performance hit comes mostly from Java loading encryption providers and would be nice to avoid on startup, but that would need more modifications to the logic of proxies - use java.net.Authenticator? - and is probably not wise at this part of the release cicle.) (In reply to comment #15) > That is rather widespread pattern and would take a bit of effort to change. Are you talking here about changing the pattern itself or JUnits? > It might be more prudent to go with the "straight line-by-line" replacement > patch for 3.4 given the time in the release cicle. I like the "little steps" approach. Szymon, could you take a look at it, too? (In reply to comment #16) > (In reply to comment #15) > > That is rather widespread pattern and would take a bit of effort to change. > Are you talking here about changing the pattern itself or JUnits? JUnits. They use similar approach in a number of cases: - Create ProxyData [1] - Create another instance of ProxyData [2] - Run some actions on ProxyData[2] - check that ProxyData[1]'s values are as expected - check that ProxyData[2]'s values are as expected Which was ok for a non-lazy approach as the userID/password data was set in the ProxyData at the moment of creation. With lazy approach the data is populated from a secure storage at the time the query is run for the fist time and will be different if the value in the secure storage changed. For JUnits to work with lazy approach they would have to be modified: - Create ProxyData [1] -> Get userID/password values from ProxyData[1] - Run some actions on ProxyData[1] - check that ProxyData[1]'s values are as expected, comparing to previously retrieved userID/password or to the desired values I don't think that "lazy" initialization approach creates any problems in the actual code, at least not in the core.net bundle. However, it breaks JUnits as they assume that information is set at the time ProxyData was instantiated rather then at the time the assertEquals(ProxyData.get...()) are run. Are there any questions/concerns/problems, something I can help with? The time is running very close to the end of M7 (which is the last real iteration for Eclipse 3.4). Having in mind that this is a whole new functional block, I'd say it is starting to run uncomfortably close. Oleg, both patches actually don't fix the problem in the summary. They are just introducing the security storage to the proxy. I think that we can use the first patch. ProxyData are value objects and I would rather not introduce any logic or lazy initialization there. Especially as ProxyData objects are used to work with native proxy providers and those providers are responsible for password/user things. Created attachment 96057 [details]
Sample approach fix slightly modified
Since the value for the user or pass node can be null, I had to make this distinction and remove the node when the value is null. The patch from Szymon released to HEAD. I'm not sure how using security storage is related to this bug... however because it seems to be :-) I created another bug (bug 227112) that this one depends on. The released patch fixes the new bug. This one still waits for a right fix. Created attachment 96079 [details]
Remove dependency on core.runtime
With the security use released, the remaining issues with removing dependency on org.eclipse.core.runtime are trivial. Here is an updated patch based on DJ's initial patch that removes the remaining references to runtime. Part of the motivation for this is so we can use the proxy support in the installer without having the installer depend on org.eclipse.core.runtime.
Can this be fixed for next week's I-build? Yes. Created attachment 96575 [details]
John's patch tidied up
The latest patch from Szymon (John's patch tidied up) applied to CVS HEAD. Marking as FIXED. Verified by code inspection. |
Created attachment 86498 [details] patch The org.eclipse.core.net bundle currently does a require-bundle on org.eclipse.core.runtime. It should be refactored to only require things that it uses. Patch provided. Note that the patch currently references an internal class in the org.eclipse.core.runtime.compatibility.auth bundle. This code will be replaced with the new security work for 3.4 once it graduates from the equinox incubator. I have added TODOs in the code to remind us of this.