DEV: simplify username suggester #14531

AndrewPrigorshnev · 2021-10-06T14:18:08Z

This PR doesn't change any behavior, but just removes code that wasn't in use. This is a pretty dangerous place to change, since it gets called during user's registration. At the same time the refactoring is very straightforward, it's clear that this code wasn't doing any work (it still needs to be double-checked during review though). Also, the test coverage of UserNameSuggester is good.

Here is the explanation of what I've done (or you can just look at the commit history in this PR). We have this code:

discourse/lib/auth/result.rb

Lines 80 to 83 in 20e70d0

    
           if SiteSetting.auth_overrides_username? && username.present? && username != user.username 
        
             user.username = UserNameSuggester.suggest(username_suggester_attributes, user.username) 
        
             change_made = true 
        
           end

If we inline username_suggester_attributes, we'll get this:

    if SiteSetting.auth_overrides_username? && username.present? && username != user.username
      user.username = UserNameSuggester.suggest(username || name || email, user.username)
      ...
    end

But we check if username.present? in the if condition, so it's always present inside the if block, so it's clear that || name || email never gets called, we can remove it:

    if SiteSetting.auth_overrides_username? && username.present? && username != user.username
      user.username = UserNameSuggester.suggest(username, user.username)
      ...
    end

Now, we pass user.username as a second parameter to UserNameSuggester, the name of that parameter is allowed_username. If the suggester has found a suggestion that's equal to allowed_username it stops looking for another available usernames and just returns this suggestion. We need it to avoid the situation when the suggester tries to suggest a username of the current user (without this parameter, the suggester would think that this name isn't free, when in fact it's fine to use this username).

But, we check if username != user.username in the condition of if block, so there is no point in passing user.username to the username suggester, we already know that this isn't the username of the current user.

    if SiteSetting.auth_overrides_username? && username.present? && username != user.username
      user.username = UserNameSuggester.suggest(username)
      ...
    end

There is only one more place when we pass the second parameter to UserNameSuggester and that place can be simplified using the same steps.

After that, we can just remove the second parameter from UserNameSuggester.suggest which simplifies the code significantly and also makes the signature logical. The signature was confusing before:

discourse/lib/user_name_suggester.rb

Line 7 in 20e70d0

def self.suggest(name_or_email, allowed_username = nil)

Why pass allowed_username to UserNameSuggester? I mean, if we already know the allowed username, why call UserNameSuggester at all? We can just use that name. After this refactoring, we'll have just:

def self.suggest(name_or_email)
  ..
end

…ly we always give suggestion based on `username` not `username || name || email`

… `allowed_username` because we already checked in enclosing if that `username != user.username`

…an remove the parameter

…ions of usernames

eviltrout

Looks good to me! Thanks for the explanation.

AndrewPrigorshnev · 2021-10-06T19:47:45Z

@eviltrout thank you for review!

ZogStriP · 2021-10-06T19:57:00Z

lib/user_name_suggester.rb

  GENERIC_NAMES = ['i', 'me', 'info', 'support', 'admin', 'webmaster', 'hello', 'mail', 'office', 'contact', 'team']
  LAST_RESORT_USERNAME = "user"

-  def self.suggest(name_or_email, allowed_username = nil)


Did you change all the places where we call this method using the 2nd arg?

Yes, there were only two such places in Core. There are no such calls from plugins.

ZogStriP · 2021-10-06T19:57:43Z

app/models/discourse_single_sign_on.rb

        user.username = username # there may be a change of case
-      elsif user.username != username
-        user.username = UserNameSuggester.suggest(username || name || email, user.username)
+      elsif user.username != UserNameSuggester.fix_username(username)


Why this change?

I've discovered this after finishing this fix and opening PR, a test failure helped to find it.

We fix the username if it is too long or contains disallowed characters. So if we want to check if username already exists, we should be checking fixed versions. This part was actually the important job that we were doing inside the username suggester when passing the second argument:

# we're kind of checking if a user already was registered with this username, # but it won't work, for example, if sso sends us a username with invalid characters: elsif user.username != username # we pass user.username as the second parameter, # just to compare it with a fixed version of the first parameter inside the suggester # in fact we comparing usernames the second time inside the suggester user.username = UserNameSuggester.suggest(username, user.username) end

It's better to check it once, explicitly, and don't call the suggester at all if a user already has this username.

I think the change makes sense, but -- and that's not your fault -- this piece of code needs some changes. Maybe in another PR?

We need to use UsernameChanger.change() instead of simply updating user.username in order to fix Updating mentions when username is changed via SSO

user.username.downcase should be replaced with user.username_lower

username.downcase should be replaced with User.normalize_username(username) to correctly handle Unicode

It might be worth checking that this and this bug reports are fixed, so that we can close them.

gschlager · 2021-10-07T12:43:42Z

app/models/discourse_single_sign_on.rb

        user.username = username # there may be a change of case
-      elsif user.username != username
-        user.username = UserNameSuggester.suggest(username || name || email, user.username)
+      elsif user.username != UserNameSuggester.fix_username(username)


I think the change makes sense, but -- and that's not your fault -- this piece of code needs some changes. Maybe in another PR?

We need to use UsernameChanger.change() instead of simply updating user.username in order to fix Updating mentions when username is changed via SSO

user.username.downcase should be replaced with user.username_lower

username.downcase should be replaced with User.normalize_username(username) to correctly handle Unicode

It might be worth checking that this and this bug reports are fixed, so that we can close them.

gschlager · 2021-10-07T12:47:18Z

lib/auth/result.rb

-    if SiteSetting.auth_overrides_username? && username.present? && username != user.username
-      user.username = UserNameSuggester.suggest(username_suggester_attributes, user.username)
+    if SiteSetting.auth_overrides_username? && username.present? && UserNameSuggester.fix_username(username) != user.username
+      user.username = UserNameSuggester.suggest(username)


Again, work for another PR: This might have the same problem as the SSO code and would require the usage of UsernameChanger.change()? On first glance, it doesn't seem to handle case changes. Maybe code can be reused between SSO and here?

@gschlager thank you for comments, these all make sense! I'm going to look at this before merging it, maybe will do a part in this PR.

It might be worth checking that this and this bug reports are fixed, so that we can close them.

I've checked them, both was fixed before this PR. We already have a test case for the first one, and I've added cases for the second one here.

@gschlager also I eventually decided to address all your other comments on subsequent PRs, I'm working on them now.

AndrewPrigorshnev added 3 commits October 5, 2021 21:11

REFACTOR: we check if username.present? in enclosing if, so effective…

9e33cbc

…ly we always give suggestion based on `username` not `username || name || email`

REFACTOR: there's no need to pass user.username to the suggester as…

4971bc7

… `allowed_username` because we already checked in enclosing if that `username != user.username`

REFACTOR: there aren't calls that pass allowed username anymore, we c…

63214f6

…an remove the parameter

AndrewPrigorshnev requested review from ZogStriP and gschlager October 6, 2021 14:18

REFACTOR: when overriding usernames we should be comparing fixed vers…

1c007b9

…ions of usernames

eviltrout approved these changes Oct 6, 2021

View reviewed changes

ZogStriP reviewed Oct 6, 2021

View reviewed changes

gschlager approved these changes Oct 7, 2021

View reviewed changes

Merge branch 'main' into dev/simplify-username-suggester

ee5c47b

AndrewPrigorshnev added the ⚠ DO NOT MERGE label Oct 12, 2021

AndrewPrigorshnev added 4 commits October 12, 2021 15:30

Merge branch 'main' into dev/simplify-username-suggester

28998c2

Merge branch 'main' into dev/simplify-username-suggester

42b74e5

Move the test

48493bc

Add one more test

682ce59

AndrewPrigorshnev mentioned this pull request Oct 18, 2021

FIX: extract and fix overriding of usernames by external auth #14637

Merged

Merge branch 'main' into dev/simplify-username-suggester

504639a

AndrewPrigorshnev removed the ⚠ DO NOT MERGE label Oct 27, 2021

AndrewPrigorshnev merged commit 19d95c6 into main Oct 27, 2021

AndrewPrigorshnev deleted the dev/simplify-username-suggester branch October 27, 2021 10:41

AndrewPrigorshnev mentioned this pull request Dec 6, 2021

FIX: auth incorrectly handles duplicate usernames #15197

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

DEV: simplify username suggester #14531

DEV: simplify username suggester #14531

Uh oh!

AndrewPrigorshnev commented Oct 6, 2021

Uh oh!

eviltrout left a comment

Uh oh!

AndrewPrigorshnev commented Oct 6, 2021

Uh oh!

ZogStriP Oct 6, 2021

Uh oh!

AndrewPrigorshnev Oct 7, 2021

Uh oh!

ZogStriP Oct 6, 2021

Uh oh!

AndrewPrigorshnev Oct 7, 2021 •

edited

Loading

Uh oh!

gschlager Oct 7, 2021

Uh oh!

gschlager Oct 7, 2021

Uh oh!

gschlager Oct 7, 2021

Uh oh!

AndrewPrigorshnev Oct 7, 2021

Uh oh!

AndrewPrigorshnev Oct 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

	if SiteSetting.auth_overrides_username? && username.present? && username != user.username
	user.username = UserNameSuggester.suggest(username_suggester_attributes, user.username)
	change_made = true
	end

DEV: simplify username suggester #14531

DEV: simplify username suggester #14531

Uh oh!

Conversation

AndrewPrigorshnev commented Oct 6, 2021

Uh oh!

eviltrout left a comment

Choose a reason for hiding this comment

Uh oh!

AndrewPrigorshnev commented Oct 6, 2021

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!

AndrewPrigorshnev Oct 7, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

AndrewPrigorshnev Oct 7, 2021 •

edited

Loading