OrchardCMS/OrchardCore

Update OpenId Application Redirect Uris Faild

Opened this issue · 5 comments

hyzx86 commented

Describe the bug

The openid module requires RedirectUri to be case sensitive
To be compatible with this problem, I add the same address, but they are different in case

I debugged the code and found that the program will use a GroupBy function to count how many applications refer to the URL
This may be caused by the fact that the database settings ignore case by default

To Reproduce

Steps to reproduce the behavior:

  1. Create an application
  2. Set the Redirect Uris as http://localhost:1688/auth/logincallback
  3. Save application
  4. Edit the application , update the Redirect Uris as http://localhost:1688/auth/logincallback http://localhost:1688/Auth/logincallback
  5. Click Save

Screenshots

If applicable, add screenshots to help explain your problem.

msedge_jFFvf4UM8g.mp4

Log text

2022-12-24 11:44:54.5695|Default|00-8e0002c6a8682b0b8fbada726ff079a7-3bed2bc93986442b-00||Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware|ERROR|An unhandled exception has occurred while executing the request. System.InvalidOperationException: Sequence contains more than one element
   at System.Linq.ThrowHelper.ThrowMoreThanOneElementException()
   at System.Linq.Enumerable.TryGetSingle[TSource](IEnumerable`1 source, Boolean& found)
   at System.Linq.Enumerable.SingleOrDefault[TSource](IEnumerable`1 source)
   at YesSql.Session.ReduceAsync()
   at YesSql.Session.FlushAsync()
   at YesSql.Session.FlushAsync()
   at YesSql.Session.SaveChangesAsync()
   at YesSql.Session.SaveChangesAsync()
   at OrchardCore.OpenId.YesSql.Stores.OpenIdApplicationStore`1.UpdateAsync(TApplication application, CancellationToken cancellationToken) in D:\SourceCodes\OrchardCore\src\OrchardCore\OrchardCore.OpenId.Core\YesSql\Stores\OpenIdApplicationStore.cs:line 501
   at OpenIddict.Core.OpenIddictApplicationManager`1.UpdateAsync(TApplication application, CancellationToken cancellationToken)
   at OpenIddict.Core.OpenIddictApplicationManager`1.UpdateAsync(TApplication application, OpenIddictApplicationDescriptor descriptor, CancellationToken cancellationToken)
   at OrchardCore.OpenId.OpenIdApplicationExtensions.UpdateDescriptorFromSettings(IOpenIdApplicationManager _applicationManager, OpenIdApplicationSettings model, Object application) in D:\SourceCodes\OrchardCore\src\OrchardCore.Modules\OrchardCore.OpenId\OpenIdApplicationExtensions.cs:line 249
   at OrchardCore.OpenId.Controllers.ApplicationController.Edit(EditOpenIdApplicationViewModel model, String returnUrl) in D:\SourceCodes\OrchardCore\src\OrchardCore.Modules\OrchardCore.OpenId\Controllers\ApplicationController.cs:line 347
   at Microsoft.AspNetCore.Mvc.Infrastructure.ActionMethodExecutor.TaskOfIActionResultExecutor.Execute(IActionResultTypeMapper mapper, ObjectMethodExecutor executor, Object controller, Object[] arguments)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeActionMethodAsync>g__Awaited|12_0(ControllerActionInvoker invoker, ValueTask`1 actionResultValueTask)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeNextActionFilterAsync>g__Awaited|10_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Rethrow(ActionExecutedContextSealed context)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeInnerFilterAsync()
--- End of stack trace from previous location ---
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeNextResourceFilter>g__Awaited|25_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ResourceExecutedContextSealed context)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeFilterPipelineAsync()
--- End of stack trace from previous location ---
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope)
   at Microsoft.AspNetCore.Routing.EndpointMiddleware.<Invoke>g__AwaitRequestTask|6_0(Endpoint endpoint, Task requestTask, ILogger logger)
   at Microsoft.AspNetCore.Authorization.AuthorizationMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Localization.RequestLocalizationMiddleware.Invoke(HttpContext context)
   at OrchardCore.Diagnostics.DiagnosticsStartupFilter.<>c__DisplayClass3_0.<<Configure>b__1>d.MoveNext() in D:\SourceCodes\OrchardCore\src\OrchardCore.Modules\OrchardCore.Diagnostics\DiagnosticsStartupFilter.cs:line 34
--- End of stack trace from previous location ---
   at Microsoft.AspNetCore.Diagnostics.StatusCodePagesMiddleware.Invoke(HttpContext context)
   at OrchardCore.Modules.ModularTenantRouterMiddleware.Invoke(HttpContext httpContext) in D:\SourceCodes\OrchardCore\src\OrchardCore\OrchardCore\Modules\ModularTenantRouterMiddleware.cs:line 63
   at OrchardCore.Modules.ModularTenantContainerMiddleware.<>c__DisplayClass4_0.<<Invoke>b__0>d.MoveNext() in D:\SourceCodes\OrchardCore\src\OrchardCore\OrchardCore\Modules\ModularTenantContainerMiddleware.cs:line 62
--- End of stack trace from previous location ---
   at OrchardCore.Environment.Shell.Scope.ShellScope.UsingAsync(Func`2 execute, Boolean activateShell) in D:\SourceCodes\OrchardCore\src\OrchardCore\OrchardCore.Abstractions\Shell\Scope\ShellScope.cs:line 244
   at OrchardCore.Environment.Shell.Scope.ShellScope.UsingAsync(Func`2 execute, Boolean activateShell) in D:\SourceCodes\OrchardCore\src\OrchardCore\OrchardCore.Abstractions\Shell\Scope\ShellScope.cs:line 249
   at OrchardCore.Environment.Shell.Scope.ShellScope.UsingAsync(Func`2 execute, Boolean activateShell) in D:\SourceCodes\OrchardCore\src\OrchardCore\OrchardCore.Abstractions\Shell\Scope\ShellScope.cs:line 254
   at OrchardCore.Environment.Shell.Scope.ShellScope.UsingAsync(Func`2 execute, Boolean activateShell) in D:\SourceCodes\OrchardCore\src\OrchardCore\OrchardCore.Abstractions\Shell\Scope\ShellScope.cs:line 259
   at OrchardCore.Modules.ModularTenantContainerMiddleware.Invoke(HttpContext httpContext) in D:\SourceCodes\OrchardCore\src\OrchardCore\OrchardCore\Modules\ModularTenantContainerMiddleware.cs:line 60
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)    at System.Linq.ThrowHelper.ThrowMoreThanOneElementException()
   at System.Linq.Enumerable.TryGetSingle[TSource](IEnumerable`1 source, Boolean& found)
   at System.Linq.Enumerable.SingleOrDefault[TSource](IEnumerable`1 source)
   at YesSql.Session.ReduceAsync()
   at YesSql.Session.FlushAsync()
   at YesSql.Session.FlushAsync()
   at YesSql.Session.SaveChangesAsync()
   at YesSql.Session.SaveChangesAsync()
   at OrchardCore.OpenId.YesSql.Stores.OpenIdApplicationStore`1.UpdateAsync(TApplication application, CancellationToken cancellationToken) in D:\SourceCodes\OrchardCore\src\OrchardCore\OrchardCore.OpenId.Core\YesSql\Stores\OpenIdApplicationStore.cs:line 501
   at OpenIddict.Core.OpenIddictApplicationManager`1.UpdateAsync(TApplication application, CancellationToken cancellationToken)
   at OpenIddict.Core.OpenIddictApplicationManager`1.UpdateAsync(TApplication application, OpenIddictApplicationDescriptor descriptor, CancellationToken cancellationToken)
   at OrchardCore.OpenId.OpenIdApplicationExtensions.UpdateDescriptorFromSettings(IOpenIdApplicationManager _applicationManager, OpenIdApplicationSettings model, Object application) in D:\SourceCodes\OrchardCore\src\OrchardCore.Modules\OrchardCore.OpenId\OpenIdApplicationExtensions.cs:line 249
   at OrchardCore.OpenId.Controllers.ApplicationController.Edit(EditOpenIdApplicationViewModel model, String returnUrl) in D:\SourceCodes\OrchardCore\src\OrchardCore.Modules\OrchardCore.OpenId\Controllers\ApplicationController.cs:line 347
   at Microsoft.AspNetCore.Mvc.Infrastructure.ActionMethodExecutor.TaskOfIActionResultExecutor.Execute(IActionResultTypeMapper mapper, ObjectMethodExecutor executor, Object controller, Object[] arguments)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeActionMethodAsync>g__Awaited|12_0(ControllerActionInvoker invoker, ValueTask`1 actionResultValueTask)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeNextActionFilterAsync>g__Awaited|10_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Rethrow(ActionExecutedContextSealed context)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeInnerFilterAsync()
--- End of stack trace from previous location ---
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeNextResourceFilter>g__Awaited|25_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ResourceExecutedContextSealed context)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeFilterPipelineAsync()
--- End of stack trace from previous location ---
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope)
   at Microsoft.AspNetCore.Routing.EndpointMiddleware.<Invoke>g__AwaitRequestTask|6_0(Endpoint endpoint, Task requestTask, ILogger logger)
   at Microsoft.AspNetCore.Authorization.AuthorizationMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Localization.RequestLocalizationMiddleware.Invoke(HttpContext context)
   at OrchardCore.Diagnostics.DiagnosticsStartupFilter.<>c__DisplayClass3_0.<<Configure>b__1>d.MoveNext() in D:\SourceCodes\OrchardCore\src\OrchardCore.Modules\OrchardCore.Diagnostics\DiagnosticsStartupFilter.cs:line 34
--- End of stack trace from previous location ---
   at Microsoft.AspNetCore.Diagnostics.StatusCodePagesMiddleware.Invoke(HttpContext context)
   at OrchardCore.Modules.ModularTenantRouterMiddleware.Invoke(HttpContext httpContext) in D:\SourceCodes\OrchardCore\src\OrchardCore\OrchardCore\Modules\ModularTenantRouterMiddleware.cs:line 63
   at OrchardCore.Modules.ModularTenantContainerMiddleware.<>c__DisplayClass4_0.<<Invoke>b__0>d.MoveNext() in D:\SourceCodes\OrchardCore\src\OrchardCore\OrchardCore\Modules\ModularTenantContainerMiddleware.cs:line 62
--- End of stack trace from previous location ---
   at OrchardCore.Environment.Shell.Scope.ShellScope.UsingAsync(Func`2 execute, Boolean activateShell) in D:\SourceCodes\OrchardCore\src\OrchardCore\OrchardCore.Abstractions\Shell\Scope\ShellScope.cs:line 244
   at OrchardCore.Environment.Shell.Scope.ShellScope.UsingAsync(Func`2 execute, Boolean activateShell) in D:\SourceCodes\OrchardCore\src\OrchardCore\OrchardCore.Abstractions\Shell\Scope\ShellScope.cs:line 249
   at OrchardCore.Environment.Shell.Scope.ShellScope.UsingAsync(Func`2 execute, Boolean activateShell) in D:\SourceCodes\OrchardCore\src\OrchardCore\OrchardCore.Abstractions\Shell\Scope\ShellScope.cs:line 254
   at OrchardCore.Environment.Shell.Scope.ShellScope.UsingAsync(Func`2 execute, Boolean activateShell) in D:\SourceCodes\OrchardCore\src\OrchardCore\OrchardCore.Abstractions\Shell\Scope\ShellScope.cs:line 259
   at OrchardCore.Modules.ModularTenantContainerMiddleware.Invoke(HttpContext httpContext) in D:\SourceCodes\OrchardCore\src\OrchardCore\OrchardCore\Modules\ModularTenantContainerMiddleware.cs:line 60
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)
hyzx86 commented

Hi @kevinchalet ,

Is this code used for other purposes in OC?

image

kevinchalet commented

Hi @hyzx86,

Sounds like a YesSql question for @sebastienros (it doesn't seem specific to OpenID/OpenIddict to me).

/cc @sebastienros

sebastienros commented

I believe the index that is used should use the lower case of the uri as the key:
https://github.com/OrchardCMS/OrchardCore/blob/main/src/OrchardCore/OrchardCore.OpenId.Core/YesSql/Indexes/OpenIdApplicationIndex.cs#L53

But it also looks like a bug because even if the result would be weird, it should not throw an exception.

sebastienros commented

Also it seems that I didn't understand SingleOrDefault. I was expecting the default value if there were 0 or more than one element and actually it will throw with multiple elements:

Returns the only element of a sequence, or a default value if the sequence is empty; this method throws an exception if there is more than one element in the sequence.

The code in yessql is checking for that mistake above (when the key could return multiple values from the database): https://github.com/sebastienros/yessql/blob/main/src/YesSql.Core/Session.cs#L1108-L1114

But in the end we would still have had an exception (with a better explanation). So yes the group result should be unique when queried from the database. For most of the dbs this is case insensitive, so the key should be normalized, probably lower cased here. And in any other Reduce Index that is based on a string key.

sebastienros commented

Changing the code shouldn't be a breaking change: the current bug prevents from having multiple non-normalized values, and the db will still match non-normalized values when the query is executed.