Bug 200634 - make "Save Password" to keyring optional, similar to Team/CVS approach
Summary: make "Save Password" to keyring optional, similar to Team/CVS approach
Status: RESOLVED FIXED
Alias: None
Product: z_Archived
Classification: Eclipse Foundation
Component: Mylyn (show other bugs)
Version: unspecified   Edit
Hardware: All All
: P4 enhancement (vote)
Target Milestone: 2.2   Edit
Assignee: Frank Becker CLA
QA Contact:
URL:
Whiteboard:
Keywords: helpwanted
Depends on: 207521 207527 207531 207654 210483
Blocks:
  Show dependency tree
 
Reported: 2007-08-20 23:15 EDT by Antony Saba CLA
Modified: 2008-08-05 05:14 EDT (History)
6 users (show)

See Also:


Attachments
mylyn/context/zip (1.44 KB, application/octet-stream)
2007-09-11 00:35 EDT, Mik Kersten CLA
no flags Details
Patch (16.24 KB, patch)
2007-09-15 16:00 EDT, Frank Becker CLA
no flags Details | Diff
mylyn/context/zip (27.22 KB, application/octet-stream)
2007-09-15 16:00 EDT, Frank Becker CLA
no flags Details
corrected Patch (16.34 KB, patch)
2007-09-19 23:48 EDT, Frank Becker CLA
no flags Details | Diff
mylyn/context/zip (28.18 KB, application/octet-stream)
2007-09-19 23:48 EDT, Frank Becker CLA
no flags Details
mylyn/context/zip (29.05 KB, application/octet-stream)
2007-10-11 18:19 EDT, Robert Elves CLA
no flags Details
New Dialog (46.24 KB, patch)
2007-10-12 17:38 EDT, Frank Becker CLA
no flags Details | Diff
mylyn/context/zip (35.81 KB, application/octet-stream)
2007-10-12 17:38 EDT, Frank Becker CLA
no flags Details
new gif for org.eclipse.mylyn.tasks.ui/icons/wizban (1.47 KB, image/gif)
2007-10-12 17:40 EDT, Frank Becker CLA
no flags Details
New Dialog and new JUnit Test (46.49 KB, patch)
2007-10-13 17:59 EDT, Frank Becker CLA
no flags Details | Diff
mylyn/context/zip (41.96 KB, application/octet-stream)
2007-10-13 17:59 EDT, Frank Becker CLA
no flags Details
New Patch (40.95 KB, patch)
2007-10-20 17:44 EDT, Frank Becker CLA
no flags Details | Diff
mylyn/context/zip (45.19 KB, application/octet-stream)
2007-10-20 17:44 EDT, Frank Becker CLA
no flags Details
new version of new patch (54.38 KB, patch)
2007-10-21 15:59 EDT, Frank Becker CLA
no flags Details | Diff
mylyn/context/zip (46.54 KB, application/octet-stream)
2007-10-21 16:01 EDT, Frank Becker CLA
no flags Details
screenshot of cvs password dialog (23.78 KB, image/png)
2007-10-24 02:54 EDT, Steffen Pingel CLA
no flags Details
dialog to edit credentials (7.41 KB, patch)
2007-10-25 18:38 EDT, Steffen Pingel CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antony Saba CLA 2007-08-20 23:15:55 EDT
Mylyn should allow users to choose whether or not to save credentials to the keyring and only store them in memory if they choose not to.  Users should be prompted for credentials when necessary, similarly to the CVS team provider.

Since most of the credential storing/loading appears to be handled in the Mylyn core and ui, it would probably affect many of the connectors.

I have observed the following behavior in the Trac connector:
 * Set up working repository connection with authenticated access.
 * Create a query
 * Close Eclipse.
 * Delete the keyring file.
 * Restart Eclipse.
 * The repository configuration dialog is shown when trying to refresh the query.

Other operations result in a simple error dialog describing the lack of credentials.
Comment 1 Mik Kersten CLA 2007-08-24 00:21:12 EDT
Antony: this sounds like a reasonable request to me and I'm going to mark this for 3.0 because we should be following the convention of making the password optional as with CVS.  I do have some concern with the fact that Mylyn generally synchronizes more often than CVS, but we do have a way of turning background synch off and now have improved support for putting repositories offline.  Marking helpwanted since a patch could get this into a release early on in the 3.0 cycle.
Comment 2 Frank Becker CLA 2007-09-03 16:52:31 EDT
I start to look at this.

What I have done:

TaskRepository.java
1) Add cashing support for password.
2) Add property for state of keyring usage.

AbstractRepositorySettingsPage.java
1) Add Button for use keyring.

Help needed for:

1) In which package I should locate the new Dialog.
2) Is there a simple Example as a strating point of how to do an dialog.
3) How do I get from package org.eclipse.mylyn.tasks.core open the dialog

Is it OK that I continue to work on this?
Comment 3 Eugene Kuleshov CLA 2007-09-04 00:19:27 EDT
Hmm. I've got an impression from an original comment that if password is not saved it will be re-requested every time. Otherwise what is the reason not to save it into the keyring, which is a standard Platform mechanism for dealing with this kind of data.

For a password dialog you can look at org.eclipse.team.internal.ccvs.ui.UserValidationDialog in C:\eclipse\eclipse-3.3RC4\plugins\org.eclipse.team.cvs.ui plugin.
Comment 4 Frank Becker CLA 2007-09-04 02:08:54 EDT
 (In reply to comment #3)
> Hmm. I've got an impression from an original comment that if password is not
> saved it will be re-requested every time. Otherwise what is the reason not to
> save it into the keyring, which is a standard Platform mechanism for dealing
> with this kind of data.
Actually only the Username is cached. When you cache the password you have a way to extend it to not store in keyring and open an Dialog with userid and password and only update the cached attributes.

Do you think that this is the right way? 

Or should I stop with my work on this?

> 
> For a password dialog you can look at
> org.eclipse.team.internal.ccvs.ui.UserValidationDialog in
> C:\eclipse\eclipse-3.3RC4\plugins\org.eclipse.team.cvs.ui plugin.
Comment 5 Eugene Kuleshov CLA 2007-09-04 10:18:56 EDT
(In reply to comment #4)
> Actually only the Username is cached. When you cache the password you have a way
> to extend it to not store in keyring and open an Dialog with userid and password
> and only update the cached attributes.
> Do you think that this is the right way?

My concern is about keeping passwords in memory outside of Eclipse's keyring API. From your previous comment it sounded like you wanted to do that and I asked for clarification why it is actually needed, because if it is a common issue maybe it should be addressed by the Platform. That is all.

> Or should I stop with my work on this?

Not at all!
Comment 6 Frank Becker CLA 2007-09-04 14:47:47 EDT
 (In reply to comment #5)
> (In reply to comment #4)
> > Actually only the Username is cached. When you cache the password you have a
> way
> > to extend it to not store in keyring and open an Dialog with userid and
> password
> > and only update the cached attributes.
> > Do you think that this is the right way?
> 
> My concern is about keeping passwords in memory outside of Eclipse's keyring
> API. From your previous comment it sounded like you wanted to do that and I
> asked for clarification why it is actually needed, because if it is a common
> issue maybe it should be addressed by the Platform. That is all.
You are right, caching the password is not the perfect way. But when I read the description and comment #1 I think that this is what should be done.

What should I do? Continue with patch or do we say caching a password is the wrong way.

> 
> > Or should I stop with my work on this?
> 
> Not at all!
Comment 7 Eugene Kuleshov CLA 2007-09-04 17:33:21 EDT
(In reply to comment #6)
> You are right, caching the password is not the perfect way. But when I read the
> description and comment #1 I think that this is what should be done.

Can you please answer my question, why do you need to cache password but NOT save them to the Platform keyring? How do you address the same issue with passwords saved by other Eclipse plugins, including global proxy settings and/or CVS passwords?
Comment 8 Frank Becker CLA 2007-09-04 18:03:15 EDT
 (In reply to comment #7)
> (In reply to comment #6)
> > You are right, caching the password is not the perfect way. But when I read
> the
> > description and comment #1 I think that this is what should be done.
> 
> Can you please answer my question, why do you need to cache password but NOT
> save them to the Platform keyring? How do you address the same issue with
> passwords saved by other Eclipse plugins, including global proxy settings and/or
> CVS passwords?

Sorry I can not give you an answer, please ask the reporter of this bug. 
I only want to help out with create a patch.
Comment 9 Antony Saba CLA 2007-09-05 01:26:47 EDT
(In reply to comment #7)
> Can you please answer my question, why do you need to cache password but NOT
> save them to the Platform keyring?
Caching a password to disk without strong encryption has been unacceptable in many environments I have worked in.  AFAIK, the keyring mechanism isn't much better than obfustication, but I can't find any documentation of what it actually does.

The basic use case which follows the conventions of the CVS Team provider (for an SSH connection) is this:
 * A user initiates an action which requires a connection to repository.
 * Credentials are loaded from the keyring if available.
 * If credentials are not available or are not accepted by the server, the user is prompted for user name and password.  The "Save Password" check box means "save between Eclipse invocations"; I assume in the keyring.
 * Once valid credentials are provided, they are cached in memory and reused until Eclipse is closed.  Someone may have to ask Platform how this is actually done.
 * If the user cancels out of the dialog, the action is canceled.  The dialog appears again the next time a repository action is initiated.

This is more complicated for Mylyn since, as mentioned in comment #2, Mylyn does some background synchronization w/out the user specifically asking for it to happen.  It is also likely that some background tasks, like updating queries, may run just fine with anonymous access with some task repository configurations.

> How do you address the same issue with
> passwords saved by other Eclipse plugins, including global proxy settings and/or
> CVS passwords?\
Do you mean as a user or as an Eclipse developer?  As a user, like team providers as described above where supported, otherwise avoided.
Comment 10 Mik Kersten CLA 2007-09-11 00:35:16 EDT
It sounds like Frank's original description was pretty close to Antony's requirements and that caching in memory for the duration of the Eclipse session is the right approach.  In other words, this solution will prevent all the problems with storing passwords in an insecure format on the filesystem, but will not prevent evil plug-ins doing bad things with passwords.  But then avoiding in-memory caching is not sufficient to stop evil plug-ins either.

Antony: we briefly looked into Eclipse's keyring storage a while back and if I recall correctly it boils down to very weak obfuscaction (Shawn can correct me if I'm wrong).  Please review the following and post if anything stated here won't meet your needs.

Frank: fantastic that you're willing to contribute this (along with your other very useful patches).  Iterating on your design this will require:
1) A Save Password check box on the Task Repository Properties page and corresponding boolean on TaskRepository
1) A field, probably on TaskRepository, for caching a password that is not stored in the keyring.  Not that we already have TaskRepository.cachedUserName which is similar, but there for performance reasons.

Regarding your questions:
> 1) In which package I should locate the new Dialog.

org.eclipse.mylyn.internal.tasks.ui

> 2) Is there a simple Example as a strating point of how to do an dialog.

The Team dialog that Eugene points out can be a good starting point.  You could also try extending or copying MessageDialog to meet your needs.

> 3) How do I get from package org.eclipse.mylyn.tasks.core open the dialog

You can't.  The right solution to this would be to test whether the repository stores it's password right before doing the synch.  

As part of the patch please provide unit test coverage that verifies the behavior of an attempted synchronization with the credentials saved and without.  Your dialog class may need an additional entry point so that the unit test can dismiss it.
Comment 11 Mik Kersten CLA 2007-09-11 00:35:22 EDT
Created attachment 78041 [details]
mylyn/context/zip
Comment 12 Eugene Kuleshov CLA 2007-09-11 00:42:05 EDT
Mik, don't you think this is a rather common issue which should be addressed at the platform level? Also note that keeping raw passwords in memory is nowhere more secure then saving them to disk.
Comment 13 Mik Kersten CLA 2007-09-11 00:54:20 EDT
Eugene: No, Mylyn should address this at the same level as Team/CVS did, as suggested by the Description.  Yes, it would be nice if Platform provided additional security/storage mechanisms, but that can be another bug and is not blocking this one.  Yes, this approach does provide a security benefit because a machine that does not have a compromised Eclipse installation will not be able to be used to steal passwords if Eclipse is not running with passwords entered.
Comment 14 Eugene Kuleshov CLA 2007-09-11 08:56:14 EDT
I wasn't aware if Team/CVS addressed that. In my observation if you choose not to save password, it will be asked every time CVS is accessed.
Comment 15 Frank Becker CLA 2007-09-12 17:34:05 EDT
 (In reply to comment #10)
> It sounds like Frank's original description was pretty close to Antony's
> requirements and that caching in memory for the duration of the Eclipse session
> is the right approach.  In other words, this solution will prevent all the
> problems with storing passwords in an insecure format on the filesystem, but
> will not prevent evil plug-ins doing bad things with passwords.  But then
> avoiding in-memory caching is not sufficient to stop evil plug-ins either.
> 
> Antony: we briefly looked into Eclipse's keyring storage a while back and if I
> recall correctly it boils down to very weak obfuscaction (Shawn can correct me
> if I'm wrong).  Please review the following and post if anything stated here
> won't meet your needs.
> 
> Frank: fantastic that you're willing to contribute this (along with your other
> very useful patches).  Iterating on your design this will require:
> 1) A Save Password check box on the Task Repository Properties page and
> corresponding boolean on TaskRepository
> 1) A field, probably on TaskRepository, for caching a password that is not
> stored in the keyring.  Not that we already have TaskRepository.cachedUserName
> which is similar, but there for performance reasons.
> 

Starting with this.

> Regarding your questions:
> > 1) In which package I should locate the new Dialog.
> 
> org.eclipse.mylyn.internal.tasks.ui
> 
> > 2) Is there a simple Example as a strating point of how to do an dialog.
> 
> The Team dialog that Eugene points out can be a good starting point.  You could
> also try extending or copying MessageDialog to meet your needs.
> 
> > 3) How do I get from package org.eclipse.mylyn.tasks.core open the dialog
> 
> You can't.  The right solution to this would be to test whether the repository
> stores it's password right before doing the synch.

What is the right class for doing this? I need a way to get the TaskRepository

> 
> As part of the patch please provide unit test coverage that verifies the
> behavior of an attempted synchronization with the credentials saved and without.
> Your dialog class may need an additional entry point so that the unit test can
> dismiss it.
Comment 16 Frank Becker CLA 2007-09-15 16:00:40 EDT
Created attachment 78503 [details]
Patch

Here my patch, please verify it.

I don't know if is it OK that I implement it this way. 

What is if the sync is not  first and we get a call of TaskRepository.getPassword() before?

What kind of test should I implement? 

Do I have to duplicate every test with getPassword()?
Comment 17 Frank Becker CLA 2007-09-15 16:00:52 EDT
Created attachment 78504 [details]
mylyn/context/zip
Comment 18 Frank Becker CLA 2007-09-15 16:02:21 EDT
What is with 

org.eclipse.mylyn.internal.bugzilla.core.BugzillaClientManager
org.eclipse.mylyn.bugzilla.tests.BugzillaRepositoryConnectorTest
org.eclipse.mylyn.bugzilla.tests.RepositoryEditorWizardTest
org.eclipse.mylyn.internal.jira.ui.wizards.JiraRepositorySettingsPage
org.eclipse.mylyn.internal.jira.ui.JiraClientFacade
org.eclipse.mylyn.tasks.core.TaskRepository
org.eclipse.mylyn.tasks.core.TaskRepositoryManager
org.eclipse.mylyn.tasks.tests.TaskRepositoryManagerTest
org.eclipse.mylyn.internal.trac.core.TracClientManager
org.eclipse.mylyn.internal.trac.ui.wizard.TracRepositorySettingsPage
org.eclipse.mylyn.internal.web.tasks.WebRepositoryConnector
org.eclipse.mylyn.tasks.ui.wizards.AbstractRepositorySettingsPage
org.eclipse.mylyn.tasks.ui.TasksUiPlugin

all have a call of getPassword()
Comment 19 Frank Becker CLA 2007-09-15 16:05:06 EDT
Hi mik,

if you have time to check this before rob is back please do so.

If not it is not a problem for me.
Comment 20 Frank Becker CLA 2007-09-19 23:48:50 EDT
Created attachment 78816 [details]
corrected Patch

I corrected some problems with my previous patch.

1) TaskUIPlugin testAndSetMemoryOnlyPassword was not included in the patch
2) AbstractRepositorySettingsPage reateControl only works for existing Repositories (if (repository != null) { test missed)
Comment 21 Frank Becker CLA 2007-09-19 23:48:56 EDT
Created attachment 78817 [details]
mylyn/context/zip
Comment 22 Mik Kersten CLA 2007-09-20 15:35:41 EDT
Frank: thanks for the patch!  Since we're trying to minimize any potentially disruptive changes for next week's 2.1 release I'd like this to wait until 2.0 (and Rob's return).  

Rob: please review once 2.1 is out.
Comment 23 Eugene Kuleshov CLA 2007-09-21 02:55:50 EDT
Can someone please confirm that observation from comment 14 is correct?
Comment 24 Antony Saba CLA 2007-09-22 01:18:19 EDT
(In reply to comment #23)
> Can someone please confirm that observation from comment 14 is correct?
> 

I think the docs describe this, since they only refer to having to cache passwords between Eclipse sessions, not individual operations (also, please note the warning about "if you have strict security requirements):
http://help.eclipse.org/help33/topic/org.eclipse.platform.doc.user/tasks/tasks-cvs-passwords.htm

If one can assume that this is cached in memory once it is created, than it certainly behaves as described:
http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.team.cvs.core/src/org/eclipse/team/internal/ccvs/core/connection/CVSRepositoryLocation.java?revision=1.100&view=markup

Subclipse, when using native JavaHL, on the other hand, will prompt on every action that requires a repository connection, since the password caching is handled by the subversion libraries invoked through JNI.  Maybe you are thinking of that?
Comment 25 Mik Kersten CLA 2007-09-24 00:08:41 EDT
Antony: does keeping passwords in memory during the duration of the session provides sufficient security for you?
Comment 26 Antony Saba CLA 2007-10-03 15:49:52 EDT
(In reply to comment #25)
> Antony: does keeping passwords in memory during the duration of the session
> provides sufficient security for you?
> 

Yes.  Thank you.
Comment 27 Robert Elves CLA 2007-10-05 01:06:20 EDT
Might get a chance to look at this further over the weekend Frank. As is, if there are multiple memory only repositories, what happens when a synch happens in terms of dialogs? Is the user presented with a barrage of password dialogs (haven't actually tested just glanced at the patch).  If this is the case, should we perhaps test for password with the repository has its turn?
Comment 28 Steffen Pingel CLA 2007-10-05 15:47:38 EDT
I have taken a brief look at the patch and have some suggestions for further improvement:

- I would prefer if the UI was consistent with CVS/Team and labeled "Save password" 
- The EnterMemoryPassordDialog could be more generic and also allow the specification of a username and the ability to save the password. The dialog used by the CVS plug-in is a good example.
- Password prompting should be on demand, either when no password was saved or when the supplied credentials are invalid. Support for this needs to be implemented on a connector level but the tasks framework should provide the necessary UI.

The patch is a good start but it will require some more work to get the user interaction right (e.g. see Rob's comment #c27) and define the API to allow a connector to callback to the framework to request a password.
Comment 29 Frank Becker CLA 2007-10-09 10:23:20 EDT
 (In reply to comment #27)
> Might get a chance to look at this further over the weekend Frank. As is, if
> there are multiple memory only repositories, what happens when a synch happens
> in terms of dialogs? Is the user presented with a barrage of password dialogs
> (haven't actually tested just glanced at the patch).  If this is the case,
> should we perhaps test for password with the repository has its turn?
Yes for every repository with a memory only password one dialog get created. But how many repositories do you have?
For me it looks like most installations will have < 5 repositories so you will get not so many dialogs.

I think It is possible to design one dialog with all password requests if you think it is needed (ScheduledTaskListSynchJob loops over all repositories).


 (In reply to comment #28)
> I have taken a brief look at the patch and have some suggestions for further
> improvement:
> 
> - I would prefer if the UI was consistent with CVS/Team and labeled "Save
> password"
> - The EnterMemoryPassordDialog could be more generic and also allow the
> specification of a username and the ability to save the password. The dialog
> used by the CVS plug-in is a good example.

What think other? I want to start with a simple implementation. If username and change the password settings to non memory only password is needed I implement this. (wait until rob give me feedback)

> - Password prompting should be on demand, either when no password was saved or
> when the supplied credentials are invalid. Support for this needs to be
> implemented on a connector level but the tasks framework should provide the
> necessary UI.
> 
> The patch is a good start but it will require some more work to get the user
> interaction right (e.g. see Rob's comment #c27) and define the API to allow a
> connector to callback to the framework to request a password.
Comment 30 Mik Kersten CLA 2007-10-09 11:20:10 EDT
(In reply to comment #29)
> For me it looks like most installations will have < 5 repositories so you will
> get not so many dialogs.

I think that the common case is that users have <=2 repositories (one for their work, one for Eclipse Bugzilla).  That said we should gracefully handle up to 10.  I think that the CVS model of popping up a single dialog each time that the repository tries to synchronize is sufficient, as long as it happens sequentially as Rob points out in comment#27.

> What think other? I want to start with a simple implementation. If username and
> change the password settings to non memory only password is needed I implement
> this. (wait until rob give me feedback)

I'll let Rob comment on the framework implications since he is looking over the patch.  However, as Steffen points out we do need to ensure that the interaction is smooth for both users and connectors.  For this sort of patch it is very important for us to have unit test coverage for cases like the following:
* Mock connector tries to synch and no in-memory password is provided (verify that dialog pops up)
* Tries to synch and in-memory password is provided (no dialog)
* Above cases but where the password is changed between synchronizations
* Some of the above cases with the standard password

It can be a considerable amount of work to write these tests, but they are hugely valuable to the project because they help ensure that contributions of this sort remain robust as the code changes.  I've written some tips on running mock tests of these sort here: http://wiki.eclipse.org/index.php/Mylyn_Contributor_Reference#Writing_Tests
Comment 31 Robert Elves CLA 2007-10-11 16:15:04 EDT
So to outline what I understand to be the remaining items:

* the user needs to be prompted (for both id and username) for each request/repository independently  (Frank)
** If we can avoid the changes necessary to implement on demand prompting this iteration we should push to a new bug report

* basic unit test coverage (Frank)

* minor clean up of ui messages (Rob)

Frank, does this look right to you?
Comment 32 Frank Becker CLA 2007-10-11 17:01:47 EDT
 (In reply to comment #31)
> 
> So to outline what I understand to be the remaining items:
> 
> * the user needs to be prompted (for both id and username) for each
> request/repository independently  (Frank)

OK, actually I did this only for the password and use the user id form the  task repository. Is it right that I should also cache the UserID in memory.   

> ** If we can avoid the changes necessary to implement on demand prompting this
> iteration we should push to a new bug report
> 
> * basic unit test coverage (Frank)

Here I need some help. I need to know if there are other tests which now must also implement with the memory password?

> 
> * minor clean up of ui messages (Rob)

Yes I think that is OK.
> 
> Frank, does this look right to you?
Comment 33 Robert Elves CLA 2007-10-11 18:19:43 EDT
 (In reply to comment #32)
> > * basic unit test coverage (Frank)
> 
> Here I need some help. I need to know if there are other tests which now must
> also implement with the memory password?

All the current tests assume a username and password. The BugzillaRepositoryConnectorTest uses repositories initialized in the parent AbstractBugzillaTest class. You could add to BugzillaRepositoryConnector test or consider creating a new test class MemoryPasswordTest and test with the MockRepositoryConnector as Mik suggests.
Comment 34 Robert Elves CLA 2007-10-11 18:19:48 EDT
Created attachment 80187 [details]
mylyn/context/zip
Comment 35 Frank Becker CLA 2007-10-12 17:38:09 EDT
Created attachment 80288 [details]
New Dialog

The new Dialog based on org.eclipse.jsch.internal.ui.UserValidationDialog
Comment 36 Frank Becker CLA 2007-10-12 17:38:15 EDT
Created attachment 80289 [details]
mylyn/context/zip
Comment 37 Frank Becker CLA 2007-10-12 17:40:50 EDT
Created attachment 80290 [details]
new gif for org.eclipse.mylyn.tasks.ui/icons/wizban

new gif keylock.gif
Comment 38 Frank Becker CLA 2007-10-13 17:59:33 EDT
Created attachment 80305 [details]
New Dialog and new JUnit Test

Replacment of Attachment #7 and #8 of this bug.

What is left is from comment #28

> - Password prompting should be on demand, either when no password was saved or
> when the supplied credentials are invalid. Support for this needs to be
> implemented on a connector level but the tasks framework should provide the
> necessary UI.

Should this meen that there is a new Method in Class TasksUiPlugin which is called from the connector level to bring up the Dialog to set the credentials
Comment 39 Frank Becker CLA 2007-10-13 17:59:54 EDT
Created attachment 80306 [details]
mylyn/context/zip
Comment 40 Steffen Pingel CLA 2007-10-17 16:03:13 EDT
 (In reply to comment #38)
> > - Password prompting should be on demand, either when no password was saved or
> > when the supplied credentials are invalid. Support for this needs to be
> > implemented on a connector level but the tasks framework should provide the
> > necessary UI.
> 
> Should this meen that there is a new Method in Class TasksUiPlugin which is
> called from the connector level to bring up the Dialog to set the credentials

Yes, something like that. One idea I had was to pass an interface to all methods that do I/O that extends IProgressMonitor which already provides facilities for user interaction. That way it might even be possible to do this in an API backwards compatible way.

Frank, please make sure your patch does not change existing API in a non-binary compatible way (e.g. please overload TaskRepository.setAuthenticationCredentials() instead of changing its signature),

One thing that also needs to be considered is that Mylyn supports authentication for three different services:

- repository authentication
- http authentication
- proxy authentication

Comment 41 Frank Becker CLA 2007-10-20 17:44:47 EDT
Created attachment 80829 [details]
New Patch

Redesign now use AbstractRepositorySettingsPage for ask for the  memoryonly password

TODO:

-  updateProperties(TaskRepository repository) in JiraRepositorySettingsPage and WebRepositorySettingsPage need to test if the used Controls are created if EditCredentialsWizard is used.

Question:

- Is TasksUiPlugin.credentialsWizards in a form that it can used in other places
Comment 42 Frank Becker CLA 2007-10-20 17:44:50 EDT
Created attachment 80830 [details]
mylyn/context/zip
Comment 43 Frank Becker CLA 2007-10-21 15:59:57 EDT
Created attachment 80840 [details]
new version of new patch

(In reply to comment #41)
> 
> TODO:
> 
> -  updateProperties(TaskRepository repository) in JiraRepositorySettingsPage and
> WebRepositorySettingsPage need to test if the used Controls are created if
> EditCredentialsWizard is used.

Included in this patch.

> 
> Question:
> 
> - Is TasksUiPlugin.credentialsWizards in a form that it can used in other places
Comment 44 Frank Becker CLA 2007-10-21 16:01:01 EDT
Created attachment 80841 [details]
mylyn/context/zip
Comment 45 Steffen Pingel CLA 2007-10-23 16:10:31 EDT
Thanks Frank! I am going to take a look at the patch and investigate how to implement the password prompting on demand.
Comment 46 Steffen Pingel CLA 2007-10-24 02:54:46 EDT
Created attachment 81025 [details]
screenshot of cvs password dialog
Comment 47 Frank Becker CLA 2007-10-24 15:34:23 EDT
(In reply to comment #46)
> Created an attachment (id=81025) [details]
> screenshot of cvs password dialog
> 
This was the Implementation of comment#38 withattachment (id=80305).


(In reply to comment #40)
> 
> One thing that also needs to be considered is that Mylyn supports
> authentication for three different services:
> 
> - repository authentication
> - http authentication
> - proxy authentication
> 

The dialog with only  userid and password results in up to 3 time opening that dialog for one repository.

For me that sound as the not wanted way, so I think we need one dialog with all 3 userid and passwords. But for proxy we need Host address and host port to.

So I think reuse of the AbstractRepositorySettingsPage with all the needed information and operation for save the changes is the right way.

Was that the wrong way?
Comment 48 Steffen Pingel CLA 2007-10-24 16:29:45 EDT
 (In reply to comment #47)
> > Created an attachment (id=81025) [details]
> > screenshot of cvs password dialog
> >
> This was the Implementation of comment#38 withattachment (id=80305).

Thanks, I have used a mix of your patches as the base for my modifications.
 
> The dialog with only  userid and password results in up to 3 time opening that
> dialog for one repository.

This will only be the case if the repository requires all 3 types of authentication and these are haven been saved. I would assume that that is a unlikely case.

> For me that sound as the not wanted way, so I think we need one dialog with all
> 3 userid and passwords. But for proxy we need Host address and host port to.
> 
> So I think reuse of the AbstractRepositorySettingsPage with all the needed
> information and operation for save the changes is the right way.

Yes, that is certainly a good idea since we already have a working implementation there. My major concern here is backwards compatibility. We can not do any changes to the API or implementation that breaks binary compatibility or requires changes in connectors. 

My current approach is to start out with a simple password dialog that has link to the repository settings dialog if any of the advanced settings need to be changed.
Comment 49 Frank Becker CLA 2007-10-24 17:23:46 EDT
 (In reply to comment #48)
> 
> This will only be the case if the repository requires all 3 types of
> authentication and these are haven been saved. I would assume that that is a
> unlikely case.

But what is if the supplied credentials are invalid ( see comment#28). What was the reason
- wrong  password for repository, http auth or proxy config
- start of using proxy config or change of proxy host adress
- .... some other more

> 
> Yes, that is certainly a good idea since we already have a working
> implementation there. My major concern here is backwards compatibility. We can
> not do any changes to the API or implementation that breaks binary compatibility
> or requires changes in connectors.

I hoped that I found a way that was save. Can you tell me what change is danger.
> 
> My current approach is to start out with a simple password dialog that has link
> to the repository settings dialog if any of the advanced settings need to be
> changed.

Please tell me wher I should continue.
Comment 50 Steffen Pingel CLA 2007-10-24 18:42:45 EDT
 (In reply to comment #49)
> But what is if the supplied credentials are invalid ( see comment#28). What was
> the reason
> - wrong  password for repository, http auth or proxy config
> - start of using proxy config or change of proxy host adress
> - .... some other more

This needs to be handled on a connector level as this is repository specific. The Trac connector for instance will have to check the HTTP status codes returned by the server to determine which credentials to request from the user.

> > Yes, that is certainly a good idea since we already have a working
> > implementation there. My major concern here is backwards compatibility. We can
> > not do any changes to the API or implementation that breaks binary
> compatibility
> > or requires changes in connectors.
> 
> I hoped that I found a way that was save. Can you tell me what change is danger.

Your patch contained  changes to the settings pages of the Trac, Jira and Bugzilla connectors. Consider a connector distributed by a 3rd party or an old version of the Trac connector. It would break with new releases of Mylyn. We won't be able to make these type of changes before 3.0.

> > My current approach is to start out with a simple password dialog that has
> link
> > to the repository settings dialog if any of the advanced settings need to be
> > changed.
> 
> Please tell me wher I should continue.

I am currently in the middle of a larger refactoring that will hopefully improve the user interaction when communicating with the repository and provide a more extensible API. I'll have a patch by next week that you can take a look at.
Comment 51 Frank Becker CLA 2007-10-24 23:52:09 EDT
 (In reply to comment #50)
> 
> This needs to be handled on a connector level as this is repository specific.
> The Trac connector for instance will have to check the HTTP status codes
> returned by the server to determine which credentials to request from the user.

Ok my fault. At home I nevver set this so I don't see that by looking at the responce code.

> Your patch contained  changes to the settings pages of the Trac, Jira and
> Bugzilla connectors. Consider a connector distributed by a 3rd party or an old
> version of the Trac connector. It would break with new releases of Mylyn. We
> won't be able to make these type of changes before 3.0.
> 

Now I know what the problem is and can look at a other way to get around the  bug that 
public void updateProperties(TaskRepository repository) did not check if the needed controls
are created (NullPointerException). 

> 
> I am currently in the middle of a larger refactoring that will hopefully improve
> the user interaction when communicating with the repository and provide a more
> extensible API. I'll have a patch by next week that you can take a look at.

I will wait until I get a note that the patch is there and then I start working on this again.
Comment 52 Steffen Pingel CLA 2007-10-25 18:38:41 EDT
Created attachment 81212 [details]
dialog to edit credentials
Comment 53 Steffen Pingel CLA 2007-11-05 14:07:33 EST
Marking as resolved as all subtasks have been completed. 

Mylyn will now prompt for credentials if authentication fails for JIRA or Trac repositories.
Comment 54 Frank Becker CLA 2007-11-05 16:18:40 EST
 (In reply to comment #53)
> Marking as resolved as all subtasks have been completed.
> 
> Mylyn will now prompt for credentials if authentication fails for JIRA or Trac
> repositories.
What is needed to done for Bugzilla repositories?
Steffen is this your task or can you give me some information so that I can do this?
Comment 55 Robert Elves CLA 2007-11-05 20:55:58 EST
 (In reply to comment #54)
> What is needed to done for Bugzilla repositories?
> Steffen is this your task or can you give me some information so that I can do
> this?
This is going to be a significant (read 'really hard') refactoring so we would like to postpone this for a few weeks allowing any necessary api changes to emerge (and us to get caught up). That said, you are more than welcome to start iterating on the design/changes for this (on bug#208839). I'd recommend posting your design decisions on that report before investing too much time in code alterations. We will try to respond in a timely fashion. 
Comment 56 Diana CLA 2008-08-05 03:30:53 EDT
I am not getting a dialog asking for the password when I synchronize my task list with my Bugzilla repository.( if I chose not to save the password.)
 I expected it to be  like the CVS dialog; instead I get a message that it cannot connect to the repository server. Now I have to go to repository properties and enter the password every time I start eclipse. It is a bit annoying. 
Comment 57 Steffen Pingel CLA 2008-08-05 05:14:40 EDT
Automatic password prompting is currently only supported by the JIRA and Trac connector. I have opened  bug 243138 to discuss an implementation for Bugzilla.