Introduce SPI to swap authentication directives #3829
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3829 +/- ##
=========================================
- Coverage 75.81% 71% -4.82%
=========================================
Files 146 145 -1
Lines 6902 6897 -5
Branches 428 418 -10
=========================================
- Hits 5233 4897 -336
- Misses 1669 2000 +331
Continue to review full report at Codecov.
|
| implicit val logging: Logging) | ||
| extends BasicAuthenticate { | ||
| protected implicit val executionContext = actorSystem.dispatcher | ||
| protected val authStore = WhiskAuthStore.datastore |
There was a problem hiding this comment.
WhiskAuthStore.datastore would use SpiLoader to load and create ArtifactStore for each request via reflection. As AuthStore does not change post startup it would be better to load it in BasicAuthenticationDirectiveProvider and pass it.
May be make BasicAuthenticate an object and pass required dependencies as validateCredentials params.
|
@chetanmeh with the last commit ,the authStore is now created once and passed implicitly into the directive. |
markusthoemmes
added
review
| class AuthenticatedRouteBasicAuth(implicit val authStore: AuthStore, | ||
| implicit val actorSystem: ActorSystem, | ||
| implicit val httpRequest: HttpRequest, | ||
| implicit val materializer: ActorMaterializer, |
There was a problem hiding this comment.
As per f2f discussion, please have a look at various extractX methods to combine the directives and avoid broad interfaces.
| actorSystem: ActorSystem, | ||
| materializer: ActorMaterializer, | ||
| logging: Logging): AuthenticationDirective[Identity] = | ||
| new AuthenticatedRouteBasicAuth().getDirective |
There was a problem hiding this comment.
I think, given all the extractX directives, we can model this to be a static directive which only needs to be instantiated once.
| @@ -191,36 +194,38 @@ class RestAPIVersion(config: WhiskConfig, apiPath: String, apiVersion: String)( | |||
| "swagger_paths" -> JsObject("ui" -> s"/$swaggeruipath".toJson, "api-docs" -> s"/$swaggerdocpath".toJson))) | |||
| } | |||
|
|
|||
| def routes(implicit transid: TransactionId): Route = { | |||
| def routes(implicit transid: TransactionId, authStore: AuthStore): Route = { | |||
There was a problem hiding this comment.
Do you need the authStore here? It's already in scope I think?
| // and allow the actions themselves to respond to options | ||
| basicAuth(validateCredentials) { user => | ||
| web.routes(user) | ||
| extractRequest { httpRequest => |
There was a problem hiding this comment.
Why is this needed?
|
[PG 3:2532 is ✅ ] |
This PR introduces a SPI to configure a different authentication directive used in the REST API
Description
This PR introduces a SPI to swap/exchange authentication directives in the REST API.
The existing SPI does basic authentication only. With this PR alternative authentication methods
like bearer token can be enabled. This capability is for example needed to support
external IAM systems.
Additionally it packages the existing functionality to verify the basic authentication header to be consumed in the SPI without functional changes.
[PG 1:3115 passed]
Related issue and scope
I explained the background of these changes on the mailing list
Extending Authentication and Entitlement - Heads up
My changes affect the following components
Types of changes
Checklist: