diff --git a/sherlock/.mockery.yaml b/sherlock/.mockery.yaml index a3551503f..2aaec604d 100644 --- a/sherlock/.mockery.yaml +++ b/sherlock/.mockery.yaml @@ -64,6 +64,7 @@ packages: interfaces: Identifier: Fields: + MayBePresentWhileRemovedFields: github.com/broadinstitute/sherlock/sherlock/internal/role_propagation/propagation_engines: interfaces: PropagationEngine: diff --git a/sherlock/config/default_config.yaml b/sherlock/config/default_config.yaml index 9813d54e0..01e26415e 100644 --- a/sherlock/config/default_config.yaml +++ b/sherlock/config/default_config.yaml @@ -165,6 +165,9 @@ retries: errorRegexesToRetry: - "googleapi: Error 500" - "googleapi: Error 503" + - "rpc error: code = Internal desc = Internal error encountered" # Another Google error + - "googleapi: Error 400: Precondition check failed" # This appears to be an error that we should retry because it is transient + - "connection reset by peer" # Azure error # The base interval between retries. This is fed into an exponential backoff # algorithm. baseAttemptInterval: 500ms @@ -362,8 +365,105 @@ rolePropagation: driftAlignmentStaleThreshold: 5m propagators: + # devAzureAccount correlates to models.Role.GrantsDevAzureAccount. + devAzureAccount: + enable: false + dryRun: false + # The client ID of the Azure AD app to use for authentication. + clientID: + # The UUID of the Azure AD tenant to work with. + tenantID: fad90753-2022-4456-9b0a-c7e5b934e408 # azure.dev.envs-terra.bio + # The path on disk that Sherlock should expect to find a token for federated workload identity. + tokenFilePath: /azure-federation/projected-ksa-token.jwt + # The suffix of all accounts in the tenant that Sherlock should manage. + tenantEmailSuffix: "@test.firecloud.org" + # Suffixes of Sherlock users' emails that should be swapped out with the memberEmailSuffix to match + # Sherlock users to Azure Entra ID user emails. + userEmailSuffixesToReplace: + - "@broadinstitute.org" + # prodAzureAccount correlates to models.Role.GrantsProdAzureAccount. + prodAzureAccount: + enable: false + dryRun: false + # The client ID of the Azure AD app to use for authentication. + clientID: + # The UUID of the Azure AD tenant to work with. + tenantID: 66bb90ac-8857-4a8a-aa0a-be2186dfa5f9 # firecloud.org + # The path on disk that Sherlock should expect to find a token for federated workload identity. + tokenFilePath: /azure-federation/projected-ksa-token.jwt + # The suffix of all accounts in the tenant that Sherlock should manage. + tenantEmailSuffix: "@firecloud.org" + # Suffixes of Sherlock users' emails that should be swapped out with the memberEmailSuffix to match + # Sherlock users to Azure Entra ID user emails. + userEmailSuffixesToReplace: + - "@broadinstitute.org" + + # devAzureB2CAccount correlates to models.Role.GrantsDevAzureAccount. It invites the account + # provisioned by devAzureAccount. + devAzureInvitedB2CAccount: + enable: false + dryRun: false + # The client ID of the Azure AD app to use for authentication against the home tenant. + homeTenantClientID: + # The UUID of the home Azure AD tenant to work with. + homeTenantID: fad90753-2022-4456-9b0a-c7e5b934e408 # azure.dev.envs-terra.bio + # The path on disk that Sherlock should expect to find a token for federated workload identity + # to use against the home tenant. + homeTenantTokenFilePath: /azure-federation/projected-ksa-token.jwt + # The client ID of the Azure AD app to use for authentication against the tenant to invite to. + inviteTenantClientID: + # The UUID of the Azure AD tenant to invite home tenant users to. + inviteTenantID: fd0bc0ef-1747-4ee6-ab3e-d4d6bb882d40 # terradevb2c.onmicrosoft.com + # The path on disk that Sherlock should expect to find a token for federated workload identity + # to use against the tenant to invite to. + inviteTenantTokenFilePath: /azure-federation/projected-ksa-token.jwt + # The domain of the email addresses in the home tenant. Should match the user principal names in + # the home tenant. + homeTenantEmailDomain: "test.firecloud.org" + # The domain of the identities in the invite tenant. Should match the end of the user principal names + # in the invite tenant, so that they're like _#EXT#@. + inviteTenantIdentityDomain: "terradevb2c.onmicrosoft.com" + # Domains of Sherlock users' emails that should be swapped out with the home tenant email domain + # to match Sherlock users to Azure Entra ID user emails. + userEmailDomainsToReplace: + - "broadinstitute.org" + # Link to instructions for how users should sign in to their homeTenantEmailDomain account. + signInInstructionsLink: https://docs.google.com/document/d/1wW0OXFsSxtn4sXOJ5pvAJ_WQWH_aCBwY2cpwMJCSl90/edit + # prodAzureB2CAccount correlates to models.Role.GrantsProdAzureAccount. It invites the account + # provisioned by prodAzureAccount. + prodAzureInvitedB2CAccount: + enable: false + dryRun: false + # The client ID of the Azure AD app to use for authentication against the home tenant. + homeTenantClientID: + # The UUID of the home Azure AD tenant to work with. + homeTenantID: 66bb90ac-8857-4a8a-aa0a-be2186dfa5f9 # firecloud.org + # The path on disk that Sherlock should expect to find a token for federated workload identity + # to use against the home tenant. + homeTenantTokenFilePath: /azure-federation/projected-ksa-token.jwt + # The client ID of the Azure AD app to use for authentication against the tenant to invite to. + inviteTenantClientID: + # The UUID of the Azure AD tenant to invite home tenant users to. + inviteTenantID: 35ea5de9-1786-4bbb-89cf-25b88261643d # terraprodb2c.onmicrosoft.com + # The path on disk that Sherlock should expect to find a token for federated workload identity + # to use against the tenant to invite to. + inviteTenantTokenFilePath: /azure-federation/projected-ksa-token.jwt + # The domain of the email addresses in the home tenant. Should match the user principal names in + # the home tenant. + homeTenantEmailDomain: "firecloud.org" + # The domain of the identities in the invite tenant. Should match the end of the user principal names + # in the invite tenant, so that they're like _#EXT#@. + inviteTenantIdentityDomain: "terraprodb2c.onmicrosoft.com" + # Domains of Sherlock users' emails that should be swapped out with the home tenant email domain + # to match Sherlock users to Azure Entra ID user emails. + userEmailDomainsToReplace: + - "broadinstitute.org" + # Link to instructions for how users should sign in to their homeTenantEmailDomain account. + signInInstructionsLink: https://docs.google.com/document/d/1wW0OXFsSxtn4sXOJ5pvAJ_WQWH_aCBwY2cpwMJCSl90/edit + devFirecloudGroup: enable: false + dryRun: false # The domain of the Google Workspace, assumed to be the email domain of all members. This should # not contain a leading "@". workspaceDomain: "test.firecloud.org" @@ -373,6 +473,7 @@ rolePropagation: - "@broadinstitute.org" qaFirecloudGroup: enable: false + dryRun: false # The domain of the Google Workspace, assumed to be the email domain of all members. This should # not contain a leading "@". workspaceDomain: "quality.firecloud.org" @@ -382,6 +483,7 @@ rolePropagation: - "@broadinstitute.org" prodFirecloudGroup: enable: false + dryRun: false # The domain of the Google Workspace, assumed to be the email domain of all members. This should # not contain a leading "@". workspaceDomain: "firecloud.org" @@ -392,6 +494,7 @@ rolePropagation: devFirecloudFolderOwner: enable: false + dryRun: false # The domain of the Google Workspace, assumed to be the email domain of all members. This should # not contain a leading "@". workspaceDomain: "test.firecloud.org" @@ -401,6 +504,7 @@ rolePropagation: - "@broadinstitute.org" qaFirecloudFolderOwner: enable: false + dryRun: false # The domain of the Google Workspace, assumed to be the email domain of all members. This should # not contain a leading "@". workspaceDomain: "quality.firecloud.org" @@ -410,6 +514,7 @@ rolePropagation: - "@broadinstitute.org" prodFirecloudFolderOwner: enable: false + dryRun: false # The domain of the Google Workspace, assumed to be the email domain of all members. This should # not contain a leading "@". workspaceDomain: "firecloud.org" @@ -420,6 +525,7 @@ rolePropagation: devAzureGroup: enable: false + dryRun: false # The client ID of the Azure AD app to use for authentication. clientID: # The UUID of the Azure AD tenant to work with. @@ -436,6 +542,7 @@ rolePropagation: - "@broadinstitute.org" prodAzureGroup: enable: false + dryRun: false # The client ID of the Azure AD app to use for authentication. clientID: # The UUID of the Azure AD tenant to work with. @@ -451,8 +558,50 @@ rolePropagation: userEmailSuffixesToReplace: - "@broadinstitute.org" + # devAzureB2CReader correlates to models.Role.GrantsDevAzureDirectoryRoles. It's the only role we grant currently. + # Additional roles can be granted by correlating additional propagators with different instantiations and + # configurations to the same boolean field. + devAzureB2CReader: + enable: false + dryRun: false + # The client ID of the Azure AD app to use for authentication. + clientID: + # The UUID of the Azure AD tenant to work with. + tenantID: fd0bc0ef-1747-4ee6-ab3e-d4d6bb882d40 # terradevb2c.onmicrosoft.com + # The path on disk that Sherlock should expect to find a token for federated workload identity. + tokenFilePath: /azure-federation/projected-ksa-token.jwt + # The suffix of all member emails. This can be thought of as a filter for what Azure users Sherlock + # will attempt to propagate roles to. This may contain a "@" (especially useful for "#EXT#@" emails); + # if it does, then the userEmailSuffixesToReplace must as well. + memberEmailSuffix: "_test.firecloud.org#EXT#@terradevb2c.onmicrosoft.com" + # Suffixes of Sherlock users' emails that should be swapped out with the memberEmailSuffix to match + # Sherlock users to Azure Entra ID users. + userEmailSuffixesToReplace: + - "@broadinstitute.org" + # prodAzureB2CReader correlates to models.Role.GrantsDevAzureDirectoryRoles. It's the only role we grant currently. + # Additional roles can be granted by correlating additional propagators with different instantiations and + # configurations to the same boolean field. + prodAzureB2CReader: + enable: false + dryRun: false + # The client ID of the Azure AD app to use for authentication. + clientID: + # The UUID of the Azure AD tenant to work with. + tenantID: 35ea5de9-1786-4bbb-89cf-25b88261643d # terraprodb2c.onmicrosoft.com + # The path on disk that Sherlock should expect to find a token for federated workload identity. + tokenFilePath: /azure-federation/projected-ksa-token.jwt + # The suffix of all member emails. This can be thought of as a filter for what Azure users Sherlock + # will attempt to propagate roles to. This may contain a "@" (especially useful for "#EXT#@" emails); + # if it does, then the userEmailSuffixesToReplace must as well. + memberEmailSuffix: "_firecloud.org#EXT#@terraprodb2c.onmicrosoft.com" + # Suffixes of Sherlock users' emails that should be swapped out with the memberEmailSuffix to match + # Sherlock users to Azure Entra ID users. + userEmailSuffixesToReplace: + - "@broadinstitute.org" + broadInstituteGroup: enable: false + dryRun: false suitabilitySynchronization: enable: true @@ -462,20 +611,6 @@ suitabilitySynchronization: interval: 60m firecloud: domain: firecloud.org - groups: - fcAdmins: fc-admins@firecloud.org - firecloudProjectOwners: firecloud-project-owners@firecloud.org - # extraPermissions can be used to grant an exact email address access to "suitable" actions inside Sherlock, - # regardless of that email correlating to a Firecloud account. This functionality should only be used for - # service accounts, and the justification for this functionality existing is that adding a service account - # here is better than actually giving the service account any actual permissions inside the Firecloud org. - # - # This is almost always unnecessary, because a caller's suitability will be evaluated even from GitHub - # Actions. Great care needs to be taken using this capability, as access to the service account needs to - # otherwise be limited to only suitable individuals. - extraPermissions: - #- email: example@dsp-tools-k8s.iam.gserviceaccount.com - # suitable: false suspendRoleAssignments: enable: true interval: 1m diff --git a/sherlock/config/test_config.yaml b/sherlock/config/test_config.yaml index 228690cef..6e7ce2f7e 100644 --- a/sherlock/config/test_config.yaml +++ b/sherlock/config/test_config.yaml @@ -69,6 +69,11 @@ rolePropagation: enable: false workspaceDomain: test.firecloud.org + devFirecloudGroupTestDryRun: + enable: true + workspaceDomain: test.firecloud.org + dryRun: true + devFirecloudGroupTestDefault: enable: true workspaceDomain: test.firecloud.org @@ -82,15 +87,5 @@ rolePropagation: toleratedUsers: - email: tolerated@test.firecloud.org -suitabilitySynchronization: - behaviors: - loadIntoDB: - # These are for testing our handling of this config; tests run fully off-line - extraPermissions: - - email: has-extra-permissions-suitable@example.com - suitable: true - - email: has-extra-permissions-non-suitable@example.com - suitable: false - bitsDataWarehouse: enable: false diff --git a/sherlock/db/migrations/000098_add_azure_account_fields.down.sql b/sherlock/db/migrations/000098_add_azure_account_fields.down.sql new file mode 100644 index 000000000..2a2648663 --- /dev/null +++ b/sherlock/db/migrations/000098_add_azure_account_fields.down.sql @@ -0,0 +1,43 @@ +drop index if exists roles_grants_dev_azure_account_unique; + +alter table roles + drop column if exists grants_dev_azure_account; + +alter table role_operations + drop column if exists from_grants_dev_azure_account; + +alter table role_operations + drop column if exists to_grants_dev_azure_account; + +drop index if exists roles_grants_prod_azure_account_unique; + +alter table roles + drop column if exists grants_prod_azure_account; + +alter table role_operations + drop column if exists from_grants_prod_azure_account; + +alter table role_operations + drop column if exists to_grants_prod_azure_account; + +drop index if exists roles_grants_dev_azure_directory_roles_unique; + +alter table roles + drop column if exists grants_dev_azure_directory_roles; + +alter table role_operations + drop column if exists from_grants_dev_azure_directory_roles; + +alter table role_operations + drop column if exists to_grants_dev_azure_directory_roles; + +drop index if exists roles_grants_prod_azure_directory_roles_unique; + +alter table roles + drop column if exists grants_prod_azure_directory_roles; + +alter table role_operations + drop column if exists from_grants_prod_azure_directory_roles; + +alter table role_operations + drop column if exists to_grants_prod_azure_directory_roles; diff --git a/sherlock/db/migrations/000098_add_azure_account_fields.up.sql b/sherlock/db/migrations/000098_add_azure_account_fields.up.sql new file mode 100644 index 000000000..c62640c3c --- /dev/null +++ b/sherlock/db/migrations/000098_add_azure_account_fields.up.sql @@ -0,0 +1,51 @@ +alter table roles + add column if not exists grants_dev_azure_account boolean; + +create unique index if not exists roles_grants_dev_azure_account_unique + on roles (grants_dev_azure_account) + where deleted_at is null and grants_dev_azure_account is not null and grants_dev_azure_account is true; + +alter table role_operations + add column if not exists from_grants_dev_azure_account boolean; + +alter table role_operations + add column if not exists to_grants_dev_azure_account boolean; + +alter table roles + add column if not exists grants_prod_azure_account boolean; + +create unique index if not exists roles_grants_prod_azure_account_unique + on roles (grants_prod_azure_account) + where deleted_at is null and grants_prod_azure_account is not null and grants_prod_azure_account is true; + +alter table role_operations + add column if not exists from_grants_prod_azure_account boolean; + +alter table role_operations + add column if not exists to_grants_prod_azure_account boolean; + +alter table roles + add column if not exists grants_dev_azure_directory_roles boolean; + +create unique index if not exists roles_grants_dev_azure_directory_roles_unique + on roles (grants_dev_azure_directory_roles) + where deleted_at is null and grants_dev_azure_directory_roles is not null and grants_dev_azure_directory_roles is true; + +alter table role_operations + add column if not exists from_grants_dev_azure_directory_roles boolean; + +alter table role_operations + add column if not exists to_grants_dev_azure_directory_roles boolean; + +alter table roles + add column if not exists grants_prod_azure_directory_roles boolean; + +create unique index if not exists roles_grants_prod_azure_directory_roles_unique + on roles (grants_prod_azure_directory_roles) + where deleted_at is null and grants_prod_azure_directory_roles is not null and grants_prod_azure_directory_roles is true; + +alter table role_operations + add column if not exists from_grants_prod_azure_directory_roles boolean; + +alter table role_operations + add column if not exists to_grants_prod_azure_directory_roles boolean; diff --git a/sherlock/db/migrations/000099_drop_name_inferred_from_github.down.sql b/sherlock/db/migrations/000099_drop_name_inferred_from_github.down.sql new file mode 100644 index 000000000..2a736d0f7 --- /dev/null +++ b/sherlock/db/migrations/000099_drop_name_inferred_from_github.down.sql @@ -0,0 +1,2 @@ +alter table users + add column name_inferred_from_github boolean; diff --git a/sherlock/db/migrations/000099_drop_name_inferred_from_github.up.sql b/sherlock/db/migrations/000099_drop_name_inferred_from_github.up.sql new file mode 100644 index 000000000..a01e18785 --- /dev/null +++ b/sherlock/db/migrations/000099_drop_name_inferred_from_github.up.sql @@ -0,0 +1,2 @@ +alter table users + drop column name_inferred_from_github; diff --git a/sherlock/go.mod b/sherlock/go.mod index 3091d5d29..8c9f3658d 100644 --- a/sherlock/go.mod +++ b/sherlock/go.mod @@ -146,6 +146,7 @@ require ( github.com/prometheus/procfs v0.15.1 // indirect github.com/prometheus/statsd_exporter v0.27.1 // indirect github.com/rs/cors v1.11.1 // indirect + github.com/sethvargo/go-password v0.3.1 // indirect github.com/sirupsen/logrus v1.9.3 // indirect github.com/spf13/afero v1.11.0 // indirect github.com/spf13/cobra v1.8.1 // indirect diff --git a/sherlock/go.sum b/sherlock/go.sum index 42ecc8d2f..041466529 100644 --- a/sherlock/go.sum +++ b/sherlock/go.sum @@ -607,6 +607,8 @@ github.com/ryanuber/go-glob v1.0.0/go.mod h1:807d1WSdnB0XRJzKNil9Om6lcp/3a0v4qIH github.com/sanity-io/litter v1.5.5 h1:iE+sBxPBzoK6uaEP5Lt3fHNgpKcHXc/A2HGETy0uJQo= github.com/sanity-io/litter v1.5.5/go.mod h1:9gzJgR2i4ZpjZHsKvUXIRQVk7P+yM3e+jAF7bU2UI5U= github.com/sean-/seed v0.0.0-20170313163322-e2103e2c3529/go.mod h1:DxrIzT+xaE7yg65j358z/aeFdxmN0P9QXhEzd20vsDc= +github.com/sethvargo/go-password v0.3.1 h1:WqrLTjo7X6AcVYfC6R7GtSyuUQR9hGyAj/f1PYQZCJU= +github.com/sethvargo/go-password v0.3.1/go.mod h1:rXofC1zT54N7R8K/h1WDUdkf9BOx5OptoxrMBcrXzvs= github.com/sirupsen/logrus v1.2.0/go.mod h1:LxeOpSwHxABJmUn/MG1IvRgCAasNZTLOkJPxbbu5VWo= github.com/sirupsen/logrus v1.4.2/go.mod h1:tLMulIdttU9McNUspp0xgXVQah82FyeX6MwdIuYE2rE= github.com/sirupsen/logrus v1.6.0/go.mod h1:7uNnSEd1DgxDLC74fIahvMZmmYsHGZGEOFrfsX/uA88= diff --git a/sherlock/internal/api/sherlock/roles_v3.go b/sherlock/internal/api/sherlock/roles_v3.go index cf70bf0d8..737274053 100644 --- a/sherlock/internal/api/sherlock/roles_v3.go +++ b/sherlock/internal/api/sherlock/roles_v3.go @@ -26,8 +26,12 @@ type RoleV3Edit struct { GrantsDevFirecloudFolderOwner *string `json:"grantsDevFirecloudFolderOwner,omitempty" form:"grantsDevFirecloudFolderOwner"` GrantsQaFirecloudFolderOwner *string `json:"grantsQaFirecloudFolderOwner,omitempty" form:"grantsQaFirecloudFolderOwner"` GrantsProdFirecloudFolderOwner *string `json:"grantsProdFirecloudFolderOwner,omitempty" form:"grantsProdFirecloudFolderOwner"` + GrantsDevAzureAccount *bool `json:"grantsDevAzureAccount,omitempty" form:"grantsDevAzureAccount"` + GrantsProdAzureAccount *bool `json:"grantsProdAzureAccount,omitempty" form:"grantsProdAzureAccount"` GrantsDevAzureGroup *string `json:"grantsDevAzureGroup,omitempty" form:"grantsDevAzureGroup"` GrantsProdAzureGroup *string `json:"grantsProdAzureGroup,omitempty" form:"grantsProdAzureGroup"` + GrantsDevAzureDirectoryRoles *bool `json:"grantsDevAzureDirectoryRoles,omitempty" form:"grantsDevAzureDirectoryRoles"` + GrantsProdAzureDirectoryRoles *bool `json:"grantsProdAzureDirectoryRoles,omitempty" form:"grantsProdAzureDirectoryRoles"` GrantsBroadInstituteGroup *string `json:"grantsBroadInstituteGroup,omitempty" form:"grantsBroadInstituteGroup"` } @@ -46,8 +50,12 @@ func (r RoleV3) toModel() models.Role { GrantsDevFirecloudFolderOwner: r.GrantsDevFirecloudFolderOwner, GrantsQaFirecloudFolderOwner: r.GrantsQaFirecloudFolderOwner, GrantsProdFirecloudFolderOwner: r.GrantsProdFirecloudFolderOwner, + GrantsDevAzureAccount: r.GrantsDevAzureAccount, + GrantsProdAzureAccount: r.GrantsProdAzureAccount, GrantsDevAzureGroup: r.GrantsDevAzureGroup, GrantsProdAzureGroup: r.GrantsProdAzureGroup, + GrantsDevAzureDirectoryRoles: r.GrantsDevAzureDirectoryRoles, + GrantsProdAzureDirectoryRoles: r.GrantsProdAzureDirectoryRoles, GrantsBroadInstituteGroup: r.GrantsBroadInstituteGroup, }, } @@ -80,8 +88,12 @@ func roleFromModel(model models.Role) RoleV3 { GrantsDevFirecloudFolderOwner: model.GrantsDevFirecloudFolderOwner, GrantsQaFirecloudFolderOwner: model.GrantsQaFirecloudFolderOwner, GrantsProdFirecloudFolderOwner: model.GrantsProdFirecloudFolderOwner, + GrantsDevAzureAccount: model.GrantsDevAzureAccount, + GrantsProdAzureAccount: model.GrantsProdAzureAccount, GrantsDevAzureGroup: model.GrantsDevAzureGroup, GrantsProdAzureGroup: model.GrantsProdAzureGroup, + GrantsDevAzureDirectoryRoles: model.GrantsDevAzureDirectoryRoles, + GrantsProdAzureDirectoryRoles: model.GrantsProdAzureDirectoryRoles, GrantsBroadInstituteGroup: model.GrantsBroadInstituteGroup, }, } diff --git a/sherlock/internal/api/sherlock/roles_v3_test.go b/sherlock/internal/api/sherlock/roles_v3_test.go index 25f5feec1..ea7affe5a 100644 --- a/sherlock/internal/api/sherlock/roles_v3_test.go +++ b/sherlock/internal/api/sherlock/roles_v3_test.go @@ -27,8 +27,12 @@ func (s *handlerSuite) Test_roleFromModel() { GrantsDevFirecloudFolderOwner: model.GrantsDevFirecloudFolderOwner, GrantsQaFirecloudFolderOwner: model.GrantsQaFirecloudFolderOwner, GrantsProdFirecloudFolderOwner: model.GrantsProdFirecloudFolderOwner, + GrantsDevAzureAccount: model.GrantsDevAzureAccount, + GrantsProdAzureAccount: model.GrantsProdAzureAccount, GrantsDevAzureGroup: model.GrantsDevAzureGroup, GrantsProdAzureGroup: model.GrantsProdAzureGroup, + GrantsDevAzureDirectoryRoles: model.GrantsDevAzureDirectoryRoles, + GrantsProdAzureDirectoryRoles: model.GrantsProdAzureDirectoryRoles, GrantsBroadInstituteGroup: model.GrantsBroadInstituteGroup, }, } diff --git a/sherlock/internal/api/sherlock/users_v3.go b/sherlock/internal/api/sherlock/users_v3.go index 00a3e5300..891623407 100644 --- a/sherlock/internal/api/sherlock/users_v3.go +++ b/sherlock/internal/api/sherlock/users_v3.go @@ -20,11 +20,8 @@ type UserV3 struct { } type userDirectlyEditableFields struct { - Name *string `json:"name,omitempty" form:"name"` - // Controls whether Sherlock should automatically update the user's name based on a connected GitHub identity. - // Will be set to true if the user account has no name and a GitHub account is linked. - NameInferredFromGithub *bool `json:"nameInferredFromGithub,omitempty" form:"nameInferredFromGithub"` - NameFrom *string `json:"nameFrom,omitempty" form:"nameFrom" enums:"sherlock,github,slack" binding:"omitempty,oneof=sherlock github slack"` + Name *string `json:"name,omitempty" form:"name"` + NameFrom *string `json:"nameFrom,omitempty" form:"nameFrom" enums:"sherlock,github,slack" binding:"omitempty,oneof=sherlock github slack"` } func (u UserV3) toModel() models.User { @@ -39,15 +36,6 @@ func (u UserV3) toModel() models.User { Name: u.Name, NameFrom: u.NameFrom, } - if u.NameFrom == nil && u.NameInferredFromGithub != nil { - if *u.NameInferredFromGithub { - githubString := "github" - ret.NameFrom = &githubString - } else { - sherlockString := "sherlock" - ret.NameFrom = &sherlockString - } - } return ret } @@ -78,14 +66,5 @@ func userFromModel(model models.User) UserV3 { return utils.NilOrCall(roleAssignmentFromModel, ra) }) } - if model.NameFrom != nil { - if *model.NameFrom == "github" { - trueBool := true - ret.NameInferredFromGithub = &trueBool - } else { - falseBool := false - ret.NameInferredFromGithub = &falseBool - } - } return ret } diff --git a/sherlock/internal/api/sherlock/users_v3_upsert.go b/sherlock/internal/api/sherlock/users_v3_upsert.go index 1945663a0..e8b8728c9 100644 --- a/sherlock/internal/api/sherlock/users_v3_upsert.go +++ b/sherlock/internal/api/sherlock/users_v3_upsert.go @@ -143,15 +143,6 @@ func processUserEdits(callingUser *models.User, directEdits userDirectlyEditable wg.Wait() - // If nameFrom wasn't set but nameInferredFromGithub was, be compatible and convert it to nameFrom - if directEdits.NameFrom == nil && directEdits.NameInferredFromGithub != nil { - if *directEdits.NameInferredFromGithub { - directEdits.NameFrom = &githubString - } else { - directEdits.NameFrom = &sherlockString - } - } - // Set nameFrom if it was provided and is different from what we already have if directEdits.NameFrom != nil && (callingUser.NameFrom == nil || *directEdits.NameFrom != *callingUser.NameFrom) { callingUser.NameFrom = directEdits.NameFrom diff --git a/sherlock/internal/api/sherlock/users_v3_upsert_test.go b/sherlock/internal/api/sherlock/users_v3_upsert_test.go index 02e0a14c8..4d08426c5 100644 --- a/sherlock/internal/api/sherlock/users_v3_upsert_test.go +++ b/sherlock/internal/api/sherlock/users_v3_upsert_test.go @@ -60,7 +60,6 @@ func (s *handlerSuite) TestUserV3Upsert_name_minimal() { s.Equal(s.TestData.User_Suitable().Email, got.Email) s.Equal("a name", *got.Name) s.Equal("sherlock", *got.NameFrom) - s.False(*got.NameInferredFromGithub) s.Run("update name", func() { code = s.HandleRequest( @@ -74,62 +73,6 @@ func (s *handlerSuite) TestUserV3Upsert_name_minimal() { s.Equal(s.TestData.User_Suitable().Email, got.Email) s.Equal("a different name", *got.Name) s.Equal("sherlock", *got.NameFrom) - s.False(*got.NameInferredFromGithub) - }) -} - -func (s *handlerSuite) TestUserV3Upsert_nameInferredFromGithub() { - var got UserV3 - code := s.HandleRequest( - s.NewRequest("PUT", "/api/users/v3", UserV3Upsert{ - userDirectlyEditableFields: userDirectlyEditableFields{ - NameInferredFromGithub: utils.PointerTo(true), - }, - }), - &got) - s.Equal(http.StatusCreated, code) - s.Equal(s.TestData.User_Suitable().Email, got.Email) - s.True(*got.NameInferredFromGithub) - - s.Run("then doesn't update name", func() { - code = s.HandleRequest( - s.NewRequest("PUT", "/api/users/v3", UserV3Upsert{ - userDirectlyEditableFields: userDirectlyEditableFields{ - Name: utils.PointerTo("a different name"), - }, - }), - &got) - s.Equal(http.StatusOK, code) - s.Equal(s.TestData.User_Suitable().Email, got.Email) - s.Nil(got.Name) - s.True(*got.NameInferredFromGithub) - }) - - s.Run("can set to false", func() { - code = s.HandleRequest( - s.NewRequest("PUT", "/api/users/v3", UserV3Upsert{ - userDirectlyEditableFields: userDirectlyEditableFields{ - NameInferredFromGithub: utils.PointerTo(false), - }, - }), - &got) - s.Equal(http.StatusCreated, code) - s.Equal(s.TestData.User_Suitable().Email, got.Email) - s.False(*got.NameInferredFromGithub) - - s.Run("then updates name", func() { - code = s.HandleRequest( - s.NewRequest("PUT", "/api/users/v3", UserV3Upsert{ - userDirectlyEditableFields: userDirectlyEditableFields{ - Name: utils.PointerTo("a different name"), - }, - }), - &got) - s.Equal(http.StatusCreated, code) - s.Equal(s.TestData.User_Suitable().Email, got.Email) - s.Equal("a different name", *got.Name) - s.False(*got.NameInferredFromGithub) - }) }) } diff --git a/sherlock/internal/models/role.go b/sherlock/internal/models/role.go index ffcc9ba69..613702bc1 100644 --- a/sherlock/internal/models/role.go +++ b/sherlock/internal/models/role.go @@ -73,6 +73,19 @@ type RoleFields struct { // Multiple values can be comma separated and whitespace will be trimmed. GrantsProdFirecloudFolderOwner *string + // GrantsDevAzureAccount, when true, indicates that a User with a RoleAssignment to this Role should have an + // Azure account created for them in our dev environment. The account will be created in a "home" tenant and be + // invited to any other tenants associated with the environment. Specifics are defined in configuration and + // within the role propagation package because it is uniquely complex and high-risk to change. Suspension is + // handled by suspending the account itself. + GrantsDevAzureAccount *bool + // GrantsProdAzureAccount, when true, indicates that a User with a RoleAssignment to this Role should have an + // Azure account created for them in our prod environment. The account will be created in a "home" tenant and be + // invited to any other tenants associated with the environment. Specifics are defined in configuration and + // within the role propagation package because it is uniquely complex and high-risk to change. Suspension is + // handled by suspending the account itself. + GrantsProdAzureAccount *bool + // GrantsDevAzureGroup, when not null, indicates that a User with an unsuspended RoleAssignment to this Role // should have their Azure account (if they have one) added to this group. // Multiple values can be comma separated and whitespace will be trimmed. @@ -82,6 +95,17 @@ type RoleFields struct { // Multiple values can be comma separated and whitespace will be trimmed. GrantsProdAzureGroup *string + // GrantsDevAzureDirectoryRoles, when true, indicates that a User with an unsuspended RoleAssignment to this Role + // should get certain directory-wide Azure roles in the environment. This is necessary when an Azure role can't + // be given to a group for usage with GrantsDevAzureGroup. Specifics are defined in configuration and within the + // role propagation package because it is uniquely complex and high-risk to change. + GrantsDevAzureDirectoryRoles *bool + // GrantsProdAzureDirectoryRoles, when true, indicates that a User with an unsuspended RoleAssignment to this Role + // should get certain directory-wide Azure roles in the environment. This is necessary when an Azure role can't + // be given to a group for usage with GrantsProdAzureGroup. Specifics are defined in configuration and within the + // role propagation package because it is uniquely complex and high-risk to change. + GrantsProdAzureDirectoryRoles *bool + // GrantsBroadInstituteGroup, when not null, indicates that a User with an unsuspended RoleAssignment to this // Role should have their Broad Institute account (assuming it is active) added to this group. // Multiple values can be comma separated and whitespace will be trimmed. diff --git a/sherlock/internal/models/test_data.go b/sherlock/internal/models/test_data.go index e32aa1f82..cb2af7182 100644 --- a/sherlock/internal/models/test_data.go +++ b/sherlock/internal/models/test_data.go @@ -360,6 +360,8 @@ func (td *testDataImpl) Role_TerraSuitableEngineer() Role { Name: utils.PointerTo("terra-suitable-engineer"), SuspendNonSuitableUsers: utils.PointerTo(true), AutoAssignAllUsers: utils.PointerTo(false), + GrantsDevAzureAccount: utils.PointerTo(true), + GrantsProdAzureAccount: utils.PointerTo(true), GrantsDevFirecloudGroup: utils.PointerTo("terra-suitable-engineer-dev"), GrantsQaFirecloudGroup: utils.PointerTo("terra-suitable-engineer-qa"), GrantsProdFirecloudGroup: utils.PointerTo("terra-suitable-engineer-prod"), @@ -368,6 +370,8 @@ func (td *testDataImpl) Role_TerraSuitableEngineer() Role { GrantsProdFirecloudFolderOwner: utils.PointerTo("folders/prod789"), GrantsDevAzureGroup: utils.PointerTo("00000000-0000-0000-0000-000000000001"), GrantsProdAzureGroup: utils.PointerTo("00000000-0000-0000-0000-000000000002"), + GrantsDevAzureDirectoryRoles: utils.PointerTo(true), + GrantsProdAzureDirectoryRoles: utils.PointerTo(true), GrantsBroadInstituteGroup: utils.PointerTo("terra-suitable-engineer-broadinstitute"), }, } diff --git a/sherlock/internal/role_propagation/boot.go b/sherlock/internal/role_propagation/boot.go index 30157768f..9749e3638 100644 --- a/sherlock/internal/role_propagation/boot.go +++ b/sherlock/internal/role_propagation/boot.go @@ -2,6 +2,7 @@ package role_propagation import ( "context" + "fmt" "github.com/broadinstitute/sherlock/sherlock/internal/models" "github.com/broadinstitute/sherlock/sherlock/internal/role_propagation/propagation_engines" ) @@ -16,9 +17,41 @@ var propagators []propagator func Init(ctx context.Context) error { propagators = []propagator{ - // This "step" is for granting pre-existing accounts access to stuff. - // We should have separate steps before this in the sequential order - // for creating accounts. + // This step is for provisioning new accounts. + ¶llelizingPropagator{ + parallelPropagators: []propagator{ + &propagatorImpl[bool, propagation_engines.AzureAccountIdentifier, propagation_engines.AzureAccountFields]{ + configKey: "devAzureAccount", + getGrants: func(role models.Role) []*bool { return []*bool{role.GrantsDevAzureAccount} }, + engine: &propagation_engines.AzureAccountEngine{}, + }, + &propagatorImpl[bool, propagation_engines.AzureAccountIdentifier, propagation_engines.AzureAccountFields]{ + configKey: "prodAzureAccount", + getGrants: func(role models.Role) []*bool { return []*bool{role.GrantsProdAzureAccount} }, + engine: &propagation_engines.AzureAccountEngine{}, + }, + }, + }, + + // This step is for inviting or importing accounts provisioned above to + // other organizations or tenants, required before they can potentially + // be granted more localized access in the next step. + ¶llelizingPropagator{ + parallelPropagators: []propagator{ + &propagatorImpl[bool, propagation_engines.AzureInvitedAccountIdentifier, propagation_engines.AzureInvitedAccountFields]{ + configKey: "devAzureInvitedB2CAccount", + getGrants: func(role models.Role) []*bool { return []*bool{role.GrantsDevAzureAccount} }, + engine: &propagation_engines.AzureInvitedAccountEngine{}, + }, + &propagatorImpl[bool, propagation_engines.AzureInvitedAccountIdentifier, propagation_engines.AzureInvitedAccountFields]{ + configKey: "prodAzureInvitedB2CAccount", + getGrants: func(role models.Role) []*bool { return []*bool{role.GrantsProdAzureAccount} }, + engine: &propagation_engines.AzureInvitedAccountEngine{}, + }, + }, + }, + + // This step is for granting pre-existing accounts access to stuff. ¶llelizingPropagator{ parallelPropagators: []propagator{ &propagatorImpl[string, propagation_engines.GoogleWorkspaceGroupIdentifier, propagation_engines.GoogleWorkspaceGroupFields]{ @@ -37,24 +70,24 @@ func Init(ctx context.Context) error { engine: &propagation_engines.GoogleWorkspaceGroupEngine{}, }, - &propagatorImpl[string, propagation_engines.GoogleWorkspaceFolderOwnerIdentifier, propagation_engines.GoogleWorkspaceFolderOwnerFields]{ + &propagatorImpl[string, propagation_engines.GoogleCloudFolderRoleIdentifier, propagation_engines.GoogleCloudFolderRoleFields]{ configKey: "devFirecloudFolderOwner", getGrants: func(role models.Role) []*string { return splitStringPointerOnCommas(role.GrantsDevFirecloudFolderOwner) }, - engine: &propagation_engines.GoogleWorkspaceFolderOwnerEngine{}, + engine: &propagation_engines.GoogleCloudFolderRoleEngine{Role: propagation_engines.GoogleCloudOwnerRole}, }, - &propagatorImpl[string, propagation_engines.GoogleWorkspaceFolderOwnerIdentifier, propagation_engines.GoogleWorkspaceFolderOwnerFields]{ + &propagatorImpl[string, propagation_engines.GoogleCloudFolderRoleIdentifier, propagation_engines.GoogleCloudFolderRoleFields]{ configKey: "qaFirecloudFolderOwner", getGrants: func(role models.Role) []*string { return splitStringPointerOnCommas(role.GrantsQaFirecloudFolderOwner) }, - engine: &propagation_engines.GoogleWorkspaceFolderOwnerEngine{}, + engine: &propagation_engines.GoogleCloudFolderRoleEngine{Role: propagation_engines.GoogleCloudOwnerRole}, }, - &propagatorImpl[string, propagation_engines.GoogleWorkspaceFolderOwnerIdentifier, propagation_engines.GoogleWorkspaceFolderOwnerFields]{ + &propagatorImpl[string, propagation_engines.GoogleCloudFolderRoleIdentifier, propagation_engines.GoogleCloudFolderRoleFields]{ configKey: "prodFirecloudFolderOwner", getGrants: func(role models.Role) []*string { return splitStringPointerOnCommas(role.GrantsProdFirecloudFolderOwner) }, - engine: &propagation_engines.GoogleWorkspaceFolderOwnerEngine{}, + engine: &propagation_engines.GoogleCloudFolderRoleEngine{Role: propagation_engines.GoogleCloudOwnerRole}, }, &propagatorImpl[string, propagation_engines.AzureGroupIdentifier, propagation_engines.AzureGroupFields]{ @@ -68,6 +101,17 @@ func Init(ctx context.Context) error { engine: &propagation_engines.AzureGroupEngine{}, }, + &propagatorImpl[bool, propagation_engines.AzureDirectoryRoleIdentifier, propagation_engines.AzureDirectoryRoleFields]{ + configKey: "devAzureB2CReader", + getGrants: func(role models.Role) []*bool { return []*bool{role.GrantsDevAzureDirectoryRoles} }, + engine: &propagation_engines.AzureDirectoryRoleEngine{RoleTemplateID: propagation_engines.AzureGlobalReaderRoleTemplateID}, + }, + &propagatorImpl[bool, propagation_engines.AzureDirectoryRoleIdentifier, propagation_engines.AzureDirectoryRoleFields]{ + configKey: "prodAzureB2CReader", + getGrants: func(role models.Role) []*bool { return []*bool{role.GrantsProdAzureDirectoryRoles} }, + engine: &propagation_engines.AzureDirectoryRoleEngine{RoleTemplateID: propagation_engines.AzureGlobalReaderRoleTemplateID}, + }, + &propagatorImpl[string, propagation_engines.NonAdminGoogleGroupIdentifier, propagation_engines.NonAdminGoogleGroupFields]{ configKey: "broadInstituteGroup", getGrants: func(role models.Role) []*string { return splitStringPointerOnCommas(role.GrantsBroadInstituteGroup) }, @@ -78,7 +122,7 @@ func Init(ctx context.Context) error { } for _, p := range propagators { if err := p.Init(ctx); err != nil { - return err + return fmt.Errorf("failed to initialize propagator: %s%w", p.LogPrefix(), err) } } return nil diff --git a/sherlock/internal/role_propagation/intermediary_user/fields.go b/sherlock/internal/role_propagation/intermediary_user/fields.go index fb05bf35f..d7146d459 100644 --- a/sherlock/internal/role_propagation/intermediary_user/fields.go +++ b/sherlock/internal/role_propagation/intermediary_user/fields.go @@ -10,3 +10,24 @@ type Fields interface { // need to be updated. EqualTo(other Fields) bool } + +// MayBePresentWhileRemovedFields is an optional superset of Fields. It should be implemented when a user's data on +// a cloud provider may be present even after the user has been removed. It helps Sherlock know that it shouldn't +// infinitely keep trying to remove remnants of the user's data. +// +// Just like Fields, implementations should be in propagation_engines and should use non-pointer receivers. +type MayBePresentWhileRemovedFields interface { + Fields + + // MayConsiderAsAlreadyRemoved should return true if Sherlock should skip attempting to remove the user from the + // grant on the cloud provider. This method should only be called before removing a user -- it's output isn't + // instructive for adding or updating a user. + // + // An example would be for a grant that provisions Firecloud accounts for users. We create accounts normally, + // update fields normally, but we don't remove normally. When Sherlock goes to remove a user, we want it to + // merely suspend the user's account, not delete it. The problem is that the user's account is still present: + // Sherlock will keep removing it over and over again, each time the propagation runs. This method fixes that. + // It could return true if the fields indicated the user was suspended, telling Sherlock that the user was + // already in an acceptably removed state. + MayConsiderAsAlreadyRemoved() bool +} diff --git a/sherlock/internal/role_propagation/intermediary_user/intermediary_user_mocks/mock_may_be_present_while_removed_fields.go b/sherlock/internal/role_propagation/intermediary_user/intermediary_user_mocks/mock_may_be_present_while_removed_fields.go new file mode 100644 index 000000000..d73c96234 --- /dev/null +++ b/sherlock/internal/role_propagation/intermediary_user/intermediary_user_mocks/mock_may_be_present_while_removed_fields.go @@ -0,0 +1,118 @@ +// Code generated by mockery v2.32.4. DO NOT EDIT. + +package intermediary_user_mocks + +import ( + intermediary_user "github.com/broadinstitute/sherlock/sherlock/internal/role_propagation/intermediary_user" + mock "github.com/stretchr/testify/mock" +) + +// MockMayBePresentWhileRemovedFields is an autogenerated mock type for the MayBePresentWhileRemovedFields type +type MockMayBePresentWhileRemovedFields struct { + mock.Mock +} + +type MockMayBePresentWhileRemovedFields_Expecter struct { + mock *mock.Mock +} + +func (_m *MockMayBePresentWhileRemovedFields) EXPECT() *MockMayBePresentWhileRemovedFields_Expecter { + return &MockMayBePresentWhileRemovedFields_Expecter{mock: &_m.Mock} +} + +// EqualTo provides a mock function with given fields: other +func (_m *MockMayBePresentWhileRemovedFields) EqualTo(other intermediary_user.Fields) bool { + ret := _m.Called(other) + + var r0 bool + if rf, ok := ret.Get(0).(func(intermediary_user.Fields) bool); ok { + r0 = rf(other) + } else { + r0 = ret.Get(0).(bool) + } + + return r0 +} + +// MockMayBePresentWhileRemovedFields_EqualTo_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'EqualTo' +type MockMayBePresentWhileRemovedFields_EqualTo_Call struct { + *mock.Call +} + +// EqualTo is a helper method to define mock.On call +// - other intermediary_user.Fields +func (_e *MockMayBePresentWhileRemovedFields_Expecter) EqualTo(other interface{}) *MockMayBePresentWhileRemovedFields_EqualTo_Call { + return &MockMayBePresentWhileRemovedFields_EqualTo_Call{Call: _e.mock.On("EqualTo", other)} +} + +func (_c *MockMayBePresentWhileRemovedFields_EqualTo_Call) Run(run func(other intermediary_user.Fields)) *MockMayBePresentWhileRemovedFields_EqualTo_Call { + _c.Call.Run(func(args mock.Arguments) { + run(args[0].(intermediary_user.Fields)) + }) + return _c +} + +func (_c *MockMayBePresentWhileRemovedFields_EqualTo_Call) Return(_a0 bool) *MockMayBePresentWhileRemovedFields_EqualTo_Call { + _c.Call.Return(_a0) + return _c +} + +func (_c *MockMayBePresentWhileRemovedFields_EqualTo_Call) RunAndReturn(run func(intermediary_user.Fields) bool) *MockMayBePresentWhileRemovedFields_EqualTo_Call { + _c.Call.Return(run) + return _c +} + +// MayConsiderAsAlreadyRemoved provides a mock function with given fields: +func (_m *MockMayBePresentWhileRemovedFields) MayConsiderAsAlreadyRemoved() bool { + ret := _m.Called() + + var r0 bool + if rf, ok := ret.Get(0).(func() bool); ok { + r0 = rf() + } else { + r0 = ret.Get(0).(bool) + } + + return r0 +} + +// MockMayBePresentWhileRemovedFields_MayConsiderAsAlreadyRemoved_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'MayConsiderAsAlreadyRemoved' +type MockMayBePresentWhileRemovedFields_MayConsiderAsAlreadyRemoved_Call struct { + *mock.Call +} + +// MayConsiderAsAlreadyRemoved is a helper method to define mock.On call +func (_e *MockMayBePresentWhileRemovedFields_Expecter) MayConsiderAsAlreadyRemoved() *MockMayBePresentWhileRemovedFields_MayConsiderAsAlreadyRemoved_Call { + return &MockMayBePresentWhileRemovedFields_MayConsiderAsAlreadyRemoved_Call{Call: _e.mock.On("MayConsiderAsAlreadyRemoved")} +} + +func (_c *MockMayBePresentWhileRemovedFields_MayConsiderAsAlreadyRemoved_Call) Run(run func()) *MockMayBePresentWhileRemovedFields_MayConsiderAsAlreadyRemoved_Call { + _c.Call.Run(func(args mock.Arguments) { + run() + }) + return _c +} + +func (_c *MockMayBePresentWhileRemovedFields_MayConsiderAsAlreadyRemoved_Call) Return(_a0 bool) *MockMayBePresentWhileRemovedFields_MayConsiderAsAlreadyRemoved_Call { + _c.Call.Return(_a0) + return _c +} + +func (_c *MockMayBePresentWhileRemovedFields_MayConsiderAsAlreadyRemoved_Call) RunAndReturn(run func() bool) *MockMayBePresentWhileRemovedFields_MayConsiderAsAlreadyRemoved_Call { + _c.Call.Return(run) + return _c +} + +// NewMockMayBePresentWhileRemovedFields creates a new instance of MockMayBePresentWhileRemovedFields. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +// The first argument is typically a *testing.T value. +func NewMockMayBePresentWhileRemovedFields(t interface { + mock.TestingT + Cleanup(func()) +}) *MockMayBePresentWhileRemovedFields { + mock := &MockMayBePresentWhileRemovedFields{} + mock.Mock.Test(t) + + t.Cleanup(func() { mock.AssertExpectations(t) }) + + return mock +} diff --git a/sherlock/internal/role_propagation/parallelizing_propagator.go b/sherlock/internal/role_propagation/parallelizing_propagator.go index 50e976f4d..41fa8428e 100644 --- a/sherlock/internal/role_propagation/parallelizing_propagator.go +++ b/sherlock/internal/role_propagation/parallelizing_propagator.go @@ -36,7 +36,7 @@ func (p *parallelizingPropagator) LogPrefix() string { func (p *parallelizingPropagator) Init(ctx context.Context) error { for _, inner := range p.parallelPropagators { if err := inner.Init(ctx); err != nil { - return err + return fmt.Errorf("failed to initialize inner propagator: %s%w", inner.LogPrefix(), err) } } return nil diff --git a/sherlock/internal/role_propagation/propagate.go b/sherlock/internal/role_propagation/propagate.go index 93eaba1d7..d0d14705d 100644 --- a/sherlock/internal/role_propagation/propagate.go +++ b/sherlock/internal/role_propagation/propagate.go @@ -144,7 +144,7 @@ func doNonConcurrentPropagation(ctx context.Context, role models.Role) { } if len(results) > 0 || len(errors) > 0 { slack.SendPermissionChangeNotification(ctx, models.SelfUser.SlackReference(true), slack.PermissionChangeNotificationInputs{ - Summary: fmt.Sprintf("propagation for Role \"%s\" made changes", *role.Name), + Summary: fmt.Sprintf("propagation for Role \"%s\" reports:", *role.Name), Results: results, Errors: errors, }) diff --git a/sherlock/internal/role_propagation/propagation_engines/azure_account.go b/sherlock/internal/role_propagation/propagation_engines/azure_account.go new file mode 100644 index 000000000..3ef6666f5 --- /dev/null +++ b/sherlock/internal/role_propagation/propagation_engines/azure_account.go @@ -0,0 +1,268 @@ +package propagation_engines + +import ( + "context" + "fmt" + "github.com/Azure/azure-sdk-for-go/sdk/azidentity" + "github.com/broadinstitute/sherlock/go-shared/pkg/utils" + "github.com/broadinstitute/sherlock/sherlock/internal/models" + "github.com/broadinstitute/sherlock/sherlock/internal/role_propagation/intermediary_user" + "github.com/knadh/koanf" + abstractions "github.com/microsoft/kiota-abstractions-go" + msgraphsdk "github.com/microsoftgraph/msgraph-sdk-go" + msgraphgocore "github.com/microsoftgraph/msgraph-sdk-go-core" + graphmodels "github.com/microsoftgraph/msgraph-sdk-go/models" + "github.com/microsoftgraph/msgraph-sdk-go/users" + "github.com/sethvargo/go-password/password" + "reflect" + "strings" +) + +type AzureAccountIdentifier struct { + UserPrincipalName string `koanf:"userPrincipalName"` +} + +func (a AzureAccountIdentifier) EqualTo(other intermediary_user.Identifier) bool { + switch other := other.(type) { + case AzureAccountIdentifier: + return a.UserPrincipalName == other.UserPrincipalName + default: + return false + } +} + +type AzureAccountFields struct { + // AccountEnabled controls whether this account can be signed in to (to access any tenant, + // home or invited) + AccountEnabled bool + // Email controls the "mail" field of the user, which can technically be different from + // the "userPrincipalName". For this account type, the userPrincipalName is the email and + // should be the same as this field. We still have this as a field here so that Sherlock + // will correct it should it get out of sync somehow (it is mutable in the UI). + Email string + // DisplayName is the human-readable name of the user + DisplayName string + // MailNickname is the prefix of the UPN before the @ symbol. It's here so Sherlock + // can correct it if it gets mutated (and because we do have to set it during creation) + MailNickname string + // OtherMails is a list of other email addresses associated with the user. Critically, + // this list must include the user's Broad email address, as this is how invites end up + // reaching people. + OtherMails []string +} + +func (a AzureAccountFields) EqualTo(other intermediary_user.Fields) bool { + switch other := other.(type) { + case AzureAccountFields: + return a.AccountEnabled == other.AccountEnabled && + a.Email == other.Email && + a.DisplayName == other.DisplayName && + a.MailNickname == other.MailNickname && + reflect.DeepEqual(a.OtherMails, other.OtherMails) + default: + return false + } +} + +// MayConsiderAsAlreadyRemoved returns true if the account is disabled, since that's how we +// remove accounts (we suspend rather than delete accounts). +func (a AzureAccountFields) MayConsiderAsAlreadyRemoved() bool { + return !a.AccountEnabled +} + +var _ PropagationEngine[bool, AzureAccountIdentifier, AzureAccountFields] = &AzureAccountEngine{} + +type AzureAccountEngine struct { + tenantEmailSuffix string + userEmailSuffixesToReplace []string + client *msgraphsdk.GraphServiceClient + passwordGenerator *password.Generator +} + +func (a *AzureAccountEngine) Init(_ context.Context, k *koanf.Koanf) error { + a.tenantEmailSuffix = k.String("tenantEmailSuffix") + a.userEmailSuffixesToReplace = k.Strings("userEmailSuffixesToReplace") + credentials, err := azidentity.NewWorkloadIdentityCredential(&azidentity.WorkloadIdentityCredentialOptions{ + ClientID: k.String("clientID"), + TenantID: k.String("tenantID"), + TokenFilePath: k.String("tokenFilePath"), + }) + if err != nil { + return fmt.Errorf("failed to create workload identity credential: %w", err) + } + + a.client, err = msgraphsdk.NewGraphServiceClientWithCredentials(credentials, nil) + if err != nil { + return fmt.Errorf("failed to create graph service client: %w", err) + } + + a.passwordGenerator, err = password.NewGenerator(nil) + if err != nil { + return fmt.Errorf("failed to create password generator: %w", err) + } + + return nil +} + +func (a *AzureAccountEngine) LoadCurrentState(ctx context.Context, _ bool) ([]intermediary_user.IntermediaryUser[AzureAccountIdentifier, AzureAccountFields], error) { + currentState := make([]intermediary_user.IntermediaryUser[AzureAccountIdentifier, AzureAccountFields], 0) + + headers := abstractions.NewRequestHeaders() + headers.Add("ConsistencyLevel", "eventual") + configuration := &users.UsersRequestBuilderGetRequestConfiguration{ + Headers: headers, + QueryParameters: &users.UsersRequestBuilderGetQueryParameters{ + // Select a non-standard set of fields + Select: []string{"userPrincipalName", "accountEnabled", "mail", "displayName", "mailNickname", "otherMails"}, + // This filter is efficient but is an advanced query, so needs an eventual consistency level header + // and the count query parameter + // https://learn.microsoft.com/en-us/graph/aad-advanced-queries?tabs=go + Filter: utils.PointerTo(fmt.Sprintf("endsWith(userPrincipalName, '%s')", a.tenantEmailSuffix)), + // Count is needed because we're using filter. It provides the count in the response *in addition to* + // the rest of the response. It's a bit weird but I think it's to help guide you towards debugging + // issues with the eventual consistency level. Since Sherlock is doing a reconciliation loop of its + // own I'm making a blanket assumption that eventual consistency on the part of a cloud provider is + // okay. + Count: utils.PointerTo(true), + // We want to get all the users, so interestingly we need to explicitly say we want just the first + // N, and then that basically sets our page size for the page iterator we use below. At the time of + // writing this, when you don't set this you seem to get the first 30 entries only, but with either + // no @odata.nextLink property or one that the page iterator below doesn't understand -- and this + // behavior doesn't really seem documented. The docs on paging do mention that the API sometimes has + // a default page size, but it implies that an @odata.nextLink property will be present. + // Anyway, if you remove just this one parameter, even if you do the page iterator below you won't + // actually end up iterating over all the users. + // The value of 25 is arbitrary and doesn't really matter since we end up iterating over everything. + // It was picked as a non-30 number to try to differentiate from the discovered default. + Top: utils.PointerTo[int32](25), + }, + } + + result, err := a.client.Users().Get(ctx, configuration) + if err != nil { + return nil, fmt.Errorf("failed to get users: %w", err) + } + + pageIterator, err := msgraphgocore.NewPageIterator[graphmodels.Userable](result, a.client.GetAdapter(), graphmodels.CreateUserCollectionResponseFromDiscriminatorValue) + if err != nil { + return nil, fmt.Errorf("failed to create page iterator for users: %w", err) + } + pageIterator.SetHeaders(headers) + + err = pageIterator.Iterate(ctx, func(pageItem graphmodels.Userable) bool { + if userPrincipalName := pageItem.GetUserPrincipalName(); userPrincipalName != nil && strings.HasSuffix(*userPrincipalName, a.tenantEmailSuffix) { + var fields AzureAccountFields + if accountEnabled := pageItem.GetAccountEnabled(); accountEnabled != nil { + fields.AccountEnabled = *accountEnabled + } + if mail := pageItem.GetMail(); mail != nil { + fields.Email = *mail + } + if displayName := pageItem.GetDisplayName(); displayName != nil { + fields.DisplayName = *displayName + } + if mailNickname := pageItem.GetMailNickname(); mailNickname != nil { + fields.MailNickname = *mailNickname + } + if otherMails := pageItem.GetOtherMails(); otherMails != nil { + fields.OtherMails = otherMails + } + currentState = append(currentState, intermediary_user.IntermediaryUser[AzureAccountIdentifier, AzureAccountFields]{ + Identifier: AzureAccountIdentifier{UserPrincipalName: *userPrincipalName}, + Fields: fields, + }) + } + return true + }) + return currentState, err +} + +func (a *AzureAccountEngine) GenerateDesiredState(_ context.Context, roleAssignments map[uint]models.RoleAssignment) (map[uint]intermediary_user.IntermediaryUser[AzureAccountIdentifier, AzureAccountFields], error) { + desiredState := make(map[uint]intermediary_user.IntermediaryUser[AzureAccountIdentifier, AzureAccountFields]) + for sherlockUserID, roleAssignment := range roleAssignments { + email := utils.SubstituteSuffix(roleAssignment.User.Email, a.userEmailSuffixesToReplace, a.tenantEmailSuffix) + if !strings.HasSuffix(email, a.tenantEmailSuffix) { + // We can short circuit here because we're only responsible for creating accounts with the + // given email suffix + continue + } + desiredState[sherlockUserID] = intermediary_user.IntermediaryUser[AzureAccountIdentifier, AzureAccountFields]{ + Identifier: AzureAccountIdentifier{UserPrincipalName: email}, + Fields: AzureAccountFields{ + AccountEnabled: roleAssignment.IsActive(), + Email: email, + DisplayName: roleAssignment.User.NameOrUsername(), + MailNickname: strings.Split(email, "@")[0], + OtherMails: []string{roleAssignment.User.Email}, + }, + } + } + return desiredState, nil +} + +func (a *AzureAccountEngine) Add(ctx context.Context, _ bool, identifier AzureAccountIdentifier, fields AzureAccountFields) (string, error) { + randomPassword, err := a.passwordGenerator.Generate(256, 32, 32, false, true) + if err != nil { + return "", fmt.Errorf("failed to generate random password: %w", err) + } + + body := graphmodels.NewUser() + body.SetUserPrincipalName(utils.PointerTo(identifier.UserPrincipalName)) + body.SetMail(utils.PointerTo(fields.Email)) + body.SetDisplayName(utils.PointerTo(fields.DisplayName)) + body.SetMailNickname(utils.PointerTo(fields.MailNickname)) + body.SetAccountEnabled(utils.PointerTo(fields.AccountEnabled)) + body.SetOtherMails(fields.OtherMails) + passwordProfile := graphmodels.NewPasswordProfile() + passwordProfile.SetForceChangePasswordNextSignIn(utils.PointerTo(true)) + // We don't send this password anywhere, forcing the user to go through the password reset flow. + passwordProfile.SetPassword(utils.PointerTo(randomPassword)) + body.SetPasswordProfile(passwordProfile) + + _, err = a.client.Users().Post(ctx, body, nil) + if err != nil { + return "", fmt.Errorf("failed to create user %s: %w", identifier.UserPrincipalName, err) + } else { + return fmt.Sprintf("created user %s", identifier.UserPrincipalName), nil + } +} + +func (a *AzureAccountEngine) Update(ctx context.Context, _ bool, identifier AzureAccountIdentifier, oldFields AzureAccountFields, newFields AzureAccountFields) (string, error) { + body := graphmodels.NewUser() + body.SetMail(utils.PointerTo(newFields.Email)) + body.SetDisplayName(utils.PointerTo(newFields.DisplayName)) + body.SetMailNickname(utils.PointerTo(newFields.MailNickname)) + body.SetAccountEnabled(utils.PointerTo(newFields.AccountEnabled)) + body.SetOtherMails(newFields.OtherMails) + _, err := a.client.Users().ByUserId(identifier.UserPrincipalName).Patch(ctx, body, nil) + if err != nil { + return "", fmt.Errorf("failed to update user %s (%s): %w", identifier.UserPrincipalName, a.describeDiff(oldFields, newFields), err) + } else { + return fmt.Sprintf("updated user %s (%s)", identifier.UserPrincipalName, a.describeDiff(oldFields, newFields)), nil + } +} + +func (a *AzureAccountEngine) describeDiff(oldFields AzureAccountFields, newFields AzureAccountFields) string { + if oldFields.AccountEnabled && !newFields.AccountEnabled { + return "disable account" + } else if !oldFields.AccountEnabled && newFields.AccountEnabled { + return "enable account" + } else if oldFields.Email != newFields.Email || oldFields.MailNickname != newFields.MailNickname || !reflect.DeepEqual(oldFields.OtherMails, newFields.OtherMails) { + return "update account email info" // This is really, *really* unlikely to happen but we'll at least handle it + } else if oldFields.DisplayName != newFields.DisplayName { + return fmt.Sprintf("update display name from `%s` to `%s`", oldFields.DisplayName, newFields.DisplayName) + } else { + return "no changes" + } +} + +func (a *AzureAccountEngine) Remove(ctx context.Context, _ bool, identifier AzureAccountIdentifier) (string, error) { + body := graphmodels.NewUser() + body.SetAccountEnabled(utils.PointerTo(false)) + _, err := a.client.Users().ByUserId(identifier.UserPrincipalName).Patch(ctx, body, nil) + if err != nil { + return "", fmt.Errorf("failed to disable user %s: %w", identifier.UserPrincipalName, err) + } else { + return fmt.Sprintf("disabled user %s", identifier.UserPrincipalName), nil + } +} diff --git a/sherlock/internal/role_propagation/propagation_engines/azure_account_test.go b/sherlock/internal/role_propagation/propagation_engines/azure_account_test.go new file mode 100644 index 000000000..925558548 --- /dev/null +++ b/sherlock/internal/role_propagation/propagation_engines/azure_account_test.go @@ -0,0 +1,449 @@ +package propagation_engines + +import ( + "context" + "github.com/broadinstitute/sherlock/go-shared/pkg/utils" + "github.com/broadinstitute/sherlock/sherlock/internal/models" + "github.com/broadinstitute/sherlock/sherlock/internal/role_propagation/intermediary_user" + "github.com/stretchr/testify/assert" + "testing" +) + +func TestAzureAccountIdentifier_EqualTo(t *testing.T) { + type fields struct { + Email string + } + type args struct { + other intermediary_user.Identifier + } + tests := []struct { + name string + fields fields + args args + want bool + }{ + { + name: "equal", + fields: fields{ + Email: "foo", + }, + args: args{ + other: AzureAccountIdentifier{ + UserPrincipalName: "foo", + }, + }, + want: true, + }, + { + name: "not equal", + fields: fields{ + Email: "foo", + }, + args: args{ + other: AzureAccountIdentifier{ + UserPrincipalName: "bar", + }, + }, + want: false, + }, + { + name: "different type", + fields: fields{ + Email: "foo", + }, + args: args{ + other: GoogleWorkspaceGroupIdentifier{ + Email: "foo", + }, + }, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + a := AzureAccountIdentifier{ + UserPrincipalName: tt.fields.Email, + } + assert.Equalf(t, tt.want, a.EqualTo(tt.args.other), "EqualTo(%v)", tt.args.other) + }) + } +} + +func TestAzureAccountFields_EqualTo(t *testing.T) { + type fields struct { + AccountEnabled bool + Email string + DisplayName string + MailNickname string + OtherMails []string + } + type args struct { + other intermediary_user.Fields + } + tests := []struct { + name string + fields fields + args args + want bool + }{ + { + name: "equal", + fields: fields{ + AccountEnabled: true, + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + args: args{ + other: AzureAccountFields{ + AccountEnabled: true, + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + }, + want: true, + }, + { + name: "account enabled not equal", + fields: fields{ + AccountEnabled: true, + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + args: args{ + other: AzureAccountFields{ + AccountEnabled: false, + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + }, + want: false, + }, + { + name: "smtp mail not equal", + fields: fields{ + AccountEnabled: true, + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + args: args{ + other: AzureAccountFields{ + AccountEnabled: true, + Email: "bar", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + }, + want: false, + }, + { + name: "display name not equal", + fields: fields{ + AccountEnabled: true, + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + args: args{ + other: AzureAccountFields{ + AccountEnabled: true, + Email: "foo", + DisplayName: "foo", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + }, + want: false, + }, + { + name: "mail nickname not equal", + fields: fields{ + AccountEnabled: true, + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + args: args{ + other: AzureAccountFields{ + AccountEnabled: true, + Email: "foo", + DisplayName: "bar", + MailNickname: "foo", + OtherMails: []string{"qux"}, + }, + }, + want: false, + }, + { + name: "other mails same length but not equal", + fields: fields{ + AccountEnabled: true, + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + args: args{ + other: AzureAccountFields{ + AccountEnabled: true, + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"foo"}, + }, + }, + want: false, + }, + { + name: "other mails not equal", + fields: fields{ + AccountEnabled: true, + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + args: args{ + other: AzureAccountFields{ + AccountEnabled: true, + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"qux", "foo"}, + }, + }, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + a := AzureAccountFields{ + AccountEnabled: tt.fields.AccountEnabled, + Email: tt.fields.Email, + DisplayName: tt.fields.DisplayName, + MailNickname: tt.fields.MailNickname, + OtherMails: tt.fields.OtherMails, + } + assert.Equalf(t, tt.want, a.EqualTo(tt.args.other), "EqualTo(%v)", tt.args.other) + }) + } +} + +func TestAzureAccountFields_MayConsiderAsAlreadyRemoved(t *testing.T) { + type fields struct { + AccountEnabled bool + Email string + DisplayName string + MailNickname string + OtherMails []string + } + tests := []struct { + name string + fields fields + want bool + }{ + { + name: "account enabled", + fields: fields{ + AccountEnabled: true, + }, + want: false, + }, + { + name: "account disabled", + fields: fields{ + AccountEnabled: false, + }, + want: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + a := AzureAccountFields{ + AccountEnabled: tt.fields.AccountEnabled, + Email: tt.fields.Email, + DisplayName: tt.fields.DisplayName, + MailNickname: tt.fields.MailNickname, + OtherMails: tt.fields.OtherMails, + } + assert.Equalf(t, tt.want, a.MayConsiderAsAlreadyRemoved(), "MayConsiderAsAlreadyRemoved()") + }) + } +} + +// We can't easily test the actual cloud logic, but we can test that we short circuit correctly for +// emails that aren't in the target domain. +// +// See also utils.SubstituteSuffix +func TestAzureAccountEngine_GenerateDesiredState_emailShortCircuit(t *testing.T) { + engine := &AzureAccountEngine{ + tenantEmailSuffix: "@example.com", + userEmailSuffixesToReplace: []string{"@example.org"}, + } + desiredState, err := engine.GenerateDesiredState(context.Background(), map[uint]models.RoleAssignment{ + 1: { + User: &models.User{ + Email: "user@example.net", + }, + RoleAssignmentFields: models.RoleAssignmentFields{ + Suspended: utils.PointerTo(false), + }, + }, + }) + assert.NoError(t, err) + assert.Empty(t, desiredState) +} + +func TestAzureAccountEngine_describeDiff(t *testing.T) { + engine := &AzureAccountEngine{} + tests := []struct { + name string + oldFields AzureAccountFields + newFields AzureAccountFields + want string + }{ + { + name: "no changes", + oldFields: AzureAccountFields{ + AccountEnabled: true, + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + newFields: AzureAccountFields{ + AccountEnabled: true, + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + want: "no changes", + }, + { + name: "account enabled", + oldFields: AzureAccountFields{ + AccountEnabled: false, + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + newFields: AzureAccountFields{ + AccountEnabled: true, + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + want: "enable account", + }, + { + name: "account disabled", + oldFields: AzureAccountFields{ + AccountEnabled: true, + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + newFields: AzureAccountFields{ + AccountEnabled: false, + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + want: "disable account", + }, + { + name: "smtp mail", + oldFields: AzureAccountFields{ + AccountEnabled: true, + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + newFields: AzureAccountFields{ + AccountEnabled: true, + Email: "bar", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + want: "update account email info", + }, + { + name: "mail nickname", + oldFields: AzureAccountFields{ + AccountEnabled: true, + Email: "foo", + DisplayName: "bar", + MailNickname: "foo", + OtherMails: []string{"qux"}, + }, + newFields: AzureAccountFields{ + AccountEnabled: true, + Email: "foo", + DisplayName: "bar", + MailNickname: "bar", + OtherMails: []string{"qux"}, + }, + want: "update account email info", + }, + { + name: "other mails", + oldFields: AzureAccountFields{ + AccountEnabled: true, + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"foo"}, + }, + newFields: AzureAccountFields{ + AccountEnabled: true, + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"bar"}, + }, + want: "update account email info", + }, + { + name: "name change", + oldFields: AzureAccountFields{ + AccountEnabled: true, + Email: "foo", + DisplayName: "foo", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + newFields: AzureAccountFields{ + AccountEnabled: true, + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + want: "update display name from `foo` to `bar`", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equalf(t, tt.want, engine.describeDiff(tt.oldFields, tt.newFields), "describeDiff(%v, %v)", tt.oldFields, tt.newFields) + }) + } +} diff --git a/sherlock/internal/role_propagation/propagation_engines/azure_directory_role.go b/sherlock/internal/role_propagation/propagation_engines/azure_directory_role.go new file mode 100644 index 000000000..10813634a --- /dev/null +++ b/sherlock/internal/role_propagation/propagation_engines/azure_directory_role.go @@ -0,0 +1,210 @@ +package propagation_engines + +import ( + "context" + "fmt" + "github.com/Azure/azure-sdk-for-go/sdk/azidentity" + "github.com/broadinstitute/sherlock/go-shared/pkg/utils" + "github.com/broadinstitute/sherlock/sherlock/internal/models" + "github.com/broadinstitute/sherlock/sherlock/internal/role_propagation/intermediary_user" + "github.com/knadh/koanf" + abstractions "github.com/microsoft/kiota-abstractions-go" + msgraphsdk "github.com/microsoftgraph/msgraph-sdk-go" + msgraphgocore "github.com/microsoftgraph/msgraph-sdk-go-core" + "github.com/microsoftgraph/msgraph-sdk-go/directoryroles" + "github.com/microsoftgraph/msgraph-sdk-go/directoryroleswithroletemplateid" + graphmodels "github.com/microsoftgraph/msgraph-sdk-go/models" + "github.com/microsoftgraph/msgraph-sdk-go/users" + "strings" +) + +const AzureGlobalReaderRoleTemplateID = "f2ef992c-3afb-46b9-b7cf-a126ee74c451" + +type AzureDirectoryRoleIdentifier struct { + // ID is technically a UUID, but this is an intermediary type so we'd just be more brittle if we enforced that. + // Also makes parsing via koanf more complicated, mapstructure requires a tiny bit of custom code to handle UUIDs. + ID string `koanf:"id"` +} + +func (a AzureDirectoryRoleIdentifier) EqualTo(other intermediary_user.Identifier) bool { + switch other := other.(type) { + case AzureDirectoryRoleIdentifier: + return a.ID == other.ID + default: + return false + } +} + +type AzureDirectoryRoleFields struct{} + +func (a AzureDirectoryRoleFields) EqualTo(other intermediary_user.Fields) bool { + switch other.(type) { + case AzureDirectoryRoleFields: + return true + default: + return false + } +} + +var _ PropagationEngine[bool, AzureDirectoryRoleIdentifier, AzureDirectoryRoleFields] = &AzureDirectoryRoleEngine{} + +// AzureDirectoryRoleEngine notably uses a boolean grant. This means that there's exactly one non-zero +// grant value -- so exactly one Sherlock role will be able to grant the Azure role an instance of this +// engine provides. +// +// The logic here, and why this is different from GoogleCloudFolderRoleEngine, is that our actual auth +// mechanism changes based on the directory we're granting a role in. At present, we just don't have a +// complex enough directory setup to justify setting client IDs and on-disk projected tokens paths through +// Sherlock's API. It's simplest for these things to be set in config, and while that means this engine +// is a bit "dumb", it does the bare minimum that we need right now. +// +// See below for why Role isn't configurable either. +type AzureDirectoryRoleEngine struct { + // RoleTemplateID is defined in Sherlock's source code where this engine is instantiated, not in + // configuration. This is because it helps avoid a foot-gun with accidentally misconfiguring the engine. + // When this engine is pointed at a directory, it wants to "own" all members of the given role. It will + // remove members that don't correlate to Sherlock role assignments. If we allowed easy configuration of + // the role, it would be easy to accidentally cause problems by pointing this engine at a role that some + // other system also wanted to manage or relied on. + RoleTemplateID string + + directoryRoleID string + memberEmailSuffix string + userEmailSuffixesToReplace []string + client *msgraphsdk.GraphServiceClient +} + +func (a *AzureDirectoryRoleEngine) Init(ctx context.Context, k *koanf.Koanf) error { + if a.RoleTemplateID == "" { + return fmt.Errorf("role template ID must be set") + } + + a.memberEmailSuffix = k.String("memberEmailSuffix") + a.userEmailSuffixesToReplace = k.Strings("userEmailSuffixesToReplace") + + credentials, err := azidentity.NewWorkloadIdentityCredential(&azidentity.WorkloadIdentityCredentialOptions{ + ClientID: k.String("clientID"), + TenantID: k.String("tenantID"), + TokenFilePath: k.String("tokenFilePath"), + }) + if err != nil { + return err + } + + a.client, err = msgraphsdk.NewGraphServiceClientWithCredentials(credentials, nil) + if err != nil { + return fmt.Errorf("failed to instantiate MS Graph client: %w", err) + } + + matchingDirectoryRoles, err := a.client.DirectoryRolesWithRoleTemplateId(&a.RoleTemplateID).Get(ctx, &directoryroleswithroletemplateid.DirectoryRolesWithRoleTemplateIdRequestBuilderGetRequestConfiguration{ + QueryParameters: &directoryroleswithroletemplateid.DirectoryRolesWithRoleTemplateIdRequestBuilderGetQueryParameters{ + Select: []string{"id"}, + }, + }) + if err != nil { + return fmt.Errorf("failed to fetch directory role ID for role template ID %s: %w", a.RoleTemplateID, err) + } else if id := matchingDirectoryRoles.GetId(); id == nil { + return fmt.Errorf("no directory role found for role template ID %s (but no error returned either -- ID was nil)", a.RoleTemplateID) + } else { + a.directoryRoleID = *id + } + + return nil +} + +func (a *AzureDirectoryRoleEngine) LoadCurrentState(ctx context.Context, _ bool) ([]intermediary_user.IntermediaryUser[AzureDirectoryRoleIdentifier, AzureDirectoryRoleFields], error) { + currentState := make([]intermediary_user.IntermediaryUser[AzureDirectoryRoleIdentifier, AzureDirectoryRoleFields], 0) + + headers := abstractions.NewRequestHeaders() + configuration := &directoryroles.ItemMembersRequestBuilderGetRequestConfiguration{ + Headers: headers, + QueryParameters: &directoryroles.ItemMembersRequestBuilderGetQueryParameters{ + Select: []string{"id"}, + // No top because that's actually not supported here + }, + } + + result, err := a.client.DirectoryRoles().ByDirectoryRoleId(a.directoryRoleID).Members().Get(ctx, configuration) + if err != nil { + return nil, fmt.Errorf("failed to fetch directory role members: %w", err) + } + + pageIterator, err := msgraphgocore.NewPageIterator[graphmodels.DirectoryObjectable](result, + a.client.GetAdapter(), graphmodels.CreateDirectoryObjectCollectionResponseFromDiscriminatorValue) + if err != nil { + return nil, fmt.Errorf("failed to create page iterator for directory role members: %w", err) + } + pageIterator.SetHeaders(headers) + + err = pageIterator.Iterate(ctx, func(pageItem graphmodels.DirectoryObjectable) bool { + if id := pageItem.GetId(); id != nil { + currentState = append(currentState, intermediary_user.IntermediaryUser[AzureDirectoryRoleIdentifier, AzureDirectoryRoleFields]{ + Identifier: AzureDirectoryRoleIdentifier{ID: *id}, + Fields: AzureDirectoryRoleFields{}, + }) + } + return true + }) + return currentState, err +} + +func (a *AzureDirectoryRoleEngine) GenerateDesiredState(ctx context.Context, roleAssignments map[uint]models.RoleAssignment) (map[uint]intermediary_user.IntermediaryUser[AzureDirectoryRoleIdentifier, AzureDirectoryRoleFields], error) { + desiredState := make(map[uint]intermediary_user.IntermediaryUser[AzureDirectoryRoleIdentifier, AzureDirectoryRoleFields]) + for sherlockUserID, roleAssignment := range roleAssignments { + if !roleAssignment.IsActive() { + // There's no concept of a suspended group member, so we just exclude any non-active users + continue + } + + email := utils.SubstituteSuffix(roleAssignment.User.Email, a.userEmailSuffixesToReplace, a.memberEmailSuffix) + if !strings.HasSuffix(email, a.memberEmailSuffix) { + // We can short-circuit here, we know that the user is not in the expected member domain so we won't bother looking + continue + } + + usersResponse, err := a.client.Users().Get(ctx, &users.UsersRequestBuilderGetRequestConfiguration{ + QueryParameters: &users.UsersRequestBuilderGetQueryParameters{ + Select: []string{"id"}, + Filter: utils.PointerTo(fmt.Sprintf("userPrincipalName eq '%s'", email)), + Top: utils.PointerTo[int32](1), + }, + }) + if err != nil { + return nil, err + } else { + for _, user := range usersResponse.GetValue() { + if id := user.GetId(); id != nil { + desiredState[sherlockUserID] = intermediary_user.IntermediaryUser[AzureDirectoryRoleIdentifier, AzureDirectoryRoleFields]{ + Identifier: AzureDirectoryRoleIdentifier{ID: *user.GetId()}, + Fields: AzureDirectoryRoleFields{}, + } + } + } + } + } + return desiredState, nil +} + +func (a *AzureDirectoryRoleEngine) Add(ctx context.Context, _ bool, identifier AzureDirectoryRoleIdentifier, _ AzureDirectoryRoleFields) (string, error) { + body := graphmodels.NewReferenceCreate() + body.SetOdataId(utils.PointerTo(fmt.Sprintf("https://graph.microsoft.com/v1.0/directoryObjects/%s", identifier.ID))) + err := a.client.DirectoryRoles().ByDirectoryRoleId(a.directoryRoleID).Members().Ref().Post(ctx, body, nil) + if err != nil { + return "", fmt.Errorf("failed to add user %s to role %s: %w", identifier.ID, a.directoryRoleID, err) + } else { + return fmt.Sprintf("added user %s to role %s", identifier.ID, a.directoryRoleID), nil + } +} + +func (a *AzureDirectoryRoleEngine) Update(_ context.Context, _ bool, _ AzureDirectoryRoleIdentifier, _ AzureDirectoryRoleFields, _ AzureDirectoryRoleFields) (string, error) { + return "", fmt.Errorf("%T.Update not implemented, %T.EqualTo should always return true", a, AzureDirectoryRoleFields{}) +} + +func (a *AzureDirectoryRoleEngine) Remove(ctx context.Context, _ bool, identifier AzureDirectoryRoleIdentifier) (string, error) { + err := a.client.DirectoryRoles().ByDirectoryRoleId(a.directoryRoleID).Members().ByDirectoryObjectId(identifier.ID).Ref().Delete(ctx, nil) + if err != nil { + return "", fmt.Errorf("failed to remove user %s from role %s: %w", identifier.ID, a.directoryRoleID, err) + } else { + return fmt.Sprintf("removed user %s from role %s", identifier.ID, a.directoryRoleID), nil + } +} diff --git a/sherlock/internal/role_propagation/propagation_engines/azure_directory_role_test.go b/sherlock/internal/role_propagation/propagation_engines/azure_directory_role_test.go new file mode 100644 index 000000000..3abeaae7c --- /dev/null +++ b/sherlock/internal/role_propagation/propagation_engines/azure_directory_role_test.go @@ -0,0 +1,153 @@ +package propagation_engines + +import ( + "context" + "github.com/broadinstitute/sherlock/go-shared/pkg/utils" + "github.com/broadinstitute/sherlock/sherlock/internal/models" + "github.com/broadinstitute/sherlock/sherlock/internal/role_propagation/intermediary_user" + "github.com/stretchr/testify/assert" + "testing" + "time" +) + +func TestAzureDirectoryRoleIdentifier_EqualTo(t *testing.T) { + type fields struct { + ID string + } + type args struct { + other intermediary_user.Identifier + } + tests := []struct { + name string + fields fields + args args + want bool + }{ + { + name: "equal", + fields: fields{ + ID: "foo", + }, + args: args{ + other: AzureDirectoryRoleIdentifier{ + ID: "foo", + }, + }, + want: true, + }, + { + name: "not equal", + fields: fields{ + ID: "foo", + }, + args: args{ + other: AzureDirectoryRoleIdentifier{ + ID: "bar", + }, + }, + want: false, + }, + { + name: "different type", + fields: fields{ + ID: "foo", + }, + args: args{ + other: GoogleWorkspaceGroupIdentifier{ + Email: "foo", + }, + }, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + a := AzureDirectoryRoleIdentifier{ + ID: tt.fields.ID, + } + assert.Equalf(t, tt.want, a.EqualTo(tt.args.other), "EqualTo(%v)", tt.args.other) + }) + } +} + +func TestAzureDirectoryRoleFields_EqualTo(t *testing.T) { + type args struct { + other intermediary_user.Fields + } + tests := []struct { + name string + args args + want bool + }{ + { + name: "same type", + args: args{ + other: AzureDirectoryRoleFields{}, + }, + want: true, + }, + { + name: "different type", + args: args{ + other: GoogleWorkspaceGroupFields{}, + }, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + a := AzureDirectoryRoleFields{} + assert.Equalf(t, tt.want, a.EqualTo(tt.args.other), "EqualTo(%v)", tt.args.other) + }) + } +} + +// We can't easily test the actual cloud logic, but we can test that we short circuit correctly for +// non-active role assignments. +func TestAzureDirectoryRoleEngine_GenerateDesiredState_isActiveShortCircuit(t *testing.T) { + engine := &AzureDirectoryRoleEngine{} + desiredState, err := engine.GenerateDesiredState(context.Background(), map[uint]models.RoleAssignment{ + 1: { + RoleAssignmentFields: models.RoleAssignmentFields{ + Suspended: utils.PointerTo(true), + }, + }, + 2: { + RoleAssignmentFields: models.RoleAssignmentFields{ + Suspended: utils.PointerTo(false), + ExpiresAt: utils.PointerTo(time.Now().Add(-time.Hour)), + }, + }, + }) + assert.NoError(t, err) + assert.Empty(t, desiredState) +} + +// We can't easily test the actual cloud logic, but we can test that we short circuit correctly for +// emails that aren't in the target domain. +// +// See also utils.SubstituteSuffix +func TestAzureDirectoryRoleEngine_GenerateDesiredState_emailShortCircuit(t *testing.T) { + engine := &AzureDirectoryRoleEngine{ + memberEmailSuffix: "@example.com", + userEmailSuffixesToReplace: []string{"@example.org"}, + } + desiredState, err := engine.GenerateDesiredState(context.Background(), map[uint]models.RoleAssignment{ + 1: { + User: &models.User{ + Email: "user@example.net", + }, + RoleAssignmentFields: models.RoleAssignmentFields{ + Suspended: utils.PointerTo(false), + }, + }, + }) + assert.NoError(t, err) + assert.Empty(t, desiredState) +} + +func TestAzureDirectoryRoleEngine_Update_errors(t *testing.T) { + engine := &AzureDirectoryRoleEngine{} + _, err := engine.Update(context.Background(), true, AzureDirectoryRoleIdentifier{}, AzureDirectoryRoleFields{}, AzureDirectoryRoleFields{}) + assert.Error(t, err) +} diff --git a/sherlock/internal/role_propagation/propagation_engines/azure_group.go b/sherlock/internal/role_propagation/propagation_engines/azure_group.go index fa1bc373c..b51522f50 100644 --- a/sherlock/internal/role_propagation/propagation_engines/azure_group.go +++ b/sherlock/internal/role_propagation/propagation_engines/azure_group.go @@ -8,7 +8,10 @@ import ( "github.com/broadinstitute/sherlock/sherlock/internal/models" "github.com/broadinstitute/sherlock/sherlock/internal/role_propagation/intermediary_user" "github.com/knadh/koanf" + abstractions "github.com/microsoft/kiota-abstractions-go" msgraphsdk "github.com/microsoftgraph/msgraph-sdk-go" + msgraphgocore "github.com/microsoftgraph/msgraph-sdk-go-core" + "github.com/microsoftgraph/msgraph-sdk-go/groups" graphmodels "github.com/microsoftgraph/msgraph-sdk-go/models" "github.com/microsoftgraph/msgraph-sdk-go/users" "strings" @@ -65,20 +68,36 @@ func (a *AzureGroupEngine) Init(_ context.Context, k *koanf.Koanf) error { func (a *AzureGroupEngine) LoadCurrentState(ctx context.Context, grant string) ([]intermediary_user.IntermediaryUser[AzureGroupIdentifier, AzureGroupFields], error) { currentState := make([]intermediary_user.IntermediaryUser[AzureGroupIdentifier, AzureGroupFields], 0) - groupMembersResponse, err := a.client.Groups().ByGroupId(grant).Members().Get(ctx, nil) + + headers := abstractions.NewRequestHeaders() + configuration := &groups.ItemMembersRequestBuilderGetRequestConfiguration{ + Headers: headers, + QueryParameters: &groups.ItemMembersRequestBuilderGetQueryParameters{ + Select: []string{"id"}, + Top: utils.PointerTo[int32](25), + }, + } + result, err := a.client.Groups().ByGroupId(grant).Members().Get(ctx, configuration) if err != nil { - return nil, err - } else { - for _, directoryObject := range groupMembersResponse.GetValue() { - if id := directoryObject.GetId(); id != nil { - currentState = append(currentState, intermediary_user.IntermediaryUser[AzureGroupIdentifier, AzureGroupFields]{ - Identifier: AzureGroupIdentifier{ID: *id}, - Fields: AzureGroupFields{}, - }) - } - } + return nil, fmt.Errorf("failed to get groups: %w", err) } - return currentState, nil + + pageIterator, err := msgraphgocore.NewPageIterator[graphmodels.DirectoryObjectable](result, a.client.GetAdapter(), graphmodels.CreateDirectoryObjectCollectionResponseFromDiscriminatorValue) + if err != nil { + return nil, fmt.Errorf("failed to create page iterator for members: %w", err) + } + pageIterator.SetHeaders(headers) + + err = pageIterator.Iterate(ctx, func(pageItem graphmodels.DirectoryObjectable) bool { + if id := pageItem.GetId(); id != nil { + currentState = append(currentState, intermediary_user.IntermediaryUser[AzureGroupIdentifier, AzureGroupFields]{ + Identifier: AzureGroupIdentifier{ID: *id}, + Fields: AzureGroupFields{}, + }) + } + return true + }) + return currentState, err } func (a *AzureGroupEngine) GenerateDesiredState(ctx context.Context, roleAssignments map[uint]models.RoleAssignment) (map[uint]intermediary_user.IntermediaryUser[AzureGroupIdentifier, AzureGroupFields], error) { diff --git a/sherlock/internal/role_propagation/propagation_engines/azure_invited_account.go b/sherlock/internal/role_propagation/propagation_engines/azure_invited_account.go new file mode 100644 index 000000000..e99e7b663 --- /dev/null +++ b/sherlock/internal/role_propagation/propagation_engines/azure_invited_account.go @@ -0,0 +1,327 @@ +package propagation_engines + +import ( + "context" + "crypto/rand" + "encoding/hex" + "fmt" + "github.com/Azure/azure-sdk-for-go/sdk/azidentity" + "github.com/broadinstitute/sherlock/go-shared/pkg/utils" + "github.com/broadinstitute/sherlock/sherlock/internal/clients/slack" + "github.com/broadinstitute/sherlock/sherlock/internal/models" + "github.com/broadinstitute/sherlock/sherlock/internal/role_propagation/intermediary_user" + "github.com/knadh/koanf" + abstractions "github.com/microsoft/kiota-abstractions-go" + msgraphsdk "github.com/microsoftgraph/msgraph-sdk-go" + msgraphgocore "github.com/microsoftgraph/msgraph-sdk-go-core" + graphmodels "github.com/microsoftgraph/msgraph-sdk-go/models" + "github.com/microsoftgraph/msgraph-sdk-go/users" + "reflect" + "strings" +) + +type AzureInvitedAccountIdentifier struct { + UserPrincipalName string `koanf:"userPrincipalName"` +} + +func (a AzureInvitedAccountIdentifier) EqualTo(other intermediary_user.Identifier) bool { + switch other := other.(type) { + case AzureInvitedAccountIdentifier: + return a.UserPrincipalName == other.UserPrincipalName + default: + return false + } +} + +// AzureInvitedAccountFields has a lot that we don't actually directly specify when inviting +// a user. That's okay, because after we invite the user we update the account fields like +// AzureAccountFields / AzureAccountEngine -- in other words, these fields are here to keep +// the account updated after creation. +type AzureInvitedAccountFields struct { + // Email controls the "mail" field of the user, which can technically be different from + // the "userPrincipalName". For this account type, the userPrincipalName is the email and + // should be the same as this field. We still have this as a field here so that Sherlock + // will correct it should it get out of sync somehow (it is mutable in the UI). + Email string + // DisplayName is the human-readable name of the user + DisplayName string + // MailNickname is the prefix of the UPN before the @ symbol. It's here so Sherlock + // can correct it if it gets mutated (and because we do have to set it during creation) + MailNickname string + // OtherMails is a list of other email addresses associated with the user. Critically, + // this list must include the user's Broad email address, as this is how invites end up + // reaching people. + OtherMails []string +} + +func (a AzureInvitedAccountFields) EqualTo(other intermediary_user.Fields) bool { + switch other := other.(type) { + case AzureInvitedAccountFields: + return a.Email == other.Email && + a.DisplayName == other.DisplayName && + a.MailNickname == other.MailNickname && + reflect.DeepEqual(a.OtherMails, other.OtherMails) + default: + return false + } +} + +// MayConsiderAsAlreadyRemoved always returns true for AzureInvitedAccountFields. This is because we never +// remove invited user records. We don't remove users at all -- we just disable them, and the way we do that +// is by disabling the user in their home directory (presumably via AzureCreatedAccountEngine). That prevents +// them from logging in to their account, blocking access to any tenant, home or invited. +func (a AzureInvitedAccountFields) MayConsiderAsAlreadyRemoved() bool { + return true +} + +var _ PropagationEngine[bool, AzureInvitedAccountIdentifier, AzureInvitedAccountFields] = &AzureInvitedAccountEngine{} + +type AzureInvitedAccountEngine struct { + homeTenantEmailDomain string + inviteTenantIdentityDomain string + userEmailDomainsToReplace []string + signInInstructionsLink string + + homeTenantClient *msgraphsdk.GraphServiceClient + inviteTenantClient *msgraphsdk.GraphServiceClient +} + +func (a *AzureInvitedAccountEngine) Init(_ context.Context, k *koanf.Koanf) error { + a.homeTenantEmailDomain = k.String("homeTenantEmailDomain") + a.inviteTenantIdentityDomain = k.String("inviteTenantIdentityDomain") + a.userEmailDomainsToReplace = k.Strings("userEmailDomainsToReplace") + a.signInInstructionsLink = k.String("signInInstructionsLink") + + homeCredentials, err := azidentity.NewWorkloadIdentityCredential(&azidentity.WorkloadIdentityCredentialOptions{ + ClientID: k.String("homeTenantClientID"), + TenantID: k.String("homeTenantID"), + TokenFilePath: k.String("homeTenantTokenFilePath"), + }) + if err != nil { + return err + } + + a.homeTenantClient, err = msgraphsdk.NewGraphServiceClientWithCredentials(homeCredentials, nil) + if err != nil { + return err + } + + inviteCredentials, err := azidentity.NewWorkloadIdentityCredential(&azidentity.WorkloadIdentityCredentialOptions{ + ClientID: k.String("inviteTenantClientID"), + TenantID: k.String("inviteTenantID"), + TokenFilePath: k.String("inviteTenantTokenFilePath"), + }) + if err != nil { + return err + } + + a.inviteTenantClient, err = msgraphsdk.NewGraphServiceClientWithCredentials(inviteCredentials, nil) + + return err +} + +func (a *AzureInvitedAccountEngine) LoadCurrentState(ctx context.Context, _ bool) ([]intermediary_user.IntermediaryUser[AzureInvitedAccountIdentifier, AzureInvitedAccountFields], error) { + currentState := make([]intermediary_user.IntermediaryUser[AzureInvitedAccountIdentifier, AzureInvitedAccountFields], 0) + + headers := abstractions.NewRequestHeaders() + configuration := &users.UsersRequestBuilderGetRequestConfiguration{ + Headers: headers, + QueryParameters: &users.UsersRequestBuilderGetQueryParameters{ + Select: []string{"userPrincipalName", "accountEnabled", "mail", "displayName", "mailNickname", "otherMails"}, + // Since this is a B2C tenant, we can't do fancy filter things to also check the #EXT# thingy in the + // user principal name. The creation type does a very good job of cutting down the response so we can + // safely check the suffix as we iterate. + Filter: utils.PointerTo("creationType eq 'Invitation'"), + Top: utils.PointerTo[int32](25), + }, + } + + result, err := a.inviteTenantClient.Users().Get(ctx, configuration) + if err != nil { + return nil, fmt.Errorf("failed to get invited users: %w", err) + } + + pageIterator, err := msgraphgocore.NewPageIterator[graphmodels.Userable](result, + a.inviteTenantClient.GetAdapter(), graphmodels.CreateUserCollectionResponseFromDiscriminatorValue) + if err != nil { + return nil, fmt.Errorf("failed to create page iterator for invited users: %w", err) + } + pageIterator.SetHeaders(headers) + + err = pageIterator.Iterate(ctx, func(pageItem graphmodels.Userable) bool { + if userPrincipalName := pageItem.GetUserPrincipalName(); userPrincipalName != nil && + strings.HasSuffix(*userPrincipalName, fmt.Sprintf("%s#EXT#@%s", a.homeTenantEmailDomain, a.inviteTenantIdentityDomain)) { + var fields AzureInvitedAccountFields + if mail := pageItem.GetMail(); mail != nil { + fields.Email = *mail + } + if displayName := pageItem.GetDisplayName(); displayName != nil { + fields.DisplayName = *displayName + } + if mailNickname := pageItem.GetMailNickname(); mailNickname != nil { + fields.MailNickname = *mailNickname + } + if otherMails := pageItem.GetOtherMails(); otherMails != nil { + fields.OtherMails = otherMails + } + currentState = append(currentState, intermediary_user.IntermediaryUser[AzureInvitedAccountIdentifier, AzureInvitedAccountFields]{ + Identifier: AzureInvitedAccountIdentifier{UserPrincipalName: *userPrincipalName}, + Fields: fields, + }) + } + return true + }) + return currentState, err +} + +func (a *AzureInvitedAccountEngine) GenerateDesiredState(ctx context.Context, roleAssignments map[uint]models.RoleAssignment) (map[uint]intermediary_user.IntermediaryUser[AzureInvitedAccountIdentifier, AzureInvitedAccountFields], error) { + desiredState := make(map[uint]intermediary_user.IntermediaryUser[AzureInvitedAccountIdentifier, AzureInvitedAccountFields]) + for sherlockUserID, roleAssignment := range roleAssignments { + // We explicitly aren't calling roleAssignment.IsActive() here. *This* propagator actually doesn't care about + // suspension! We don't have a notion of suspending an invited account, but we actually don't delete them either! + // We rely on the propagator that manages the home tenant identity to disable the user there, which suspends their + // ability to log in here too. + // We choose to still propagate the user here because we want to keep the user's name up to date in the invite tenant. + + email := utils.SubstituteSuffix(roleAssignment.User.Email, a.userEmailDomainsToReplace, a.homeTenantEmailDomain) + if !strings.HasSuffix(email, a.homeTenantEmailDomain) { + // We can short-circuit here, we know that the user doesn't have an email suffix we'd expect in the home tenant + // so we won't bother looking + continue + } + + usersResponse, err := a.homeTenantClient.Users().Get(ctx, &users.UsersRequestBuilderGetRequestConfiguration{ + QueryParameters: &users.UsersRequestBuilderGetQueryParameters{ + Select: []string{"userPrincipalName"}, + Filter: utils.PointerTo(fmt.Sprintf("userPrincipalName eq '%s'", email)), + Top: utils.PointerTo[int32](1), + }, + }) + if err != nil { + return nil, err + } else { + for _, user := range usersResponse.GetValue() { + if userPrincipalName := user.GetUserPrincipalName(); userPrincipalName != nil { + // The user principal name here is the UPN *from the home tenant*. For how we've got these + // tenants set up, it's the user's home email address. We'll need to format out our actual + // UPN as it'll look in the invite tenant. + upn := fmt.Sprintf("%s#EXT#@%s", strings.ReplaceAll(*userPrincipalName, "@", "_"), a.inviteTenantIdentityDomain) + desiredState[sherlockUserID] = intermediary_user.IntermediaryUser[AzureInvitedAccountIdentifier, AzureInvitedAccountFields]{ + Identifier: AzureInvitedAccountIdentifier{ + UserPrincipalName: upn, + }, + Fields: AzureInvitedAccountFields{ + Email: email, + DisplayName: roleAssignment.User.NameOrUsername(), + MailNickname: strings.Split(upn, "@")[0], + OtherMails: []string{roleAssignment.User.Email}, + }, + } + } + } + } + } + return desiredState, nil +} + +func (a *AzureInvitedAccountEngine) Add(ctx context.Context, _ bool, identifier AzureInvitedAccountIdentifier, fields AzureInvitedAccountFields) (string, error) { + // For phishing protection, we generate a random identifying string that we'll include in both logs and communications. + randomBytes := make([]byte, 8) + _, err := rand.Read(randomBytes) + if err != nil { + return "", fmt.Errorf("failed to generate random identifying string for inviting %s: %w", identifier.UserPrincipalName, err) + } + identifyingString := hex.EncodeToString(randomBytes) + + // First, we create an invite for the user. This creates a user record and in theory will send an email to the user + // with a redemption link. + invitation := graphmodels.NewInvitation() + invitation.SetInvitedUserEmailAddress(utils.PointerTo(fields.Email)) + invitation.SetInviteRedirectUrl(utils.PointerTo("https://portal.azure.com")) + invitation.SetInvitedUserType(utils.PointerTo("member")) + invitation.SetInvitedUserDisplayName(utils.PointerTo(fields.DisplayName)) + invitation.SetSendInvitationMessage(utils.PointerTo(true)) + invitationMessage := graphmodels.NewInvitedUserMessageInfo() + invitationMessage.SetCustomizedMessageBody(utils.PointerTo(a.invitationEmailMessageBody(fields, identifyingString))) + invitation.SetInvitedUserMessageInfo(invitationMessage) + invitationResponse, err := a.inviteTenantClient.Invitations().Post(ctx, invitation, nil) + if err != nil { + return "", fmt.Errorf("failed to invite %s: %w", identifier.UserPrincipalName, err) + } else if invitationResponse.GetInvitedUser() == nil || invitationResponse.GetInvitedUser().GetId() == nil { + return "", fmt.Errorf("failed to invite %s: no user ID returned", identifier.UserPrincipalName) + } else if invitationResponse.GetInviteRedeemUrl() == nil { + return "", fmt.Errorf("failed to invite %s: no redemption URL returned", identifier.UserPrincipalName) + } + + // Now we have to mutate the user that just got created to set their otherEmails field. + // In theory, this'll help the emailed invite go to the right place. + invitedUserEdits := graphmodels.NewUser() + invitedUserEdits.SetOtherMails(fields.OtherMails) + _, err = a.inviteTenantClient.Users().ByUserId(*invitationResponse.GetInvitedUser().GetId()).Patch(ctx, invitedUserEdits, nil) + if err != nil { + return "", fmt.Errorf("failed to set otherMails for newly invited user %s (message identifier `%s`): %w", identifier.UserPrincipalName, identifyingString, err) + } + + // In reality though, the email often doesn't seem to get sent. So we do the next best thing: we Slack the user + // the redemption URL directly. + slackID, _, _, err := slack.GetUser(ctx, fields.OtherMails[0]) + if err != nil { + return "", fmt.Errorf("invited %s (message identifier `%s`), but failed to get Slack ID for user so couldn't Slack them: %w", identifier.UserPrincipalName, identifyingString, err) + } + err = slack.SendMessageReturnError(ctx, slackID, a.invitationSlackMessageBody(fields, slackID, identifyingString, *invitationResponse.GetInviteRedeemUrl()), nil) + if err != nil { + return "", fmt.Errorf("invited %s (message identifier `%s`), but failed to Slack the user: %w", identifier.UserPrincipalName, identifyingString, err) + } + + return fmt.Sprintf("invited %s (invite email and Slack message sent with identifying string `%s`)", identifier.UserPrincipalName, identifyingString), nil +} + +func (a *AzureInvitedAccountEngine) invitationEmailMessageBody(fields AzureInvitedAccountFields, messageIdentifierString string) string { + return "This invitation has been generated by the DSP DevOps platform via Microsoft Graph API. " + + fmt.Sprintf("This invitation is meant to grant your %s Microsoft account access to %s. ", fields.Email, a.inviteTenantIdentityDomain) + + "You should reach out to DevOps to confirm the origin of this message before clicking the link. " + + fmt.Sprintf("They can match this message to a security event with this identifying string: %s. ", messageIdentifierString) +} + +func (a *AzureInvitedAccountEngine) invitationSlackMessageBody(fields AzureInvitedAccountFields, slackID string, messageIdentifierString string, redemptionURL string) string { + return fmt.Sprintf("Hi <@%s>, this is an automatic message from the DSP DevOps platform. ", slackID) + + fmt.Sprintf("You've been added to a role in Beehive that grants your %s Microsoft account access to %s. ", fields.Email, a.inviteTenantIdentityDomain) + + fmt.Sprintf("You'll need to click a redemption link and sign in with your %s Microsoft account to complete the process. ", fields.Email) + + fmt.Sprintf("*That link might've just been sent to you via email, but %s.* ", slack.LinkHelper(redemptionURL, "here it is too")) + + fmt.Sprintf("If you've never signed in to your %s Microsoft account before, you can follow the instructions %s. ", fields.Email, slack.LinkHelper(a.signInInstructionsLink, "here")) + + fmt.Sprintf("You can confirm that this isn't phishing by checking with DevOps about their security event with this identifying string: `%s`", messageIdentifierString) +} + +func (a *AzureInvitedAccountEngine) Update(ctx context.Context, _ bool, identifier AzureInvitedAccountIdentifier, oldFields AzureInvitedAccountFields, newFields AzureInvitedAccountFields) (string, error) { + // We can't update an invitation (an email has already been sent), but if the fields are different that means fields (and thus the user) + // already exist on the remote. + body := graphmodels.NewUser() + body.SetMail(utils.PointerTo(newFields.Email)) + body.SetDisplayName(utils.PointerTo(newFields.DisplayName)) + body.SetMailNickname(utils.PointerTo(newFields.MailNickname)) + body.SetOtherMails(newFields.OtherMails) + _, err := a.inviteTenantClient.Users().ByUserId(identifier.UserPrincipalName).Patch(ctx, body, nil) + if err != nil { + return "", fmt.Errorf("failed to update user %s (%s): %w", identifier.UserPrincipalName, a.describeDiff(oldFields, newFields), err) + } else { + return fmt.Sprintf("updated user %s (%s)", identifier.UserPrincipalName, a.describeDiff(oldFields, newFields)), nil + } +} + +func (a *AzureInvitedAccountEngine) describeDiff(oldFields AzureInvitedAccountFields, newFields AzureInvitedAccountFields) string { + if oldFields.Email != newFields.Email { + return fmt.Sprintf("update email from `%s` to `%s`", oldFields.Email, newFields.Email) + } else if oldFields.MailNickname != newFields.MailNickname { + return fmt.Sprintf("update mail nickname from `%s` to `%s`", oldFields.MailNickname, newFields.MailNickname) + } else if !reflect.DeepEqual(oldFields.OtherMails, newFields.OtherMails) { + return fmt.Sprintf("update other emails from `%v` to `%v`", oldFields.OtherMails, newFields.OtherMails) + } else if oldFields.DisplayName != newFields.DisplayName { + return fmt.Sprintf("update display name from `%s` to `%s`", oldFields.DisplayName, newFields.DisplayName) + } else { + return "no changes" + } +} + +func (a *AzureInvitedAccountEngine) Remove(_ context.Context, _ bool, _ AzureInvitedAccountIdentifier) (string, error) { + return "", fmt.Errorf("%T.Remove not implemented, %T.MayConsiderAsAlreadyRemoved should always return true", a, AzureInvitedAccountFields{}) +} diff --git a/sherlock/internal/role_propagation/propagation_engines/azure_invited_account_test.go b/sherlock/internal/role_propagation/propagation_engines/azure_invited_account_test.go new file mode 100644 index 000000000..066e95cdd --- /dev/null +++ b/sherlock/internal/role_propagation/propagation_engines/azure_invited_account_test.go @@ -0,0 +1,405 @@ +package propagation_engines + +import ( + "context" + "github.com/broadinstitute/sherlock/go-shared/pkg/utils" + "github.com/broadinstitute/sherlock/sherlock/internal/models" + "github.com/broadinstitute/sherlock/sherlock/internal/role_propagation/intermediary_user" + "github.com/stretchr/testify/assert" + "testing" +) + +func TestAzureInvitedAccountIdentifier_EqualTo(t *testing.T) { + type fields struct { + UserPrincipalName string + } + type args struct { + other intermediary_user.Identifier + } + tests := []struct { + name string + fields fields + args args + want bool + }{ + { + name: "equal", + fields: fields{ + UserPrincipalName: "foo", + }, + args: args{ + other: AzureInvitedAccountIdentifier{ + UserPrincipalName: "foo", + }, + }, + want: true, + }, + { + name: "not equal", + fields: fields{ + UserPrincipalName: "foo", + }, + args: args{ + other: AzureInvitedAccountIdentifier{ + UserPrincipalName: "bar", + }, + }, + want: false, + }, + { + name: "different type", + fields: fields{ + UserPrincipalName: "foo", + }, + args: args{ + other: GoogleWorkspaceGroupIdentifier{ + Email: "foo", + }, + }, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + a := AzureInvitedAccountIdentifier{ + UserPrincipalName: tt.fields.UserPrincipalName, + } + assert.Equalf(t, tt.want, a.EqualTo(tt.args.other), "EqualTo(%v)", tt.args.other) + }) + } +} + +func TestAzureInvitedAccountFields_EqualTo(t *testing.T) { + type fields struct { + Email string + DisplayName string + MailNickname string + OtherMails []string + } + type args struct { + other intermediary_user.Fields + } + tests := []struct { + name string + fields fields + args args + want bool + }{ + { + name: "equal", + fields: fields{ + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + args: args{ + other: AzureInvitedAccountFields{ + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + }, + want: true, + }, + { + name: "smtp mail not equal", + fields: fields{ + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + args: args{ + other: AzureInvitedAccountFields{ + Email: "bar", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + }, + want: false, + }, + { + name: "display name not equal", + fields: fields{ + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + args: args{ + other: AzureInvitedAccountFields{ + Email: "foo", + DisplayName: "foo", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + }, + want: false, + }, + { + name: "mail nickname not equal", + fields: fields{ + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + args: args{ + other: AzureInvitedAccountFields{ + Email: "foo", + DisplayName: "bar", + MailNickname: "foo", + OtherMails: []string{"qux"}, + }, + }, + want: false, + }, + { + name: "other mails same length but not equal", + fields: fields{ + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + args: args{ + other: AzureInvitedAccountFields{ + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"foo"}, + }, + }, + want: false, + }, + { + name: "other mails not equal", + fields: fields{ + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + args: args{ + other: AzureInvitedAccountFields{ + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"qux", "foo"}, + }, + }, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + a := AzureInvitedAccountFields{ + Email: tt.fields.Email, + DisplayName: tt.fields.DisplayName, + MailNickname: tt.fields.MailNickname, + OtherMails: tt.fields.OtherMails, + } + assert.Equalf(t, tt.want, a.EqualTo(tt.args.other), "EqualTo(%v)", tt.args.other) + }) + } +} + +func TestAzureInvitedAccountFields_MayConsiderAsAlreadyRemoved(t *testing.T) { + type fields struct { + Email string + DisplayName string + MailNickname string + OtherMails []string + } + tests := []struct { + name string + fields fields + want bool + }{ + { + name: "empty fields", + fields: fields{}, + want: true, + }, + { + name: "not empty fields", + fields: fields{ + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + want: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + a := AzureInvitedAccountFields{ + Email: tt.fields.Email, + DisplayName: tt.fields.DisplayName, + MailNickname: tt.fields.MailNickname, + OtherMails: tt.fields.OtherMails, + } + assert.Equalf(t, tt.want, a.MayConsiderAsAlreadyRemoved(), "MayConsiderAsAlreadyRemoved()") + }) + } +} + +// We can't easily test the actual cloud logic, but we can test that we short circuit correctly for +// emails that aren't in the target domain. +// +// See also utils.SubstituteSuffix +func TestAzureInvitedAccountEngine_GenerateDesiredState_emailShortCircuit(t *testing.T) { + engine := &AzureInvitedAccountEngine{ + homeTenantEmailDomain: "example.com", + userEmailDomainsToReplace: []string{"example.org"}, + } + desiredState, err := engine.GenerateDesiredState(context.Background(), map[uint]models.RoleAssignment{ + 1: { + User: &models.User{ + Email: "user@example.net", + }, + RoleAssignmentFields: models.RoleAssignmentFields{ + Suspended: utils.PointerTo(false), + }, + }, + }) + assert.NoError(t, err) + assert.Empty(t, desiredState) +} + +func TestAzureInvitedAccountEngine_invitationEmailMessageBody(t *testing.T) { + fields := AzureInvitedAccountFields{ + Email: "example@example.com", + } + engine := &AzureInvitedAccountEngine{ + inviteTenantIdentityDomain: "some-domain.com", + } + identifyingString := "some-identifying-string" + body := engine.invitationEmailMessageBody(fields, identifyingString) + assert.Contains(t, body, engine.inviteTenantIdentityDomain) + assert.Contains(t, body, identifyingString) + assert.Contains(t, body, fields.Email) +} + +func TestAzureInvitedAccountEngine_invitationSlackMessageBody(t *testing.T) { + fields := AzureInvitedAccountFields{ + Email: "example@example.com", + } + engine := &AzureInvitedAccountEngine{ + inviteTenantIdentityDomain: "some-domain.com", + } + slackID := "some-slack-id" + identifyingString := "some-identifying-string" + redemptionURL := "https://example.com" + body := engine.invitationSlackMessageBody(fields, slackID, identifyingString, redemptionURL) + assert.Contains(t, body, engine.inviteTenantIdentityDomain) + assert.Contains(t, body, identifyingString) + assert.Contains(t, body, fields.Email) + assert.Contains(t, body, slackID) +} + +func TestAzureInvitedAccountEngine_Remove_errors(t *testing.T) { + engine := &AzureInvitedAccountEngine{} + _, err := engine.Remove(context.Background(), true, AzureInvitedAccountIdentifier{}) + assert.Error(t, err) +} + +func TestAzureInvitedAccountEngine_describeDiff(t *testing.T) { + engine := &AzureInvitedAccountEngine{} + tests := []struct { + name string + oldFields AzureInvitedAccountFields + newFields AzureInvitedAccountFields + want string + }{ + { + name: "no changes", + oldFields: AzureInvitedAccountFields{ + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + newFields: AzureInvitedAccountFields{ + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + want: "no changes", + }, + { + name: "smtp mail", + oldFields: AzureInvitedAccountFields{ + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + newFields: AzureInvitedAccountFields{ + Email: "bar", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + want: "update email from `foo` to `bar`", + }, + { + name: "mail nickname", + oldFields: AzureInvitedAccountFields{ + Email: "foo", + DisplayName: "bar", + MailNickname: "foo", + OtherMails: []string{"qux"}, + }, + newFields: AzureInvitedAccountFields{ + Email: "foo", + DisplayName: "bar", + MailNickname: "bar", + OtherMails: []string{"qux"}, + }, + want: "update mail nickname from `foo` to `bar`", + }, + { + name: "other mails", + oldFields: AzureInvitedAccountFields{ + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"foo"}, + }, + newFields: AzureInvitedAccountFields{ + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"bar"}, + }, + want: "update other emails from `[foo]` to `[bar]`", + }, + { + name: "name change", + oldFields: AzureInvitedAccountFields{ + Email: "foo", + DisplayName: "foo", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + newFields: AzureInvitedAccountFields{ + Email: "foo", + DisplayName: "bar", + MailNickname: "baz", + OtherMails: []string{"qux"}, + }, + want: "update display name from `foo` to `bar`", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equalf(t, tt.want, engine.describeDiff(tt.oldFields, tt.newFields), "describeDiff(%v, %v)", tt.oldFields, tt.newFields) + }) + } +} diff --git a/sherlock/internal/role_propagation/propagation_engines/google_workspace_folder_owner.go b/sherlock/internal/role_propagation/propagation_engines/google_cloud_folder_role.go similarity index 54% rename from sherlock/internal/role_propagation/propagation_engines/google_workspace_folder_owner.go rename to sherlock/internal/role_propagation/propagation_engines/google_cloud_folder_role.go index 523bd315a..2f8fd1a9b 100644 --- a/sherlock/internal/role_propagation/propagation_engines/google_workspace_folder_owner.go +++ b/sherlock/internal/role_propagation/propagation_engines/google_cloud_folder_role.go @@ -14,49 +14,57 @@ import ( "strings" ) -// We hardcode the owner role here because it avoids a foot-gun with configuring this propagation engine. -// When this propagation engine is pointed at a folder, it wants to "own" all user-level bindings for this -// role. If we allowed configuration of this, it would be easier to accidentally cause problems by -// pointing this engine at a role that some other system also wanted to manage. -const ownerRole = "roles/owner" +const GoogleCloudOwnerRole = "roles/owner" -type GoogleWorkspaceFolderOwnerIdentifier struct { +type GoogleCloudFolderRoleIdentifier struct { Email string `koanf:"email"` } -func (g GoogleWorkspaceFolderOwnerIdentifier) EqualTo(other intermediary_user.Identifier) bool { +func (g GoogleCloudFolderRoleIdentifier) EqualTo(other intermediary_user.Identifier) bool { switch other := other.(type) { - case GoogleWorkspaceFolderOwnerIdentifier: + case GoogleCloudFolderRoleIdentifier: return g.Email == other.Email default: return false } } -type GoogleWorkspaceFolderOwnerFields struct{} +type GoogleCloudFolderRoleFields struct{} -func (g GoogleWorkspaceFolderOwnerFields) EqualTo(other intermediary_user.Fields) bool { +func (g GoogleCloudFolderRoleFields) EqualTo(other intermediary_user.Fields) bool { switch other.(type) { - case GoogleWorkspaceFolderOwnerFields: + case GoogleCloudFolderRoleFields: return true default: return false } } -var _ PropagationEngine[string, GoogleWorkspaceFolderOwnerIdentifier, GoogleWorkspaceFolderOwnerFields] = &GoogleWorkspaceFolderOwnerEngine{} +var _ PropagationEngine[string, GoogleCloudFolderRoleIdentifier, GoogleCloudFolderRoleFields] = &GoogleCloudFolderRoleEngine{} -// GoogleWorkspaceFolderOwnerEngine can grant roles outside the workspace domain. The workspace domain and +// GoogleCloudFolderRoleEngine can grant roles outside the workspace domain. The workspace domain and // admin directory access is used to determine what users exist, not to restrict what those users can be // granted access to. -type GoogleWorkspaceFolderOwnerEngine struct { +type GoogleCloudFolderRoleEngine struct { + // Role is defined in Sherlock's source code where this engine is instantiated, not in configuration. + // This is because it helps avoid a foot-gun with accidentally misconfiguring the engine. When this + // engine is pointed at a folder, it wants to "own" all user-level bindings for this role. It will remove + // user-level bindings that don't correlate to Sherlock role assignments. If we allowed easy configuration + // of the role, it would be easy to accidentally cause problems by pointing this engine at a role that some + // other system also wanted to manage or relied on. + Role string + workspaceDomain string userEmailSuffixesToReplace []string adminService *admin.Service foldersClient *resourcemanager.FoldersClient } -func (g *GoogleWorkspaceFolderOwnerEngine) Init(ctx context.Context, k *koanf.Koanf) error { +func (g *GoogleCloudFolderRoleEngine) Init(ctx context.Context, k *koanf.Koanf) error { + if g.Role == "" { + return fmt.Errorf("role must be set") + } + g.workspaceDomain = k.String("workspaceDomain") g.userEmailSuffixesToReplace = k.Strings("userEmailSuffixesToReplace") var err error @@ -68,21 +76,21 @@ func (g *GoogleWorkspaceFolderOwnerEngine) Init(ctx context.Context, k *koanf.Ko return err } -func (g *GoogleWorkspaceFolderOwnerEngine) LoadCurrentState(ctx context.Context, grant string) ([]intermediary_user.IntermediaryUser[GoogleWorkspaceFolderOwnerIdentifier, GoogleWorkspaceFolderOwnerFields], error) { +func (g *GoogleCloudFolderRoleEngine) LoadCurrentState(ctx context.Context, grant string) ([]intermediary_user.IntermediaryUser[GoogleCloudFolderRoleIdentifier, GoogleCloudFolderRoleFields], error) { policy, err := g.foldersClient.GetIamPolicy(ctx, &iampb.GetIamPolicyRequest{ Resource: grant, }) if err != nil { return nil, err } else { - currentState := make([]intermediary_user.IntermediaryUser[GoogleWorkspaceFolderOwnerIdentifier, GoogleWorkspaceFolderOwnerFields], 0) + currentState := make([]intermediary_user.IntermediaryUser[GoogleCloudFolderRoleIdentifier, GoogleCloudFolderRoleFields], 0) for _, binding := range policy.Bindings { - if binding.Role == ownerRole { + if binding.Role == GoogleCloudOwnerRole { for _, member := range binding.Members { if strings.HasPrefix(member, "user:") { - currentState = append(currentState, intermediary_user.IntermediaryUser[GoogleWorkspaceFolderOwnerIdentifier, GoogleWorkspaceFolderOwnerFields]{ - Identifier: GoogleWorkspaceFolderOwnerIdentifier{Email: strings.TrimPrefix(member, "user:")}, - Fields: GoogleWorkspaceFolderOwnerFields{}, + currentState = append(currentState, intermediary_user.IntermediaryUser[GoogleCloudFolderRoleIdentifier, GoogleCloudFolderRoleFields]{ + Identifier: GoogleCloudFolderRoleIdentifier{Email: strings.TrimPrefix(member, "user:")}, + Fields: GoogleCloudFolderRoleFields{}, }) } } @@ -92,8 +100,8 @@ func (g *GoogleWorkspaceFolderOwnerEngine) LoadCurrentState(ctx context.Context, } } -func (g *GoogleWorkspaceFolderOwnerEngine) GenerateDesiredState(ctx context.Context, roleAssignments map[uint]models.RoleAssignment) (map[uint]intermediary_user.IntermediaryUser[GoogleWorkspaceFolderOwnerIdentifier, GoogleWorkspaceFolderOwnerFields], error) { - desiredState := make(map[uint]intermediary_user.IntermediaryUser[GoogleWorkspaceFolderOwnerIdentifier, GoogleWorkspaceFolderOwnerFields]) +func (g *GoogleCloudFolderRoleEngine) GenerateDesiredState(ctx context.Context, roleAssignments map[uint]models.RoleAssignment) (map[uint]intermediary_user.IntermediaryUser[GoogleCloudFolderRoleIdentifier, GoogleCloudFolderRoleFields], error) { + desiredState := make(map[uint]intermediary_user.IntermediaryUser[GoogleCloudFolderRoleIdentifier, GoogleCloudFolderRoleFields]) for id, roleAssignment := range roleAssignments { if !roleAssignment.IsActive() { // There's no concept of a suspended group member, so we just exclude any non-active users @@ -114,9 +122,9 @@ func (g *GoogleWorkspaceFolderOwnerEngine) GenerateDesiredState(ctx context.Cont Pages(ctx, func(workspaceUsers *admin.Users) error { for _, workspaceUser := range workspaceUsers.Users { if workspaceUser.PrimaryEmail == email { - desiredState[id] = intermediary_user.IntermediaryUser[GoogleWorkspaceFolderOwnerIdentifier, GoogleWorkspaceFolderOwnerFields]{ - Identifier: GoogleWorkspaceFolderOwnerIdentifier{Email: email}, - Fields: GoogleWorkspaceFolderOwnerFields{}, + desiredState[id] = intermediary_user.IntermediaryUser[GoogleCloudFolderRoleIdentifier, GoogleCloudFolderRoleFields]{ + Identifier: GoogleCloudFolderRoleIdentifier{Email: email}, + Fields: GoogleCloudFolderRoleFields{}, } } } @@ -129,7 +137,7 @@ func (g *GoogleWorkspaceFolderOwnerEngine) GenerateDesiredState(ctx context.Cont return desiredState, nil } -func (g *GoogleWorkspaceFolderOwnerEngine) Add(ctx context.Context, grant string, identifier GoogleWorkspaceFolderOwnerIdentifier, fields GoogleWorkspaceFolderOwnerFields) (string, error) { +func (g *GoogleCloudFolderRoleEngine) Add(ctx context.Context, grant string, identifier GoogleCloudFolderRoleIdentifier, fields GoogleCloudFolderRoleFields) (string, error) { policy, err := g.foldersClient.GetIamPolicy(ctx, &iampb.GetIamPolicyRequest{ Resource: grant, }) @@ -138,7 +146,7 @@ func (g *GoogleWorkspaceFolderOwnerEngine) Add(ctx context.Context, grant string } for _, binding := range policy.Bindings { - if binding.Role == ownerRole && binding.Condition == nil { + if binding.Role == GoogleCloudOwnerRole && binding.Condition == nil { binding.Members = append(binding.Members, "user:"+identifier.Email) } } @@ -148,17 +156,17 @@ func (g *GoogleWorkspaceFolderOwnerEngine) Add(ctx context.Context, grant string Policy: policy, }) if err != nil { - return "", fmt.Errorf("failed to grant %s %s on %s: %w", identifier.Email, ownerRole, grant, err) + return "", fmt.Errorf("failed to grant %s %s on %s: %w", identifier.Email, GoogleCloudOwnerRole, grant, err) } else { - return fmt.Sprintf("granted %s %s on %s", identifier.Email, ownerRole, grant), nil + return fmt.Sprintf("granted %s %s on %s", identifier.Email, GoogleCloudOwnerRole, grant), nil } } -func (g *GoogleWorkspaceFolderOwnerEngine) Update(_ context.Context, _ string, _ GoogleWorkspaceFolderOwnerIdentifier, _ GoogleWorkspaceFolderOwnerFields, _ GoogleWorkspaceFolderOwnerFields) (string, error) { - return "", fmt.Errorf("%T.Update not implemented; %T.EqualTo should always equal true", g, GoogleWorkspaceFolderOwnerFields{}) +func (g *GoogleCloudFolderRoleEngine) Update(_ context.Context, _ string, _ GoogleCloudFolderRoleIdentifier, _ GoogleCloudFolderRoleFields, _ GoogleCloudFolderRoleFields) (string, error) { + return "", fmt.Errorf("%T.Update not implemented; %T.EqualTo should always equal true", g, GoogleCloudFolderRoleFields{}) } -func (g *GoogleWorkspaceFolderOwnerEngine) Remove(ctx context.Context, grant string, identifier GoogleWorkspaceFolderOwnerIdentifier) (string, error) { +func (g *GoogleCloudFolderRoleEngine) Remove(ctx context.Context, grant string, identifier GoogleCloudFolderRoleIdentifier) (string, error) { policy, err := g.foldersClient.GetIamPolicy(ctx, &iampb.GetIamPolicyRequest{ Resource: grant, }) @@ -167,7 +175,7 @@ func (g *GoogleWorkspaceFolderOwnerEngine) Remove(ctx context.Context, grant str } for _, binding := range policy.Bindings { - if binding.Role == ownerRole && binding.Condition == nil { + if binding.Role == GoogleCloudOwnerRole && binding.Condition == nil { members := make([]string, 0, len(binding.Members)-1) for _, member := range binding.Members { if member != "user:"+identifier.Email { @@ -183,8 +191,8 @@ func (g *GoogleWorkspaceFolderOwnerEngine) Remove(ctx context.Context, grant str Policy: policy, }) if err != nil { - return "", fmt.Errorf("failed to revoke %s's %s on %s: %w", identifier.Email, ownerRole, grant, err) + return "", fmt.Errorf("failed to revoke %s's %s on %s: %w", identifier.Email, GoogleCloudOwnerRole, grant, err) } else { - return fmt.Sprintf("revoke %s's %s on %s", identifier.Email, ownerRole, grant), nil + return fmt.Sprintf("revoked %s's %s on %s", identifier.Email, GoogleCloudOwnerRole, grant), nil } } diff --git a/sherlock/internal/role_propagation/propagation_engines/google_workspace_folder_owner_test.go b/sherlock/internal/role_propagation/propagation_engines/google_cloud_folder_role_test.go similarity index 74% rename from sherlock/internal/role_propagation/propagation_engines/google_workspace_folder_owner_test.go rename to sherlock/internal/role_propagation/propagation_engines/google_cloud_folder_role_test.go index cabb095b8..a446be752 100644 --- a/sherlock/internal/role_propagation/propagation_engines/google_workspace_folder_owner_test.go +++ b/sherlock/internal/role_propagation/propagation_engines/google_cloud_folder_role_test.go @@ -10,7 +10,7 @@ import ( "time" ) -func TestGoogleWorkspaceFolderOwnerIdentifier_EqualTo(t *testing.T) { +func TestGoogleCloudFolderRoleIdentifier_EqualTo(t *testing.T) { type fields struct { Email string } @@ -29,7 +29,7 @@ func TestGoogleWorkspaceFolderOwnerIdentifier_EqualTo(t *testing.T) { Email: "foo", }, args: args{ - other: GoogleWorkspaceFolderOwnerIdentifier{ + other: GoogleCloudFolderRoleIdentifier{ Email: "foo", }, }, @@ -41,7 +41,7 @@ func TestGoogleWorkspaceFolderOwnerIdentifier_EqualTo(t *testing.T) { Email: "foo", }, args: args{ - other: GoogleWorkspaceFolderOwnerIdentifier{ + other: GoogleCloudFolderRoleIdentifier{ Email: "bar", }, }, @@ -62,7 +62,7 @@ func TestGoogleWorkspaceFolderOwnerIdentifier_EqualTo(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - a := GoogleWorkspaceFolderOwnerIdentifier{ + a := GoogleCloudFolderRoleIdentifier{ Email: tt.fields.Email, } assert.Equalf(t, tt.want, a.EqualTo(tt.args.other), "EqualTo(%v)", tt.args.other) @@ -70,7 +70,7 @@ func TestGoogleWorkspaceFolderOwnerIdentifier_EqualTo(t *testing.T) { } } -func TestGoogleWorkspaceFolderOwnerFields_EqualTo(t *testing.T) { +func TestGoogleCloudFolderRoleFields_EqualTo(t *testing.T) { type args struct { other intermediary_user.Fields } @@ -82,7 +82,7 @@ func TestGoogleWorkspaceFolderOwnerFields_EqualTo(t *testing.T) { { name: "same type", args: args{ - other: GoogleWorkspaceFolderOwnerFields{}, + other: GoogleCloudFolderRoleFields{}, }, want: true, }, @@ -96,7 +96,7 @@ func TestGoogleWorkspaceFolderOwnerFields_EqualTo(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - f := GoogleWorkspaceFolderOwnerFields{} + f := GoogleCloudFolderRoleFields{} assert.Equalf(t, tt.want, f.EqualTo(tt.args.other), "EqualTo(%v)", tt.args.other) }) } @@ -104,8 +104,10 @@ func TestGoogleWorkspaceFolderOwnerFields_EqualTo(t *testing.T) { // We can't easily test the actual cloud logic, but we can test that we short circuit correctly for // non-active role assignments. -func TestGoogleWorkspaceFolderOwnerEngine_GenerateDesiredState_isActiveShortCircuit(t *testing.T) { - engine := &GoogleWorkspaceFolderOwnerEngine{} +func TestGoogleCloudFolderRoleEngine_GenerateDesiredState_isActiveShortCircuit(t *testing.T) { + engine := &GoogleCloudFolderRoleEngine{ + Role: GoogleCloudOwnerRole, + } desiredState, err := engine.GenerateDesiredState(context.Background(), map[uint]models.RoleAssignment{ 1: { RoleAssignmentFields: models.RoleAssignmentFields{ @@ -127,8 +129,9 @@ func TestGoogleWorkspaceFolderOwnerEngine_GenerateDesiredState_isActiveShortCirc // emails that aren't in the target domain. // // See also utils.SubstituteSuffix -func TestGoogleWorkspaceFolderOwnerEngine_GenerateDesiredState_emailShortCircuit(t *testing.T) { - engine := &GoogleWorkspaceFolderOwnerEngine{ +func TestGoogleCloudFolderRoleEngine_GenerateDesiredState_emailShortCircuit(t *testing.T) { + engine := &GoogleCloudFolderRoleEngine{ + Role: GoogleCloudOwnerRole, workspaceDomain: "example.com", userEmailSuffixesToReplace: []string{"@example.org"}, } @@ -146,8 +149,8 @@ func TestGoogleWorkspaceFolderOwnerEngine_GenerateDesiredState_emailShortCircuit assert.Empty(t, desiredState) } -func TestGoogleWorkspaceFolderOwnerEngine_Update_errors(t *testing.T) { - engine := &GoogleWorkspaceFolderOwnerEngine{} - _, err := engine.Update(context.Background(), "", GoogleWorkspaceFolderOwnerIdentifier{}, GoogleWorkspaceFolderOwnerFields{}, GoogleWorkspaceFolderOwnerFields{}) +func TestGoogleCloudFolderRoleEngine_Update_errors(t *testing.T) { + engine := &GoogleCloudFolderRoleEngine{} + _, err := engine.Update(context.Background(), "", GoogleCloudFolderRoleIdentifier{}, GoogleCloudFolderRoleFields{}, GoogleCloudFolderRoleFields{}) assert.Error(t, err) } diff --git a/sherlock/internal/role_propagation/propagator.go b/sherlock/internal/role_propagation/propagator.go index 7a3ddd6ef..16228740f 100644 --- a/sherlock/internal/role_propagation/propagator.go +++ b/sherlock/internal/role_propagation/propagator.go @@ -72,6 +72,10 @@ type propagatorImpl[ // rolePropagation.propagators..enabled. _enable bool + // _dryRun stores whether this propagator is set to dry-run in the _config, from + // rolePropagation.propagators..dryRun. + _dryRun bool + // _timeout is the amount of time the propagator will be allowed to run during Propagate. It's read from the // configuration at rolePropagation.propagators..timeout, with a default read from the config at // rolePropagation.defaultTimeout. @@ -84,6 +88,16 @@ type propagatorImpl[ // deactivated *everyone* except its equivalent of this list. Sherlock seeks to prevent such issues with the // power of "writing tests" but we keep the guardrails that have worked in the past.) _toleratedUsers []Identifier + + // _ignoredUsers is a set of users that we won't Add, Remove, or Update on the remote for any reason. This + // can be helpful for cases where a particular user's account is problematic and we want to avoid having + // Sherlock interact with it. + // + // An example here is that there's technically a maximum number of Google Groups someone can be in. Because + // of how Terra works, some folks actually hit this limit. Sometimes those folks may deprioritize Sherlock's + // groups -- meaning they would want Sherlock to always skip them for a particular grant type -- so they can + // have as many groups as possible for other purposes. + _ignoredUsers []Identifier } func (p *propagatorImpl[Grant, Identifier, Fields]) LogPrefix() string { diff --git a/sherlock/internal/role_propagation/propagator_calculate_alignment_operations.go b/sherlock/internal/role_propagation/propagator_calculate_alignment_operations.go index d39916318..b1e45fc2e 100644 --- a/sherlock/internal/role_propagation/propagator_calculate_alignment_operations.go +++ b/sherlock/internal/role_propagation/propagator_calculate_alignment_operations.go @@ -2,6 +2,7 @@ package role_propagation import ( "context" + "fmt" "github.com/broadinstitute/sherlock/sherlock/internal/role_propagation/intermediary_user" ) @@ -29,6 +30,14 @@ currentlyGrantedUserLoop: for _, unsafeCurrentlyGrantedUser := range currentState { currentlyGrantedUser := unsafeCurrentlyGrantedUser + // Let's first check if we're set to ignore the currently granted user completely. + for _, ignoredUser := range p._ignoredUsers { + if currentlyGrantedUser.Identifier.EqualTo(ignoredUser) { + // Match! Let's move on to the next currently granted user, we'll leave this one alone. + continue currentlyGrantedUserLoop + } + } + // Seek match from copyOfDesiredState for unsafeDesiredSherlockUserID, unsafeDesiredUser := range copyOfDesiredState { desiredSherlockUserID := unsafeDesiredSherlockUserID @@ -40,7 +49,7 @@ currentlyGrantedUserLoop: // added at the end. if !currentlyGrantedUser.Fields.EqualTo(desiredUser.Fields) { alignmentOperations = append(alignmentOperations, func() (string, error) { - return p.engine.Update(ctx, grant, desiredUser.Identifier, currentlyGrantedUser.Fields, desiredUser.Fields) + return p.updateOperation()(ctx, grant, desiredUser.Identifier, currentlyGrantedUser.Fields, desiredUser.Fields) }) } @@ -58,18 +67,68 @@ currentlyGrantedUserLoop: } } - // No match in desiredState or toleratedUsers! Remove the grant from the currently granted user. + // No match in desiredState or toleratedUsers! Let's check if we may consider the user as being already + // effectively removed. We need to check two things: + // 1. If the fields are the *type* that may still be present while the user is effectively removed. + // 2. If those fields indicate *this user* can be considered as effectively already removed. + // + // (We can't type-assert on a type parameter, so we convert to the interface supertype and then assert on that.) + if mayBePresentWhileRemovedFields, ok := intermediary_user.Fields(currentlyGrantedUser.Fields).(intermediary_user.MayBePresentWhileRemovedFields); ok && + mayBePresentWhileRemovedFields.MayConsiderAsAlreadyRemoved() { + continue currentlyGrantedUserLoop + } + + // No match in desiredState or toleratedUsers, and our check if we could treat the user as being effectively + // already removed didn't pass. We remove the grant from the currently granted user. alignmentOperations = append(alignmentOperations, func() (string, error) { - return p.engine.Remove(ctx, grant, currentlyGrantedUser.Identifier) + return p.removeOperation()(ctx, grant, currentlyGrantedUser.Identifier) }) } // If there are any desired users left, add them. +desiredUserLoop: for _, unsafeDesiredUser := range copyOfDesiredState { desiredUser := unsafeDesiredUser + + // Let's first check if we're set to ignore the desired user completely. + for _, ignoredUser := range p._ignoredUsers { + if desiredUser.Identifier.EqualTo(ignoredUser) { + // Match! Let's move on to the next desired user, we'll leave this one alone. + continue desiredUserLoop + } + } + + // If we get here, we want to add the user. alignmentOperations = append(alignmentOperations, func() (string, error) { - return p.engine.Add(ctx, grant, desiredUser.Identifier, desiredUser.Fields) + return p.addOperation()(ctx, grant, desiredUser.Identifier, desiredUser.Fields) }) } return alignmentOperations } + +func (p *propagatorImpl[Grant, Identifier, Fields]) addOperation() func(ctx context.Context, grant Grant, identifier Identifier, fields Fields) (string, error) { + if p._dryRun { + return func(ctx context.Context, grant Grant, identifier Identifier, fields Fields) (string, error) { + return fmt.Sprintf("DRY-RUN: called for adding of %+v with fields %+v", identifier, fields), nil + } + } + return p.engine.Add +} + +func (p *propagatorImpl[Grant, Identifier, Fields]) updateOperation() func(ctx context.Context, grant Grant, identifier Identifier, oldFields Fields, newFields Fields) (string, error) { + if p._dryRun { + return func(ctx context.Context, grant Grant, identifier Identifier, oldFields Fields, newFields Fields) (string, error) { + return fmt.Sprintf("DRY-RUN: called for update of %+v from %+v to %+v", identifier, oldFields, newFields), nil + } + } + return p.engine.Update +} + +func (p *propagatorImpl[Grant, Identifier, Fields]) removeOperation() func(ctx context.Context, grant Grant, identifier Identifier) (string, error) { + if p._dryRun { + return func(ctx context.Context, grant Grant, identifier Identifier) (string, error) { + return fmt.Sprintf("DRY-RUN: called for removal of %+v", identifier), nil + } + } + return p.engine.Remove +} diff --git a/sherlock/internal/role_propagation/propagator_calculate_alignment_operations_test.go b/sherlock/internal/role_propagation/propagator_calculate_alignment_operations_test.go index 228d6d483..f21fb0d74 100644 --- a/sherlock/internal/role_propagation/propagator_calculate_alignment_operations_test.go +++ b/sherlock/internal/role_propagation/propagator_calculate_alignment_operations_test.go @@ -2,7 +2,9 @@ package role_propagation import ( "context" + "fmt" "github.com/broadinstitute/sherlock/sherlock/internal/role_propagation/intermediary_user" + "github.com/broadinstitute/sherlock/sherlock/internal/role_propagation/intermediary_user/intermediary_user_mocks" "github.com/broadinstitute/sherlock/sherlock/internal/role_propagation/propagation_engines/propagation_engines_mocks" "github.com/stretchr/testify/assert" "testing" @@ -73,6 +75,26 @@ func Test_propagatorImpl_calculateAlignmentOperations(t *testing.T) { desiredState[6] = testIntermediaryUser{Identifier: identifier6, Fields: fields6} engine.EXPECT().Add(ctx, grant, identifier6, fields6).Return("", nil).Once() + // User in both current and desired, fields differ, but in ignored, expect nothing to be called + identifier7 := testIdentifier{"user7"} + fields7 := testFields{"field7"} + fields7Desired := testFields{"field7Desired"} + currentState = append(currentState, testIntermediaryUser{Identifier: identifier7, Fields: fields7}) + desiredState[7] = testIntermediaryUser{Identifier: identifier7, Fields: fields7Desired} + p._ignoredUsers = append(p._ignoredUsers, identifier7) + + // User in current, not in desired, but in ignored, expect nothing to be called + identifier8 := testIdentifier{"user8"} + fields8 := testFields{"field8"} + currentState = append(currentState, testIntermediaryUser{Identifier: identifier8, Fields: fields8}) + p._ignoredUsers = append(p._ignoredUsers, identifier8) + + // User in desired, not in current, but in ignored, expect nothing to be called + identifier9 := testIdentifier{"user9"} + fields9 := testFields{"field9"} + desiredState[9] = testIntermediaryUser{Identifier: identifier9, Fields: fields9} + p._ignoredUsers = append(p._ignoredUsers, identifier9) + currentStateLen := len(currentState) desiredStateLen := len(desiredState) @@ -90,6 +112,95 @@ func Test_propagatorImpl_calculateAlignmentOperations(t *testing.T) { engine.AssertExpectations(t) } +func Test_propagatorImpl_calculateAlignmentOperations_dryRun(t *testing.T) { + engine := propagation_engines_mocks.NewMockPropagationEngine[string, testIdentifier, testFields](t) + p := &propagatorImpl[string, testIdentifier, testFields]{ + engine: engine, + _dryRun: true, + } + + ctx := context.Background() + grant := "grant" + currentState := make([]testIntermediaryUser, 0) + desiredState := make(map[uint]testIntermediaryUser) + results := make([]string, 0) + + // User in both current and desired, no changes, expect nothing to be called + identifier1 := testIdentifier{"user1"} + fields1 := testFields{"field1"} + currentState = append(currentState, testIntermediaryUser{Identifier: identifier1, Fields: fields1}) + desiredState[1] = testIntermediaryUser{Identifier: identifier1, Fields: fields1} + + // User in both current and desired, fields differ, expect Update to be called + identifier2 := testIdentifier{"user2"} + fields2 := testFields{"field2"} + fields2Desired := testFields{"field2Desired"} + currentState = append(currentState, testIntermediaryUser{Identifier: identifier2, Fields: fields2}) + desiredState[2] = testIntermediaryUser{Identifier: identifier2, Fields: fields2Desired} + results = append(results, fmt.Sprintf("DRY-RUN: called for update of %+v from %+v to %+v", identifier2, fields2, fields2Desired)) + + // User in current, not in desired, expect Remove to be called + identifier3 := testIdentifier{"user3"} + fields3 := testFields{"field3"} + currentState = append(currentState, testIntermediaryUser{Identifier: identifier3, Fields: fields3}) + results = append(results, fmt.Sprintf("DRY-RUN: called for removal of %+v", identifier3)) + + // User in current, not in desired, but in tolerated, expect nothing to be called + identifier4 := testIdentifier{"user4"} + fields4 := testFields{"field4"} + currentState = append(currentState, testIntermediaryUser{Identifier: identifier4, Fields: fields4}) + p._toleratedUsers = append(p._toleratedUsers, identifier4) + + // User in tolerated, not in current or desired, expect nothing to be called + identifier5 := testIdentifier{"user5"} + p._toleratedUsers = append(p._toleratedUsers, identifier5) + + // User in desired, not in current, expect Add to be called + identifier6 := testIdentifier{"user6"} + fields6 := testFields{"field6"} + desiredState[6] = testIntermediaryUser{Identifier: identifier6, Fields: fields6} + results = append(results, fmt.Sprintf("DRY-RUN: called for adding of %+v with fields %+v", identifier6, fields6)) + + // User in both current and desired, fields differ, but in ignored, expect nothing to be called + identifier7 := testIdentifier{"user7"} + fields7 := testFields{"field7"} + fields7Desired := testFields{"field7Desired"} + currentState = append(currentState, testIntermediaryUser{Identifier: identifier7, Fields: fields7}) + desiredState[7] = testIntermediaryUser{Identifier: identifier7, Fields: fields7Desired} + p._ignoredUsers = append(p._ignoredUsers, identifier7) + + // User in current, not in desired, but in ignored, expect nothing to be called + identifier8 := testIdentifier{"user8"} + fields8 := testFields{"field8"} + currentState = append(currentState, testIntermediaryUser{Identifier: identifier8, Fields: fields8}) + p._ignoredUsers = append(p._ignoredUsers, identifier8) + + // User in desired, not in current, but in ignored, expect nothing to be called + identifier9 := testIdentifier{"user9"} + fields9 := testFields{"field9"} + desiredState[9] = testIntermediaryUser{Identifier: identifier9, Fields: fields9} + p._ignoredUsers = append(p._ignoredUsers, identifier9) + + currentStateLen := len(currentState) + desiredStateLen := len(desiredState) + + alignmentOperations := p.calculateAlignmentOperations(ctx, grant, currentState, desiredState) + actualResults := make([]string, 0) + for _, alignmentOperation := range alignmentOperations { + result, err := alignmentOperation() + assert.NoError(t, err) + actualResults = append(actualResults, result) + } + + assert.ElementsMatch(t, results, actualResults) + + // Make sure that the above operations didn't mutate the state parameters + assert.Len(t, currentState, currentStateLen) + assert.Len(t, desiredState, desiredStateLen) + + engine.AssertExpectations(t) +} + func Test_propagatorImpl_calculateAlignmentOperations_empty(t *testing.T) { engine := propagation_engines_mocks.NewMockPropagationEngine[string, testIdentifier, testFields](t) p := &propagatorImpl[string, testIdentifier, testFields]{ @@ -105,3 +216,41 @@ func Test_propagatorImpl_calculateAlignmentOperations_empty(t *testing.T) { engine.AssertExpectations(t) } + +type testMayConsiderAsAlreadyRemovedIntermediaryUser = intermediary_user.IntermediaryUser[testIdentifier, *intermediary_user_mocks.MockMayBePresentWhileRemovedFields] + +func Test_propagatorImpl_calculateAlignmentOperations_mayConsiderAsAlreadyRemoved(t *testing.T) { + engine := propagation_engines_mocks.NewMockPropagationEngine[string, testIdentifier, *intermediary_user_mocks.MockMayBePresentWhileRemovedFields](t) + p := &propagatorImpl[string, testIdentifier, *intermediary_user_mocks.MockMayBePresentWhileRemovedFields]{ + engine: engine, + } + + ctx := context.Background() + grant := "grant" + currentState := make([]testMayConsiderAsAlreadyRemovedIntermediaryUser, 0) + desiredState := make(map[uint]testMayConsiderAsAlreadyRemovedIntermediaryUser) + + // A user that should be removed + identifier1 := testIdentifier{"user1"} + fields1 := intermediary_user_mocks.NewMockMayBePresentWhileRemovedFields(t) + fields1.EXPECT().MayConsiderAsAlreadyRemoved().Return(false).Once() + currentState = append(currentState, testMayConsiderAsAlreadyRemovedIntermediaryUser{Identifier: identifier1, Fields: fields1}) + engine.EXPECT().Remove(ctx, grant, identifier1).Return("", nil).Once() + + // A user that should be considered as already removed -- expect no Remove to be called + identifier2 := testIdentifier{"user2"} + fields2 := intermediary_user_mocks.NewMockMayBePresentWhileRemovedFields(t) + fields2.EXPECT().MayConsiderAsAlreadyRemoved().Return(true).Once() + currentState = append(currentState, testMayConsiderAsAlreadyRemovedIntermediaryUser{Identifier: identifier2, Fields: fields2}) + + alignmentOperations := p.calculateAlignmentOperations(ctx, grant, currentState, desiredState) + for _, alignmentOperation := range alignmentOperations { + // These operations are pure mocks so there's no point to testing their return values, + // we're just calling the outputs so the mock observes the calls + _, _ = alignmentOperation() + } + + fields1.AssertExpectations(t) + fields2.AssertExpectations(t) + engine.AssertExpectations(t) +} diff --git a/sherlock/internal/role_propagation/propagator_init.go b/sherlock/internal/role_propagation/propagator_init.go index 9478635c6..2e8bcde97 100644 --- a/sherlock/internal/role_propagation/propagator_init.go +++ b/sherlock/internal/role_propagation/propagator_init.go @@ -16,6 +16,8 @@ func (p *propagatorImpl[Grant, Identifier, Fields]) Init(ctx context.Context) er return nil } + p._dryRun = p._config.Bool("dryRun") + p.initTimeout() if err := p.engine.Init(ctx, p._config); err != nil { @@ -26,6 +28,10 @@ func (p *propagatorImpl[Grant, Identifier, Fields]) Init(ctx context.Context) er return err } + if err := p.initIgnoredUsers(ctx); err != nil { + return err + } + return nil } @@ -60,3 +66,18 @@ func (p *propagatorImpl[Grant, Identifier, Fields]) initToleratedUsers(ctx conte return nil } + +func (p *propagatorImpl[Grant, Identifier, Fields]) initIgnoredUsers(ctx context.Context) error { + if ignoredUsers := p._config.Slices("ignoredUsers"); len(ignoredUsers) > 0 { + p._ignoredUsers = make([]Identifier, 0, len(ignoredUsers)) + for index, unparsed := range ignoredUsers { + var ignored Identifier + if err := unparsed.UnmarshalWithConf("", &ignored, koanf.UnmarshalConf{Tag: "koanf"}); err != nil { + return fmt.Errorf("failed to unmarshal ignored user at rolePropagation.propagators.%s.ignoredUsers[%d]: %w", p.configKey, index, err) + } + p._ignoredUsers = append(p._ignoredUsers, ignored) + } + } + + return nil +} diff --git a/sherlock/internal/role_propagation/propagator_init_test.go b/sherlock/internal/role_propagation/propagator_init_test.go index b0e3001d2..0d64ecb1d 100644 --- a/sherlock/internal/role_propagation/propagator_init_test.go +++ b/sherlock/internal/role_propagation/propagator_init_test.go @@ -62,6 +62,19 @@ func Test_propagatorImpl_Init(t *testing.T) { }, wantErr: true, }, + { + name: "dry run", + p: propagatorImpl[string, propagation_engines.GoogleWorkspaceGroupIdentifier, propagation_engines.GoogleWorkspaceGroupFields]{ + configKey: "devFirecloudGroupTestDryRun", + }, + engineFunc: func(c *propagation_engines_mocks.MockPropagationEngine[string, propagation_engines.GoogleWorkspaceGroupIdentifier, propagation_engines.GoogleWorkspaceGroupFields]) { + c.EXPECT().Init(ctx, mock.Anything).Return(nil) + }, + extraAssertions: func(t *testing.T, p propagatorImpl[string, propagation_engines.GoogleWorkspaceGroupIdentifier, propagation_engines.GoogleWorkspaceGroupFields]) { + assert.True(t, p._dryRun) + }, + wantErr: false, + }, { name: "config", p: propagatorImpl[string, propagation_engines.GoogleWorkspaceGroupIdentifier, propagation_engines.GoogleWorkspaceGroupFields]{ @@ -183,3 +196,17 @@ func Test_propagatorImpl_initToleratedUsers_calculator(t *testing.T) { {Email: "dynamic@example.com"}, }) } + +func Test_propagatorImpl_initIgnoredUsers_error(t *testing.T) { + k := koanf.New(".") + require.NoError(t, k.Set("ignoredUsers", []any{ + map[string]any{ + "number": "definitely not a number", + }, + })) + p := propagatorImpl[string, identifierWithInt, blankFields]{ + _config: k, + } + + assert.Errorf(t, p.initIgnoredUsers(context.Background()), "expected an error") +} diff --git a/sherlock/internal/role_propagation/propagator_propagate.go b/sherlock/internal/role_propagation/propagator_propagate.go index d36bccc01..c441cd1a9 100644 --- a/sherlock/internal/role_propagation/propagator_propagate.go +++ b/sherlock/internal/role_propagation/propagator_propagate.go @@ -7,6 +7,8 @@ import ( "github.com/broadinstitute/sherlock/sherlock/internal/config" "github.com/broadinstitute/sherlock/sherlock/internal/models" "github.com/broadinstitute/sherlock/sherlock/internal/role_propagation/intermediary_user" + "github.com/rs/zerolog" + "github.com/rs/zerolog/log" "sync" ) @@ -87,5 +89,17 @@ func (p *propagatorImpl[Grant, Identifier, Fields]) Propagate(ctx context.Contex errors = append(errors, errorsForGrant...) } + var l *zerolog.Event + if len(errors) > 0 { + l = log.Error() + } else { + l = log.Info() + } + roleName := "(unknown role)" + if role.Name != nil { + roleName = *role.Name + } + l.Strs("results", results).Errs("errors", errors).Msgf("PROP | %s%s propagation had %d results and %d errors", p.LogPrefix(), roleName, len(results), len(errors)) + return results, errors } diff --git a/sherlock/internal/suitability_synchronization/load_from_config.go b/sherlock/internal/suitability_synchronization/load_from_config.go deleted file mode 100644 index c1b9be2a5..000000000 --- a/sherlock/internal/suitability_synchronization/load_from_config.go +++ /dev/null @@ -1,24 +0,0 @@ -package suitability_synchronization - -import ( - "fmt" - "github.com/broadinstitute/sherlock/go-shared/pkg/utils" - "github.com/broadinstitute/sherlock/sherlock/internal/config" - "github.com/broadinstitute/sherlock/sherlock/internal/models" -) - -func fromConfig() ([]models.Suitability, error) { - var result []models.Suitability - for index, entry := range config.Config.Slices("suitabilitySynchronization.behaviors.loadIntoDB.extraPermissions") { - email := entry.String("email") - if email == "" { - return nil, fmt.Errorf("suitabilitySynchronization.behaviors.loadIntoDB.extraPermissions.extraPermissions[%d].email is required", index) - } - result = append(result, models.Suitability{ - Email: &email, - Suitable: utils.PointerTo(entry.Bool("suitable")), - Description: utils.PointerTo("suitability set via Sherlock configuration"), - }) - } - return result, nil -} diff --git a/sherlock/internal/suitability_synchronization/load_from_config_test.go b/sherlock/internal/suitability_synchronization/load_from_config_test.go deleted file mode 100644 index d620d9e45..000000000 --- a/sherlock/internal/suitability_synchronization/load_from_config_test.go +++ /dev/null @@ -1,27 +0,0 @@ -package suitability_synchronization - -import ( - "github.com/broadinstitute/sherlock/go-shared/pkg/utils" - "github.com/broadinstitute/sherlock/sherlock/internal/config" - "github.com/broadinstitute/sherlock/sherlock/internal/models" - "github.com/stretchr/testify/assert" - "testing" -) - -func Test_fromConfig(t *testing.T) { - config.LoadTestConfig() - results, err := fromConfig() - assert.NoError(t, err) - assert.Equal(t, []models.Suitability{ - { - Email: utils.PointerTo("has-extra-permissions-suitable@example.com"), - Suitable: utils.PointerTo(true), - Description: utils.PointerTo("suitability set via Sherlock configuration"), - }, - { - Email: utils.PointerTo("has-extra-permissions-non-suitable@example.com"), - Suitable: utils.PointerTo(false), - Description: utils.PointerTo("suitability set via Sherlock configuration"), - }, - }, results) -} diff --git a/sherlock/internal/suitability_synchronization/load_from_firecloud.go b/sherlock/internal/suitability_synchronization/load_from_firecloud.go index d60fab551..8b045a468 100644 --- a/sherlock/internal/suitability_synchronization/load_from_firecloud.go +++ b/sherlock/internal/suitability_synchronization/load_from_firecloud.go @@ -4,7 +4,6 @@ import ( "context" "encoding/json" "fmt" - "github.com/broadinstitute/sherlock/go-shared/pkg/utils" "github.com/broadinstitute/sherlock/sherlock/internal/config" "github.com/broadinstitute/sherlock/sherlock/internal/models" "github.com/rs/zerolog/log" @@ -13,53 +12,10 @@ import ( ) func fromFirecloud(ctx context.Context) ([]models.Suitability, error) { - adminService, err := admin.NewService(ctx, option.WithScopes(admin.AdminDirectoryUserReadonlyScope, admin.AdminDirectoryGroupMemberReadonlyScope)) + adminService, err := admin.NewService(ctx, option.WithScopes(admin.AdminDirectoryUserReadonlyScope)) if err != nil { return nil, fmt.Errorf("failed to authenticate to Google Workspace: %w", err) } - - var fcAdminsGroupEmails []string - err = adminService.Members.List(config.Config.MustString("suitabilitySynchronization.behaviors.loadIntoDB.firecloud.groups.fcAdmins")).Pages(ctx, func(members *admin.Members) error { - if members == nil { - return fmt.Errorf("suitability synchronization got a nil %s member page from Google", - config.Config.MustString("suitabilitySynchronization.behaviors.loadIntoDB.firecloud.groups.fcAdmins")) - } else { - for _, member := range members.Members { - if member == nil { - return fmt.Errorf("suitability synchronization got a nil %s member from Google", - config.Config.MustString("suitabilitySynchronization.behaviors.loadIntoDB.firecloud.groups.fcAdmins")) - } else { - fcAdminsGroupEmails = append(fcAdminsGroupEmails, member.Email) - } - } - } - return nil - }) - if err != nil { - return nil, err - } - - var firecloudProjectOwnersGroupEmails []string - err = adminService.Members.List(config.Config.MustString("suitabilitySynchronization.behaviors.loadIntoDB.firecloud.groups.firecloudProjectOwners")).Pages(ctx, func(members *admin.Members) error { - if members == nil { - return fmt.Errorf("suitability synchronization got a nil %s member page from Google", - config.Config.MustString("suitabilitySynchronization.behaviors.loadIntoDB.firecloud.groups.fcAdmins")) - } else { - for _, member := range members.Members { - if member == nil { - return fmt.Errorf("suitability synchronization got a nil %s member from Google", - config.Config.MustString("suitabilitySynchronization.behaviors.loadIntoDB.firecloud.groups.fcAdmins")) - } else { - firecloudProjectOwnersGroupEmails = append(firecloudProjectOwnersGroupEmails, member.Email) - } - } - } - return nil - }) - if err != nil { - return nil, err - } - resultSet := make(map[string]models.Suitability) err = adminService.Users.List().Domain(config.Config.MustString("suitabilitySynchronization.behaviors.loadIntoDB.firecloud.domain")).Pages(ctx, func(workspaceUsers *admin.Users) error { if workspaceUsers == nil { @@ -69,7 +25,7 @@ func fromFirecloud(ctx context.Context) ([]models.Suitability, error) { if workspaceUser == nil { return fmt.Errorf("suitability synchronization got a nil user from Google") } else { - suitable, description := parseFirecloudUser(workspaceUser, fcAdminsGroupEmails, firecloudProjectOwnersGroupEmails) + suitable, description := parseFirecloudUser(workspaceUser) if workspaceUser.PrimaryEmail != "" { resultSet[workspaceUser.PrimaryEmail] = models.Suitability{ Email: &workspaceUser.PrimaryEmail, @@ -134,7 +90,7 @@ func fromFirecloud(ctx context.Context) ([]models.Suitability, error) { return result, err } -func parseFirecloudUser(workspaceUser *admin.User, fcAdminsGroupEmails []string, firecloudProjectOwnersGroupEmails []string) (suitable bool, description string) { +func parseFirecloudUser(workspaceUser *admin.User) (suitable bool, description string) { if workspaceUser.PrimaryEmail == "" { return false, "firecloud user doesn't appear to have a primary email? something's amiss, marking as not suitable" } else if !workspaceUser.AgreedToTerms { @@ -147,12 +103,6 @@ func parseFirecloudUser(workspaceUser *admin.User, fcAdminsGroupEmails []string, config.Config.Duration("suitabilitySynchronization.behaviors.loadIntoDB.interval")) } else if workspaceUser.Archived { return false, "firecloud user is archived" - } else if !utils.Contains(fcAdminsGroupEmails, workspaceUser.PrimaryEmail) { - return false, fmt.Sprintf("firecloud user isn't in fc-admins group (reach out to #dsp-devops-champions for help; the user will need to wait %s after being added for Sherlock to pick it up)", - config.Config.Duration("suitabilitySynchronization.behaviors.loadIntoDB.interval")) - } else if !utils.Contains(firecloudProjectOwnersGroupEmails, workspaceUser.PrimaryEmail) { - return false, fmt.Sprintf("firecloud user isn't in firecloud-project-owners group (reach out to #dsp-devops-champions for help; the user will need to wait %s after being added for Sherlock to pick it up)", - config.Config.Duration("suitabilitySynchronization.behaviors.loadIntoDB.interval")) } else { return true, "firecloud user is suitable" } diff --git a/sherlock/internal/suitability_synchronization/load_from_firecloud_test.go b/sherlock/internal/suitability_synchronization/load_from_firecloud_test.go index f93cd9250..9256a1eda 100644 --- a/sherlock/internal/suitability_synchronization/load_from_firecloud_test.go +++ b/sherlock/internal/suitability_synchronization/load_from_firecloud_test.go @@ -8,9 +8,7 @@ import ( func Test_parseFirecloudUser(t *testing.T) { type args struct { - workspaceUser *admin.User - fcAdminsGroupEmails []string - firecloudProjectOwnersGroupEmails []string + workspaceUser *admin.User } tests := []struct { name string @@ -74,37 +72,6 @@ func Test_parseFirecloudUser(t *testing.T) { wantSuitable: false, wantDescriptionSubstring: "is archived", }, - { - name: "not in fc-admins group", - args: args{ - workspaceUser: &admin.User{ - PrimaryEmail: "foo@example.com", - AgreedToTerms: true, - IsEnrolledIn2Sv: true, - Suspended: false, - Archived: false, - }, - fcAdminsGroupEmails: []string{}, - }, - wantSuitable: false, - wantDescriptionSubstring: "isn't in fc-admins group", - }, - { - name: "not in firecloud-project-owners group", - args: args{ - workspaceUser: &admin.User{ - PrimaryEmail: "foo@example.com", - AgreedToTerms: true, - IsEnrolledIn2Sv: true, - Suspended: false, - Archived: false, - }, - fcAdminsGroupEmails: []string{"foo@example.com"}, - firecloudProjectOwnersGroupEmails: []string{}, - }, - wantSuitable: false, - wantDescriptionSubstring: "isn't in firecloud-project-owners group", - }, { name: "suitable", args: args{ @@ -115,8 +82,6 @@ func Test_parseFirecloudUser(t *testing.T) { Suspended: false, Archived: false, }, - fcAdminsGroupEmails: []string{"foo@example.com"}, - firecloudProjectOwnersGroupEmails: []string{"foo@example.com"}, }, wantSuitable: true, wantDescriptionSubstring: "is suitable", @@ -124,9 +89,9 @@ func Test_parseFirecloudUser(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - gotSuitable, gotDescription := parseFirecloudUser(tt.args.workspaceUser, tt.args.fcAdminsGroupEmails, tt.args.firecloudProjectOwnersGroupEmails) - assert.Equalf(t, tt.wantSuitable, gotSuitable, "parseFirecloudUser(%v, %v, %v)", tt.args.workspaceUser, tt.args.fcAdminsGroupEmails, tt.args.firecloudProjectOwnersGroupEmails) - assert.Containsf(t, gotDescription, tt.wantDescriptionSubstring, "parseFirecloudUser(%v, %v, %v)", tt.args.workspaceUser, tt.args.fcAdminsGroupEmails, tt.args.firecloudProjectOwnersGroupEmails) + gotSuitable, gotDescription := parseFirecloudUser(tt.args.workspaceUser) + assert.Equalf(t, tt.wantSuitable, gotSuitable, "parseFirecloudUser(%v)", tt.args.workspaceUser) + assert.Containsf(t, gotDescription, tt.wantDescriptionSubstring, "parseFirecloudUser(%v)", tt.args.workspaceUser) }) } } diff --git a/sherlock/internal/suitability_synchronization/load_into_db.go b/sherlock/internal/suitability_synchronization/load_into_db.go index 1912d4d0e..ae5bfc344 100644 --- a/sherlock/internal/suitability_synchronization/load_into_db.go +++ b/sherlock/internal/suitability_synchronization/load_into_db.go @@ -34,10 +34,6 @@ func LoadIntoDB(ctx context.Context, db *gorm.DB) error { if !config.Config.Bool("suitabilitySynchronization.enable") || !config.Config.Bool("suitabilitySynchronization.behaviors.loadIntoDB.enable") { return nil } - suitabilitiesFromConfig, err := fromConfig() - if err != nil { - return err - } suitabilitiesFromFirecloud, err := fromFirecloud(ctx) if err != nil { if strings.Contains(err.Error(), "dailyLimitExceeded") { @@ -46,12 +42,11 @@ func LoadIntoDB(ctx context.Context, db *gorm.DB) error { } return err } - suitabilities := append(suitabilitiesFromConfig, suitabilitiesFromFirecloud...) // Assume super-user privileges for this operation (required to edit this table) superUserDB := models.SetCurrentUserForDB(db, models.SelfUser) - for _, suitability := range suitabilities { + for _, suitability := range suitabilitiesFromFirecloud { if err = superUserDB. Where(&models.Suitability{ Email: suitability.Email,