Implement UserId

[deanmarcussen]

Related to #5409

This is a start towards having a UserId throughout the OC ecosystem.

It implements a UserId property for Users

From the issue

The identifier of a user account should not be editable

Done in this pr

Implications

We can't use the UserName as an identifier, as it is provided by user input

Done in this pr

We need to create a new property to store the identifier

Done in this pr

We can use the existing usernames as identifiers for existing accounts provided we can't edit identifier anymore. That will prevent a data migration.

Done in this pr (see notes on where this maybe an issue)

We need to understand what User.Identity.Name represents and how it's used relative to the username property, which was used an an identifier as of now.

In the context of permissions, this line from the ContentTypeAuthorizationHandler
user.Identity.Name == content.Owner;

This should related to the Name Claim property on claims.
Nothing in this pr changes that.
The UserId (and previously DocumentId) are mapped to the NameIdentifier claim, ref: https://docs.microsoft.com/en-us/dotnet/api/system.security.claims.claimtypes.nameidentifier

User.Identity.Name should map to the Name claim, ref: https://docs.microsoft.com/en-us/dotnet/api/system.security.claims.claimtypes.nameidentifier

So for this pr nothing changes in relation to content ownership (next pr).

We should not use the document id anymore but the new identifier.

Done in this pr

The idea here is to introduce the UserId property on the IUser as a guid / generated id, and later consider this in the context of content item ownership.

I suspect we will want to consider storing both the Owner property as a historical value / friendly name for the ui, and an OwnerId which would map to the Claim.NameIdentifier -> UserId

[agriffard]

Please fix the conflicts.

[Skrypt]

What does it breaks ? 🥰

[Skrypt]

Can't we create simply a SQL migration that would generate UID's for all the current existing user accounts instead of saving the actual NormalizedUserName in the UserId?

Here is some SQL to generate UID's on the UserIndex, Document, ContentItemIndex table for existing users :

DECLARE @count int
DECLARE @i int = 1
DECLARE @Migrate bit = 1
DECLARE @DocumentId int
DECLARE @NormalizedUserName nvarchar(255)
DECLARE @UserId nvarchar(26)

IF(@Migrate = 1)
BEGIN
	SET @count = (SELECT COUNT(*) FROM UserIndex)
	SET @i = 1
	WHILE(@i <= @count)
	BEGIN
		WITH OrderedContentItem AS
		(
			SELECT DocumentId, NormalizedUserName,
			ROW_NUMBER() OVER (ORDER BY Id) AS 'RowNumber'
			FROM UserIndex
		)
		SELECT @DocumentId = DocumentId, @NormalizedUserName = NormalizedUserName FROM OrderedContentItem WHERE RowNumber = @i
		SET @UserId = LEFT(LOWER(REPLACE(NEWID(), '-', '')), 26)

		UPDATE UserIndex SET UserId = @UserId WHERE DocumentId = @DocumentId

		-- We update Users in the Document table
		UPDATE Document
		SET Content = JSON_MODIFY(Content, '$.UserId', @UserId)
		WHERE Id = @DocumentId

		-- We add UserId on all ContentItems in Document table
		UPDATE Document
		SET Content = JSON_MODIFY(Content, '$.UserId', @UserId)
		WHERE Type = 'OrchardCore.ContentManagement.ContentItem, OrchardCore.ContentManagement.Abstractions'
		AND JSON_VALUE(Content, '$.Owner') = @NormalizedUserName

		SELECT @i = @i + 1
	END
END

-- Update ContentItemIndex table
UPDATE ContentItemIndex SET UserId = (SELECT JSON_VALUE(Content, '$.UserId') FROM Document WHERE Id = ContentItemIndex.DocumentId)

@deanmarcussen Also in ContentItemIndex table we should also add a UserId column now.

[Skrypt]

In OrchardCore.ContentManagement migrations.cs file.

       public int UpdateFrom5()
        {
            SchemaBuilder.AlterTable(nameof(ContentItemIndex), table => table
                .AddColumn<string>("UserId", column => column.Nullable().WithLength(26))
            );
            return 6;
        }

[deanmarcussen]

So I did this to be migration free, because @sebastienros didn't want to run a migration.

Otherwise when we do the OwnerId on ContentItems we will also have to run a migration against all the content items to move the owner over as well.

Lot of risk of that failing due to timeouts on sites with large quantities of content items (or large quantities of users).

Because of that I can't set the UserId column to 26 because the current UserName property is not limited in length. It defaults to 256 on SQL Server, but will be different on other databases (probably only sqlite as it ignores max lengths).

(we should probably fix that in code as well as currently it will just throw an exception depending on the database)

If we did a migration we'd probably have to do it with YesSql, rather than SQL directly, to get support across all the databases (your sql script is great, but would it work on anything that isn't MS SQL?, i.e. postgres / mysql etc)

[Skrypt]

I personally think it will require a SQL migration because a YesSQL migration could potentially cause a resquest timeout (maybe that's what meant @sebastienros). Also, I for exemple have a UserProfileIndex table which will require me to do a "custom migration" to a new UserId column .

Of course, the SQL I wrote is a mock-up and works only in SQL Server but nothing prevents to compose the equivalent queries for other rdb's. Here, I just tried to show that it can be done without requiring YesSQL or adding a complex fixed code in OC. We're still in RC1, so we can still do breaking changes,

I'd rather compose a SQL migration with some comments/procedures rather than trying to make something that migrates the data correctly only 90% with YesSQL... or that would require some changes to make it work without throwing a request timeout for large databases. Also using the email as a userid doesn't make it harder to guess like a uid is, I'd prefer generating a uid for all of them and be done with it at least.

Otherwise when we do the OwnerId on ContentItems we will also have to run a migration against all the content items to move the owner over as well.

The SQL Query covers that part and would be executed directly on the DB itself. Which means on prod the website would need to be closed for maintenance for the time the Query runs. And I think this will be required for all db types.

@sebastienros On ne fait pas d'omelettes sans casser des œufs pour celle-ci 😉

An other option would be to create a small cross-plat app that could run the migration as a SQL Query with options to add the UserId on selected custom Index tables ; with database backup procedure and error logging. (More complex, but more efficient)

[agriffard]

Among every pending PRs, please focus on this one.
Explaining if there are breaking changes and what they are is of course an important part.

[sebastienros]

Upgrade step, from the admin.

[deanmarcussen]

Ok @Skrypt I made an OwnerId property on ContentItem and an upgrade step.

Not as automatic as you might have liked.

Still much todo, and merge conflicts to resolve, but some progress...

[Skrypt]

Ok will pick up from there and make changes in a separate branch if I need to.
I will try to fix conflicts here though.

[deanmarcussen]

@kevinchalet and @MichaelPetrinolis

Here I am trying to do big risky things (without breaking anything) for good reasons...

Would appreciate any input as to whether this will have any flow on effect on OpenId by changing all the security to use Claims.NameIdentifier over User.Identity.Name or any other thoughts?

[agriffard]

Is the upgrade step completely transparent?

[deanmarcussen]

It is, as there is no longer an upgrade step, but equally this pr is not finished / ready

Still to be discussed.
Should we still store the OwnerName property somewhere?

The blog recipe uses it for displaying the owner of a content item, and this is now being translated into a Guid.

Should we have something like we do for tags, where OwnerName becomes a less well known property (not updated on change).

Or should we be doing a lookup on the user table to get an owner name? (extra sql call)

[Skrypt]

It is quite impossible to make the upgrade step completely transparent. Some people will have custom index providers that will need to be updated. If we talk about a fresh clean install then it will work. If we talk about existing websites with custom code it will surely require them to fix the code everywhere they make a relation between the User and the Owner name...

We talk here about custom IndexingProviders, ContentEventHandlers and else.

[sebastienros]

About OwnerName, why not use the Author property that should contain the displayed value?

[Skrypt]

@sebastienros I'm not sure what you are proposing. An author is quite different than a owner?

[sebastienros]

I understand that, still one rock two birds.
First bird is that we don't need to do a query to get the owner's name, we can use the author's name for the current version.
Second bird is that what we are displaying (on the front-end at least) should probably not be the owner of the content item, but what we think is the Author?

Then from what I see in the code, the author is "kept" across edits, which means the field is only changed if the users decides to do it, and we can even put names of people that don't exist. Audit trail with still record who (user id, TBD) changed the content.

[Skrypt]

I think we need to define what is Author, Owner and what they should be used for. I see here in Github they use something similar but they also allow to link a user to a shortcode @Skrypt

What would happen for example if we would change the Owner name now that we would have a UserId. Does that @skrypt-also shortcode will still render the proper UserId page? Same thing with the ContentItemIndex. We could keep the Owner name as a reference "in time" and also the "author" for that same matter. Though, the UserId should also be resolved in any case. So, if someone changes it's UserName, we don't require to update every content items he is related to. @skrypt here should be able resolve to 2 different UserId profile page if anyone took it in different time spans. Of course this seems awkward and maybe this is not what we want but then we always have the second option of migrating the database on every UserName change to update the Owner name on every ContentItem.

So basically 2 different use cases and which one we want to follow as a default?

I think Audit Trail is an other topic of it's own unless we decide to have it enabled all the time by default.

[deanmarcussen]

About OwnerName, why not use the Author property that should contain the displayed value?
I understand that, still one rock two birds.

I think this is the way to go - we are only talking about the sample blog recipe.

For the future I think Owner and Author will never be satisfactory for all purposes.
Owner is an Authorization concern, and Author a friendly helper.

One of the issues that bought this pr about was a UserPickerField - which we can't do, if we don't have the UserId to fix it against.

It won't go into this pr, but it would follow shortly after, and I think it addresses the need for people to be able to define their own identification properties, for the variety of reasons / use cases, that they may have.

So my proposal is keep it simple oob, and allow extension via a UserPickerField for those who want it / need it.

[deanmarcussen]

Updated after review with @sebastienros yesterday

• It now runs a small migration on the user to set the new UserId property to that of the existing UserName, and update the YesSql Index. This is the current value of the Owner of any existing Content Items. Ensuring that existing content is still owned, by the same user, and ensuring that the UserId property is unique and set in the indexes.
• New users are assigned a unique UserId
• New content made by any user assigns their UserId value, whether guid, or unique name, to the ContentItem.Owner property

Reviews welcome.

@Skrypt you have done much with content permissions recently, so a test drive to ensure all continues correctly would be helpful.

[Skrypt]

LGTM

[sebastienros]

@agriffard google for "trigger-happy"

[Skrypt]

lol, it's fine really. I think the structure is there. If there's anything we will surely know it in the few next days.
I've applied this on my current website and permissions we're working fine and all.
I think it is the more "transparent" that we can have for now.
Remaining is custom related and should be done with SQL.

I really like the fact that we now use the Owner data as it should.