After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 552583 - Passwords don't get stored when using other authentication type than "Password"
Passwords don't get stored when using other authentication type than "Password"
Status: RESOLVED FIXED
Product: evolution
Classification: Applications
Component: Mailer
2.24.x (obsolete)
Other Linux
: Normal normal
: ---
Assigned To: evolution-mail-maintainers
Evolution QA team
evolution[passwords]
: 556519 558232 560188 561847 563849 564135 564140 565140 565290 565768 566815 567758 585583 (view as bug list)
Depends on: 563740 563849
Blocks: 562917
 
 
Reported: 2008-09-17 03:37 UTC by Sebastian Keller
Modified: 2009-12-22 12:42 UTC
See Also:
GNOME target: ---
GNOME version: 2.23/2.24


Attachments
proposed evo patch (998 bytes, patch)
2008-09-17 18:07 UTC, Milan Crha
rejected Details | Review
Revised patch (1.88 KB, patch)
2008-11-03 18:46 UTC, Matthew Barnes
committed Details | Review
New approach (3.43 KB, patch)
2008-12-03 18:16 UTC, Matthew Barnes
committed Details | Review
Another one (840 bytes, patch)
2008-12-15 08:51 UTC, Srinivasa Ragavan
committed Details | Review
proposed evo patch (leak) (900 bytes, patch)
2008-12-15 15:35 UTC, Milan Crha
committed Details | Review
proposed evo patch (1.35 KB, patch)
2009-03-24 16:10 UTC, Milan Crha
committed Details | Review
proposed evo patch (4.90 KB, patch)
2009-04-20 16:09 UTC, Milan Crha
committed Details | Review
proposed eds patch (4.78 KB, patch)
2009-04-20 16:17 UTC, Milan Crha
committed Details | Review

Description Sebastian Keller 2008-09-17 03:37:39 UTC
This happens because in "get_password" (mail/mail-session.c) the url-string gets generated with camel_url_to_string(service->url, CAMEL_URL_HIDE_ALL). Because of HIDE_ALL, the authentication method gets removed from that string and thus is missing later on.

get_password => mail_config_get_account_by_source_url =>
provider->url_equal (which is "camel_url_equal", at least for pop3)

With authentication "APOP":
Breakpoint 3, camel_url_equal (v=0xacc00740, v2=0xacc006e0) at camel-url.c:705
705	in camel-url.c
2: {struct _CamelURL} v2 = {protocol = 0xacc005a8 "pop", 
  user = 0xacc00710 "2617XXX", authmech = 0x0, passwd = 0x0, 
  host = 0xacc00720 "pop.gmx.net", port = 0, path = 0xacc00730 "/", 
  params = 0x0, query = 0x0, fragment = 0x0}
1: {struct _CamelURL} v = {protocol = 0xacc00770 "pop", 
  user = 0xacc00790 "2617XXX", authmech = 0xacc00780 "+APOP", passwd = 0x0, 
  host = 0xacc007a0 "pop.gmx.net", port = 0, path = 0xacc007e8 "/", 
  params = 0xa78ecd0, query = 0x0, fragment = 0x0}

As a result you can only select "remember for the current session" instead of remembering it always. If you however have already the password in the keyring this problem won't happen to you, like this is the case on my other pc. I guess it just got there because of the .gnome2_private transition.
Comment 1 Milan Crha 2008-09-17 18:07:20 UTC
Created attachment 118897 [details] [review]
proposed evo patch

for evolution;
Comment 2 Milan Crha 2008-09-17 18:11:22 UTC
You've got more attention when attaching patch too, also you've receive credits.
Further more when you know the code :)
Comment 3 Srinivasa Ragavan 2008-09-25 12:38:06 UTC
Go ahead for stable/trunk
Comment 4 Milan Crha 2008-09-26 09:09:01 UTC
Committed to trunk. Committed revision 36458.
Committed to gnome-2-24. Committed revision 36459.
Comment 5 Srinivasa Ragavan 2008-10-17 04:44:04 UTC
Milan, this breaks SMTP. SMTP now remembers only for session and asks every time on start. Unfortunately youare on leave, Im gonna rever and ask Bharath to work on it, for 2.24.1
Comment 6 Akhil Laddha 2008-10-20 04:14:56 UTC
Patch reverted from trunk in revision 36656.
Patch reverted from gnome-2-24 in revision 36657.
Comment 7 Tyler 2008-11-03 18:07:56 UTC
I stumbled across this bug in the current version of ubuntu, intrepid with evolution 2.24.1.

The bug is still here, I tried to use login type "login" with an ssl protected pop3 server.

Filed also a bug in launchpad here: https://bugs.launchpad.net/ubuntu/+source/evolution/+bug/292806
Comment 8 Matthew Barnes 2008-11-03 18:26:32 UTC
*** Bug 558232 has been marked as a duplicate of this bug. ***
Comment 9 Matthew Barnes 2008-11-03 18:39:53 UTC
Perhaps the same changes to mail_config_get_account_by_transport_url() from bug #532472 should be applied to mail_config_get_account_by_source_url().

That is, instead of preserving the authmech attribute in the two URLs being compared, strip the attributes out of both URLs and just do a simple string comparison.  We're just trying to match a URL to an account here.  The authentication method shouldn't be relevant.
Comment 10 Matthew Barnes 2008-11-03 18:46:50 UTC
Created attachment 121892 [details] [review]
Revised patch

Try this.  I rewrote mail_config_get_account_by_source_url() to mimic the logic in mail_config_get_account_by_transport_url() as described above.
Comment 11 Srinivasa Ragavan 2008-11-04 03:42:40 UTC
Bharath/Alhil, can you give it a test?
Comment 12 Michael Monreal 2008-11-04 13:16:25 UTC
This seems to do the trick for me, works fine again after the patch.
Comment 13 Milan Crha 2008-11-06 18:36:04 UTC
Sebastien, does it work for you too? (In case you are able to test the patch).
Comment 14 Akhil Laddha 2008-11-07 05:47:00 UTC
Patch goes fine in all three back ends (IMAP/Exchange/GW).
Comment 15 Bharath Acharya 2008-11-07 06:34:53 UTC
accepted-commit_now as requested by Srini
Comment 16 Bharath Acharya 2008-11-07 06:37:50 UTC
Committed to trunk as r36758.
Committed to gnome-2-24 as r36759.
Comment 17 Sankar P 2008-11-10 07:51:51 UTC
I believe strcmp to compare two urls is wrong.

If you have a plugin on EMPopupTarget and you right-click on a folder, you will get a EMPopupTarget object. This will have a uri field which will have the foldername also as an end in the url.

for instance: 
t->uri = groupwise://bharath@164.99.99.179/Mailbox

This will result in not getting any matching account when we use this uri against, mail_config_get_source_url. This will lead to crashes in all those plugins (unless we are gonna add parse-and-remove-folder-name code to them).

Instead, I believe a better solution is to, continue make use of provider->url_equal so that any provider specific checking can also be done, for instance, GroupWise has a SOAP port which should also be compared to know if an account is different from another. If we use strcmp, we lose this ability.

We can break the url_equal signature and add an extra parameter (or) fix SMTP's (or whichever provider's) url_equal comparison methods to ignore unnecessary parameters (that way, every provider can check whatever it wants to check).

Sounds good ?

Comment 18 Sankar P 2008-11-10 09:53:17 UTC
I just confirmed that the SMTP issue was indeed happening because of the changes made to the mail_config_get_account_by_transport_url changes (removing provider->url_equal and substituting with strcmp)

So, on removing matthew's hunk on transport url, the password was asked to "remember forever" and not session. 

I will look at bug #532472 and understand why the change was incorporated and will get back, with more details. 
Comment 19 Matthew Barnes 2008-11-10 12:04:48 UTC
EAccount URIs include the authentication mechanism.  Password requests from Camel do not.  camel_url_equal() regards that as not equal, so the two "mail_config_get_foo_url()" functions were failing to find a match.  That was the cause of this bug and bug #532472.

If I'm reading the camel_url_equal() code correctly, it looks like the port is always retained in the string no matter what flags are passed.  So I don't think that part's an issue.

I'm not sure I understand your EMPopupTarget example.  None of the EAccount URIs will have the "/Mailbox" part, so you won't get a match whether you use strcmp() or camel_url_equal().  Right?  What am I missing?
Comment 20 Sankar P 2008-11-12 08:17:40 UTC
*** Bug 560469 has been marked as a duplicate of this bug. ***
Comment 21 Milan Crha 2008-11-19 15:10:48 UTC
I uploaded patch to bug #560188 which is a crasher because of this change.
Comment 22 Matthew Barnes 2008-11-21 19:42:25 UTC
*** Bug 561847 has been marked as a duplicate of this bug. ***
Comment 23 Matthew Barnes 2008-12-02 06:48:50 UTC
Looks like this also breaks emftm_uri_to_key() which is related to expanding folders or accounts in the sidebar.

In my test case, passing "imap://mbarnes@mail.corp.redhat.com/INBOX" to mail_config_get_account_by_source_url() fails to find a match because the URI contains a path and the EAccount URIs do not.
Comment 24 Matthew Barnes 2008-12-02 07:12:11 UTC
For reference, parts used by providers when comparing URIs:


                 PROTO  USER  AUTH  HOST  PATH  PORT  QUERY  FRAG
-----------------------------------------------------------------
camel_url_equal    x      x     x     x     x     x     x
stripped string    x      x           x     x     x     x      x
-----------------------------------------------------------------
groupwise          x      x     x     x           x
hula               x      x     x     x           x
imap               x      x     x     x           x
imap4              x      x     x     x           x
imapp [1]          x      x     x     x     x     x     x
local              x                        x
nntp               x      x           x           x
pop3 [1]           x      x     x     x     x     x     x
sendmail [1]       x      x     x     x     x     x     x
smtp [1]           x      x     x     x     x     x     x


The "stripped string" line is what my string comparison approach using the HIDE_ALL flag checks.  It correctly leaves out the "authmech" part, but incorrectly compares the "path", "query" and "fragment" parts.

What this chart tells me is that relying on provider-specific URI comparisons as currently written for the purpose of matching a URI to an account is broken for groupwise, hula, imap, imap4, imapp, pop3, sendmail, and smtp; because they all compare the "authmech" part.  That's why my rewrite of mail_config_get_account_by_transport_url() worked.

I think the right approach for this issue is to continue with the string comparison but also strip off everything after the host:post part of the URI.  That is, ignore the path, query, and fragment parts.  Perhaps we could add new CAMEL_URL_HIDE flags to make this easier.


[1] Provider defers to camel_url_equal().
Comment 25 Bharath Acharya 2008-12-03 04:00:49 UTC
Message filtering is broken. Looks to be a regression of this bug. em-utils.c:em_uri_from_camel() The folder uri is local@local for all the providers and hence fails because of the broken mail_config_get_account_by_source_url
Comment 26 Matthew Barnes 2008-12-03 04:34:31 UTC
It occurred to me (in a forehead-slapping "duh" moment) that rather than converting the URIs to strings for comparison and extending the API to strip out the path/query/fragment parts from the strings, I guess we could just compare the CamelURL parts directly like all the other providers do.

The point I was trying to make in comment #24 is I think deferring to the providers for comparing URIs is wrong for these two "get-account-by-url" functions.  To match a URI (for any provider) to an EAccount, we should only compare the protocol, user, host and port and disregard the rest.

Does that sound correct?
Comment 27 Sankar P 2008-12-03 05:17:41 UTC
(In reply to comment #26)
> The point I was trying to make in comment #24 is I think deferring to the
> providers for comparing URIs is wrong for these two "get-account-by-url"
> functions.  To match a URI (for any provider) to an EAccount, we should only
> compare the protocol, user, host and port and disregard the rest.
> 
> Does that sound correct?
> 

Exactly. Spot on. This is what I explained poorly in comment #17 . I was trying to send a mail yesterday on the same lines, explaining more. But somehow @redhat.com didnt seem to like me then :(
Comment 28 Matthew Barnes 2008-12-03 18:16:27 UTC
Created attachment 123887 [details] [review]
New approach

Hopefully this will resolve the regressions.
Comment 29 Milan Crha 2008-12-04 11:16:06 UTC
It seems you do not like my approach of one function for both, but no big deal. :)
Comment 30 Milan Crha 2008-12-04 11:18:09 UTC
*** Bug 560188 has been marked as a duplicate of this bug. ***
Comment 31 Sankar P 2008-12-04 11:40:10 UTC
Patch looks perfect for me.

Also, I guess we can remove:

account == NULL check in the while loop though. As we check for matching account and return within the loop anyway.
Comment 32 Matthew Barnes 2008-12-04 11:49:28 UTC
Milan: I wasn't aware of (or else don't remember) that.  Do you have a patch or something?

Sankar: Actually no, I removed the return within the loop and added the NULL check in the "while" condition.  Seemed more... elegant, I guess.
Comment 33 Matthew Barnes 2008-12-04 15:55:58 UTC
Committed to trunk (revision 36832) and gnome-2-24 (revision 36833).

Milan, feel free to refactor the code if you still want to.
Comment 34 Milan Crha 2008-12-04 18:24:12 UTC
(In reply to comment #32)
> Milan: I wasn't aware of (or else don't remember) that.  Do you have a patch or
> something?

I wrote here:

(In reply to comment #21)
> I uploaded patch to bug #560188 which is a crasher because of this change.
Comment 35 Sankar P 2008-12-08 04:58:36 UTC
There were some issues with the patch. Local account will be returned for any and all queries of mail_config_get_account_by_source|transport_url. I'll be fixing this.
Comment 37 Matthew Barnes 2008-12-08 15:40:14 UTC
Crap.  Good catch, Sankar.
Comment 38 Milan Crha 2008-12-09 19:15:00 UTC
Yeah, good catch Sankar. I extended a bit your approach in trunk, just a little tiny, nothing serious. See revision 36859 for more info. (By the way, it took me some time to understand what you fixed and why, really good catch.)
Comment 39 Sankar P 2008-12-10 05:48:10 UTC
(In reply to comment #38)
> I extended a bit your approach in trunk, just a little
> tiny, nothing serious. See revision 36859 for more info. 
> 

Neat. Looks nice. I hope you committed in stable branch as well.
Comment 40 Milan Crha 2008-12-10 10:33:38 UTC
Nope, I didn't. Do you think it worth it there?
Comment 41 Kandepu Prasad 2008-12-10 13:13:10 UTC
*** Bug 563849 has been marked as a duplicate of this bug. ***
Comment 42 Sankar P 2008-12-10 17:31:50 UTC
(In reply to comment #40)
> Nope, I didn't. Do you think it worth it there?
> 

Yes. Very Much :-)
Comment 43 Milan Crha 2008-12-10 18:06:27 UTC
OK then, committed to gnome-2-24. Committed revision 36866.
Comment 44 Kandepu Prasad 2008-12-12 11:42:41 UTC
*** Bug 564140 has been marked as a duplicate of this bug. ***
Comment 45 Kandepu Prasad 2008-12-12 11:43:42 UTC
*** Bug 564135 has been marked as a duplicate of this bug. ***
Comment 46 Srinivasa Ragavan 2008-12-15 08:51:01 UTC
Created attachment 124712 [details] [review]
Another one

Just committed to stable/trunk
Comment 47 Srinivasa Ragavan 2008-12-15 08:51:30 UTC
Good that we caught it during Sanity of 2.25.3. Thx to Prasad
Comment 48 Milan Crha 2008-12-15 15:35:59 UTC
Created attachment 124732 [details] [review]
proposed evo patch (leak)

for evolution;

Valgrind found a leak.
This bug seems to be very popular, half of the team is continuously working on it :)
Comment 49 Srinivasa Ragavan 2008-12-15 15:55:35 UTC
Yeah, commit it. Its effects seems to be so wide.
Comment 50 Milan Crha 2008-12-16 12:29:49 UTC
Committed to trunk. Committed revision 36903.
Committed to gnome-2-24. Committed revision 36904.
Comment 51 Sankar P 2008-12-21 08:14:18 UTC
*** Bug 565140 has been marked as a duplicate of this bug. ***
Comment 52 Oded Arbel 2008-12-21 23:16:56 UTC
*** Bug 565290 has been marked as a duplicate of this bug. ***
Comment 53 André Klapper 2009-01-08 16:35:53 UTC
*** Bug 566815 has been marked as a duplicate of this bug. ***
Comment 54 André Klapper 2009-01-15 20:51:20 UTC
*** Bug 567758 has been marked as a duplicate of this bug. ***
Comment 55 Patrik Martinsson 2009-03-20 14:58:02 UTC
I dont know if this comes from this bug, but after talking around in the irc-channel it seems very plausable. 

I read comment #25 from Bharath Acharya, about the message filter beeing broken. I also read comment #35 about this beeing fixed. However i must sadly inform that this does not seem to work when making filters that are exchange related. 

Im using evolution 2.24.5-1, and when i make a filter that "filters" on the subject or sender (or whatever) and choose action to move it to a folder in my exchange acount, this is whats created in the filter.xml, 

<folder uri="email://local@local/personal/subfolder"/> 

althou it should have been something like, 

<folder uri="exchange://username;auth=foo@bar.com/personal/subfolder"/>. 

Changing this line manually make the filter work. 
Please correct me if im wrong here. 

Best regards, 
Patrik Martinsson
Sweden. 

Comment 56 Milan Crha 2009-03-24 16:10:15 UTC
Created attachment 131273 [details] [review]
proposed evo patch

for evolution;

It was comparing user name with and without domain name, thus it failed. Added some code there, though I see Sankar's point of view to use provider's URL compare function here.
Comment 57 Milan Crha 2009-03-24 16:26:53 UTC
Committed to trunk. Committed revision 37471.
Comment 58 Milan Crha 2009-04-17 14:26:23 UTC
Believe or not, choosing folder from maildir (or MH and mbox account?) still doesn't work. Really do that as Sankar said, use provider's url_equal, otherwise this will never work correctly.
Comment 59 Milan Crha 2009-04-20 16:09:27 UTC
Created attachment 132964 [details] [review]
proposed evo patch

for evolution;
Comment 60 Milan Crha 2009-04-20 16:17:19 UTC
Created attachment 132965 [details] [review]
proposed eds patch

for evolution-data-server;
Comment 61 Srinivasa Ragavan 2009-04-22 12:42:31 UTC
Milan seems fine to me. But we need to test it and then push it in. Doesn't this reqd for other providers also ?
Comment 62 Milan Crha 2009-04-22 18:47:48 UTC
(In reply to comment #61)
> Milan seems fine to me. But we need to test it and then push it in.

Akhil question? ;)

> Doesn't this reqd for other providers also ?

Those other are doing that same as the mail-config now. So should be OK.
Comment 63 Milan Crha 2009-04-24 17:21:58 UTC
Created commit c649c92 in eds master.
Created commit 23df769 in evo master.

As a question of testing I pushed to master only.
Comment 64 Milan Crha 2009-05-26 13:06:16 UTC
*** Bug 565768 has been marked as a duplicate of this bug. ***
Comment 65 Milan Crha 2009-11-24 19:14:35 UTC
*** Bug 585583 has been marked as a duplicate of this bug. ***
Comment 66 Milan Crha 2009-12-22 12:42:34 UTC
*** Bug 556519 has been marked as a duplicate of this bug. ***