Cargo owner username disambiguation - #14213
Conversation
This comment has been minimized.
This comment has been minimized.
| let username = user.username.to_owned(); | ||
|
|
||
| if let Some(oauth_github) = oauth_github | ||
| && oauth_github.login != user.username |
There was a problem hiding this comment.
The surrounding username lookups are case-insensitive, but this comparison is case-sensitive. A crates.io username Alice associated with GitHub login alice would be reported as ambiguous even though both namespaces treat those names as equal. Could we compare using the same canonicalization and add a case-only regression test? _ vs. - might also be relevant for this.
There was a problem hiding this comment.
Thanks, the case-only comparison and regression tests look fixed. One part is still inconsistent with the new canonical username behavior, though: this comparison uses lowercase only, so foo-bar and foo_bar are still treated as different. Since the new index and #13903 use canon_username(), could we use the same canonicalization here and add a separator regression test?
| login: &str, | ||
| ) -> QueryResult<OauthGithub> { | ||
| oauth_github::table | ||
| .filter(lower(oauth_github::login).eq(login.to_lowercase())) |
There was a problem hiding this comment.
oauth_github.login is not unique (yet), so this unordered .first() can resolve github:<name> to an arbitrary crates.io account. The previous User::find_by_login() explicitly ordered by descending GitHub ID. Could we preserve the intended deterministic tie-breaker, or reject multiple matches, and cover duplicate case-insensitive logins in a regression test? 🙏
There was a problem hiding this comment.
updated to preserve original functionality of explicitly ordering by Github ID.
There was a problem hiding this comment.
Thanks, ordering by descending account_id restores the deterministic selection. Could we also add the duplicate case-insensitive login regression test requested here? That is the state which makes the ordering observable and protects it from being removed again later.
| ) -> QueryResult<User> { | ||
| User::query() | ||
| .filter(lower(users::gh_login).eq(login.to_lowercase())) | ||
| .filter(lower(users::username).eq(username.to_lowercase())) |
There was a problem hiding this comment.
Both new lookup paths filter on lower(...), but there are no matching functional indexes: the existing user index covers lower(gh_login), while the OAuth table only has an index on user_id. A request can resolve up to ten owners, causing repeated full scans of these identity tables. Could we add indexes for lower(users.username) and lower(oauth_github.login) as part of this change? Or given #13903 I think we might actually want canon_username(users.username) and lower(oauth_github.login)?
There was a problem hiding this comment.
good catch. i've added functional indexes for both columns.
There was a problem hiding this comment.
The indexes themselves now have the intended expressions, but the users lookup still filters on lower(users::username). PostgreSQL cannot use the canon_username(username) index for that expression, and the lookup does not canonicalize - and _. Could we change the query to use canon_username() consistently on both sides and add coverage for the separator case?
There was a problem hiding this comment.
updated to use canon_username and added coverage for separator cases. thanks!
454d638 to
a420795
Compare
This comment has been minimized.
This comment has been minimized.
f43ce65 to
64efb97
Compare
542309c to
6811381
Compare
|
@Turbo87 thanks for the review! I've addressed your comments. the pr is ready for re-review. |
a228f14 to
8a44eae
Compare
This comment has been minimized.
This comment has been minimized.
| pub gh_login: String, | ||
| // Rename this field to gh_login when gh_login is removed from this struct. | ||
| #[diesel(select_expression = oauth_github::login.nullable())] | ||
| pub gh_username: Option<String>, |
There was a problem hiding this comment.
this is the same as gh_login but reading from oauth_github instead of the users table. I considered a change to stop reading from users.gh_login and start reading from oauth_github.login throughout the codebase instead (similar to #14258), but I think that would be a much bigger change that would warrant being tackled in a separate pr
|
@carols10cents would also love your review on this when you have some time |
185aa74 to
1f3ff8a
Compare
This comment has been minimized.
This comment has been minimized.
1f3ff8a to
4c528c3
Compare
There was a problem hiding this comment.
Took a quick look and saw one little thing, and I agree with the comments @Turbo87 made.
| }, | ||
| /// GitHub user (e.g. `github:username`). | ||
| GitHub(&'a str), | ||
| /// crates.io user (`crates.io:username`). |
There was a problem hiding this comment.
I think this comment should be:
| /// crates.io user (`crates.io:username`). | |
| /// crates.io user (`cratesio:username`). |
based on the rest of this PR, right?
There was a problem hiding this comment.
I think I'd prefer crates.io. is there a technical reason why we would need to drop the .?
There was a problem hiding this comment.
there is no technical reason for dropping the . here, but i updated this to be consistent with all other comments and error messages that include cratesio:username. i don't have a strong preference for either cratesio or crates.io, so happy to update this to crates.io everywhere if you think it would be better.
There was a problem hiding this comment.
yeah, I think unless there is a technical reason why we would need to avoid the . character we should use our usual brand name and adjust it in the other "comments and error messages" too :)
This comment has been minimized.
This comment has been minimized.
… ownership check in case of shared logins
3e8fc63 to
b09e6a3
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@Turbo87 @carols10cents ready for re-review. thanks! |
4 participants
fixes: #13769