Issue 288960043: Kik messenger sqlite parser #535 (Closed)
|
Created:
8 years, 2 months ago by Ken Macleod Modified:
8 years, 1 month ago CC:
log2timeline-dev_googlegroups.com Visibility:
Public. |
DescriptionKik messenger sqlite parser #535
Patch Set 1 #
Total comments: 14
Patch Set 2 : moved function from parser to formatter #535 #
Total comments: 8
Patch Set 3 : class names changed, formatting corrected #
Total comments: 4
Patch Set 4 : mainly name changes and formatting #
MessagesTotal messages: 24
Look pretty good! A couple of questions though: https://codereview.appspot.com/288960043/diff/1/plaso/parsers/sqlite_plugins/... File plaso/parsers/sqlite_plugins/kik.py (right): https://codereview.appspot.com/288960043/diff/1/plaso/parsers/sqlite_plugins/... plaso/parsers/sqlite_plugins/kik.py:16: DATA_TYPE = u'iphone:kik:messaging' Does this only work for kik on iPhone? https://codereview.appspot.com/288960043/diff/1/plaso/parsers/sqlite_plugins/... plaso/parsers/sqlite_plugins/kik.py:41: """Parse Kik messenger database.""" "SQLite plugin for Kik iPhone database", or something similar https://codereview.appspot.com/288960043/diff/1/plaso/parsers/sqlite_plugins/... plaso/parsers/sqlite_plugins/kik.py:55: MESSAGE_TYPE = { Let's move this functionality to the formatter. https://codereview.appspot.com/288960043/diff/1/tests/parsers/sqlite_plugins/... File tests/parsers/sqlite_plugins/kik.py (right): https://codereview.appspot.com/288960043/diff/1/tests/parsers/sqlite_plugins/... tests/parsers/sqlite_plugins/kik.py:42: expected_username = 8 # This produces an error when I use = u'8' Why is this? Is this an int in the schema? Looking at some writeups, it looks like this might be an index into the contacts table...
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
Thanks for the feedback. I've worked on each of the issues below. https://codereview.appspot.com/288960043/diff/1/plaso/parsers/sqlite_plugins/... File plaso/parsers/sqlite_plugins/kik.py (right): https://codereview.appspot.com/288960043/diff/1/plaso/parsers/sqlite_plugins/... plaso/parsers/sqlite_plugins/kik.py:16: DATA_TYPE = u'iphone:kik:messaging' My understanding is that the android database has a different structure (haven't checked yet). I intend to create a parser specifically for android Kik too. https://codereview.appspot.com/288960043/diff/1/plaso/parsers/sqlite_plugins/... plaso/parsers/sqlite_plugins/kik.py:41: """Parse Kik messenger database.""" On 2016/01/29 16:51:29, onager wrote: > "SQLite plugin for Kik iPhone database", or something similar Done. https://codereview.appspot.com/288960043/diff/1/plaso/parsers/sqlite_plugins/... plaso/parsers/sqlite_plugins/kik.py:55: MESSAGE_TYPE = { On 2016/01/29 16:51:29, onager wrote: > Let's move this functionality to the formatter. Done. https://codereview.appspot.com/288960043/diff/1/tests/parsers/sqlite_plugins/... File tests/parsers/sqlite_plugins/kik.py (right): https://codereview.appspot.com/288960043/diff/1/tests/parsers/sqlite_plugins/... tests/parsers/sqlite_plugins/kik.py:42: expected_username = 8 # This produces an error when I use = u'8' Yes, it is an integer in the schema that relates to the actual user name in the 'ZKIKUSER' contacts table. I plan to extract this later, but wanted to keep my first submission very simple to begin with. So, when the code reads: expected_username = u'8' keeping consistency with the rest of the tests, it returns an error: 8 != u'8' which I'm unsure how to correct, other than what I've done (removed the u'')
Sign in to reply to this message.
Sign in to reply to this message.
https://codereview.appspot.com/288960043/diff/1/plaso/parsers/sqlite_plugins/... File plaso/parsers/sqlite_plugins/kik.py (right): https://codereview.appspot.com/288960043/diff/1/plaso/parsers/sqlite_plugins/... plaso/parsers/sqlite_plugins/kik.py:16: DATA_TYPE = u'iphone:kik:messaging' On 2016/01/31 22:11:15, Ken Macleod wrote: > My understanding is that the android database has a different structure (haven't > checked yet). I intend to create a parser specifically for android Kik too. > OK, you should change the names of classes, and correct the docstrings to make that clear then. Also, IOS over iPhone. https://codereview.appspot.com/288960043/diff/1/tests/parsers/sqlite_plugins/... File tests/parsers/sqlite_plugins/kik.py (right): https://codereview.appspot.com/288960043/diff/1/tests/parsers/sqlite_plugins/... tests/parsers/sqlite_plugins/kik.py:42: expected_username = 8 # This produces an error when I use = u'8' On 2016/01/31 22:11:16, Ken Macleod wrote: > Yes, it is an integer in the schema that relates to the actual user name in the > 'ZKIKUSER' contacts table. I plan to extract this later, but wanted to keep my > first submission very simple to begin with. > So, when the code reads: > expected_username = u'8' > keeping consistency with the rest of the tests, it returns an error: > 8 != u'8' > which I'm unsure how to correct, other than what I've done (removed the u'') OK, this isn't the username then, but the local_contact_id or something. Since it's and in, not a string, the comparison you're doing is fine, and there's no need for a comment about the error. https://codereview.appspot.com/288960043/diff/20001/plaso/formatters/kik.py File plaso/formatters/kik.py (right): https://codereview.appspot.com/288960043/diff/20001/plaso/formatters/kik.py#n... plaso/formatters/kik.py:35: 10: u'unknown', Maybe remove the "unknown"s from this, since you substitute "unknown" by default anyway. https://codereview.appspot.com/288960043/diff/20001/plaso/formatters/kik.py#n... plaso/formatters/kik.py:68: message_type = event_values.get( This should fit on one line. https://codereview.appspot.com/288960043/diff/20001/plaso/formatters/kik.py#n... plaso/formatters/kik.py:73: message_type, u'UNKNOWN')) This should fit one line too, I think. https://codereview.appspot.com/288960043/diff/20001/plaso/parsers/sqlite_plug... File plaso/parsers/sqlite_plugins/kik.py (right): https://codereview.appspot.com/288960043/diff/20001/plaso/parsers/sqlite_plug... plaso/parsers/sqlite_plugins/kik.py:40: class KikPlugin(interface.SQLitePlugin): KikIOSPlugin
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
code updated as per comments https://codereview.appspot.com/288960043/diff/1/plaso/parsers/sqlite_plugins/... File plaso/parsers/sqlite_plugins/kik.py (right): https://codereview.appspot.com/288960043/diff/1/plaso/parsers/sqlite_plugins/... plaso/parsers/sqlite_plugins/kik.py:16: DATA_TYPE = u'iphone:kik:messaging' On 2016/02/03 13:19:26, onager wrote: > On 2016/01/31 22:11:15, Ken Macleod wrote: > > My understanding is that the android database has a different structure > (haven't > > checked yet). I intend to create a parser specifically for android Kik too. > > > > OK, you should change the names of classes, and correct the docstrings to make > that clear then. Also, IOS over iPhone. Done. https://codereview.appspot.com/288960043/diff/1/tests/parsers/sqlite_plugins/... File tests/parsers/sqlite_plugins/kik.py (right): https://codereview.appspot.com/288960043/diff/1/tests/parsers/sqlite_plugins/... tests/parsers/sqlite_plugins/kik.py:42: expected_username = 8 # This produces an error when I use = u'8' On 2016/02/03 13:19:26, onager wrote: > On 2016/01/31 22:11:16, Ken Macleod wrote: > > Yes, it is an integer in the schema that relates to the actual user name in > the > > 'ZKIKUSER' contacts table. I plan to extract this later, but wanted to keep my > > first submission very simple to begin with. > > So, when the code reads: > > expected_username = u'8' > > keeping consistency with the rest of the tests, it returns an error: > > 8 != u'8' > > which I'm unsure how to correct, other than what I've done (removed the u'') > > OK, this isn't the username then, but the local_contact_id or something. Since > it's and in, not a string, the comparison you're doing is fine, and there's no > need for a comment about the error. Acknowledged. https://codereview.appspot.com/288960043/diff/20001/plaso/formatters/kik.py File plaso/formatters/kik.py (right): https://codereview.appspot.com/288960043/diff/20001/plaso/formatters/kik.py#n... plaso/formatters/kik.py:35: 10: u'unknown', On 2016/02/03 13:19:26, onager wrote: > Maybe remove the "unknown"s from this, since you substitute "unknown" by default > anyway. Done. https://codereview.appspot.com/288960043/diff/20001/plaso/formatters/kik.py#n... plaso/formatters/kik.py:68: message_type = event_values.get( On 2016/02/03 13:19:26, onager wrote: > This should fit on one line. Done. https://codereview.appspot.com/288960043/diff/20001/plaso/formatters/kik.py#n... plaso/formatters/kik.py:73: message_type, u'UNKNOWN')) On 2016/02/03 13:19:26, onager wrote: > This should fit one line too, I think. Done. https://codereview.appspot.com/288960043/diff/20001/plaso/parsers/sqlite_plug... File plaso/parsers/sqlite_plugins/kik.py (right): https://codereview.appspot.com/288960043/diff/20001/plaso/parsers/sqlite_plug... plaso/parsers/sqlite_plugins/kik.py:40: class KikPlugin(interface.SQLitePlugin): On 2016/02/03 13:19:26, onager wrote: > KikIOSPlugin Done.
Sign in to reply to this message.
https://codereview.appspot.com/288960043/diff/1/tests/parsers/sqlite_plugins/... File tests/parsers/sqlite_plugins/kik.py (right): https://codereview.appspot.com/288960043/diff/1/tests/parsers/sqlite_plugins/... tests/parsers/sqlite_plugins/kik.py:42: expected_username = 8 # This produces an error when I use = u'8' On 2016/02/08 16:16:15, Ken Macleod wrote: > On 2016/02/03 13:19:26, onager wrote: > > On 2016/01/31 22:11:16, Ken Macleod wrote: > > > Yes, it is an integer in the schema that relates to the actual user name in > > the > > > 'ZKIKUSER' contacts table. I plan to extract this later, but wanted to keep > my > > > first submission very simple to begin with. > > > So, when the code reads: > > > expected_username = u'8' > > > keeping consistency with the rest of the tests, it returns an error: > > > 8 != u'8' > > > which I'm unsure how to correct, other than what I've done (removed the u'') > > > > OK, this isn't the username then, but the local_contact_id or something. Since > > it's and in, not a string, the comparison you're doing is fine, and there's no > > need for a comment about the error. > > Acknowledged. Please still change the name of this field - this isn't the username that sent the message. https://codereview.appspot.com/288960043/diff/40001/plaso/parsers/sqlite_plug... File plaso/parsers/sqlite_plugins/kik.py (right): https://codereview.appspot.com/288960043/diff/40001/plaso/parsers/sqlite_plug... plaso/parsers/sqlite_plugins/kik.py:1: # -*- coding: utf-8 -*- This needs to be renamed too, to kik_ios or something. https://codereview.appspot.com/288960043/diff/40001/plaso/parsers/sqlite_plug... plaso/parsers/sqlite_plugins/kik.py:4: Kik messages on iOS devices are stored in an The original indentation was correct.
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
code updated as per latest review https://codereview.appspot.com/288960043/diff/1/tests/parsers/sqlite_plugins/... File tests/parsers/sqlite_plugins/kik.py (right): https://codereview.appspot.com/288960043/diff/1/tests/parsers/sqlite_plugins/... tests/parsers/sqlite_plugins/kik.py:42: expected_username = 8 # This produces an error when I use = u'8' On 2016/02/09 10:54:52, onager wrote: > On 2016/02/08 16:16:15, Ken Macleod wrote: > > On 2016/02/03 13:19:26, onager wrote: > > > On 2016/01/31 22:11:16, Ken Macleod wrote: > > > > Yes, it is an integer in the schema that relates to the actual user name > in > > > the > > > > 'ZKIKUSER' contacts table. I plan to extract this later, but wanted to > keep > > my > > > > first submission very simple to begin with. > > > > So, when the code reads: > > > > expected_username = u'8' > > > > keeping consistency with the rest of the tests, it returns an error: > > > > 8 != u'8' > > > > which I'm unsure how to correct, other than what I've done (removed the > u'') > > > > > > OK, this isn't the username then, but the local_contact_id or something. > Since > > > it's and in, not a string, the comparison you're doing is fine, and there's > no > > > need for a comment about the error. > > > > Acknowledged. > > Please still change the name of this field - this isn't the username that sent > the message. I have updated the SQLite query which will now return both a username and a display name. https://codereview.appspot.com/288960043/diff/40001/plaso/parsers/sqlite_plug... File plaso/parsers/sqlite_plugins/kik.py (right): https://codereview.appspot.com/288960043/diff/40001/plaso/parsers/sqlite_plug... plaso/parsers/sqlite_plugins/kik.py:1: # -*- coding: utf-8 -*- On 2016/02/09 10:54:52, onager wrote: > This needs to be renamed too, to kik_ios or something. Done. https://codereview.appspot.com/288960043/diff/40001/plaso/parsers/sqlite_plug... plaso/parsers/sqlite_plugins/kik.py:4: Kik messages on iOS devices are stored in an On 2016/02/09 10:54:52, onager wrote: > The original indentation was correct. Done.
Sign in to reply to this message.
code updated
Sign in to reply to this message.
On 2016/02/10 12:49:42, Ken Macleod wrote: > code updated I'd like to merge this, but I don't see a branch on your github repo containing the changes - perhaps you need to push the changes from your local repo still?
Sign in to reply to this message.
It should be there now. Apologies. Still trying to get my head round the whole git / github thing. On Sat, Feb 13, 2016 at 3:31 PM, <onager@deerpie.com> wrote: > On 2016/02/10 12:49:42, Ken Macleod wrote: > >> code updated >> > > I'd like to merge this, but I don't see a branch on your github repo > containing the changes - perhaps you need to push the changes from your > local repo still? > > https://codereview.appspot.com/288960043/ >
Sign in to reply to this message.
On 2016/02/15 13:58:29, kenmacleod2013_gmail.com wrote: > It should be there now. > > Apologies. Still trying to get my head round the whole git / github thing. > > On Sat, Feb 13, 2016 at 3:31 PM, <mailto:onager@deerpie.com> wrote: > > > On 2016/02/10 12:49:42, Ken Macleod wrote: > > > >> code updated > >> > > > > I'd like to merge this, but I don't see a branch on your github repo > > containing the changes - perhaps you need to push the changes from your > > local repo still? > > > > https://codereview.appspot.com/288960043/ > > LGTM
Sign in to reply to this message.
On 2016/02/15 20:43:42, onager wrote: > On 2016/02/15 13:58:29, http://kenmacleod2013_gmail.com wrote: > > It should be there now. > > > > Apologies. Still trying to get my head round the whole git / github thing. > > > > On Sat, Feb 13, 2016 at 3:31 PM, <mailto:onager@deerpie.com> wrote: > > > > > On 2016/02/10 12:49:42, Ken Macleod wrote: > > > > > >> code updated > > >> > > > > > > I'd like to merge this, but I don't see a branch on your github repo > > > containing the changes - perhaps you need to push the changes from your > > > local repo still? > > > > > > https://codereview.appspot.com/288960043/ > > > > > LGTM Still not quite right with git, I'm afraid. Try the following 1. Create a new fork of Plaso: https://github.com/log2timeline/plaso#fork-destination-box 2. Checkout your copy locally: "git clone https://github.com/MacleodKen/plaso.git" 3. Add an upstream remote for Plaso (this will make things easier later) "git remote add upstream https://github.com/log2timeline/plaso.git" 4. Create a new branch in your fork: "git checkout -b kik_ios" 5. Apply your changes to the branch, add them and commit them. 6. "git push" to push the changes to github 7. I'll apply them to Plaso, and you'll have a nice set up for future changes 8. You'll want to "git checkout master;git pull --rebase upstream master;git push" from time to time to keep your fork up to date with other Plaso changes. We're almost there!
Sign in to reply to this message.
Hi, 1. I created a fork. 2. I checked out a local copy with "git clone https://github.com/MacleodKen/plaso.git" 3. I added "git remote add upstream https://github.com/log2timeline/plaso.git" 4. I created a new branch with "git checkout -b kik_ios" 5. I applied changes to the branch, add them and commit them. 6. I did the "git checkout master;git pull --rebase upstream master;git push" as there wee other plaso changes. 7. I then did "git checkout kik_ios; git merge master' 8. and finally "git push" The response was "Everything up-to-date", but I see no changes to my github fork. Was I supposed to merge by kik_ios branch to my fork first? I appreciate your patience. On Mon, Feb 15, 2016 at 8:55 PM, <onager@deerpie.com> wrote: > On 2016/02/15 20:43:42, onager wrote: > >> On 2016/02/15 13:58:29, http://kenmacleod2013_gmail.com wrote: >> > It should be there now. >> > >> > Apologies. Still trying to get my head round the whole git / github >> > thing. > >> > >> > On Sat, Feb 13, 2016 at 3:31 PM, <mailto:onager@deerpie.com> wrote: >> > >> > > On 2016/02/10 12:49:42, Ken Macleod wrote: >> > > >> > >> code updated >> > >> >> > > >> > > I'd like to merge this, but I don't see a branch on your github >> > repo > >> > > containing the changes - perhaps you need to push the changes from >> > your > >> > > local repo still? >> > > >> > > https://codereview.appspot.com/288960043/ >> > > >> > > LGTM >> > > Still not quite right with git, I'm afraid. Try the following > 1. Create a new fork of Plaso: > https://github.com/log2timeline/plaso#fork-destination-box > 2. Checkout your copy locally: "git clone > https://github.com/MacleodKen/plaso.git" > 3. Add an upstream remote for Plaso (this will make things easier later) > "git remote add upstream https://github.com/log2timeline/plaso.git" > 4. Create a new branch in your fork: "git checkout -b kik_ios" > 5. Apply your changes to the branch, add them and commit them. > 6. "git push" to push the changes to github > 7. I'll apply them to Plaso, and you'll have a nice set up for future > changes > 8. You'll want to "git checkout master;git pull --rebase upstream > master;git push" from time to time to keep your fork up to date with > other Plaso changes. > > We're almost there! > > https://codereview.appspot.com/288960043/ >
Sign in to reply to this message.
You did not push your changes to your fork: https://github.com/MacleodKen/plaso Try: git push --set-upstream origin kik_ios
Sign in to reply to this message.
Still WIP but also see: https://github.com/log2timeline/l2tdocs/pull/1/files?short_path=552505a#diff-...
Sign in to reply to this message.
I've "git push --set-upstream origin kik_ios" Is this all I do, or should I be running ./utils/review.py update On Tue, Feb 16, 2016 at 7:39 AM, <joachim.metz@gmail.com> wrote: > Still WIP but also see: > > https://github.com/log2timeline/l2tdocs/pull/1/files?short_path=552505a#diff-... > > https://codereview.appspot.com/288960043/ >
Sign in to reply to this message.
This is fine, I've created a PR for automated testing: https://github.com/log2timeline/plaso/pull/592 Once that passes onager can merge the changes
Sign in to reply to this message.
Changes have been merged with master branch. To close the review and clean up the feature branch you can run: python ./utils/review.py close kik_ios
Sign in to reply to this message.
When I try to close I get: ERROR:root:Unsupported project name: kikplaso. Close aborted - unable to determine project name. When I originally cloned I put it in a folder named kikplaso. On Wed, Feb 17, 2016 at 7:14 PM, <reply@codereview-hr.appspotmail.com> wrote: > Changes have been merged with master branch. To close the review and > clean up the feature branch you can run: python ./utils/review.py close > kik_ios > > https://codereview.appspot.com/288960043/ >
Sign in to reply to this message.
On 2016/02/18 22:19:38, kenmacleod2013_gmail.com wrote: > When I try to close I get: > ERROR:root:Unsupported project name: kikplaso. > Close aborted - unable to determine project name. > > When I originally cloned I put it in a folder named kikplaso. > > On Wed, Feb 17, 2016 at 7:14 PM, <mailto:reply@codereview-hr.appspotmail.com> > wrote: > > > Changes have been merged with master branch. To close the review and > > clean up the feature branch you can run: python ./utils/review.py close > > kik_ios > > > > https://codereview.appspot.com/288960043/ > > Oh, that's using the close script, which probably won't work at the moment (I have an update to the script in progress. You can just close this review by clicking the circle with a cross through it near the name of the issue.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
