RFC for injecting server secrets + refactoring auth code [azuredevops bintray bitbucket] #3410
Conversation
paulmelnikow
added
service-badge
|
@calebcartwright @chris48s would be great to hear your thoughts on this. |
paulmelnikow
changed the title
|
services/auth.js
Outdated
| } | ||
| } | ||
|
|
||
| function optionalAuth(serviceInstance, userKey, passKey) { |
There was a problem hiding this comment.
Could we use named params/object here, or do we want consumers to explicitly declare value for the userKey arg when they don't need (like in the case of Azure DevOps)?
| @@ -152,35 +145,6 @@ t.create('pr (server, not found)') | |||
| message: 'not found', | |||
| }) | |||
|
|
|||
| t.create('pr (auth)') | |||
There was a problem hiding this comment.
What's the thinking behind removing these tests, and would you anticipate removing the same types of tests in other services?
There was a problem hiding this comment.
Sorry I was reading from the bottom up, I see this validation is now handled above in the new spec file 👍
|
I've still not gone through this fully yet, but wanted to go ahead and share that I like the looks of it and the direction! I don't really have a preference one way or the other, but what do we see as the pros/cons of implementing the auth/secret access capabilities via helper functions that are imported (which is very similar to the existing |
|
Hmm, interesting, I could see how adding I also thought about adding an auth helper object that would tuck all this stuff into a namespace, but wasn't thrilled about adding allocation overhead to every request. Though as I think about it more, in the context of all the work that is done for every request, this is probably a pointless optimization to consider. Something that goes back to #963 that I'd like to do is create one instance of a service for the life of the server. Since all the request-specific state is already passed through method calls, this is fine; the stuff being passed into the constructor is mostly the same. The reason this hasn't been possible is that the |
|
I could try a second version of this that:
|
|
Sounds intriguing! |
Cherrypicked from #3410, which needs to be reworked.
…handler Cherry-picked from #3410; should simplify reworking it.
Cherry-picked from #3410; should simplify reworking it.
Cherry-picked from #3410; should simplify reworking it.
This encapsulates the fetch methods slightly better. Cherry-picked from #3410, which needs to be reworked. This change should simplify dropping in the new auth method when that happens.
This is a reworking of #3410 based on some feedback @calebcartwright left on that PR. The goals of injecting the secrets are threefold: 1. Simplify testing 2. Be consistent with all of the other config (which is injected) 3. Encapsulate the sensitive auth-related code in one place so it can be studied and tested thoroughly - Rather than add more code to BaseService to handle authorization logic, it delegates that to an AuthHelper class. - When the server starts, it fetches the credentials from `config` and injects them into `BaseService.register()` which passes them to `invoke()`. - In `invoke()` the service's auth configuration is checked (`static get auth()`, much like `static get route()`). - If the auth config is present, an AuthHelper instance is created and attached to the new instance. - Then within the service, the password, basic auth config, or bearer authentication can be accessed via e.g. `this.authHelper.basicAuth` and passed to `this._requestJson()` and friends. - Everything is being done very explicitly, so it should be very clear where and how the configured secrets are being used. - Testing different configurations of services can now be done by injecting the config into `invoke()` in `.spec` files instead of mocking global state in the service tests as was done before. See the new Jira spec files for a good example of this. Ref #3393
I started some work on #3393 and thought I'd ask for some feedback before I applied it to all the services.
Come to think of it, it may actually make sense to keep the old version working so this can be done in a more gradual way.