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

Created Secret token validator middleware #1190

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

BlueXTX
Copy link

@BlueXTX BlueXTX commented Jan 11, 2023

resolve issue #1112

@karb0f0s
Copy link
Member

First of all, @BlueXTX thank you for your contribution!
Though I have a few concerns about this change:

  • The biggest issue is ASP.NET dependencies. I'm not sure how they play out for legacy platforms, covered by .netstandard2.0 target. It might be safer to introduce this change under #if DOTNET6_OR_GREATER directive to avoid collision with older .NET frameworks.
  • I don't see a way to differentiate between controllers for this middleware. If I have other endpoints in my app, they would require X-Telegram-Bot-Api-Secret-Token header too.
  • IMO helper functions and middleware classes would greatly benefit from some level of XML documentation on them.

@tuscen WDYT?

@BlueXTX
Copy link
Author

BlueXTX commented Jan 12, 2023

First of all, @BlueXTX thank you for your contribution! Though I have a few concerns about this change:

  • The biggest issue is ASP.NET dependencies. I'm not sure how they play out for legacy platforms, covered by .netstandard2.0 target. It might be safer to introduce this change under #if DOTNET6_OR_GREATER directive to avoid collision with older .NET frameworks.
  • I don't see a way to differentiate between controllers for this middleware. If I have other endpoints in my app, they would require X-Telegram-Bot-Api-Secret-Token header too.
  • IMO helper functions and middleware classes would greatly benefit from some level of XML documentation on them.

@tuscen WDYT?

  1. Microsoft.Extensions.Options and Microsoft.AspNetCore.Http.Abstractions have support for netstandard2.0.
  2. Will it be enough if I add configuration like this? app.UseWhen(context => context.Request.Path.StartsWithSegments("/YourPath"), appBuilder => { appBuilder.UseMiddleware<SecretTokenValidatorMiddleware>(); });

@tuscen
Copy link
Member

tuscen commented Jan 12, 2023

I don't think we should take dependency on M.E.x libraries in the core package. It's ok to make a separate package for ASP.NET Core integration though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants