GNOME Bugzilla – Bug 552583
Passwords don't get stored when using other authentication type than "Password"
Last modified: 2009-12-22 12:42:34 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.
Created attachment 118897 [details] [review] proposed evo patch for evolution;
You've got more attention when attaching patch too, also you've receive credits. Further more when you know the code :)
Go ahead for stable/trunk
Committed to trunk. Committed revision 36458. Committed to gnome-2-24. Committed revision 36459.
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
Patch reverted from trunk in revision 36656. Patch reverted from gnome-2-24 in revision 36657.
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
*** Bug 558232 has been marked as a duplicate of this bug. ***
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.
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.
Bharath/Alhil, can you give it a test?
This seems to do the trick for me, works fine again after the patch.
Sebastien, does it work for you too? (In case you are able to test the patch).
Patch goes fine in all three back ends (IMAP/Exchange/GW).
accepted-commit_now as requested by Srini
Committed to trunk as r36758. Committed to gnome-2-24 as r36759.
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 ?
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.
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?
*** Bug 560469 has been marked as a duplicate of this bug. ***
I uploaded patch to bug #560188 which is a crasher because of this change.
*** Bug 561847 has been marked as a duplicate of this bug. ***
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.
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().
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
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?
(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 :(
Created attachment 123887 [details] [review] New approach Hopefully this will resolve the regressions.
It seems you do not like my approach of one function for both, but no big deal. :)
*** Bug 560188 has been marked as a duplicate of this bug. ***
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.
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.
Committed to trunk (revision 36832) and gnome-2-24 (revision 36833). Milan, feel free to refactor the code if you still want to.
(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.
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.
Committed. http://svn.gnome.org/viewvc/evolution?view=revision&revision=36840 - Stable branch. http://svn.gnome.org/viewvc/evolution?view=revision&revision=36839 - Trunk
Crap. Good catch, Sankar.
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.)
(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.
Nope, I didn't. Do you think it worth it there?
*** Bug 563849 has been marked as a duplicate of this bug. ***
(In reply to comment #40) > Nope, I didn't. Do you think it worth it there? > Yes. Very Much :-)
OK then, committed to gnome-2-24. Committed revision 36866.
*** Bug 564140 has been marked as a duplicate of this bug. ***
*** Bug 564135 has been marked as a duplicate of this bug. ***
Created attachment 124712 [details] [review] Another one Just committed to stable/trunk
Good that we caught it during Sanity of 2.25.3. Thx to Prasad
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 :)
Yeah, commit it. Its effects seems to be so wide.
Committed to trunk. Committed revision 36903. Committed to gnome-2-24. Committed revision 36904.
*** Bug 565140 has been marked as a duplicate of this bug. ***
*** Bug 565290 has been marked as a duplicate of this bug. ***
*** Bug 566815 has been marked as a duplicate of this bug. ***
*** Bug 567758 has been marked as a duplicate of this bug. ***
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.
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.
Committed to trunk. Committed revision 37471.
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.
Created attachment 132964 [details] [review] proposed evo patch for evolution;
Created attachment 132965 [details] [review] proposed eds patch for evolution-data-server;
Milan seems fine to me. But we need to test it and then push it in. Doesn't this reqd for other providers also ?
(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.
Created commit c649c92 in eds master. Created commit 23df769 in evo master. As a question of testing I pushed to master only.
*** Bug 565768 has been marked as a duplicate of this bug. ***
*** Bug 585583 has been marked as a duplicate of this bug. ***
*** Bug 556519 has been marked as a duplicate of this bug. ***