-
Notifications
You must be signed in to change notification settings - Fork 81
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
feat(security)!: Add authentication hooks to routes #1437
Conversation
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## main #1437 +/- ##
==========================================
+ Coverage 66.79% 66.83% +0.04%
==========================================
Files 36 36
Lines 3075 3088 +13
==========================================
+ Hits 2054 2064 +10
- Misses 888 891 +3
Partials 133 133
|
BREAKING CHANGE: EdgeX standard routes will require authentication. AddCustomRoute is a new interface method that enables adding authenticated routes. Closes #1435 Signed-off-by: Bryon Nevis <bryon.nevis@intel.com>
@jim-wang-intel Added unit test. |
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.
LGTM
_ = sdk.AddCustomRoute("/test", interfaces.Unauthenticated, func(http.ResponseWriter, *http.Request) {}, http.MethodGet) | ||
_ = router.Walk(func(route *mux.Route, router *mux.Router, ancestors []*mux.Route) error { |
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.
Tests should always use verify no errors rather than ignore them.
err := ...
require.NoError(t, err)
_ = sdk.AddCustomRoute("/test", interfaces.Authenticated, func(http.ResponseWriter, *http.Request) {}, http.MethodGet) | ||
_ = router.Walk(func(route *mux.Route, router *mux.Router, ancestors []*mux.Route) error { |
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.
Same error check
// ApplicationService defines the interface for an edgex Application Service | ||
type ApplicationService interface { | ||
// AppContext returns the application service context used to detect cancelled context when the service is terminating. | ||
// Used by custom app service to appropriately exit any long-running functions. | ||
AppContext() context.Context | ||
// AddRoute a custom REST route to the application service's internal webserver | ||
// AddRoute adds a custom REST route to the application service's internal webserver | ||
// A reference to this ApplicationService is add the the context that is passed to the handler, which | ||
// can be retrieved using the `AppService` key | ||
AddRoute(route string, handler func(http.ResponseWriter, *http.Request), methods ...string) error |
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.
Need to make this a Deprecated
@bnevis-i Need a PR do document this new API |
BREAKING CHANGE: EdgeX standard routes will require authentication.
AddCustomRoute is a new interface method that enables adding authenticated routes.
Closes #1435
PR Checklist
Please check if your PR fulfills the following requirements:
BREAKING CHANGE:
describing the break)Testing Instructions
Not fully tested in a full running system, but have added unit tests (make test) and verified basic security functionality.
New Dependency Instructions (If applicable)