Skip to content
This repository has been archived by the owner on Dec 14, 2021. It is now read-only.

Metadata cleanups for datastore and autofill. #1102

Merged
merged 4 commits into from
Dec 10, 2019

Conversation

jhugman
Copy link
Contributor

@jhugman jhugman commented Dec 2, 2019

Fixes #1064.

This PR does a number of related things, each too small for their own PR:

  • Detect which credential the user selected during autofill send a touch to the data store.
  • On touch update the list, so that the Item list when ranked by recency is updated.
  • Add a change to the updateItem method to allow password changes to be detected, and the timePasswordChanged field to be updated.
  • On capture from autofill, the item list is updated, so new credentials show up immediately.

@jhugman jhugman requested a review from a team as a code owner December 2, 2019 19:21
Copy link
Contributor

@eliserichards eliserichards left a comment

Choose a reason for hiding this comment

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

Looks good! I have a few questions, but pending those I am happy to ✅

Comment on lines +170 to +174
if (isRunning) {
// we might have already been called when logging
// a previously chosen dataset.
return
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. Do we maybe want to fail/throw an exception here instead of just returning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

initializeService can be called twice. This makes sure it's idempotent (i.e. we only actually initialized once).

ServerPassword(
id = emptyId,
hostname = domain,
username = capturedUsername,
password = capturedPassword,
formSubmitURL = parsedStructure.webDomain ?: domain
formSubmitURL = webDomain ?: domain
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a formal definition for what the formSubmitURL should be somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the formActionOrigin, which we don't get. We assume here that it's the origin of the page itself (which we do), and is likely given it's a login page.

val structure = request.fillContexts.last().structure
val activityPackageName = structure.activityComponent.packageName
if (this.packageName == activityPackageName) {
callback.onSuccess(null)
callback.onFailure(null)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these being changed from success -> failure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a bit debatable whether this is a failure or success.

But from the look of the event history, not having any fillable things is likely to be a failure.

Copy link
Contributor

@eliserichards eliserichards left a comment

Choose a reason for hiding this comment

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

:shipit: 🚀 Thanks for answering my questions! This looks great!

@jhugman jhugman merged commit df21ab6 into master Dec 10, 2019
@jhugman jhugman deleted the jhugman/1064-autofill-cleanup branch December 10, 2019 16:55
eliserichards pushed a commit that referenced this pull request Dec 13, 2019
* Touch items as they are used to autofill

* Add a scheme to the form action origin / formSubmitURL

* Added original credential to the updateItem flow

This will check the updated item to check if the password has changed and update the time accordingly.

* gradlew lint
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cleanup add/update new login data
2 participants