[ActorOTP] Add sender address to OneTimeToken class [chromium/src : main]
Ioana Treib (Gerrit)
Ioana Treib voted and added 7 comments![]( "Open in Gerrit")
Votes added by Ioana Treib
| Commit-Queue | +1 |
7 comments
Bekzhan KassenovHi Bekzhan,
Please take a look! I am not quite sure of the exact format of the sender, can you please double-check that the parsing of the domain makes sense?
Thank you,
Ioana
Ioana TreibI think @lotte...@google.com would know this better.
The CL changed a bit so leaving the 2 examples I have here:
- nor...@example.com
- Example Support <sup...@example.org>
Are both of these possible? Are there more formats?
std::optional<std::string> ExtractDomainFromSenderAddress(Ioana TreibIf we still end up keeping this function here (see my comment on one_time_token.h), please cover this function with unit tests.
I've removed the function for now. I'll re-add it with the matching logic in the matching place.
if (sender_address.empty()) {
return std::nullopt;
}
// `sender_address` can be formatted as a plain email address
// (e.g., "sup...@example.com") or as a display name with angle brackets
// (e.g., "Example Support <sup...@example.com>"). We extract the domain
// after the '@' and strip any trailing '>' to handle both formats.
size_t at_pos = sender_address.rfind('@');
if (at_pos != std::string::npos && at_pos + 1 < sender_address.length()) {
std::string domain = sender_address.substr(at_pos + 1);
if (!domain.empty() && domain.back() == '>') {
domain.pop_back();
}
if (!domain.empty()) {
return domain;
}
} else if (at_pos == std::string::npos) {
return sender_address;
}
return std::nullopt;Ioana Treibopt: consider using gaia::ExtractDomainName
Oh, I didn't know about it. Thanks for the tip!
[[nodiscard]] const std::optional<std::string>& sender_domain() const {Ioana TreibI don't think putting only the domain here is the best approach.
1. We lose some fidelity in case if the matching logic starts to need more fields.
2. EmailOneTimeTokenFetcher was planned to be a pure backend fetcher, but now it contains some business logic.I'd recommend simply propagating the full sender address, and deciding on what's needed down in the matching logic.
SGTM!
[[nodiscard]] const std::optional<std::string>& sender_domain() const {Ioana TreibOn class evolution: it would be good to be able to group Gmail-specific fields together, and make them disjoint from potential SMS-based fields.
I don't know C++ enough to propose a good pattern here, but in proto I'd suggest putting this into a submessage with the purpose of turning it into a `oneof` later.
Documenting what I got from our offline discussion:
- std::variant could work for now, but when a third type of OTP is added that has some overlapping fields it will become awkward.
- using inheritance with a different object type per OTP type is probably the way to go, if this class becomes too complext.
// The sender domain of the OTP email. This is only relevant for Gmail OTPs
// and is `std::nullopt` otherwise.Ioana TreibThis statement is not validated on construction.
Done
bool OneTimeToken::operator==(const OneTimeToken& other) const {
return type_ == other.type_ && value_ == other.value_ &&
sender_domain_ == other.sender_domain_;
}Ioana TreibI don't think OneTimeToken is a good candidate to have `==` defined on it, since there are different use-cases with different equality requirements... We even went out of our way to design components/one_time_tokens/core/browser/util/expiring_cache.h to don't require `==` defined.
Could you please add a TODO to eliminate this operator?
Done. Added it in the header.
Related details
- Bekzhan Kassenov
- Thomas Lottermann
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bekzhan Kassenov (Gerrit)
Bekzhan Kassenov voted and added 3 comments![]( "Open in Gerrit")
Votes added by Bekzhan Kassenov
| Code-Review | +1 |
3 comments
response.sender_address()));I think absent proto field should be cast to `std::nullopt`.
response.sender_address()));Proto accessors return `const std::string&`, is that correct? Does this get auto-wrapped into an optional?
response.set_sender_address("no-r...@example.com");Please add a test for an empty response field. At least server-side implementation permits that: http://google3/java/com/google/chrome/passwords/boq/onetimetoken/service/FetchEmailOneTimeTokenAction.kt;l=161;rcl=908027161
Related details
- Ioana Treib
- Thomas Lottermann
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ioana Treib (Gerrit)
Ioana Treib voted and added 3 comments![]( "Open in Gerrit")
Votes added by Ioana Treib
| Commit-Queue | +1 |
3 comments
Proto accessors return `const std::string&`, is that correct? Does this get auto-wrapped into an optional?
It's a std::optional<std::string>, so it should create a copy of the string and wrap it in optional, yes.
I think absent proto field should be cast to `std::nullopt`.
I don't think this one can be absent. It will be an empty string. I've added an if checking that above and I'm returning invalid response now.
Please add a test for an empty response field. At least server-side implementation permits that: http://google3/java/com/google/chrome/passwords/boq/onetimetoken/service/FetchEmailOneTimeTokenAction.kt;l=161;rcl=908027161
- Thomas Lottermann
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bekzhan Kassenov (Gerrit)
Bekzhan Kassenov voted Code-Review+1![]( "Open in Gerrit")
- Ioana Treib
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ioana Treib (Gerrit)
Ioana Treib added 2 comments![]( "Open in Gerrit")
Bekzhan KassenovHi Bekzhan,
Please take a look! I am not quite sure of the exact format of the sender, can you please double-check that the parsing of the domain makes sense?
Thank you,
Ioana
Ioana TreibI think @lotte...@google.com would know this better.
The CL changed a bit so leaving the 2 examples I have here:
- nor...@example.com
- Example Support <sup...@example.org>
Are both of these possible? Are there more formats?
I'll take this comment offline, so that I can submit this CL. It's anyway only relevant for the next change.
Some tests still needed updating. Please take another look!
Related details
- Bekzhan Kassenov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bekzhan Kassenov (Gerrit)
Bekzhan Kassenov voted Code-Review+1![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ioana Treib (Gerrit)
Ioana Treib added 1 comment![]( "Open in Gerrit")
Thank you, Bekzhan!
Rodney, could you please take a look at attempt_otp_filling_tool_browsertest.cc?
Related details
- Rodney Ding
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |