Allow name removal using SSO

Using /admin/users/sync_sso endpoint to sync SSO data, it is not possible to remove someone’s name from the account, even if it is not required. In other other words, it’s not possible to change an account’s name from something to nothing. This is happening with full_name_required = false (and sso_overrides_name = true).

I believe the issue is here:

https://github.com/discourse/discourse/blob/88ecb650a98921e5e3cd345e29f84d9dd204b777/app/models/discourse_single_sign_on.rb#L268

but I’m afraid my knowledge of Ruby/Discourse isn’t that hot to submit a PR.

3 Likes

This feels like a feature request to me. At the moment you can use the admin API to clear names in cases like this. See:

1 Like

Sorry, but I disagree. I don’t see how this can be a feature request and not a bug.

I understand there’s other API endpoints which can be used. But the main point of /admin/users/sync_sso is precisely to keep this sort of data, well, in sync. It already allows to set the account name field — that works. But, it assumes name is always a required field and doesn’t allow it to be set it to blank. So it can’t be used to keep data in sync.

The code below seems to make it work as expected on my sandbox, but again, I don’t feel confident enough to submit a PR at this point. Feel free to use it/adapt it, etc.

-    if SiteSetting.sso_overrides_name && user.name != name && name.present?
-      user.name = name || User.suggest_name(username.blank? ? email : username)
+    if SiteSetting.sso_overrides_name && user.name != name
+      if SiteSetting.full_name_required && name.present?
+        user.name = name || User.suggest_name(username.blank? ? email : username)
+      else
+        user.name = name
+      end

Its a feature request, the semantics are ambiguous. Lack of name could mean

  1. Clear the name
  2. Leave the name what it was

You are asking for a protocol change here. I usually prefer explicitness so we don’t have a confusing api.

2 Likes

I see your point now.

Once again, I don’t know the internals (or ruby) enough… but there’s no way to know if the field name was provided on that request? Not present at all, don’t touch the name field of the account. Field name provided, set it (if blank, clear it). Wouldn’t this respect that protocol/behavior where only the provided fields are synched?

Sorry if this makes no sense — just trying to grasp from what I understand.

The problem is that the protocol semantics now of “name” param is there and set to blank, is the same as no “name” param. It simply does not touch it.

A change here is a change to protocol semantics. We could I guess change is so name= becomes name is blanked, and absence of name means “don’t touch name”, but people already rely on the old behavior so this is technically a breaking change.

Can you explain why you are removing names?

We are not removing everyone’s names — the users are updating their own data whenever they update their account on our website (SSO provider). We rely on /admin/users/sync_sso to keep data in sync (username, name, avatar, etc) in their Discourse account. Name is an optional field and can be set to blank/empty.

I just realized the issue happens not just with name but also with updating bio, avatar, etc: it’s not possible to keep those SSO records in sync through /admin/users/sync_sso if they need to be updated to blank/empty, regardless whether they are mandatory fields or not.

I understand the point that people may be relying on existing behavior (although nobody reported this issue before?), but if this is the protocol, it seems it has significant limitations for its purpose of synchronizing SSO records.

I’m also getting bitten by this, individual users are unable to remove their own personal information (name, avatars, bio, custom fields etc) from Discourse which has ramifications as you can imagine.

Agree with not changing the semantics of how it works now but could you not allow us to set these attributes as false in the SSO payload or something similar to be explicit about removing them?

1 Like

Previously, we were working around this SSO limitation by composing the call to /admin/users/sync_sso with another call to /u/{username} endpoint just to clear the name (if the new value for name was empty).

This however seems to also have stopped working at some point in a recent version, possibly because it checks if sso_overrides_name = true before updating the name.

So, as is, when using SSO and sso_overrides_name = true, now it seems to be impossible for the SSO provider to clear the name field on Discourse via the API.

Can you see any workaround this, @sam?

I guess an extra param for our sync_sso route is in order? Something like &clear_name not sure. This feels like such an edge case. What is the use case for blank names? Maybe just set it to username if you don’t have any and UI can suppress duplication.

It’s confusing to me that you find this to be a edge case, so I guess you must be used to instances where name is the required field.

We have it the other way around: username is what everyone must have, and name is an optional field (we use prioritize_username_in_ux = true and full name required = false). Think twitter accounts, where everyone must have a username/handle, and can optionally have a name too. I don’t think it’s an edge case scenario that one wants to clear the name field (or other personal data).

Right now, once one has filled in a name, it’s impossible to remove it. This was a sync_sso limitation that we worked around with an extra API call to update the user, but now that doesn’t work either.

We considered it, but it induces some people to assume one’s username is also their real name: we run an international forum and it’s often not clear what’s a person’s name and what’s a username (other than their positioning on the interface).

I should mention that if memory doesn’t fail me, the exact same issue happens with removing one’s avatar through the sync_sso as I think that doesn’t work either — we work around that one by providing our own URL for a default avatar.

If there’s multiple fields that one may not be able to clear in some way, perhaps an array (or csv list) of what fields to clear/reset?