Aguafrommars / TheIdServer

OpenID/Connect, OAuth2, WS-Federation and SAML 2.0 server based on Duende IdentityServer and ITFoxtec Identity SAML 2.0 with its admin UI
https://theidserver-duende.herokuapp.com/
Apache License 2.0
692 stars 83 forks source link

Regression: Client settings lost on save in UI #407

Closed ben-jacobs closed 3 years ago

ben-jacobs commented 3 years ago

Howdy :)

When using RavenDB as store, Clients created prior to 2bb302 lose settings on save if edited in the UI (no changes made to the affected properties).

Before: {... "AllowedGrantTypes": [ { "Id": "clientgranttype/d90b4ce8-fd22-44d5-8603-fa65e9dd69cb", "ClientId": null, "GrantType": null, "CreatedAt": "0001-01-01T00:00:00.0000000", "ModifiedAt": null, "Client": null } ], "RedirectUris": [ { "Id": "clienturi/a142fea6-1b57-4718-83f4-36bc78b3bfe2", "ClientId": null, "Uri": null, "SanetizedCorsUri": null, "Kind": 0, "CreatedAt": "0001-01-01T00:00:00.0000000", "ModifiedAt": null, "Client": null } ], "AllowedScopes": [ { "Id": "clientscope/8b9ec9b8-54a9-4b82-b847-3110012c3e03", "ClientId": null, "Scope": null, "CreatedAt": "0001-01-01T00:00:00.0000000", "ModifiedAt": null, "Client": null } ], ... }

After: "AllowedGrantTypes": null, "RedirectUris": null, "AllowedScopes": null,

aguacongas commented 3 years ago

Hi Ben, Do you use the master branch ? Did you pull latest change ?

aguacongas commented 3 years ago

Oups sorry you give me the commit :-(

ben-jacobs commented 3 years ago

Yes master was where I noticed the problem (b9c191), but I worked backwards a bit and it also seemed to happen on 2bb302 - Unfortunately I can't tell which commit I created the clients on, but its within the last couple of days since you merged the ravendb branch.

I'm wondering if it's related to that code cleanup you did to remove dead code? (haven't done any investigation yet)

ben-jacobs commented 3 years ago

If it helps, I just spun up master, initialised the clients etc. in a fresh DB, edited the client in the UI and all the settings were lost in the DB (Razor app crashed too). Almost certainly the app crash is because the API calls would have been rejected because of the lost settings on the client (i.e. the AllowedScopes)

aguacongas commented 3 years ago

Probably, can you post the crach stack trace please

aguacongas commented 3 years ago

@ben-jacobs can you try the fix on branch feature/ravendb ?

ben-jacobs commented 3 years ago

As requested the client stack trace when client dies following saving the changes to a client in the UI.

I think the client exception might be unrelated (not sure yet) ... I'm working on a repro, but in some cases the settings will be lost on save without the exception being thrown. My culture by the way is en-AU and I'm getting plenty of console warning about missing resources.

blazor.webassembly.js:1 crit: Microsoft.AspNetCore.Components.WebAssembly.Rendering.WebAssemblyRenderer[100]
      Unhandled exception rendering component: Exception has been thrown by the target of an invocation.
System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation.
 ---> System.ArgumentNullException: Value cannot be null. (Parameter 'source')
   at System.Linq.ThrowHelper.ThrowArgumentNullException(ExceptionArgument argument)
   at System.Linq.Enumerable.Where[ClientLocalizedResource](IEnumerable`1 source, Func`2 predicate)
   at Aguacongas.TheIdServer.BlazorApp.Validators.EntityResourceValidator`1[[Aguacongas.IdentityServer.Store.Entity.ClientLocalizedResource, Aguacongas.IdentityServer.Store, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]]..ctor(ILocalizable`1 model, EntityResourceKind kind, IStringLocalizer localizer)
   at Aguacongas.TheIdServer.BlazorApp.Validators.ClientValidator..ctor(Client client, IStringLocalizer localizer)
   at System.Reflection.RuntimeConstructorInfo.InternalInvoke(Object obj, Object[] parameters, Boolean wrapExceptions)
   --- End of inner exception stack trace ---
   at System.Reflection.RuntimeConstructorInfo.InternalInvoke(Object obj, Object[] parameters, Boolean wrapExceptions)
   at System.Reflection.RuntimeConstructorInfo.DoInvoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.Reflection.RuntimeConstructorInfo.Invoke(BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.RuntimeType.CreateInstanceImpl(BindingFlags bindingAttr, Binder binder, Object[] args, CultureInfo culture)
   at System.Activator.CreateInstance(Type type, BindingFlags bindingAttr, Binder binder, Object[] args, CultureInfo culture, Object[] activationAttributes)
   at System.Activator.CreateInstance(Type type, Object[] args)
   at Microsoft.AspNetCore.Components.Forms.EditContextExtensions.GetValidatorForModel(Object entity, Object model, IStringLocalizer localizer)
   at Microsoft.AspNetCore.Components.Forms.EditContextExtensions.ValidateModel(EditContext editContext, ValidationMessageStore messages, IStringLocalizer localizer)
   at Microsoft.AspNetCore.Components.Forms.EditContextExtensions.<>c__DisplayClass0_0.<AddFluentValidation>b__0(Object sender, ValidationRequestedEventArgs eventArgs)
   at Microsoft.AspNetCore.Components.Forms.EditContext.Validate()
   at Aguacongas.TheIdServer.BlazorApp.Pages.EntityModel`1[[Aguacongas.IdentityServer.Store.Entity.Client, Aguacongas.IdentityServer.Store, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]].HandleModificationState_OnStateChange(ModificationKind kind, Object _)
   at Aguacongas.TheIdServer.BlazorApp.Services.HandleModificationState.EntityDeleted[ClientLocalizedResource](ClientLocalizedResource entity)
   at Aguacongas.TheIdServer.BlazorApp.Components.EntityResourcesComponentBase`1[[Aguacongas.IdentityServer.Store.Entity.ClientLocalizedResource, Aguacongas.IdentityServer.Store, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]].OnDeleteResource(IEntityResource resource)
   at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
--- End of stack trace from previous location ---
   at Microsoft.AspNetCore.Components.ComponentBase.CallStateHasChangedOnAsyncCompletion(Task task)
   at Microsoft.AspNetCore.Components.ComponentBase.CallStateHasChangedOnAsyncCompletion(Task task)
   at Microsoft.AspNetCore.Components.RenderTree.Renderer.GetErrorHandledTask(Task taskToHandle)
ben-jacobs commented 3 years ago

So here's a bit of a repro -

Edit record, save and NOT lose settings.

  1. Edit an authorization_code client (as an example, happens with other types)
  2. Add an an additional scope
  3. Save the record

Edit record, save and settings lost:

  1. Edit an authorization_code client (as an example, happens with other types)
  2. Edit the description
  3. Tab out of the description field
  4. Save .... settings lost.
aguacongas commented 3 years ago

Thanks, it should be fixed now, can you confirm ?

ben-jacobs commented 3 years ago

Tested on a freshly initialized DB, repro as above does not produce error (settings saved). Generally messed with changing settings for clients etc and couldn't see any errors, but didn't necessarily test all possible paths.

Do you think it's likely that this issue will affect other entities like the APIs/Providers/etc ?

aguacongas commented 3 years ago

It was, because it was removing all navigation properties on update, but it's fix now .
Can I ask you to review the doc changed regarding RavenDb config on #408 ?