Add Desktop Checks to `check_valid` by lougeniaC64 · Pull Request #2262 · mozilla/application-services

lougeniaC64 · 2019-11-22T15:39:16Z

Pull Request checklist

Quality: This PR builds and tests run cleanly
- cargo test --all produces no test failures
- cargo clippy --all --all-targets --all-features runs without emitting any warnings
- cargo fmt does not produce any changes to the code
- ./gradlew ktlint detekt runs without emitting any warnings
- swiftformat --swiftversion 4 megazords components/*/ios && swiftlint runs without emitting any warnings or producing changes
- Note: For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry in CHANGES_UNRELEASED.md or an explanation of why it does not need one
- Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
Dependencies: This PR follows our dependency management guidelines
- Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

thomcc

Looks good, I'd like to see the error names in particular changed to something that's a little bit more clear for external consumers, though (and the other nits, ideally).

Sign in to view

 throw!(InvalidLogin::NoTarget);
 }
+
+ let field_data = [


thomcc · 2019-11-23T06:32:05Z

+ });
+ }
+
+ // An origin with "\ \(" won't roundtrip.


This is a comment taken from desktop, and it's almost entirely devoid of meaning here (and on desktop too, these days, as it refers to a file format that is no longer in use). Something like "Desktop doesn't like origins with this pattern", or whatever, might be better.
Ditto for the comment above about a single "."

thomcc · 2019-11-23T06:48:39Z

+ #[fail(display = "Neither `formSubmitUrl` or `httpRealm` are present")]
 NoTarget,
+ #[fail(display = "`{}` contains null", _0)]
+ NullValue { field_name: String },


Honestly this and NewLineValue should be combined into InvalidCharacter, or something.
Also, it's probably better to refer to the nul character as NUL (as man ascii, the rust stdlib, and others do), in situations where it can be confused with e.g. JSON null.

Also the field names are all static strings, so we should be able to use &'static str (or even an enum) for them, not that it particularly matters that much.

thomcc · 2019-11-23T06:49:28Z

+ #[fail(display = "`{}` contains newline", _0)]
+ NewLineValue { field_name: String },
+ #[fail(display = "`{}` is a period", _0)]
+ PeriodValue { field_name: String },


I'd also probably say this and MalformedOriginParens should be combined into 'IllegalFieldValue' or something, as-is I don't think anybody would understand this without reading the source (or possibly documetnation in the future).

@thomcc I'll use the INVALID_LOGIN_UNSPECIFIED error @rfk created. I wasn't sure how specific these errors needed to be.

FWIW I like the name IllegalFieldValue better, "unspecified" was entirely me punting on a good name until we had a conversation like this one ^

I'm starting to wonder whether I should be throwing errors here at all. Maybe it makes more sense for this PR to be based on the fixup code with logic that strips these invalid characters from the respective fields. I guess this is a larger question of how important it is for this code to be similar in behavior to the desktop logic.
@rfk @thomcc Thoughts?

I think we should throw errors here. We should indicate if we're unable to insert directly. If they'd like us to add an API that attempts fixup I'd be open to that though.

Sounds good. I'll make the requested updates and finalize this PR--thanks!

rfk

Looks good to me, with a couple of nits 👍

rfk · 2019-11-28T00:52:31Z

+
+### What's new
+
+- Added invalid character checks from Desktop to `LoginsStorage.ensureValid`. ([#2262](https://github.com/mozilla/application-services/pull/2262))


I wonder if it's worth calling out the new error type, which calling code might want to catch and log or handle.

rfk · 2019-11-28T00:56:51Z

+ "Invalid login: Login has illegal field: Origin is Malformed",
+ InvalidLoginReason.ILLEGAL_FIELD_VALUE)
+ }
+ }


Oh. I'm sad you had to duplicate all of this logic into here, and I wonder if we still need this separate implementation. The only consumer I could find from some quick rging around was the Lockwise android app, where is seems to be used for testing. I wonder if we could replace this with e.g. an in-memory or tempfile-based SQLite DB. @thomcc thoughts?
(That would be for a separate PR in any case)

Oh. I'm sad you had to duplicate all of this logic into here, and I wonder if we still need this separate implementation. The only consumer I could find from some quick rging around was the Lockwise android app, where is seems to be used for testing. I wonder if we could replace this with e.g. an in-memory or tempfile-based SQLite DB. @thomcc thoughts?
(That would be for a separate PR in any case)
@rfk I hope this is possible. I felt really bad duplicating this logic, particularly because it deviates from the original rust code a tad to appease the android linter.

@rfk the goal is so that this could be used in cases where calling into Rust wasn't possible. This effectively precludes any implementation other than completely duplicating the Rust code in Kotlin.
That said I'd love to see it go, I've never liked it, and really understand why we were providing it in the first place (especially since the other products just mock things on their own usually, and you can call driectly into the rust now).
I also imagine it's not 100% compatible with the real impl (probably merely in the high-90s). Moreover, it forces us to have to have an interface defining things, which complicates our API compatibility story (every addition is a breaking change), as well as complicating the android-components mirror of this API (slightly). And, as you mention, much of its functionality can be emulated by using an in-memory DB, which can be done by passing ":memory:" as the path to the database (https://www.sqlite.org/inmemorydb.html).
That said, it shouldn't be done as part of this PR.

I filed #2310 to follow up.

rfk · 2019-11-28T00:57:37Z

+ hostname = "https://www.test.org",
+ httpRealm = "Some Other Realm",
+ password = "MyPassword",
+ username = "\u0000MyUsername2")


nit: this is named newlineLogin but it looks like it's actually testing NUL bytes rather than newlines.

rfk · 2019-11-28T01:00:20Z

+ id: "",
+ password: "hunter3",
+ hostname: "https://www.example6.com",
+ username: "\0cooluser56",


ditto here re: newlineLogin naming versus null byte.

thomcc

Looks good to me!

thomcc · 2019-12-02T18:47:52Z

+ "Invalid login: Login has illegal field: Origin is Malformed",
+ InvalidLoginReason.ILLEGAL_FIELD_VALUE)
+ }
+ }


@rfk the goal is so that this could be used in cases where calling into Rust wasn't possible. This effectively precludes any implementation other than completely duplicating the Rust code in Kotlin.
That said I'd love to see it go, I've never liked it, and really understand why we were providing it in the first place (especially since the other products just mock things on their own usually, and you can call driectly into the rust now).
I also imagine it's not 100% compatible with the real impl (probably merely in the high-90s). Moreover, it forces us to have to have an interface defining things, which complicates our API compatibility story (every addition is a breaking change), as well as complicating the android-components mirror of this API (slightly). And, as you mention, much of its functionality can be emulated by using an in-memory DB, which can be done by passing ":memory:" as the path to the database (https://www.sqlite.org/inmemorydb.html).
That said, it shouldn't be done as part of this PR.

lougeniaC64 force-pushed the add-checks-to-check-valid branch 2 times, most recently from dd6aed0 to 4369a4eCompare November 22, 2019 23:35

lougeniaC64 self-assigned this Nov 22, 2019

thomcc suggested changes Nov 25, 2019
View reviewed changes

lougeniaC64 force-pushed the add-checks-to-check-valid branch from 4369a4e to c771d6eCompare November 26, 2019 23:50

rfk mentioned this pull request Nov 27, 2019
Add reference docs about the expected behaviour of login record fields. #2158
Merged

lougeniaC64 force-pushed the add-checks-to-check-valid branch 2 times, most recently from 9c7bad9 to 3e5dc6dCompare November 27, 2019 21:17

lougeniaC64 marked this pull request as ready for review November 27, 2019 21:40

lougeniaC64 requested review from rfk and thomcc November 27, 2019 21:40

rfk approved these changes Nov 28, 2019
View reviewed changes

Added additional checks to check_valid
46155b0

lougeniaC64 force-pushed the add-checks-to-check-valid branch from 3e5dc6d to 46155b0Compare December 2, 2019 18:43

thomcc approved these changes Dec 2, 2019
View reviewed changes

lougeniaC64 merged commit e1a0f6b into masterDec 2, 2019

lougeniaC64 deleted the add-checks-to-check-valid branch December 2, 2019 19:04


		### What's new

		- Added invalid character checks from Desktop to `LoginsStorage.ensureValid`. ([#2262](https://github.com/mozilla/application-services/pull/2262))

Uh oh!

Conversation

lougeniaC64 commented Nov 22, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request checklist

Uh oh!

thomcc left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lougeniaC64Nov 25, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rfk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomcc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lougeniaC64 commented Nov 22, 2019•
edited
Loading

lougeniaC64Nov 25, 2019•
edited
Loading