Bug 379787 - [tests] Fix secure storage usage in org.eclipse.rse.tests
Summary: [tests] Fix secure storage usage in org.eclipse.rse.tests
Status: RESOLVED FIXED
Alias: None
Product: Target Management
Classification: Tools
Component: RSE (show other bugs)
Version: 3.3.1   Edit
Hardware: PC Linux
: P1 major (vote)
Target Milestone: 3.4.2   Edit
Assignee: David Dykstal CLA
QA Contact: Martin Oberhuber CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 359719
  Show dependency tree
 
Reported: 2012-05-17 03:47 EDT by Anna Dushistova CLA
Modified: 2013-01-15 12:46 EST (History)
1 user (show)

See Also:
anna.dushistova: review+


Attachments
patch to add test "API" to prohibit access to secure storage (13.39 KB, patch)
2012-06-03 23:37 EDT, David Dykstal CLA
no flags Details | Diff
patch to avoid accessing secure store using a property (17.00 KB, patch)
2013-01-15 11:17 EST, David Dykstal CLA
ddykstal.eclipse: review+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anna Dushistova CLA 2012-05-17 03:47:59 EDT
When I run RSE tests on my localhost, it asks me about the secure
storage password.
Now I deleted the .eclipse directory, but it still does the same. If I
do not provide a password,
tests cannot proceed and that leads to the build hangout. I suspect this causes hangout of the hudson job as well. Currently junit tests are disabled.
Comment 1 Martin Oberhuber CLA 2012-05-30 08:24:55 EDT
Is this really still open, and what is missing ?
Comment 2 David Dykstal CLA 2012-05-30 08:45:34 EDT
The problem is that the secure storage support in the base is asking for the password to unlock the store making this incapable of running as part of a build. We can disable this particular test for the build (easiest) or put a preference in that disables access to secure storage in any way so that the tests run as part of the build.
Comment 3 David Dykstal CLA 2012-06-03 23:37:19 EDT
Created attachment 216745 [details]
patch to add test "API" to prohibit access to secure storage

The patch adds test methods to the PasswordPersistenceManager that will cause it to not access secure preferences. The PasswordsTest class has also been modified to be sensitive to this setting.
Comment 4 David Dykstal CLA 2012-06-03 23:40:02 EDT
Martin and Anna - please review the patch.

Anna - you will need to use the API as part of the test setup so that secure storage isn't accessed during tests. I'm not sure how you want to handle this.
Comment 5 Anna Dushistova CLA 2012-06-04 13:12:15 EDT
Dave,
should I set up some preference to disable secure storage?
Which one exactly?
Comment 6 David Dykstal CLA 2012-06-04 13:41:48 EDT
Anna --

I added a test-only "API" method to turn it off.

PasswordPersistenceManager ppm = PasswordPersistenceManager.getInstance();
ppm.disableSecureStorageAccess();

If you want to use a preference or a defines you can implement that and then invoke this test-only API.
Comment 7 David Dykstal CLA 2012-06-04 13:42:47 EDT
I believe I need two signoffs at this stage before integrating, correct?
Comment 8 Anna Dushistova CLA 2012-06-04 15:57:28 EDT
(In reply to comment #6)
> Anna --
> 
> I added a test-only "API" method to turn it off.
> 
> PasswordPersistenceManager ppm = PasswordPersistenceManager.getInstance();
> ppm.disableSecureStorageAccess();
> 
> If you want to use a preference or a defines you can implement that and then
> invoke this test-only API.

I would probably need it in the rse.tests Activator#start method unless you can suggest some other way to disable it for headless builds.
Comment 9 Anna Dushistova CLA 2012-06-04 15:59:43 EDT
I think so.
(In reply to comment #7)
> I believe I need two signoffs at this stage before integrating, correct?
Comment 10 Martin Oberhuber CLA 2012-06-05 04:27:37 EDT
I'm afraid it's too late for this kind of changes.

I'm not in favor of adding API just for tests (if this is just about a toggle, a System Property would also do or maybe "internal" non-API). Plus, I don't quite see how you intend logging in to a remote system during tests.

The last scheduled build was yesterday. We can think about putting a solution without using public API into 3.4.1.
Comment 11 David Dykstal CLA 2012-06-05 10:04:54 EDT
The public methods added are not API. They are tagged as "@noreference" and the javadoc explicitly states they are not API.

I'm not a big fan of defining new system properties because of the potential for misuse and conflict. Documenting them is outside the usual scope of javadoc. They are hard to track and hard to discover. This is a personal bias and I can be persuaded otherwise if it convenient.

That said, I agree it is probably too late to put this in. So we can discuss how best to implement the switch for testing in this bugzilla.

Anna -- from your perspective is a system property easier to incorporate into the test regimen? If so, I'll remove the methods I added and define an appropriate property.
Comment 12 Anna Dushistova CLA 2012-06-05 13:36:31 EDT
(In reply to comment #11)
> 
> Anna -- from your perspective is a system property easier to incorporate into
> the test regimen? If so, I'll remove the methods I added and define an
> appropriate property.

I think it's about the same difficulty as disabling in Activator#start.
Comment 13 Martin Oberhuber CLA 2013-01-11 02:56:01 EST
Has this been committed into TM 3.5 yet ?
Comment 14 David Dykstal CLA 2013-01-14 08:57:06 EST
This is will integrated into 3.4.2 as a property change instead of the original "test API" change. Since this is still the master branch it will make it into 3.5 as well.
Comment 15 David Dykstal CLA 2013-01-14 08:57:43 EST
Resetting the review counters.
Comment 16 David Dykstal CLA 2013-01-15 10:36:11 EST
I will be attaching a patch for review later today. Anna are you available for review?
Comment 17 David Dykstal CLA 2013-01-15 11:17:52 EST
Created attachment 225643 [details]
patch to avoid accessing secure store using a property

The patch looks for the "rse.enableSecureStoreAccess" property. If absent it defaults to true. If present then the value must be true to enable access to the secure store.

To disable access to the secure store during JUnit testing you would do the following:
System.setProperty("rse.enableSecureStoreAccess", "false");
Comment 18 David Dykstal CLA 2013-01-15 12:46:17 EST
change reviewed and committed