Closed
Bug 84834
Opened 23 years ago
Closed 23 years ago
We need an API that returns the password given the host and/or user name
Categories
(SeaMonkey :: Passwords & Permissions, defect, P1)
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla0.9.2
People
(Reporter: racham, Assigned: morse)
References
Details
(Whiteboard: [pdt+])
Attachments
(4 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
Today, a password stored in the password manager is obtained when when we go
through routines like PromptPassword ( & GetPasswordwithUI()), which involve UI
elements to be passed in as input params. This arrangement is not convenient for
some other tasks where password is needed (if exists) without having to go
through UI tied up routines.
For example, in mailnews, we have a feature (bug 80296) where one can get
messages for multiple accounts by clicking 'Get All New Messages' on GetMsg
button dropmarker menupopup. What we do there is to get the server list and
query each server for the password. If the password value exists (say stored in
the password mgr in one of the earlier sessions), then that account is
authenticated. So, we can safely (wihtout having to worrying about password
popup) call getmessages on that server and get messages. Today, we are getting
null values back for some of the servers who passwords are already stored. Hence
the feature doesn't work as expected. So, the routine GetPassword() in
nsMsgIncomingServer has to be smarter than it is right now in returning the
password and we need Password Mgr's help to do that.
All GetPassword() does today is to make a copy of m_password and return the
same.
http://lxr.mozilla.org/seamonkey/source/mailnews/base/util/nsMsgIncomingServer.cpp#661
Today sometimes, Though the password actually exists, the m_password is not set
yet.
Reason : we haven't made a call to GetPasswordWithUI yet (which will set the
m_password) which then will go contact SignonService and gets the password.
Is it legal to get the password without having to go thru GetPasswordWithUI..?
Yes. It should be.
When I query a server (whose password is stored) for it's password and if I get
back a null/empty string..then that's an error condition. So, GetPassword(), if
m_password is empty, should call nsPasswordMgr's API (that I am requesting here)
and return the passwrod if it is there. That's why we need that API. I am sure
some one else will also need such a thing as we go forward.
Another instance in mailnews where we need this feature desparately is the Biff
feature. Today Biff on any second account will fail as
http://lxr.mozilla.org/seamonkey/source/mailnews/base/src/nsMsgBiffManager.cpp#316
simply returns a null password. It works for the first account because user
might have actually clicked on GetMsg button (or some other authentication)
which calls GetPasswordWithUI(). Sheela, if you have bug number for this, please
state it here.
We need an API in nsIPasswordManager that goes something like
string getPassword(string hostname, wstring username) or
string getPassword(string hostname)
I spoke to Steve Morse about the current abilities of the password manager. He
already has an idea on what I am looking for.
I talked to mScott about the current scenario and we both agree we may need such
an API. I also talked to Sheel about the existing error condition in Biff.
Adding Seth & Scott Putterman to the cc list.
bhuvan
Adding the dependency and adjusted milestone of this bug based on that.
Comment 3•23 years ago
|
||
Scott (Putterman) - could you add more on why this is an rtm stopper, I could
not follow the description in bug 80296 completely. Steve - how easy is this to
add i.e. time taken?
Assignee | ||
Comment 4•23 years ago
|
||
It's simple to implement, probably no more than a day or two. It's a matter of
adding the following additional attribute to
extensions/wallet/public/nsIPassword.idl
readonly attribute wstring password;
and then writing some c++ code in nsPasswordManager.cpp to fetch that attribute.
Comment 5•23 years ago
|
||
Vishy, mailnews has a couple of bugs where biff and Get Msgs for multiple
accounts should work for all accounts that we know the password for on startup.
One of the goals for mailnews was to have better multiple account management
and without this the user is going to be forced to access each account before
they can do anything with them.
Bhuvan, in a worse case scenario, could you check your patch in?
Assignee | ||
Comment 6•23 years ago
|
||
Unless I misunderstood the patch, I don't think it's the kind of thing you want
to check in. It's not a fix at all but merely a test.
Are you going to be fetching all the mail passwords when mail starts up? If so,
the user would then be prompted for his master password if he chose to have his
data encrypted. Is that the behavior that you want?
Steve,
When user clicks on GetAllNewMessages, I do want to check all those servers
which can get messages for their passwords. Then if I find password for a given
server, it's considered as an authenticated account and hence we will get
messages for that server without putting any password prompt dialogs..
With API you susggested 'readonly attribute wstring password;' which sitename's
password is this going to return ? The API I am looking for is more specifically
looking for the password for a given hostname (and/or username). I would like
PasswordMgr to do the enumeration or any other kind of algorithm to find the
corresponding password.
If the API you will add is just the attribute, then the caller has to do the
enumeration to extract the password from the password objects..right ? I would
like PasswordMgr to handle that logic. So, besides the API you suggested, it
will be useful if you can also provide us another API which returns password
like the ones I suggested in my previous updates
* wstring getPassword(string hostname, wstring username)
* wstring getPassword(string hostname)
I will be on aim as bhuvanr, please do ping me or I will ping you if I find you.
thanks.
Scott,
We can go with workaround. The only issue with that will be, it does these
enumerations everytime to find out if the password for a given server exists and
that may cost us something. That's why if we have the password string back, then
we can simply make GetPassword() to learn about it (by filling m_password) and
make it not ask the password again.
bhuvan
Assignee | ||
Comment 8•23 years ago
|
||
I disagree. The enumeration interface is currently the way the password manager
makes it's data available. So if a caller wants the password for a particular
url, it should be the caller's responsibility to go through the enumeration
until finding the desired one.
OK Steve. That's fine. Please go ahead with readonly attribute wstring password;
in nsIPassword.idl and corresponding cpp code as you suggested.
I will attach a new patch based on that API in the dependent bugs. Once you
check in, I will do testing for correctness and then request reviews.
thanks
bhuvan
Comment 10•23 years ago
|
||
To answer Steve's question, for the case of biff, we don't want the master
password dialog coming up. For Get All Messages I think that would be ok.
Assignee | ||
Comment 11•23 years ago
|
||
> To answer Steve's question, for the case of biff, we don't want the master
> password dialog coming up.
You won't be able to stop it. If your data is encrypted, PSM will automatically
pop up that dialog and I have no control over that. So tell me if you still
want me to implement this interface change.
Comment 12•23 years ago
|
||
Jennifer what's your feeling on this? Would you rather see a master password
come up when doing biff on startup or not have biff work for accounts that
remember their password.
Steve is there any way to make an api that allows us to specify that we want the
password only if it's unencrypted?
Assignee | ||
Comment 13•23 years ago
|
||
> Steve is there any way to make an api that allows us to specify that we
> want the password only if it's unencrypted?
Not cleanly.
Assignee | ||
Comment 14•23 years ago
|
||
Assignee | ||
Comment 15•23 years ago
|
||
racham, please review the patch, alecf please superreview it. Thanks.
Status: NEW → ASSIGNED
Reporter | ||
Comment 16•23 years ago
|
||
Steve,
What is the master password and how/when is it set by the user ? Will the user
be asked for master password when the password to be featched is an encrypted
one ? Howmuch effort is involved if we have to only fetch unencrypted passwords
?
Though I only need info to the extent of knowing whether or not password is
avialable to solve my bugs, it is not optimal at all. Because every time I do
GetAllNewMessages OR when biff activity is triggered, I have to enumearate thru
all password elements to find if the password entry is then there..that's
expensive.
bhuvan
Reporter | ||
Comment 17•23 years ago
|
||
r=racham.
Reporter | ||
Comment 18•23 years ago
|
||
I will also test it with my patch in mailnews and post the test results.
Assignee | ||
Comment 19•23 years ago
|
||
> What is the master password and how/when is it set by the user ?
When the user decides he wants to encrypt his data, he needs to select an
encryption key. That's the "master password." He "sets" it at the time that he
asks to have his data encrypted. Furthermore, he needs to supply it in each
browser session at the first point in time that the database is to be accessed.
> Will the user be asked for master password when the password to be featched
> is an encrypted one ?
Yes, if he has not already supplied it for the browser session.
> Howmuch effort is involved if we have to only fetch unencrypted passwords ?
There's no clean way for you to specify that in the API
> every time I do GetAllNewMessages OR when biff activity is triggered,
> I have to enumearate thru all password elements to find if the password
> entry is then there..that's expensive.
If you are suggesting as an alternative that password manager do the iteration
for you, that's just as expensive but on the other side of the interface. So
nothing would be saved.
Comment 20•23 years ago
|
||
it seems unnecessary (and unfair to developers whose first language is not
english and might not understand such abbreviations) to call this "pswd" when it
could just be called "password"
sr=alecf if you rename that attribute to "password"
Assignee | ||
Comment 21•23 years ago
|
||
> it seems unnecessary (and unfair to developers whose first language is not
> english and might not understand such abbreviations) to call this "pswd"
> when it could just be called "password"
But there is already a "password" object being used in this coding
Assignee | ||
Comment 22•23 years ago
|
||
Ignore above comment -- I hit submit button by accident when I meant to hit
clear.
Assignee | ||
Comment 23•23 years ago
|
||
Comment 24•23 years ago
|
||
(my sr=alecf still stands.. thanks for the change!)
Reporter | ||
Comment 25•23 years ago
|
||
Works fine when tested with mailnews patches in the dependent bugs.
Steve, thanks for adding the interface as needed so quickly. I have few more
questions regd the master password and the ways we can avoid the dialog..Will
send you mail regd that.
thanks,
bhuvan.
Comment 26•23 years ago
|
||
The hand-off of memory allocated by SINGSIGN_Enumerate to its caller follows
XPCOM rules, but then that memory is handed off to new nsPassword(...) in a
non-XPCOM fashion, and there's a leak if operator new fails:
- nsIPassword *password = new nsPassword(host, user);
+ nsIPassword *password = new nsPassword(host, user, pswd);
if (password == nsnull) {
return NS_ERROR_OUT_OF_MEMORY;
}
All of host, user and pswd are leaked should NS_ERROR_OUT_OF_MEMORY bite here.
Why not use nsXPIDLStrings and have new nsPassword copy? Or to minimize change
and copying overhead, at least document the non-XPCOM handoff here and spot-fix
the leak with some nsMemory::Free calls in the error return case?
/be
Assignee | ||
Comment 27•23 years ago
|
||
Updated•23 years ago
|
Comment 28•23 years ago
|
||
>Jennifer what's your feeling on this? Would you rather see a master password
>come up when doing biff on startup or not have biff work for accounts that
>remember their password.
If I understand correctly, if a user is using the Master Password feature to
store their Mail passwords, when Mail launches, and a password is needed to
retrieve messages, if the user has not already supplied their Master Password
for another component, they should be asked for their Master Password now, at
startup of Mail.
Comment 29•23 years ago
|
||
a=blizzard for drivers for the trunk
Assignee | ||
Comment 30•23 years ago
|
||
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Updated•20 years ago
|
Product: Browser → Seamonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•