-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Validate incoming JWT tokens from the bot framework #11129
Validate incoming JWT tokens from the bot framework #11129
Conversation
rasa/core/channels/botframework.py
Outdated
self._update_cached_jwk_keys() | ||
|
||
def _update_cached_jwk_keys(self) -> None: | ||
if datetime.datetime.now() - self.jwt_update_time < datetime.timedelta(days=1): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, I'd say a daily update of the JWT keys is plenty, and also currently no need to make this configurable.
cafe4e9
to
f0cee1c
Compare
268f3b2
to
83c9eb1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 😎
Made some style suggestions, and if possible, some unit tests covering different scenarios for peace of mind 🙏🏼
83c9eb1
to
c01d966
Compare
c01d966
to
f92eddb
Compare
@ancalita I think I found a good way to test the auth stuff, thanks for pointing that out. Also, made me learn a lot about JWT in the process 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work 💯
Left some existential questions about the type of status to be returned in some cases 😄 .
Also please look at the additional comments in the unit tests to strengthen the existing assertions 💪🏼
f92eddb
to
b349ec5
Compare
🚀 A preview of the docs have been deployed at the following URL: https://11129--rasahq-docs-rasa-v2.netlify.app/docs/rasa |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 Thank you for addressing those extra assertions 💯
Hello, thank you for implementing this.
You could consider switching to this class instead of handling cache by your own. Hope you find this useful. |
@Maxinho96 thank you for pointing the |
Proposed changes:
This PR enables JSON Web Token validation for the JWTs coming in from the Azure botframework channel. This is achieved by periodically updating the keys that Microsoft uses to sign the tokens, saving those in memory, and then validating the tokens and verifying their signature for incoming requests from the bot framework.
For details on how the botframework does authentication, see here.
Status (please check what you already did):
black
(please check Readme for instructions)