Add JWT roles based throttling mechanism #4506
Conversation
course_discovery/settings/base.py
Outdated
|
|
||
| # Keywords that will be searched for inside the `roles` key of the JWT in case a user uses JWT authentication. | ||
| # If the keyword is found, the user has more lenient throttling limits. | ||
| ENHANCED_THROTTLE_JWT_ROLE_KEYWORDS = ['enterprise'] |
There was a problem hiding this comment.
Unsure if this should be a list or string.
There was a problem hiding this comment.
I think keeping this as a list makes sense, as it's fairly generic as is and could theoretically be used for non-enterprise purposes, too.
course_discovery/settings/base.py
Outdated
|
|
||
| # Keywords that will be searched for inside the `roles` key of the JWT in case a user uses JWT authentication. | ||
| # If the keyword is found, the user has more lenient throttling limits. | ||
| ENHANCED_THROTTLE_JWT_ROLE_KEYWORDS = ['enterprise'] |
There was a problem hiding this comment.
I think keeping this as a list makes sense, as it's fairly generic as is and could theoretically be used for non-enterprise purposes, too.
| @@ -16,6 +18,23 @@ def throttling_cache(): | |||
| return caches['default'] | |||
|
|
|||
|
|
|||
| def is_enterprise_user(request): | |||
There was a problem hiding this comment.
nit: if using settings (i.e., settings.ENHANCED_THROTTLE_JWT_ROLE_KEYWORDS and settings.ENHANCED_THROTTLE_LIMIT, where these settings could theoretically be configured for other non-enterprise use cases, too, might it make sense to rename this method more generically, e.g. is_priviledged_user_with_enhanced_throttle or similar?
course_discovery/settings/base.py
Outdated
|
|
||
| # Keywords that will be searched for inside the `roles` key of the JWT in case a user uses JWT authentication. | ||
| # If the keyword is found, the user has more lenient throttling limits. | ||
| ENHANCED_THROTTLE_JWT_ROLE_KEYWORDS = ['enterprise'] |
There was a problem hiding this comment.
Also worth considering: it might make sense to define the base setting as an empty list (i.e., no change for the Open edX platform), and configure the setting via edx-internal such that the enhanced throttle rate change is only applicable for the edX.org instance. The ENHANCED_THROTTLE_LIMIT setting could stay hardcoded, though.
| in the JWT to identify them. Enterprise customers are guaranteed to have one of a small number of fixed roles assigned to them. | ||
| Once we identify that an incoming request's user has one of these roles in their JWT, we will enable higher rate limits for them. |
There was a problem hiding this comment.
It may make sense to expand on the decision to capture the configurable settings in the ADR related to the change, and while the immediate use case is for enterprise learners, the approach is generic for non-enterprise use cases, too.
There was a problem hiding this comment.
Documented the two new settings and their usage.
zawan-ila
force-pushed
the
anawaz/prod-4139
branch
from
0f2deac to
ba49503
Compare
PROD-4139
This PR adds the ADR for JWT-role based throttling. The code is mostly copied from 4394 Thanks to @adamstankiewicz for the idea and the POC!