Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Migrate the OpenID module to OpenIddict 5.0 #14808

Merged
merged 1 commit into from
Dec 18, 2023
Merged

Migrate the OpenID module to OpenIddict 5.0 #14808

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/OrchardCore.Build/Dependencies.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@
<PackageManagement Include="NJsonSchema" Version="10.9.0" />
<PackageManagement Include="NLog.Web.AspNetCore" Version="5.3.7" />
<PackageManagement Include="NodaTime" Version="3.1.9" />
<PackageManagement Include="OpenIddict.AspNetCore" Version="4.10.1" />
<PackageManagement Include="OpenIddict.Core" Version="4.10.1" />
<PackageManagement Include="OpenIddict.AspNetCore" Version="5.0.0" />
<PackageManagement Include="OpenIddict.Core" Version="5.0.0" />
<PackageManagement Include="OrchardCore.Translations.All" Version="1.7.1" />
<PackageManagement Include="PdfPig" Version="0.1.8" />
<PackageManagement Include="Serilog.AspNetCore" Version="7.0.0" />
Expand Down
4 changes: 2 additions & 2 deletions src/OrchardCore.Modules/OrchardCore.OpenId/OpenIdApplicationSettings.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,14 @@ public static async Task UpdateDescriptorFromSettings(this IOpenIdApplicationMan
descriptor.ClientId = model.ClientId;
descriptor.ConsentType = model.ConsentType;
descriptor.DisplayName = model.DisplayName;
descriptor.Type = model.Type;
descriptor.ClientType = model.Type;

if (!string.IsNullOrEmpty(model.ClientSecret))
{
descriptor.ClientSecret = model.ClientSecret;
}

if (string.Equals(descriptor.Type, OpenIddictConstants.ClientTypes.Public, StringComparison.OrdinalIgnoreCase))
if (string.Equals(descriptor.ClientType, OpenIddictConstants.ClientTypes.Public, StringComparison.OrdinalIgnoreCase))
{
descriptor.ClientSecret = null;
}
Expand Down
2 changes: 1 addition & 1 deletion src/OrchardCore/OrchardCore.OpenId.Core/Services/Managers/OpenIdApplicationManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ public virtual async ValueTask<ImmutableArray<string>> GetRolesAsync(
return builder.ToImmutable();
}

return ImmutableArray.Create<string>();
return [];
}
}

Expand Down
34 changes: 23 additions & 11 deletions src/OrchardCore/OrchardCore.OpenId.Core/YesSql/Models/OpenIdApplication.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ public class OpenIdApplication
/// </summary>
public string ApplicationId { get; set; }

/// <summary>
/// Gets or sets the application type associated with the current application.
/// </summary>
public string ApplicationType { get; set; }

/// <summary>
/// Gets or sets the client identifier associated with the current application.
/// </summary>
Expand Down Expand Up @@ -49,17 +54,21 @@ public class OpenIdApplication
/// </summary>
public long Id { get; set; }

/// <summary>
/// Gets or sets the JSON Web Key Set associated with the current application.
/// </summary>
// TODO: change the property type to JsonWebKeySet after migrating to System.Text.Json.
public JObject JsonWebKeySet { get; set; }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opted for JObject for consistency with other similar properties, but we'll be able to change it to JsonWebKeySet (an IdentityModel type that is now annotated with the System.Text.Json attributes since 7.0) as soon as YesSql supports System.Text.Json-based serialization/deserialization.


/// <summary>
/// Gets or sets the permissions associated with the application.
/// </summary>
public ImmutableArray<string> Permissions { get; set; }
= ImmutableArray.Create<string>();
public ImmutableArray<string> Permissions { get; set; } = [];

/// <summary>
/// Gets the logout callback URLs associated with the current application.
/// </summary>
public ImmutableArray<string> PostLogoutRedirectUris { get; set; }
= ImmutableArray.Create<string>();
public ImmutableArray<string> PostLogoutRedirectUris { get; set; } = [];

/// <summary>
/// Gets or sets the additional properties associated with the current application.
Expand All @@ -69,23 +78,26 @@ public class OpenIdApplication
/// <summary>
/// Gets or sets the callback URLs associated with the current application.
/// </summary>
public ImmutableArray<string> RedirectUris { get; set; }
= ImmutableArray.Create<string>();
public ImmutableArray<string> RedirectUris { get; set; } = [];

/// <summary>
/// Gets or sets the requirements associated with the current application.
/// </summary>
public ImmutableArray<string> Requirements { get; set; }
= ImmutableArray.Create<string>();
public ImmutableArray<string> Requirements { get; set; } = [];

/// <summary>
/// Gets or sets the roles associated with the application.
/// </summary>
public ImmutableArray<string> Roles { get; set; }
= ImmutableArray.Create<string>();
public ImmutableArray<string> Roles { get; set; } = [];

/// <summary>
/// Gets or sets the application type associated with the current application.
/// Gets or sets the settings associated with the application.
/// </summary>
public ImmutableDictionary<string, string> Settings { get; set; }
= ImmutableDictionary.Create<string, string>();

/// <summary>
/// Gets or sets the client type associated with the current application.
/// </summary>
public string Type { get; set; }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, we should rename this property to ClientType but it's not clear to me what would be the proper way to do it with YesSql /cc @sebastienros

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kevinchalet In the migrations, before any DDL commands I would create a new session and query the Documents table. Then in another session i would read the Applications by id (read from the previous query) and update the new ClientType with the Type value in Document. @sebastienros maybe YESSQL could provide a way to query the documents instead of writing a document query every time.
eg.

       public async Task<int> UpdateFrom1Async()
       {
           var sqlBuilder = new SqlBuilder(_session.Store.Configuration.TablePrefix, _session.Store.Configuration.SqlDialect);

           var tableName = _session.Store.Configuration.TableNameConvention.GetDocumentTable(Publication.Collection);
           var schema = _session.Store.Configuration.Schema;

           // Select Documents Ids
           sqlBuilder.Select();
           sqlBuilder.Selector(nameof(Document.Id));
           sqlBuilder.Table(tableName, null, schema);
           sqlBuilder.WhereAnd($" {sqlBuilder.FormatColumn(tableName, nameof(Document.Type), schema)} = @type");

           using (var documentsSession = _session.Store.CreateSession())
           {
               using (var connection = await documentsSession.CreateConnectionAsync())
               using (var migrateDocumentsSession = _session.Store.CreateSession())
               {
                   IEnumerable<int> documentIds;

                   documentIds = await connection.QueryAsync<int>(
                       sqlBuilder.ToSqlString(), new { type = $"{typeof(Publication).FullName}, {typeof(Publication).Assembly.GetName().Name}" });

                   sqlBuilder = new SqlBuilder(_session.Store.Configuration.TablePrefix, _session.Store.Configuration.SqlDialect);

                   // Select Documents
                   sqlBuilder.Select();
                   sqlBuilder.Selector("*");
                   sqlBuilder.Table(tableName, null, schema);
                   sqlBuilder.WhereAnd($" {sqlBuilder.FormatColumn(tableName, nameof(Document.Id), schema)} IN @ids");

                   IEnumerable<Document> documents;
                   foreach (IEnumerable<int> batch in documentIds.PagesOf(BATCH_SIZE))
                   {
                       var ids = batch.ToArray();

                       documents = await connection.QueryAsync<Document>(sqlBuilder.ToSqlString(), new { ids });

                       var publications = (await migrateDocumentsSession.GetAsync<Publication>(ids, PUBLICATION_COLLECTION)).GetEnumerator();

                       foreach (var document in documents)
                       {
                           var p = JObject.Parse(document.Content);
                           publications.MoveNext();
                           if (p.TryGetValue("DeployedUtc", out var jt))
                           {
                               publications.Current.CompletedUtc = jt.Value<DateTime?>();
                               migrateDocumentsSession.Save(publications.Current, PUBLICATION_COLLECTION);
                           };
                       }

                       await migrateDocumentsSession.SaveChangesAsync();
                   }
               }
           }

           SchemaBuilder.AlterIndexTable<PublicationIndex>(table => table
               .RenameColumn("PublishedUtc", nameof(PublicationIndex.CompletedUtc)),
                   collection: PUBLICATION_COLLECTION);

           return 2;

       }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I think there is no need for two sessions/connections, if you get a connection after SaveChangesAsync()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kevinchalet, you can mark Type as Obsolete and add a new property called ClientType. Setting Type can also set ClientType if ClientType is null.

@MichaelPetrinolis you may want to use await using to dispose the connection asynchronously. Also, that could should be call that query using ShellScope.AddDeferredTask to ensure that query is called after everything has been setup (specially during new setup)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MikeAlhayek the Deferred Tasks execute after the migrations complete? If this is the case and the migration fails, subsequently the deferred task will also fail, as the DB won't have the new index table column name (if we also rename the column name 'Type' to 'ClientType' in the index). Maybe adding a DataMigration step that executes after schema migrations (considering the current schema version) would be handy.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes differed task will be executed at the end of the request. You can call it after the alter command. If the alter exceptionally failed, the next line will not be called.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MikeAlhayek the issue with the deferred task is that the schema might get updated, but the data are not updated if an error during the update occurs. I think we need a two-step migration process, one to update the schema (existing) and one to run data migrations for the new Schema Version as deferred task.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MichaelPetrinolis @MikeAlhayek thanks for your suggestions!

That said, it looks a bit risky and it's likely I won't have too much time to troubleshot any potential issue that may occur due to this change.

If anyone wants to submit a PR to change the name of the Type column to ClientType (once OpenIddict 5.0 ships next week), please feel free! 😃

}
Expand Down
3 changes: 1 addition & 2 deletions src/OrchardCore/OrchardCore.OpenId.Core/YesSql/Models/OpenIdAuthorization.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,7 @@ public class OpenIdAuthorization
/// <summary>
/// Gets or sets the scopes associated with the current authorization.
/// </summary>
public ImmutableArray<string> Scopes { get; set; }
= ImmutableArray.Create<string>();
public ImmutableArray<string> Scopes { get; set; } = [];

/// <summary>
/// Gets or sets the status of the current authorization.
Expand Down
3 changes: 1 addition & 2 deletions src/OrchardCore/OrchardCore.OpenId.Core/YesSql/Models/OpenIdScope.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ public class OpenIdScope
/// <summary>
/// Gets or sets the resources associated with the current scope.
/// </summary>
public ImmutableArray<string> Resources { get; set; }
= ImmutableArray.Create<string>();
public ImmutableArray<string> Resources { get; set; } = [];
}
}
Loading