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.

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

Add interface for IdentityServerTools #1454

Merged
merged 4 commits into from
Nov 20, 2023

Conversation

matt-goldman
Copy link
Contributor

What issue does this PR address?
IdentityServerTools is useful and I expect used in a lot of places. However as it's a concrete class it is difficult to test services that depend on it. This PR creates an abstraction IIdentityServerTools which can be injected instead, and can therefore be mocked easily for tests. This is important for testing services that depend upon IdentityServerTools but don't explicitly need it for testing purposes.

Note: This would be a breaking change. Personally I think that's ok, but if you wanted to maintain backward compatibility, we could keep both registrations:

builder.Services.AddTransient<IIdentityServerTools, IdentityServerTools>();
builder.Services.AddTransient<IdentityServerTools>();

Important: Any code or remarks in your Pull Request are under the following terms:

If You provide us with any comments, bug reports, feedback, enhancements, or modifications proposed or suggested by You for the Software, such Feedback is provided on a non-confidential basis (notwithstanding any notice to the contrary You may include in any accompanying communication), and Licensor shall have the right to use such Feedback at its discretion, including, but not limited to the incorporation of such suggested changes into the Software. You hereby grant Licensor a perpetual, irrevocable, transferable, sublicensable, nonexclusive license under all rights necessary to incorporate and use your Feedback for any purpose, including to make and sell any products and services.

(see our license, section 7)

@brockallen brockallen added this to the 7.0.0 milestone Nov 1, 2023
@brockallen
Copy link
Member

@AndersAbel check in our docs for updates based on this: https://docs.duendesoftware.com/identityserver/v6/tokens/internal/

@brockallen brockallen added enhancement New feature or request and removed feature request labels Nov 1, 2023
@brockallen
Copy link
Member

Thanks for this @matt-goldman -- we'll work on it!

- Mark as obsolote to discourage direct use
@josephdecock
Copy link
Member

Am I right to think that this is a breaking change only in the sense that the class is marked obsolete?

@AndersAbel
Copy link
Member

If someone has customized the service/class and replaced it with their own in DI this will revert back to using the default version of the service. I added Obsolete because I think that helps flagging this for anyone that has usage/customization.

@AndersAbel AndersAbel merged commit 0b6f4b5 into DuendeSoftware:main Nov 20, 2023
5 checks passed
@josephdecock josephdecock changed the title Provide abstraction for IdentityServerTools Add interface for IdentityServerTools Jan 5, 2024
@josephdecock josephdecock mentioned this pull request Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking_change enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants