Reevaluate "rename-user" command

[hallundbaek]

Looking at the reasoning for its addition in the first place #2818, it seems to be necessitated by the inability to support multiple idP's.

As far as I can see, linking multiple ids to a single user was solved in #2366, which should (?) have made this command obsolete. But there is probably something that I am missing in this regard.

Why should it be reevaluated? Looking at the CLI implementation:

rems/src/clj/rems/main.clj

Lines 265 to 275 in 3f4ebbc

	"rename-user"
	(let [[_ old-userid new-userid] args]
	(if (not (and old-userid new-userid))
	(do (usage)
	(System/exit 1))
	(do (println "\n\n*** Renaming a user's identity can't easily be undone. ***\nType 'YES' to proceed or anything else to run a simulation only.")
	(let [simulate? (not= "YES" (read-line))]
	(println (if simulate? "Simulating only..." "Renaming..."))
	(mount/start #'rems.config/env #'rems.db.core/*db*)
	(rems.service.fix-userid/fix-all old-userid new-userid simulate?)
	(println "Finished.\n\nConsider rebooting the server process next to refresh all the caches, most importantly the application cache.")))))

The following wording causes a bit of concern towards its implementation:

Renaming a user's identity can't easily be undone.

It seems to say that the very operation you are performing, cannot easily be done. I'm not sure how to parse this as an admin.

In the changelog, this feature is introduced as "experimental", but this is not visible in the CLI.

rems/CHANGELOG.md

Line 275 in 3f4ebbc

- There is a new experimental CLI command `rename-user` to handle the case where a single user's identity has changed. After running this command, one should also refresh the caches of the application server by restart.

Its implications in /src/clj/rems/service/fix_userid.clj are quite far reaching, making it heavy to maintain.

The naming of the command rename-user seems more innocent than its semantics, you are not renaming a user, you are updating the user's internal identifier, with all the cascading effects of that operation.

Something along the lines of update-internal-user-identifier would probably be more fitting, but ideally its necessity should be made obsolete through other means, which I can see that you are aware of in #2818.

[Macroz]

Sometimes a user's IdP identity changes although there is no change of user's real identity. This can be the as simple as changing an email address due to changing name. Sometimes a user also has multiple IdPs that they can use, and they may have inadvertedly used the wrong one, or more than one. This is a problem with IdPs in general, that nothing can guarantee an identity (hash) remaining the same although the user is in fact the same. REMS uses the identity provided by the IdP as the internal identity (:userid). Should the Idp provided identity change, the user may lose access to applications or other functionality.

The rename-user command is a low level command to change the identity of a user, for example to merge two identities into one.

There exists other functionality (user-mapping) that allows several identities from IdPs lead to one internal REMS identity (:userid). This is achieved by configuring multiple identifying IdP attributes that are looked for a match. However, this may not always be satisfactory.

It's likely that the experimental status could be removed and the command exposed. This has certainly been used in production already. If the IdP is able to provide the new and old identities in some attributes, it is possible to use the user-mapping functionality instead.

[hallundbaek]

Yeah, I understand its necessity given that REMS uses external idP names as internal identifiers.

But I would say that in and of itself is a bit of an issue. Another implementation would have a unique internal identifier map to one or more external identifiers, that implementation would not need the functionality of rename-user to be as wide reaching as the current implementation. It would simply either add or replace the identifier in the mapping and be done.

Given the current implementation, my remaining points still stands: The command at face value seems less involved than it is, and the wording around its use is quite confusing.

Both are mostly just technical debt. But migrating to an implementation that does not rely on external identifiers would reduce complexity quite drastically.

[Macroz]

REMS does not as of today depend on what the :userid identifier value is, it is simply something created when the user first logs in, based on what the IdP has passed on to REMS. Instead of a pure synthetic id REMS opts for a natural id, one of the passed in attributes. Some IdPs provide a hash of some identifying attributes, but for clarity REMS is deterministic with the one it chooses and shows it as it is, as per it has been configured.

Using a natural id could be useful if the identities are stable. When an outside service wants to refer to a REMS user it is good for them to be able to fetch the data without first having to figure out what the identity of that specific user is (they may know how their IdP assigns identities, the identities can come from an outside identity repository). REMS does support specifying as the user identity the :userid whatever it is, or any of the configured identifying attributes of the day the query is done (provided that the user has logged-in and they have been managed to be stored, i.e. REMS knows about them).

The identity of a user is completely in the custody of REMS. The user-mapping feature allows either automatic (if IdP can provide old and new identity) or manual (inserting rows to the user mapping table) management of the identity of users. However, this does not solve all the cases where an outside mistake would cause problems for a user. rename-user is a way to fix them without leaving a messy state or history.

I see no direct technical debt in this. It is a matter of REMS having to deal with an imperfect world of IdPs and identities. There is certainly some complexity in that code, but that is pretty much unavoidable. For an outsider, to write the necessary SQL (or code) to fix an issue with a user, it would be too much to ask.

[aatkin]

Migrated the issue into discussions, as there did not seem to be a concrete bug or feature request. You may open a new issue if this was not the case.