fix(auth): keep realm isolation opt-in for gcp to preserve legacy paths#2085
fix(auth): keep realm isolation opt-in for gcp to preserve legacy paths#2085aknysh merged 11 commits intocloudposse:mainfrom
Conversation
…lm for GCP The mandatory empty-realm rejection in NewAuthManager (added in 0848941) was overly broad — it broke AWS/Azure users who don't configure auth.realm. Changes: - Remove the blanket empty-realm rejection from NewAuthManager. Empty realm is now allowed; AWS/Azure handle it gracefully via filepath.Join. - GCP service account identity: when realm is empty, auto-generate a deterministic SHA-256 based realm from ServiceAccountEmail (first 8 bytes, prefixed with "auto-"). This ensures unique credential paths per account without requiring explicit auth.realm configuration. - GCP still errors if both realm AND email are empty (no fallback possible). - Update tests: requireRealm() with valid email now succeeds, empty-email test still asserts ErrEmptyRealm. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add 35 tests across 4 files covering the ADC authentication flow: - Provider layer: stateless behavior, realm independence, lifecycle - GCP project identity: credential passthrough, realm independence - GCP service account identity: impersonation, auto-realm isolation - Manager level: chain building, whoami, multi-identity scenarios Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning This PR exceeds the recommended limit of 1,000 lines.Large PRs are difficult to review and may be rejected due to their size. Please verify that this PR does not address multiple issues. |
📝 WalkthroughWalkthroughAllows empty propagated realms from the manager and makes realm handling conditional downstream; adds extensive ADC-focused tests for GCP project and service-account identities, ADC provider, provider/identity chain resolution, and manager integrations. No public API signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthManager
participant ADC as "GCP ADC Provider"
participant Identity as "Identity (gcp_project / gcp_service_account)"
participant Store as "CredentialStore"
Client->>AuthManager: Request auth (ADC flow)
AuthManager->>ADC: Authenticate via ADC provider
ADC-->>AuthManager: Token, ProjectID, Email
AuthManager->>Identity: Build chain / forward ADC creds
Identity->>Identity: resolve realm (empty -> identity-level fallback)
Identity->>Store: Persist credentials under resolved path
Store-->>AuthManager: Ack
AuthManager-->>Client: Return auth result (env, project, token)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pkg/auth/identities/gcp_project/identity_test.go (1)
390-584: Solid ADC critical-path test coverage for gcp/project.Tests cover passthrough, no-base-creds, full lifecycle, and realm-independence well. The use of
t.Setenvin the lifecycle test (line 461-463) is good practice.Minor note: bare type assertions like
creds.(*types.GCPCredentials)at lines 452, 485, 541, 567 will panic on wrong type rather than producing a clean test failure. Consider using the two-value form withrequire.True(t, ok)as done elsewhere in this file (e.g., line 425-426). Not a blocker since panics in tests still surface failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/identities/gcp_project/identity_test.go` around lines 390 - 584, Replace the bare type assertions that may panic with the safe two-value form and assert the success before using the value: where tests call creds.(*types.GCPCredentials) or loadedCreds.(*types.GCPCredentials) (e.g., in TestADC_ProjectIdentity_PassthroughCreds, TestADC_ProjectIdentity_EmptyRealm_FullLifecycle, TestADC_ProjectIdentity_NoBaseCreds, and TestADC_ProjectIdentity_RealmIndependent), change them to v, ok := creds.(*types.GCPCredentials) (or loadedCreds) and add require.True(t, ok) immediately after (keeping the existing require.NoError checks), then use v instead of the original assertion.pkg/auth/manager.go (1)
134-137: Correct removal of mandatory realm enforcement.Letting identity types handle their own realm requirements is a clean separation of concerns.
The comments here and at line 560-562 mention specific cloud providers (AWS/Azure/GCP). Per prior feedback, the auth manager should remain cloud-agnostic. Consider generalizing these comments, e.g., "Identity types that require filesystem isolation generate fallback realms internally." Not a blocker.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/manager.go` around lines 134 - 137, Update the provider-specific comments in pkg/auth/manager.go to be cloud-agnostic: replace mentions of AWS/Azure/GCP with a general explanation that empty realms are allowed for backward compatibility and that identity types which require filesystem isolation should generate fallback realms internally (e.g., SHA-based from their identifiers). Locate the comment block near the auth manager logic (around the realm handling in manager.go) and the similar comment later in the same file (the block referencing "fallback realm"), and change both to the generalized wording so the auth manager does not reference specific cloud providers.pkg/auth/identities/gcp_service_account/identity_test.go (1)
995-995: Consider guarding type assertions withrequire.IsType.Bare type assertions like
loaded.(*types.GCPCredentials)will panic with an unhelpful stack trace if the type is wrong. Arequire.IsType(t, &types.GCPCredentials{}, loaded)before the assertion gives a clearer failure message. Minor nit — the pattern is used elsewhere in this file too.Also applies to: 1079-1079, 1083-1083
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/identities/gcp_service_account/identity_test.go` at line 995, The test uses a bare type assertion loaded.(*types.GCPCredentials) which can panic; add a type guard using require.IsType(t, &types.GCPCredentials{}, loaded) immediately before performing the assertion (e.g., replace or precede the line assigning loadedGCP) so failures produce clear messages; apply the same change to the other occurrences noted (the assertions at the positions around lines referencing loaded, e.g., the other casts to *types.GCPCredentials in this file) to ensure all type assertions are guarded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/auth/identities/gcp_project/identity_test.go`:
- Around line 390-584: Replace the bare type assertions that may panic with the
safe two-value form and assert the success before using the value: where tests
call creds.(*types.GCPCredentials) or loadedCreds.(*types.GCPCredentials) (e.g.,
in TestADC_ProjectIdentity_PassthroughCreds,
TestADC_ProjectIdentity_EmptyRealm_FullLifecycle,
TestADC_ProjectIdentity_NoBaseCreds, and
TestADC_ProjectIdentity_RealmIndependent), change them to v, ok :=
creds.(*types.GCPCredentials) (or loadedCreds) and add require.True(t, ok)
immediately after (keeping the existing require.NoError checks), then use v
instead of the original assertion.
In `@pkg/auth/identities/gcp_service_account/identity_test.go`:
- Line 995: The test uses a bare type assertion loaded.(*types.GCPCredentials)
which can panic; add a type guard using require.IsType(t,
&types.GCPCredentials{}, loaded) immediately before performing the assertion
(e.g., replace or precede the line assigning loadedGCP) so failures produce
clear messages; apply the same change to the other occurrences noted (the
assertions at the positions around lines referencing loaded, e.g., the other
casts to *types.GCPCredentials in this file) to ensure all type assertions are
guarded.
In `@pkg/auth/manager.go`:
- Around line 134-137: Update the provider-specific comments in
pkg/auth/manager.go to be cloud-agnostic: replace mentions of AWS/Azure/GCP with
a general explanation that empty realms are allowed for backward compatibility
and that identity types which require filesystem isolation should generate
fallback realms internally (e.g., SHA-based from their identifiers). Locate the
comment block near the auth manager logic (around the realm handling in
manager.go) and the similar comment later in the same file (the block
referencing "fallback realm"), and change both to the generalized wording so the
auth manager does not reference specific cloud providers.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pkg/auth/identities/gcp_project/identity_test.go (1)
554-555: Nit: empty-string realm produces a less readable subtest name.
"realm="in test output is easy to miss. Consider mapping the empty case to a display label like"empty".Suggested tweak
for _, realmValue := range realms { - t.Run("realm="+realmValue, func(t *testing.T) { + displayName := realmValue + if displayName == "" { + displayName = "empty" + } + t.Run("realm="+displayName, func(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/identities/gcp_project/identity_test.go` around lines 554 - 555, The subtest name uses "realm="+realmValue which is hard to read when realmValue is empty; change the test to compute a display label (e.g., display := realmValue; if display == "" { display = "empty" }) and call t.Run("realm="+display, func(t *testing.T) { ... }) while continuing to use realmValue inside the test body for assertions; update the loop around realms and the t.Run invocation accordingly.pkg/auth/identities/gcp_service_account/identity_test.go (2)
898-943: Solid impersonation flow test.One small note: line 928 sets
ServiceAccountEmail: "developer@company.com"on the ADC base credentials. That's not a valid GCP service account email format (no.iam.gserviceaccount.comsuffix). That's fine here since it's the user's ADC identity, not a service account — but a brief inline comment would help future readers understand why a non-SA email is intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/identities/gcp_service_account/identity_test.go` around lines 898 - 943, Add a brief inline comment in TestADC_ServiceAccount_ImpersonationFlow next to the adcBaseCreds.ServiceAccountEmail assignment explaining that this value intentionally omits the .iam.gserviceaccount.com suffix because it represents a user ADC identity (gcloud user credentials) rather than a service account; update the comment near the adcBaseCreds variable (or the ServiceAccountEmail field) so future readers understand why a non-service-account email is used in this test.
458-494: Deterministic and uniqueness tests are well-structured.These two tests nail the key guarantees of SHA-based realm generation: same input → same output, different input → different output. Consider also asserting the expected exact realm value for at least one known email to serve as a golden-value regression test — if the hashing logic ever changes accidentally, a prefix-only check won't catch it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/identities/gcp_service_account/identity_test.go` around lines 458 - 494, Add a golden-value assertion to lock the exact generated realm for a known email (so accidental changes to the hashing logic are caught). Update TestRequireRealm_Deterministic (or add a small new test e.g. TestRequireRealm_GoldenValue) to call Identity.requireRealm for a chosen canonical email (use the existing GCPServiceAccountIdentityPrincipal.ServiceAccountEmail value), run it locally once to capture the current realm string, then replace the runtime equality assertion with an assert.Equal against that hard-coded expected realm literal while keeping the require.NoError checks; refer to Identity.requireRealm and the Identity/principal setup to locate the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/auth/identities/gcp_project/identity_test.go`:
- Around line 554-555: The subtest name uses "realm="+realmValue which is hard
to read when realmValue is empty; change the test to compute a display label
(e.g., display := realmValue; if display == "" { display = "empty" }) and call
t.Run("realm="+display, func(t *testing.T) { ... }) while continuing to use
realmValue inside the test body for assertions; update the loop around realms
and the t.Run invocation accordingly.
In `@pkg/auth/identities/gcp_service_account/identity_test.go`:
- Around line 898-943: Add a brief inline comment in
TestADC_ServiceAccount_ImpersonationFlow next to the
adcBaseCreds.ServiceAccountEmail assignment explaining that this value
intentionally omits the .iam.gserviceaccount.com suffix because it represents a
user ADC identity (gcloud user credentials) rather than a service account;
update the comment near the adcBaseCreds variable (or the ServiceAccountEmail
field) so future readers understand why a non-service-account email is used in
this test.
- Around line 458-494: Add a golden-value assertion to lock the exact generated
realm for a known email (so accidental changes to the hashing logic are caught).
Update TestRequireRealm_Deterministic (or add a small new test e.g.
TestRequireRealm_GoldenValue) to call Identity.requireRealm for a chosen
canonical email (use the existing
GCPServiceAccountIdentityPrincipal.ServiceAccountEmail value), run it locally
once to capture the current realm string, then replace the runtime equality
assertion with an assert.Equal against that hard-coded expected realm literal
while keeping the require.NoError checks; refer to Identity.requireRealm and the
Identity/principal setup to locate the code.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2085 +/- ##
==========================================
+ Coverage 76.20% 76.21% +0.01%
==========================================
Files 829 829
Lines 78555 78551 -4
==========================================
+ Hits 59859 59867 +8
+ Misses 14965 14946 -19
- Partials 3731 3738 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@shirkevich apologies again, but please review the following: in #2075, we implemented: but in this PR we are putting it back: can you please make it consistent? Please point Claude at this and it will fix it. Also ask to review and add tests. Thanks! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/auth/identities/gcp_project/identity_test.go (1)
438-456: Near-duplicate ofTestAuthenticate_NoBaseCreds(lines 159–175).The only meaningful addition here is
SetRealm(""). Consider whether this warrants a standalone test or could be folded into the existing one. Not a big deal — it works as ADC-group documentation — but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/identities/gcp_project/identity_test.go` around lines 438 - 456, The test TestADC_ProjectIdentity_NoBaseCreds is a near-duplicate of TestAuthenticate_NoBaseCreds; remove the duplication by either deleting TestADC_ProjectIdentity_NoBaseCreds or folding its unique assertion (the call to id.SetRealm("") and subsequent checks) into TestAuthenticate_NoBaseCreds as an additional subtest or branch; ensure you preserve the identity creation, the call to Authenticate, and the assertions on returned *types.GCPCredentials.ProjectID and AccessToken so behavior stays covered.pkg/auth/identities/gcp_service_account/identity_test.go (2)
124-144: Manual mock — consider migrating tomockgenwhen convenient.
MockProvideris a hand-rolled mock. The coding guidelines call forgo.uber.org/mock/mockgenwith//go:generatedirectives. This is pre-existing, but since the new ADC tests (lines 903+) depend on it, it's worth flagging for a future cleanup pass. No urgency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/identities/gcp_service_account/identity_test.go` around lines 124 - 144, Replace the hand-written MockProvider with a mock generated by go.uber.org/mock/mockgen: add a //go:generate directive above the provider interface declaration, run mockgen to produce a generated mock (e.g., MockProvider) and commit the generated file, then update tests that currently import or instantiate the manual MockProvider to use the generated mock type; reference the existing MockProvider symbol to locate the current mock to remove and the ADC-related tests that rely on it to update their instantiation and expectations.
458-476: Consider adding a comment to the golden value explaining its derivation.The golden value
"auto-b21924d31a143eec"correctly represents SHA256 of the test email's first 8 bytes as hex. The implementation (identity.go:88-89) is clear and the test structure is solid. Adding a brief comment like// SHA256("sa@proj.iam.gserviceaccount.com")[:8] as hexon line 475 would help future maintainers quickly understand and update the value if the hashing logic ever changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/identities/gcp_service_account/identity_test.go` around lines 458 - 476, Add a clarifying comment next to the golden string in TestRequireRealm_Deterministic that documents how it was derived: note that the expected value "auto-b21924d31a143eec" is formed by taking SHA256("sa@proj.iam.gserviceaccount.com") and using the first 8 bytes as hex (matching the logic in requireRealm/identity.go). Update the test function TestRequireRealm_Deterministic to include this one-line comment immediately above or beside the golden value so future maintainers can verify or regenerate the expected value easily.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/auth/identities/gcp_project/identity_test.go`:
- Around line 461-464: The test's environment cleanup is incomplete:
PostAuthenticate (which calls os.Setenv for all keys returned by
id.Environment()) sets GCLOUD_PROJECT, CLOUDSDK_COMPUTE_REGION,
GOOGLE_CLOUD_ZONE, and CLOUDSDK_COMPUTE_ZONE too, but the teardown only uses
t.Setenv for GOOGLE_CLOUD_PROJECT, CLOUDSDK_CORE_PROJECT and
GOOGLE_CLOUD_REGION; update the cleanup in identity_test.go to t.Setenv for the
missing keys (GCLOUD_PROJECT, CLOUDSDK_COMPUTE_REGION, GOOGLE_CLOUD_ZONE,
CLOUDSDK_COMPUTE_ZONE) — or iterate id.Environment() in the test and call
t.Setenv for each key to ensure all env vars set by PostAuthenticate are
protected from leakage.
---
Nitpick comments:
In `@pkg/auth/identities/gcp_project/identity_test.go`:
- Around line 438-456: The test TestADC_ProjectIdentity_NoBaseCreds is a
near-duplicate of TestAuthenticate_NoBaseCreds; remove the duplication by either
deleting TestADC_ProjectIdentity_NoBaseCreds or folding its unique assertion
(the call to id.SetRealm("") and subsequent checks) into
TestAuthenticate_NoBaseCreds as an additional subtest or branch; ensure you
preserve the identity creation, the call to Authenticate, and the assertions on
returned *types.GCPCredentials.ProjectID and AccessToken so behavior stays
covered.
In `@pkg/auth/identities/gcp_service_account/identity_test.go`:
- Around line 124-144: Replace the hand-written MockProvider with a mock
generated by go.uber.org/mock/mockgen: add a //go:generate directive above the
provider interface declaration, run mockgen to produce a generated mock (e.g.,
MockProvider) and commit the generated file, then update tests that currently
import or instantiate the manual MockProvider to use the generated mock type;
reference the existing MockProvider symbol to locate the current mock to remove
and the ADC-related tests that rely on it to update their instantiation and
expectations.
- Around line 458-476: Add a clarifying comment next to the golden string in
TestRequireRealm_Deterministic that documents how it was derived: note that the
expected value "auto-b21924d31a143eec" is formed by taking
SHA256("sa@proj.iam.gserviceaccount.com") and using the first 8 bytes as hex
(matching the logic in requireRealm/identity.go). Update the test function
TestRequireRealm_Deterministic to include this one-line comment immediately
above or beside the golden value so future maintainers can verify or regenerate
the expected value easily.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/auth/identities/gcp_service_account/identity_test.go (1)
751-788:⚠️ Potential issue | 🟡 MinorMissing
XDG_CONFIG_HOMEisolation in four sub-tests — real filesystem pollution.The
PrepareEnvironment,Logout,CredentialsExist, andLoadCredentialssub-tests don't setXDG_CONFIG_HOME. All of them eventually callGetProviderDirwhich invokesos.MkdirAll, creating~/.config/atmos/gcp/gcp-adc/in the real filesystem. ThePostAuthenticatesub-test correctly sets it; the others should too.🛠️ Proposed fix — add temp dir isolation to each affected sub-test
t.Run("PrepareEnvironment", func(t *testing.T) { + tmp := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", tmp) id := makeIdentity() _, err := id.PrepareEnvironment(context.Background(), nil)t.Run("Logout", func(t *testing.T) { + tmp := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", tmp) id := makeIdentity() err := id.Logout(context.Background())t.Run("CredentialsExist", func(t *testing.T) { + tmp := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", tmp) id := makeIdentity()t.Run("LoadCredentials", func(t *testing.T) { + tmp := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", tmp) id := makeIdentity()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/identities/gcp_service_account/identity_test.go` around lines 751 - 788, The tests PrepareEnvironment, Logout, CredentialsExist, and LoadCredentials call GetProviderDir which creates real ~/.config entries; to avoid filesystem pollution, in each of those sub-tests (where you call makeIdentity() and id.PrepareEnvironment / id.Logout / id.CredentialsExist / id.LoadCredentials) create a temp dir via tmp := t.TempDir() and set it with t.Setenv("XDG_CONFIG_HOME", tmp) (same pattern as the PostAuthenticate sub-test) so the provider dir is created inside the test temp directory instead of the real user config directory.pkg/auth/cloud/gcp/files.go (1)
293-296:⚠️ Potential issue | 🟡 MinorStale comment:
WriteAccessTokenFilestill says realm is required.The comment on line 295 ("Realm is required for credential isolation.") contradicts the new opt-in behavior. Same for
CleanupIdentityFilesat line 331. Per coding guidelines, comments should be updated to match the code when refactoring.✏️ Proposed fix
// WriteAccessTokenFile writes a simple access token file for tools that need it. // Uses file locking to prevent concurrent modification conflicts. -// Realm is required for credential isolation. +// Realm is optional; empty realm uses legacy paths without a realm subdirectory.// CleanupIdentityFiles removes all credential files for an identity. // Uses file locking to prevent concurrent modification conflicts. -// Realm is required for credential isolation. +// Realm is optional; empty realm uses legacy paths without a realm subdirectory.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/cloud/gcp/files.go` around lines 293 - 296, The doc comments for WriteAccessTokenFile and CleanupIdentityFiles are stale — they state "Realm is required for credential isolation." but the functions now use opt-in behavior; update both function comments (the comment blocks above WriteAccessTokenFile and CleanupIdentityFiles) to reflect that realm is optional and only used for credential isolation when provided (or describe the new opt-in semantics), removing the assertion that realm is required and ensuring wording matches current parameter behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/auth/cloud/gcp/files.go`:
- Around line 293-296: The doc comments for WriteAccessTokenFile and
CleanupIdentityFiles are stale — they state "Realm is required for credential
isolation." but the functions now use opt-in behavior; update both function
comments (the comment blocks above WriteAccessTokenFile and
CleanupIdentityFiles) to reflect that realm is optional and only used for
credential isolation when provided (or describe the new opt-in semantics),
removing the assertion that realm is required and ensuring wording matches
current parameter behavior.
In `@pkg/auth/identities/gcp_service_account/identity_test.go`:
- Around line 751-788: The tests PrepareEnvironment, Logout, CredentialsExist,
and LoadCredentials call GetProviderDir which creates real ~/.config entries; to
avoid filesystem pollution, in each of those sub-tests (where you call
makeIdentity() and id.PrepareEnvironment / id.Logout / id.CredentialsExist /
id.LoadCredentials) create a temp dir via tmp := t.TempDir() and set it with
t.Setenv("XDG_CONFIG_HOME", tmp) (same pattern as the PostAuthenticate sub-test)
so the provider dir is created inside the test temp directory instead of the
real user config directory.
|
These changes were released in v1.207.0. |
Summary
Align GCP auth behavior with #2075: realm isolation is opt-in and empty realm keeps backward-compatible paths.
What changed
auth.realmnorATMOS_AUTH_REALMis set, GCP credential files now use legacy paths without a realm segment.Why
PR #2075 restored backward-compatible behavior after #2043 by making realm isolation opt-in. This PR ensures GCP follows the same model and does not reintroduce always-on realm isolation semantics.
Summary by CodeRabbit
New Features
Bug Fixes
Tests