Skip to content

Conversation

@cmonfortep
Copy link
Contributor

@cmonfortep cmonfortep commented May 1, 2020

Task/Issue URL: https://app.asana.com/0/0/1170688450957740/f
Tech Design URL: https://app.asana.com/0/481882893211075/1170688450957739/f
CC:

Description:

What's included in this PR?
Implement logic to remove cookies preserving the ones related to fireproof website.

When clearing cookies:

  • Try to directly remove cookies from WebView database, preserving the ones with hosts related to a fireproof website
  • If process fails, fallback to remove all the cookies to avoid any leak
  • Send pixels in the following scenarios:
  • database path not found
  • database can't be opened
  • delete query fails

What's NOT included in this PR?

  • UI improvements from design feedback
  • Pixels from user interactions with Fireproof options

Steps to test this PR:

Test 1: No cookies preserved

  1. Fresh install
  2. Visit some different websites
  3. Press fire button
  4. Access device file explorer
  5. Ensure cookie database doesn't exist or database is empty

Test 2: DDG preserved

  1. Fresh install
  2. Visit duckduckgo website
  3. Set a theme (it will create a cookies)
  4. Press fire button
  5. Visit duckduckgo again
  6. Ensure theme has been preserved

Test 3: Fireproof website preserved

  1. Fresh install
  2. Visit any website (e.g: twitter.com)
  3. Perform an action that will create cookies (e.g: login)
  4. Fireproof website
  5. Press fire button
  6. Visit fireproof website
  7. Ensure state has been preserved (e.g: you are logged in)

Test 4: Database not found: Database not found, all cookies removed

  1. Replace following line from WebViewDatabaseLocator:
    Line 28: private val knownLocations = listOf("/fakePath")
  2. Fresh install
  3. Visit any website (e.g: twitter.com)
  4. Press fire button
  5. Ensure pixel m_cdb_nf sent
  6. Ensure captured and recorded
  7. Access device file explorer
  8. Ensure cookie database doesn't exist or database is empty

Test 5: Error opening database, all cookies removed

  1. Replace the following line from SQLCookieRemover, line 70:
            SQLiteDatabase.openDatabase(
                "fakepath",
                null,
                SQLiteDatabase.OPEN_READWRITE,
                DatabaseErrorHandler { Timber.e("COOKIE: onCorruption") })
  1. Fresh install
  2. Visit any website (e.g: twitter.com)
  3. Press fire button
  4. Ensure pixel m_cdb_oe sent
  5. Ensure captured and recorded
  6. Access device file explorer
  7. Ensure cookie database doesn't exist or database is empty

Test 6: Error deleting cookies, all cookies removed

  1. Replace the following line from SQLCookieRemover, line 87:
val number = delete("faketable", whereClause, excludedSites.toTypedArray())
  1. Fresh install
  2. Visit any website (e.g: twitter.com)
  3. Press fire button
  4. Ensure pixel m_cdb_de sent
  5. Ensure captured and recorded
  6. Access device file explorer
  7. Ensure cookie database doesn't exist or database is empty

Additional tests:
Instead of using firebutton, you can use auto cleaner data from settings.


Internal references:

Software Engineering Expectations
Technical Design Template

* Introduced subtle background for favicon
* changes to align with current design
disabled if home screen or domain exists in database
enabled if doesn't exist in database
@cmonfortep cmonfortep marked this pull request as ready for review May 4, 2020 06:34
@CDRussell
Copy link
Member

There is a strange transition when going from Settings to Fireproof Website settings. I'm only noticing it on my Android R (DP3) Pixel 2 so there's very good chance it's not related to any changes you've made.
device-2020-05-06-151027

Copy link
Member

@CDRussell CDRussell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally, working well and an excellent PR! 🎉

I have some concerns about the possibility of pixels getting accidentally dropped for you to consider

@cmonfortep cmonfortep requested a review from CDRussell May 12, 2020 14:39
@cmonfortep
Copy link
Contributor Author

@CDRussell this is ready for review. thanks.

}.pixelName
}

private fun sendPixelCount(counter: KMutableProperty0<Int>, pixelName: Pixel.PixelName): Completable {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slick! 🎉

@cmonfortep cmonfortep merged commit 1019ac6 into feature/cristian/fireproof_websites May 13, 2020
cmonfortep added a commit that referenced this pull request May 14, 2020
* Fireproof websites UI (#796)

* Create new database table to persist sites where cookies should be preserved

* Removed divider from bookmarks list

* bookmarks title show in single line

* background favicon compatible with dark theme

* introduce fireproof site option menu

* Fireproof option menu reacts to database state.

* Fireproof website screen created

* Remove cookies preserving fireproof websites (#808)

* Implement logic to remove cookies preserving the ones related to fireproof website.

- Try to directly remove cookies from WebView database, preserving the ones with hosts related to a fireproof website
- If process fails, fallback to remove all the cookies to avoid any leak
- Send pixels in the following scenarios:
- database path not found
- database can't be opened
- delete query fails
- database corruption

* Fireproof websites empty state and Feature pixels (#810)

* Empty state for fireproof websites screen
* Feature pixels
- User clicks on "fireproof a website"
- User undo "fireproof website" action (confirmation snackbar after fireproffing a website)
- User removed a website from "fireproof websites"
@cmonfortep cmonfortep deleted the feature/cristian/webview_database_datasource branch May 26, 2020 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants