ProfileService.IsUserActiveAsync() defaults

[tom-crayon]

Which version of Duende IdentityServer are you using? 7.x

Which version of .NET are you using? 8.x

Describe the bug

This is not a bug, but an obscured default. The implementation of ProfileService<T>.IsUserActiveAsync(T user) means that there is no user state check by default, it always returns true. The ASP.NET Identity IdentityUser model doesn't include any state fields, so this seems reasonable.

I expect most IdentityServer with ASP.NET Identity implementations extend the default model to add user state so they can be disabled if necessary. It's then not obvious that IsUserActiveAsync(T user) needs to be overridden to perform the state check, which is used by several key processes.

To Reproduce

1. Use ASP.NET Identity with IdentityServer to authenticate to a web app
2. Have the web app obtain an access token and refresh token
3. Set the user to disabled (ASP.NET Identity extension)
4. Trigger a flow to obtain a new access token using the refresh token
5. A new access token will be issued

Expected behavior

At step 5, the user should be prevented from obtaining a new access token. This is the case, and IdentityServer works correctly if IsUserActiveAsync(T user) is overridden with the proper logic.

Additional context

I have suggestions to help people avoid this mistake.

I propose a change is made to ProfileService<T> to force implementations that extend the IdentityUser model to also specify when the user is active. This would prompt implementers to make a conscious decision based on their setup.

I also suggest expanding the ASP.NET Identity Integration docs page to call out the need to override IsUserActiveAsync(T user) when using a custom IdentityUser model.

Many thanks!

[AndersAbel]

Thanks for your feedback. I would personally have preferred if Asp.Net had a built in enabled/disabled flag of users. But now they don't and our out-of-the-box experience for working with Asp.Net Identity should be working-out-of-the-box with the features that are in Asp.Net Identity out-of-the-box. So while I appreciate that forcing an override would make it more obvious, it would also make it harder to get started.

Having said that, I think it would make a lot of sense to point out in the docs that our Asp.Net Identity integration simply implements IsUserActiveAsync as return true.