Conversation
|
|
||
| // Web sessions | ||
| h.POST("/webapi/sessions/web", httplib.WithCSRFProtection(h.WithLimiterHandlerFunc(h.createWebSession))) | ||
| h.DELETE("/webapi/sessions/web", h.WithAuth(h.deleteSession)) |
There was a problem hiding this comment.
nit: let's call these deleteWebSession and renewWebSession for clarity/consistency.
| h.POST("/webapi/sessions/web", httplib.WithCSRFProtection(h.WithLimiterHandlerFunc(h.createWebSession))) | ||
| // App sessions | ||
| h.POST("/webapi/sessions/app", h.WithAuth(h.createAppSession)) | ||
| h.DELETE("/webapi/sessions", h.WithAuth(h.deleteSession)) |
There was a problem hiding this comment.
I wonder if we should keep the old routes around for a while to avoid breakages. Are these only ever used by the Web UI?
There was a problem hiding this comment.
I only see them being used by the Web UI. Although maybe there's some compatibility guarantee we make towards 3rd party clients?
There was a problem hiding this comment.
I'm ok with adding a "DELETE IN 12.0" as long as we don't finally get to deleting in 17.1 😂
There was a problem hiding this comment.
Added a deprecated session for the older routes. Didn't know when to put a "DELETE" but left a comment to this PR to give context.
There was a problem hiding this comment.
I think the compatibility guarantee is more in terms of binary versions than APIs, but this is more to play it safe.
There was a problem hiding this comment.
Let's put DELETE IN 13 and let's go back and do this as soon as we cut the v12 branch next week so we don't forget again.
| h.DELETE("/webapi/sessions", h.WithAuth(h.deleteSession)) | ||
| h.POST("/webapi/sessions/renew", h.WithAuth(h.renewSession)) | ||
|
|
||
| // Deprecated web sessions routes |
There was a problem hiding this comment.
Suggestion:
| // Deprecated web sessions routes | |
| // DELETE IN 13, deprecated/unused web sessions routes (avatus) |
We do grep for those (at least I do), so it's a useful mark. Username added for context, although it also makes it easy to find your own cleanups ;)
| h.POST("/webapi/sessions/web", httplib.WithCSRFProtection(h.WithLimiterHandlerFunc(h.createWebSession))) | ||
| // App sessions | ||
| h.POST("/webapi/sessions/app", h.WithAuth(h.createAppSession)) | ||
| h.DELETE("/webapi/sessions", h.WithAuth(h.deleteSession)) |
There was a problem hiding this comment.
I think the compatibility guarantee is more in terms of binary versions than APIs, but this is more to play it safe.
|
This PR was backported in the following PRs here Thank you @sfreiberg !! |
* Remove deprecated router and add rate limiting * Update test * Fix more tests * Fix rate limiting bug * Update webapi/sessions route (#19892) Co-authored-by: Alan Parra <alan.parra@goteleport.com> Co-authored-by: Michael <michael.myers@goteleport.com>
* Add rate limiting * Fix rate limiting bug * Update webapi/sessions route (#19892) Co-authored-by: Alan Parra <alan.parra@goteleport.com> Co-authored-by: Michael <michael.myers@goteleport.com>
6 participants
Update the webapi routes to include
/webto better clarify they are for web sessions onlywebapps buddy: gravitational/webapps#1486