[Syncevolution-issues] [Bug 1360] creating client configurations

http://bugs.meego.com/show_bug.cgi?id=1360 --- Comment #2 from jingke <jingke.zhang(a)intel.com&gt; 2010-04-26 19:39:47 PDT --- Final parts of comments. Comment #21 From pohly 2010-01-15 01:53:11 PST (-) [reply] ------- The LCS code can be found in bug #2432. ------- Comment #22 From Chen Congwu 2010-01-17 20:28:43 PST (-) [reply] ------- (In reply to comment #20) Ok, I updated the code using your LCS based matching (I fixed some minor problem in your code so Patrick please double check). I would summarize previous discussions as below: Man_Mod based matching: The input string has a pre-defined format, the matching code is a little complex not straightforward to understand. The good point might be: the search might works better: we can give a higher weight in the Manufacture matching part(because phones from the same manufacture tend to have more configurations in common, if the manufacture does not match exactly, we will better consider this as not match); in the model matching part, this part is based on fuzzy match, however if there is digits within the string, we will not do character based matching but convert them to numbers and compare the difference). LCS based matching: The input string can be any, the matching is based on well known LCS algorithm so is easier to understand. It works good enough at this moment. I squashed these patches into one. I see, almost forgot this.. I mark this as TODO because I am not sure how to define the active transport if there is more than one listed? I remember there was discussions about this, you mentioned this should be handled by the code. We can do this if the user only have one transport library available. If both HTTP/BLUETOOTH transport is aviable and the user specified both a HTTP based URL and a Bluetooth based URL, which one should we pick up during createTransportAgent()? We may fall back to pick up the first URL appeared. Do you agree? ------- Comment #23 From Chen Congwu 2010-01-17 20:41:07 PST (-) [reply] ------- Another point forgot to mention: the cmd line --template \?[device] implementation changed a little so that when device is not null, I will match against *only* templates for SyncML server mode. I got stuck here: If matching all templates, the templates for built-in servers (Funambol, Scheduleworld) will be inevitably listed (They must be visible to the template matching process because we might need to load them for thumbnails). We can further improve this by changing the cmd line to let the user tell us which mode he wants match exactly (For a SyncML client or For a SyncML server). This works well at this moment, later if we start adding peer templates at the client mode, it need revisiting. ------- Comment #24 From pohly 2010-01-18 01:28:18 PST (-) [reply] ------- (In reply to comment #22) Can you add my code verbatim to util.h/cpp, then fix it? That way it'll be easier to review for me (and my mistakes will be exposed mercilessly, as they deserve! ;-) Please do this in separate commits, not roled into the "Configuration templates matching" patch. Another point in favor of LCS: when we look at a Bluetooth device name, we cannot assume that it has any kind of format and thus need a fuzzy matching. I know from some real-world examples that it is more like that people use "<person> <model>" rather than "<manufacturer> <model>". This is a different problem. Basically we would have to look at which transport is currently available and then try first one, then the other. Coming back to the current code: event if we assume that syncURL is only single-value, the code still fails for syncURL=obex-bt://<mac>:<channel>. if (url.find ("obex-bt://") ==0 && url.substr(strlen("obex-bt://"), url.npos) == btAddr) { url.substr() will evaluate to <mac>:<channel>, which is != btAddr == <mac>. ------- Comment #25 From Chen Congwu 2010-01-18 23:33:32 PST (-) [reply] ------- (In reply to comment #24) Ok, done. I think I already fixed this in that commit: + url = url.substr (0, url.find("+")); I will first truncate the url to remove any '+' postfix. ------- Comment #26 From pohly 2010-01-19 09:08:34 PST (-) [reply] ------- I have merged this, with some changes: - reordered patches so that LCS code is added first (the matching wouldn't have compiled otherwise because there was a reference to an LCS type) - moved LCS code into its own src/syncevo/lcs.h because it is sufficiently large and other users of util.h shouldn't have to parse it - added lcs.cpp with a unit test based on the original main() function for file comparison - for it to pass, merged your fixes into that patch - used resize() instead of reserve(); shorter and perhaps faster than the push_back - instead of building a sequence of characters plus zero cost, I added an LCS::accessor_sequence which provides a constant cost function and can read from a normal sequence (std::string in the matching) - replaced "super" source name with "calendar+todo", because "super" is a term about how this is implemented, not something that a user will understand; the sync-UI special-cases the "calendar+todo" name to localize it (like it does for the other standard names) In your code, "client-test SyncEvo::CmdlineTest::testMatchTemplate" does not always pass because without "make install" the templates are not found. As soon as templates get added, the test would have to be updated. I solved this by adding the current set of templates to the test files which need to be copied into "testcases". I have some more questions: do we really want to use .template.ini (hidden) instead of template.ini? This makes working with the templates harder without an apparent advantage. Should the default Nokia template really use the default values? Using something as for the 7120c might be more suitable. ------- Comment #27 From pohly 2010-01-19 10:41:54 PST (-) [reply] ------- (In reply to comment #26) I noticed that you had also created copies of the current templates under "test/templates", but I don't see where these files are used. Or did break the merging? Can we remove these duplicate templates? I'd prefer to avoid duplication, even if that means that the testcases have to be updated when updating the real templates. ------- Comment #28 From Chen Congwu 2010-01-19 20:54:34 PST (-) [reply] ------- (In reply to comment #26) That was the original proposal. Thinking again, I can not see any benefits either because we may allow user edit the file directly since we are using a fault tolerant LCS matching... At this moment (and possibly SyncEvolution 1.0) release, this is the right solution I think. When we start enabling more and more phones, we might found another phone model is more appropriate for this job. (In reply to comment #27) Yes. So you prefer to test with the real templates? I thought we can conduct the unit tests with an older and stable version of templates, so that we don't have to update the testcases from time to time; the list of devices may evolves larger and lager when we add more real templates into our system. I updated these and pushed to configuration-template branch, also fixed a link failure. ------- Comment #29 From pohly 2010-01-21 00:17:53 PST (-) [reply] ------- (In reply to comment #28) That's why I wrote the rules in src/Makefile which populate the src/testcases directory so that only the current, known set of template files are copied. Adding new templates won't affect the testing, only modifying the existing templates might. Thanks, merged. Remaining open work: creating a client config on-the-fly when an authorized client connects to an HTTP server. Lowering impact. ------- Comment #30 From Chen Congwu 2010-04-07 23:52:49 PST (-) [reply] ------- (In reply to comment #5) Wondering whether such 'special config' is necessary, I think we can: 1) each time there is a client contacting us with no configuration, create the configuration automatically with its device ID 2) If synchronization failed with authentication problem, mark the configuration as re-usable; otherwise just continue smoothly The 're-usable' configuration can be reused for a later new coming client (just as the special config) or clean up entirely. I think the benefits is in normal path there will be only one SyncML session better than the previous two different session with same SyncML message; in the authentication failure path, I don't think there will be any non-desirable information exposed to client or disastrous impact on server side data because this is what SyncML authentication works (How can a client has any negative effect to the server before it has been authenticated?) ------- Comment #31 From pohly 2010-04-08 02:54:37 PST (-) [reply] ------- (In reply to comment #30) I agree, with the "reusable" attribute this should be doable. My concern was that if unauthorized clients are allowed to create a new config which is kept permanently, a denial of service attack becomes possible. We don't need a special config property. Just some naming convention should also work. ------- Comment #32 From Chen Congwu 2010-04-08 03:10:16 PST (-) [reply] ------- (In reply to comment #31) 'reusable' meaning that we only keep one copy of such configurations inside syncevolution. If later another request comes, no configuration will be created, it will reuse the existing 'reusable' configuration. So marking it as 'reusable' also implies resetting 'remoteDeviceId' for this configuration to empty. ------- Comment #33 From Chen Congwu 2010-04-09 02:13:14 PST (-) [reply] ------- (In reply to comment #30) There is one problem: we don't know the session status from server side, which means we can't tell whether the session is authenticated failure.. Without this information, we have to be conservative that configurations is never 'reusable', which might suffer denial of service attack as Patrick already pointed out. Is there other solutions from synthesis that we can tell whether a server side session authenticated failed? Otherwise I would like to keep 'reusable' always 'false' and improve that once bug #7709 got fixed. ------- Comment #34 From pohly 2010-04-09 04:16:42 PST (-) [reply] ------- (In reply to comment #33) It should be possible to hook into some of the scripts invoked as part of a server session and set a global session variable once the client is authenticated. See "INTEGER delayedabort" in SyncContext.cpp for an example. ------- Comment #35 From Chen Congwu 2010-04-13 01:25:59 PST (-) [reply] ------- Patch in mb7838 (Requires another patch in libsynthesis) I am OK with adding 'reusale' property in the configuration, if no objection I will further adapt the test cases for that. One problem in checkForLoginSuccess(), I have to call it repeatedly inside Session Step loop because If I tested it and the end of the session, the script context already gone.. ------- Comment #36 From pohly 2010-04-13 04:56:51 PST (-) [reply] ------- (In reply to comment #35) I need a user-level documentation for this new feature before I can do the final review and merge. This can be either a README or an addendum (marked as experimental) to the http://syncevolution.org/development/http-server-howto Key point that I haven't seen discussed is: - How does the user set up a server so that it can create new configs correctly? In particular, which username/password is expected by the server? - What is the naming of the new configs? I'm not sure yet whether we need the property. Can't we just define that configuration "temporary-config" is "reusable"? We are almost done with 1.0. Instead of merging features that few users will need, I'd rather refine this. Perhaps in 1.1 we'll have server-side status codes and then don't need hacks. ------- Comment #37 From Chen Congwu 2010-04-13 19:40:40 PST (-) [reply] ------- (In reply to comment #36) > (In reply to comment #35) > > > I need a user-level documentation for this new feature before I can do the > final review and merge. This can be either a README or an addendum (marked as > experimental) to the http://syncevolution.org/development/http-server-howto Let's finalize that here before I update it in the doc. Using 'SyncEvolution Client' template is enough (of source syncURL should set correctly), username/password was already inside the template. The name is 'deviceID-time', any better ideas? That can work if we hook at the end of the first successful session, at which point we have to rename the config from 'temporary-config' to a permanent config for that peer 'deviceID-time' It works but I think renaming a configuration is a bit confusing. Alright, let's go with the older solution that always create new configs for any new coming client. ------- Comment #38 From pohly 2010-04-13 23:53:24 PST (-) [reply] ------- (In reply to comment #37) In other words, the whole world knows the credentials which are necessary to connect to my machine, create a config and access my data, and that as soon as I start syncevo-http-server. This is not secure ;-) I see several solutions: 1. Add username/password as command line options to syncevo-http-server, communicate them to syncevo-dbus-server when a new request comes in, using new keys in the meta information. syncevo-dbus-server then uses those credentials. 2. We add new config properties, to be set inside ~/.config/syncevolution/config.ini, which specify the default credentials. In both cases creating new configs is disabled if no credentials are set. Solution 1 has the advantage that this feature is discoverable by users of syncevo-http-server and can be set on a case-by-case basis (run daemon with specific credentials, connect with new client, restart daemon without credentials or different ones for a different client). Solution 2 has the advantage that it can (and should) reuse the secure password storage features of syncevo-dbus-server. I tend to prefer solution 1, although it might be worth asking on the mailing list. > > The name is 'deviceID-time', any better ideas? Can we compose the name from manufacturer + model? That may only work after completing the sync and thus would depend on renaming the config. Why? Because the user sees a "temporary-config"? ------- Comment #39 From Chen Congwu 2010-04-14 00:33:20 PST (-) [reply] ------- (In reply to comment #38) Ah, good catch! I like solution 1 too, such credential is part of HTTP server I think. > > > > > The name is 'deviceID-time', any better ideas? > > Can we compose the name from manufacturer + model? That may only work after > completing the sync and thus would depend on renaming the config. That still may not be unique, so we still have to append some string to make it unique.. I think the real problem is we should found a way to notify the user we have created a configuration named xxx for the client, so that he understands the configuration name. Exactly, that's really confuse when I sync with the same client, the first time server said 'temporary-config', later some other name. If there is a UI depicts profile information about such sync, the sync history would also be confusing.. ------- Comment #40 From Chen Congwu 2010-04-15 01:57:23 PST (-) [reply] ------- (In reply to comment #39) This is implemented. > > > > > > The name is 'deviceID-time', any better ideas? > > > > Can we compose the name from manufacturer + model? That may only work after > > completing the sync and thus would depend on renaming the config. > That still may not be unique, so we still have to append some string to make it > unique.. I think the real problem is we should found a way to notify the user > we have created a configuration named xxx for the client, so that he > understands the configuration name. This is still open. Any comments? ------- Comment #41 From pohly 2010-04-15 02:32:40 PST (-) [reply] ------- (In reply to comment #40) Comments on the patch, please update: -usage = """usage: %prog http://localhost:<port>/<path> +usage = """usage: %prog http://localhost:<port>/<path -u username -p password> Note that the options were inserted inside <path>. Besides, they shouldn't be listed here. The Python modules does that for us. Please use usage: %prog <options> http://localhost:<port>/<path> The documentation for the new options also needs a clarification: help = "The user name accepted by the server" should become: help = "The user name checked by by the server when an unknown client connects. Only if the client provides the right credentials, will a new configuration be created for it automatically, using these credentials." > > > > > > > > The name is 'deviceID-time', any better ideas? > > > > > > Can we compose the name from manufacturer + model? That may only work after > > > completing the sync and thus would depend on renaming the config. > > That still may not be unique, so we still have to append some string to make it > > unique.. I think the real problem is we should found a way to notify the user > > we have created a configuration named xxx for the client, so that he > > understands the configuration name. > This is still open. Any comments? We cannot notify the client. Printing a message to the screen in syncevo-dbus-server or writing into a log also doesn't help much. Therefore I think we need meaningful names so that a server user can list configs and recognize which ones where created for him for which device. I agree that renaming the config has issues, but it seems to me to be the least evil one. Perhaps we can also rename the logdir used by the initial successful session at the end of the session? Note that the Synthesis engine should better be shut down at that point, because it would continue using the old log dir path. ------- Comment #42 From Chen Congwu 2010-04-15 02:50:40 PST (-) [reply] ------- (In reply to comment #41) > > > > > > > > > > The name is 'deviceID-time', any better ideas? > > > > > > > > Can we compose the name from manufacturer + model? That may only work after > > > > completing the sync and thus would depend on renaming the config. > > > That still may not be unique, so we still have to append some string to make it > > > unique.. I think the real problem is we should found a way to notify the user > > > we have created a configuration named xxx for the client, so that he > > > understands the configuration name. > > This is still open. Any comments? > > We cannot notify the client. Printing a message to the screen in > syncevo-dbus-server or writing into a log also doesn't help much. Therefore I > think we need meaningful names so that a server user can list configs and > recognize which ones where created for him for which device. > > I agree that renaming the config has issues, but it seems to me to be the least > evil one. Perhaps we can also rename the logdir used by the initial successful > session at the end of the session? Note that the Synthesis engine should better > be shut down at that point, because it would continue using the old log dir > path. Depending on extract information from DevInf looks to me not work well: 1) Will the peer always send its DevInf? 2) Will the DevInf be meaningful? For example, we have seen Nokia N900 as 'Nokia Nxx', that seems to me not make sense. I still think we need to find other ways to notify the client.. ------- Comment #43 From pohly 2010-04-15 04:22:04 PST (-) [reply] ------- (In reply to comment #42) [naming of config based on DevInf] If not, Synthesis will request it. That's why I said that the information should be available after a sync, but cannot necessarily be extracted from the first message. Of course, there will be clients which have no DevInf at all (which I find unlikely) or no useful information inside. Suggestions welcome, of course. ------- Comment #44 From pohly 2010-04-15 06:00:35 PST (-) [reply] ------- Replacing the "*Feature Request" to keep this issue out of the queue of issues ready to be migrated. ------- Comment #45 From Chen Congwu 2010-04-15 21:48:56 PST (-) [reply] ------- (In reply to comment #43) Since I don't think we can figure out that automatically, we still need some user intervention, isn't it? Extracting information from DevInf might also needs same tricks/hacks as I already had with 'Authentication failure detection' patch which was not nice. Can we ask the user provide this information when he starts the sync? For example, can we reuse 'deviceID' for that purpose? Or some other field in SyncML? ------- Comment #46 From Chen Congwu 2010-04-15 22:03:30 PST (-) [reply] ------- Reset status in comment #44 ------- Comment #47 From Chen Congwu 2010-04-18 23:39:55 PST (-) [reply] ------- (In reply to comment #45) No, there is a callback in Synthesis DBPlugin API. However I still don't think such sometimes works, sometimes not works approach as a solution. Current patch does the following: 1) Add 'serverConfig' property shared by peers inside the same context, the value depicts a configuration that will be used for new coming clients, empty if none exits. 2) When a new clients comes, if 'serverConfig' is not empty, reuse the configuration defined by it's value; otherwise create the configuration automatically using (removeDeivceID and time as the name) and recored the name inside 'serverConfig' 3) When a session (with name equals the value inside 'serverConfig') got a session without authentication error, set the value in 'serverConfig' as empty. The remaining problem is such newly created configuration name is not meaningful, but since we don't have a good solution so far, I would like to take that as an enhancement that later we can work on. -- Configure bugmail: http://bugs.meego.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching the assignee of the bug. You are watching someone on the CC list of the bug.