alkem-io
Use Kratos subjectID
Description
As an architect I want to have a single identity source As a user I want to be able to update my email address
Acceptance criteria
- [ ] rename accountUpn field on user profile to be subjectID
- [ ] add mutation to set the subject ID for all users to the kratos subject ID
- [ ] creation of new users to set the subjectID
- [ ] business logic that uses the email as the core identifier, also to Kratos, to be updated to use the subjectID
Additional Context
The subjectID is needed to enable robust identities in Matrix The usage of subjectID later enables the updating of a users email address as the email address is no longer the link between Kratos + Alkemio
Areas that will be affected
To be added during the refinement
OK, there are some changes and my 2 cents:
-
I decided against reusing accountUpn field. We should not mix introduction of this new feature with deprecation and sanitization of accountUpn through system, for number of reasons. Newly added column/field is called "authId", as if we separate userId and id from authentication system, which means we remains authentication provider agnostic, then it is more clear in my opinion, than subjectID. But if someone insists, I can change.
-
Universal user ID which must be used through all system must be, after all, not Kratos ID (as Kratos technically stays as replaceable authentication provider) but Alkemio userID. Synapse user ID's must be also formed from Alkemio userID, and that one must be present also in OIDC claims. (coming next).
-
pull request #5582 implements database mutation, and support of setting of authId on user creation.
-
it also introduces internal REST EP which can be used to resolve mapping authID->userID. When it is called with authID which is valid Kratos ID but user is not yet created in alkemio DB, it will create such user (covers edge case here when user logs into Synapse via OIDC without doing at first login in the web-client). All information available at creation of user on first login, also available present in this case, so it is direct equivalent, which covers not only Synapse, but all possible future potential OIDC uses.
-
I would postpone implementation of business logic change for next release, as it requires some serious testing. #5582 does not affect existing business logic and functionality by design (except setting authId field on user creations).
-
most (actually almost all) of business logic in the system must rely on Alkemio userID, and only few specific requests to Kratos must use Kratos ID. Email, indeed, should never fly through the system. This is, in addition, covers some concerns about privacy. By the way, we also need to remove users emails from logging. This is also bad practice in terms of privacy.
-
I was also toying with an idea to add Alkemio userID to metadata on the Kratos side, which would make couple of edge cases easier, but ruled against it due to number of reasons.
-
With all bells, whistles, tests, validations etc, patch turned out to be quite heavy, about 2.5K lines of code. Not couple of lines :)
@antst super, thanks for the update. Agree with userID being core, and that krator authentication ID in principle being replaceable. Note that the user.communicationID I think with this set of updates also becomes redundant. 4. Prefer to only have one actual creation path, so is it possible that the current user creation path also goes through this - or that the rest end point uses the same user creation logic? 5. + 6. agree, lets get at least the authentication ID operational. So we need a separate issue to track moving away from using email, will create that. 7. agree, lets make the dependency / knowledge one way 8. yes a lot of code, especially related to the testing setup that has been put in place. To discuss if all of that is needed, esp re migration testing.
One structural piece that must be changed is that the migration logic cannot have any dependency back into the main server code.
@antst super, thanks for the update. Agree with userID being core, and that krator authentication ID in principle being replaceable. Note that the user.communicationID I think with this set of updates also becomes redundant.
yes, we push this, then push business logic changes, then we analyze and sanitize from deprecated. Step by step. I don't like huge massive changes where everything is a sandy ground.
Prefer to only have one actual creation path, so is it possible that the current user creation path also goes through this - or that the rest end point uses the same user creation logic?
Yes, it is. I have to recheck, but it has to be different calls to the same logic, as there is not "second logic". Same information on input, same result on output.
- agree, lets get at least the authentication ID operational. So we need a separate issue to track moving away from using email, will create that.
Yes this is separate topic, as there will be number of things involved across the system.
- yes a lot of code, especially related to the testing setup that has been put in place. To discuss if all of that is needed, esp re migration testing.
We are going to migrate production system. I wanted to I put enough of safety checks around. Once we migrate it all, we just revert most of this PR. As there is no single reason to keep it in codebase after. Keeping schema migration is enough.
One structural piece that must be changed is that the migration logic cannot have any dependency back into the main server code.
It is temporary, once we migrate, we remove this code. No need to increase size and complexity of codebase. it is single-use code.
Thanks for clarifying - agree with the logic with only area of discussion is the last point re the migration. I would prefer not to touch that area once it is committed, and that the migrations are purely additive in their logic (not that oh we had that migration but then changed it) - which is why was suggesting the logic for doing the updates of the authenticationID field was a mutation in its own module that we can manually trigger rather than in the migration code. Should be an easy enough switch - and then we mark that whole module as deprecated.
@techsmyth I am preparing new simplistic PR. But, to be honest, I would leave mutation out of code altogether, as I highly doubt it has future life. It is just adding code for one-time use. As EP which resolves mapping (and which follows the same route as first login), in case of missing authenticationID, will just do remapping, to support current flow of changing user emails/identities, It is enough to write 20-liner, which loop through all userIDs and call this EP for every userID. And run this as k8s job.