refactor: BIPS-38082 share AuthenticationObj across muxed providers#196
Open
btfhernandez wants to merge 2 commits into
Open
refactor: BIPS-38082 share AuthenticationObj across muxed providers#196btfhernandez wants to merge 2 commits into
btfhernandez wants to merge 2 commits into
Conversation
One signin in Configure, one signout in main, shared cookie jar. Fixes 401s caused by framework and sdkv2 each owning their own jar.
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors the muxed provider to share a single AuthenticationObj (and session/cookie jar) across both the framework and SDKv2 provider implementations. Previously each provider built its own auth and a package-level refcount tried to share signin/signout calls but not the underlying HTTP/cookie state, causing one provider to make API calls against an invalidated cookie. The new approach lazily builds and caches the auth object in Configure keyed on provider-block config, and signs out once on plugin shutdown.
Changes:
- New
providers/utils/session.gowithInitSharedAuth/ShutdownSharedAuth/ResetSharedAuthForTest; removed the oldAuthenticate/SignOut/DeleteAssetByID/SignInCount/AuthMumachinery frommethods.go. - Both providers'
Configurepaths now callInitSharedAuthwith a deterministic cache key; SDKv2 stores a newproviderMeta{authObj, signAppin}instead of a raw*AuthenticationObj, and all per-handlerAuthenticate/SignOutwrappers in resources, data sources, and ephemerals are deleted.main.gocallsShutdownSharedAuthaftertf5server.Servereturns and before anylog.Fatal. - Tests rewritten: new
TestInitSharedAuth_*/TestShutdownSharedAuth_*cases; SDKv2 provider config tests now run against a TLS mock signin server and callResetSharedAuthForTest; SDKv2 resource tests updated to wrapauthenticatein&providerMeta{authObj: …}.
Reviewed changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
providers/utils/session.go |
New shared session lifecycle (init/shutdown/reset) keyed by config hash. |
providers/utils/methods.go |
Removed old refcount-based Authenticate, SignOut, DeleteAssetByID, and associated globals. |
providers/utils/methods_test.go |
Replaced old auth/signout/delete tests with InitSharedAuth / ShutdownSharedAuth coverage. |
main.go |
Calls utils.ShutdownSharedAuth() before any log.Fatal after tf5server.Serve returns. |
providers/provider_sdkv2/provider.go |
providerConfigure builds cache key and uses InitSharedAuth; returns *providerMeta. |
providers/provider_sdkv2/common.go |
Removed authenticate / signOut wrappers; introduced providerMeta. |
providers/provider_sdkv2/{folders,safes,managed_account,secrets}.go |
Updated to use *providerMeta and dropped per-call signin/signout. |
providers/provider_sdkv2/provider_test.go |
Added TLS mock signin server and ResetSharedAuthForTest calls. |
providers/provider_sdkv2/{folders_safes,managed_account,secrets}_test.go |
Updated resource tests to wrap authenticate in &providerMeta{authObj: …}. |
providers/provider_framework/provider.go |
Configure uses InitSharedAuth with same cache key shape as SDKv2. |
providers/provider_framework/assets_resource.go |
Inlined assets.NewAssetObj + DeleteAssetById in delete paths; removed getAssetObj signin/signout. |
providers/provider_framework/managed_systems_by_database_resource.go |
Removed APISignOut helper and signin/signout in getManagedSystemObj. |
providers/provider_framework/managed_systems_by_{asset,workgroup}_resource.go |
Dropped APISignOut and per-handler auth/signout. |
providers/provider_framework/{databases,functional_accounts,workgroups}_resource.go |
Dropped per-handler utils.Authenticate and utils.SignOut. |
providers/provider_framework/{managed_account,secrets}_ephemeral.go |
Dropped per-handler signin/signout. |
providers/provider_framework/*_datasource.go (assets, databases, folders, functional_accounts, managed_account, managed_system, platforms, safes, workgroups) |
Dropped per-read utils.Authenticate and deferred utils.SignOut. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Automation tests CheckedAutomation tests failedPlease check the Automation tests run here |
Merging this branch will not change overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
dtejadav
approved these changes
May 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose of the PR
AuthenticationObjthat both muxed providers (framework + sdkv2) receive at configure time and hold for the entire plugin lifetime.Linked JIRA issue(s)
Summary of changes
New
providers/utils/session.gointroduces three functions that own the shared session lifetime:InitSharedAuth(cacheKey, build)— lazily builds theAuthenticationObj, callsGetPasswordSafeAuthenticationonce, and caches both the object and theSignAppinResponse. Subsequent callers with the same key hit the cache; a changed key (e.g., acceptance tests rotating mock servers) signs the old session out first.ShutdownSharedAuth()— called frommain()aftertf5server.Servereturns; signs out and clears the cache. Safe to call multiple times.ResetSharedAuthForTest()— test-only helper to clear cache state between acceptance-test cases without making an HTTP round-trip.providers/utils/methods.go— removed ~77 lines: the package-level globalsSignInCount,AuthMu,signAppinResponse, and theAuthenticate,SignOut,DeleteAssetByIDfunctions that used them. The refcount-based session management is no longer needed.main.go— importsutilsand callsutils.ShutdownSharedAuth()betweentf5server.Servereturning andlog.Fatal, becauselog.Fatalcallsos.Exitand would skip any deferred cleanup.providers/provider_sdkv2/provider.goandproviders/provider_framework/provider.go— bothproviderConfigure/Configurefunctions now callInitSharedAuthwith a deterministic cache key (pipe-joined url|apiVersion|apiKey|clientId|clientSecret|accountName|verifyca|certName). Both providers produce the same key from identical provider-block config, so the second one to configure hits the cache and pays no network cost.providers/provider_sdkv2/common.go— removed theauthenticate()andsignOut()wrapper functions; introducedproviderMetastruct holdingauthObj *auth.AuthenticationObjandsignAppin entities.SignAppinResponse.providerConfigurenow returns*providerMetainstead of a bare*AuthenticationObj.All sdkv2 resources (
folders.go,safes.go,managed_account.go,secrets.go) — replacedm.(*auth.AuthenticationObj)casts withm.(*providerMeta)and removed everyauthenticate(d, m)/signOut(d, m)call pair.All framework resources and data sources (
assets_resource.go,databases_resource.go,workgroups_resource.go,functional_accounts_resource.go,managed_account_ephemeral.go,secrets_ephemeral.go, and all*_datasource.gofiles) — removed the per-handlerutils.Authenticate+ deferredutils.SignOutblocks. TheAPISignOuthelper inmanaged_systems_by_database_resource.gois also deleted.assets_resource.go—DeleteAssetByID(the utils helper that wrapped Authenticate+SignOut) replaced with an inlineassets.NewAssetObj+assetObj.DeleteAssetByIdcall, since auth and signout are no longer the caller's responsibility.providers/utils/methods_test.go— rewrote the auth/session test suite against the new API surface (TestInitSharedAuth_*,TestShutdownSharedAuth_*); removedTestAuthenticate,TestSignOut,TestSignOutError,TestDeleteAssetByID. Net result: ~363 lines vs ~600 before.providers/provider_sdkv2/provider_test.go— addednewSignInMockServerhelper andutils.ResetSharedAuthForTest()calls soTestProviderConfigureWithApiKey/TestProviderConfigureWithCredentialscan exercise the fullproviderConfigurepath end-to-end. Tests now point at a real TLS mock server instead of the fake constant URL.All sdkv2 test files (
folders_safes_test.go,managed_account_test.go,secrets_test.go) — updated call sites fromauthenticate(bare*AuthenticationObj) to&providerMeta{authObj: authenticate}.Net change: -519 lines across 33 files.
Checklist
Release
Testing
Automation