diff --git a/sherlock/config/default_config.yaml b/sherlock/config/default_config.yaml index a8e10f986..9813d54e0 100644 --- a/sherlock/config/default_config.yaml +++ b/sherlock/config/default_config.yaml @@ -371,7 +371,6 @@ rolePropagation: # Sherlock users to Google Workspace users. This must contain a "@". userEmailSuffixesToReplace: - "@broadinstitute.org" - qaFirecloudGroup: enable: false # The domain of the Google Workspace, assumed to be the email domain of all members. This should @@ -381,7 +380,6 @@ rolePropagation: # Sherlock users to Google Workspace users. This must contain a "@". userEmailSuffixesToReplace: - "@broadinstitute.org" - prodFirecloudGroup: enable: false # The domain of the Google Workspace, assumed to be the email domain of all members. This should @@ -392,6 +390,34 @@ rolePropagation: userEmailSuffixesToReplace: - "@broadinstitute.org" + devFirecloudFolderOwner: + enable: 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" + # Suffixes of Sherlock users' emails that should be swapped out with "@"+workspaceDomain to match + # Sherlock users to Google Workspace users. This must contain a "@". + userEmailSuffixesToReplace: + - "@broadinstitute.org" + qaFirecloudFolderOwner: + enable: 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" + # Suffixes of Sherlock users' emails that should be swapped out with "@"+workspaceDomain to match + # Sherlock users to Google Workspace users. This must contain a "@". + userEmailSuffixesToReplace: + - "@broadinstitute.org" + prodFirecloudFolderOwner: + enable: 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" + # Suffixes of Sherlock users' emails that should be swapped out with "@"+workspaceDomain to match + # Sherlock users to Google Workspace users. This must contain a "@". + userEmailSuffixesToReplace: + - "@broadinstitute.org" + devAzureGroup: enable: false # The client ID of the Azure AD app to use for authentication. @@ -408,7 +434,6 @@ rolePropagation: # Sherlock users to Azure Entra ID users. userEmailSuffixesToReplace: - "@broadinstitute.org" - prodAzureGroup: enable: false # The client ID of the Azure AD app to use for authentication. diff --git a/sherlock/db/migrations/000097_add_firecloud_folder_owner_fields.down.sql b/sherlock/db/migrations/000097_add_firecloud_folder_owner_fields.down.sql new file mode 100644 index 000000000..10e53f75e --- /dev/null +++ b/sherlock/db/migrations/000097_add_firecloud_folder_owner_fields.down.sql @@ -0,0 +1,32 @@ +drop index if exists roles_grants_dev_firecloud_folder_owner_unique; + +alter table roles + drop column if exists grants_dev_firecloud_folder_owner; + +alter table role_operations + drop column if exists from_grants_dev_firecloud_folder_owner; + +alter table role_operations + drop column if exists to_grants_dev_firecloud_folder_owner; + +drop index if exists roles_grants_qa_firecloud_folder_owner_unique; + +alter table roles + drop column if exists grants_qa_firecloud_folder_owner; + +alter table role_operations + drop column if exists from_grants_qa_firecloud_folder_owner; + +alter table role_operations + drop column if exists to_grants_qa_firecloud_folder_owner; + +drop index if exists roles_grants_prod_firecloud_folder_owner_unique; + +alter table roles + drop column if exists grants_prod_firecloud_folder_owner; + +alter table role_operations + drop column if exists from_grants_prod_firecloud_folder_owner; + +alter table role_operations + drop column if exists to_grants_prod_firecloud_folder_owner; diff --git a/sherlock/db/migrations/000097_add_firecloud_folder_owner_fields.up.sql b/sherlock/db/migrations/000097_add_firecloud_folder_owner_fields.up.sql new file mode 100644 index 000000000..41025c7dc --- /dev/null +++ b/sherlock/db/migrations/000097_add_firecloud_folder_owner_fields.up.sql @@ -0,0 +1,38 @@ +alter table roles + add column if not exists grants_dev_firecloud_folder_owner text; + +create unique index if not exists roles_grants_dev_firecloud_folder_owner_unique + on roles (grants_dev_firecloud_folder_owner) + where deleted_at is null and grants_dev_firecloud_folder_owner is not null and grants_dev_firecloud_folder_owner != ''; + +alter table role_operations + add column if not exists from_grants_dev_firecloud_folder_owner text; + +alter table role_operations + add column if not exists to_grants_dev_firecloud_folder_owner text; + +alter table roles + add column if not exists grants_qa_firecloud_folder_owner text; + +create unique index if not exists roles_grants_qa_firecloud_folder_owner_unique + on roles (grants_qa_firecloud_folder_owner) + where deleted_at is null and grants_qa_firecloud_folder_owner is not null and grants_qa_firecloud_folder_owner != ''; + +alter table role_operations + add column if not exists from_grants_qa_firecloud_folder_owner text; + +alter table role_operations + add column if not exists to_grants_qa_firecloud_folder_owner text; + +alter table roles + add column if not exists grants_prod_firecloud_folder_owner text; + +create unique index if not exists roles_grants_prod_firecloud_folder_owner_unique + on roles (grants_prod_firecloud_folder_owner) + where deleted_at is null and grants_prod_firecloud_folder_owner is not null and grants_prod_firecloud_folder_owner != ''; + +alter table role_operations + add column if not exists from_grants_prod_firecloud_folder_owner text; + +alter table role_operations + add column if not exists to_grants_prod_firecloud_folder_owner text; diff --git a/sherlock/go.mod b/sherlock/go.mod index 03aaab5e5..3091d5d29 100644 --- a/sherlock/go.mod +++ b/sherlock/go.mod @@ -1,13 +1,15 @@ module github.com/broadinstitute/sherlock/sherlock -go 1.22 +go 1.22.0 -toolchain go1.22.5 +toolchain go1.23.0 require ( cloud.google.com/go/bigquery v1.62.0 cloud.google.com/go/cloudsqlconn v1.12.0 + cloud.google.com/go/iam v1.2.0 cloud.google.com/go/kms v1.19.0 + cloud.google.com/go/resourcemanager v1.10.0 contrib.go.opencensus.io/exporter/prometheus v0.4.2 github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.7.0 github.com/PagerDuty/go-pagerduty v1.8.0 @@ -19,7 +21,7 @@ require ( github.com/gin-contrib/cors v1.7.2 github.com/gin-gonic/gin v1.10.0 github.com/go-jose/go-jose/v4 v4.0.4 - github.com/golang-migrate/migrate/v4 v4.17.1 + github.com/golang-migrate/migrate/v4 v4.18.1 github.com/google/go-cmp v0.6.0 github.com/google/go-github/v58 v58.0.0 github.com/google/uuid v1.6.0 @@ -39,7 +41,7 @@ require ( go.opencensus.io v0.24.0 golang.org/x/crypto v0.27.0 golang.org/x/exp v0.0.0-20240823005443-9b4947da3948 - golang.org/x/net v0.28.0 + golang.org/x/net v0.29.0 golang.org/x/oauth2 v0.23.0 golang.org/x/text v0.18.0 google.golang.org/api v0.195.0 @@ -55,7 +57,6 @@ require ( cloud.google.com/go/auth v0.9.1 // indirect cloud.google.com/go/auth/oauth2adapt v0.2.4 // indirect cloud.google.com/go/compute/metadata v0.5.0 // indirect - cloud.google.com/go/iam v1.2.0 // indirect cloud.google.com/go/longrunning v0.6.0 // indirect filippo.io/edwards25519 v1.1.0 // indirect github.com/Azure/azure-sdk-for-go/sdk/azcore v1.14.0 // indirect @@ -163,7 +164,7 @@ require ( go.opentelemetry.io/otel/trace v1.29.0 // indirect go.uber.org/atomic v1.11.0 // indirect golang.org/x/arch v0.9.0 // indirect - golang.org/x/mod v0.20.0 // indirect + golang.org/x/mod v0.21.0 // indirect golang.org/x/sync v0.8.0 // indirect golang.org/x/sys v0.25.0 // indirect golang.org/x/time v0.6.0 // indirect diff --git a/sherlock/go.sum b/sherlock/go.sum index 0833a6318..42ecc8d2f 100644 --- a/sherlock/go.sum +++ b/sherlock/go.sum @@ -45,6 +45,8 @@ cloud.google.com/go/pubsub v1.0.1/go.mod h1:R0Gpsv3s54REJCy4fxDixWD93lHJMoZTyQ2k cloud.google.com/go/pubsub v1.1.0/go.mod h1:EwwdRX2sKPjnvnqCa270oGRyludottCI76h+R3AArQw= cloud.google.com/go/pubsub v1.2.0/go.mod h1:jhfEVHT8odbXTkndysNHCcx0awwzvfOlguIAii9o8iA= cloud.google.com/go/pubsub v1.3.1/go.mod h1:i+ucay31+CNRpDW4Lu78I4xXG+O1r/MAHgjpRVR+TSU= +cloud.google.com/go/resourcemanager v1.10.0 h1:oqO6UInOJ1ZBBEYTKPJms2+FKdGmZEYAYBKyt0oqpEI= +cloud.google.com/go/resourcemanager v1.10.0/go.mod h1:kIx3TWDCjLnUQUdjQ/e8EXsS9GJEzvcY+YMOHpADxrk= cloud.google.com/go/storage v1.0.0/go.mod h1:IhtSnM/ZTZV8YYJWCY8RULGVqBDmpoyjwiyrjsg+URw= cloud.google.com/go/storage v1.5.0/go.mod h1:tpKbwo567HUNpVclU5sGELwQWBDZ8gh0ZeosJ0Rtdos= cloud.google.com/go/storage v1.6.0/go.mod h1:N7U0C8pVQ/+NIKOBQyamJIeKQKkZ+mxpohlUTyfDhBk= @@ -71,8 +73,8 @@ github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03 github.com/BurntSushi/xgb v0.0.0-20160522181843-27f122750802/go.mod h1:IVnqGOEym/WlBOVXweHU+Q+/VP0lqqI8lqeDx9IjBqo= github.com/KyleBanks/depth v1.2.1 h1:5h8fQADFrWtarTdtDudMmGsC7GPbOAu6RVB3ffsVFHc= github.com/KyleBanks/depth v1.2.1/go.mod h1:jzSb9d0L43HxTQfT+oSA1EEp2q+ne2uh6XgeJcm8brE= -github.com/Microsoft/go-winio v0.6.1 h1:9/kr64B9VUZrLm5YYwbGtUJnMgqWVOdUAXu6Migciow= -github.com/Microsoft/go-winio v0.6.1/go.mod h1:LRdKpFKfdobln8UmuiYcKPot9D2v6svN5+sAH+4kjUM= +github.com/Microsoft/go-winio v0.6.2 h1:F2VQgta7ecxGYO8k3ZZz3RS8fVIXVxONVUPlNERoyfY= +github.com/Microsoft/go-winio v0.6.2/go.mod h1:yd8OoFMLzJbo9gZq8j5qaps8bJ9aShtEA8Ipt1oGCvU= github.com/PagerDuty/go-pagerduty v1.8.0 h1:MTFqTffIcAervB83U7Bx6HERzLbyaSPL/+oxH3zyluI= github.com/PagerDuty/go-pagerduty v1.8.0/go.mod h1:nzIeAqyFSJAFkjWKvMzug0JtwDg+V+UoCWjFrfFH5mI= github.com/alecthomas/template v0.0.0-20160405071501-a0175ee3bccc/go.mod h1:LOuyumcjzFXgccqObfd/Ljyb9UuFJ6TxHnclSeseNhc= @@ -141,14 +143,14 @@ github.com/davecgh/go-spew v0.0.0-20161028175848-04cdfd42973b/go.mod h1:J7Y8YcW2 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= -github.com/dhui/dktest v0.4.1 h1:/w+IWuDXVymg3IrRJCHHOkMK10m9aNVMOyD0X12YVTg= -github.com/dhui/dktest v0.4.1/go.mod h1:DdOqcUpL7vgyP4GlF3X3w7HbSlz8cEQzwewPveYEQbA= -github.com/docker/distribution v2.8.2+incompatible h1:T3de5rq0dB1j30rp0sA2rER+m322EBzniBPB6ZIzuh8= -github.com/docker/distribution v2.8.2+incompatible/go.mod h1:J2gT2udsDAN96Uj4KfcMRqY0/ypR+oyYUYmja8H+y+w= -github.com/docker/docker v24.0.9+incompatible h1:HPGzNmwfLZWdxHqK9/II92pyi1EpYKsAqcl4G0Of9v0= -github.com/docker/docker v24.0.9+incompatible/go.mod h1:eEKB0N0r5NX/I1kEveEz05bcu8tLC/8azJZsviup8Sk= -github.com/docker/go-connections v0.4.0 h1:El9xVISelRB7BuFusrZozjnkIM5YnzCViNKohAFqRJQ= -github.com/docker/go-connections v0.4.0/go.mod h1:Gbd7IOopHjR8Iph03tsViu4nIes5XhDvyHbTtUxmeec= +github.com/dhui/dktest v0.4.3 h1:wquqUxAFdcUgabAVLvSCOKOlag5cIZuaOjYIBOWdsR0= +github.com/dhui/dktest v0.4.3/go.mod h1:zNK8IwktWzQRm6I/l2Wjp7MakiyaFWv4G1hjmodmMTs= +github.com/distribution/reference v0.6.0 h1:0IXCQ5g4/QMHHkarYzh5l+u8T3t73zM5QvfrDyIgxBk= +github.com/distribution/reference v0.6.0/go.mod h1:BbU0aIcezP1/5jX/8MP0YiH4SdvB5Y4f/wlDRiLyi3E= +github.com/docker/docker v27.2.0+incompatible h1:Rk9nIVdfH3+Vz4cyI/uhbINhEZ/oLmc+CBXmH6fbNk4= +github.com/docker/docker v27.2.0+incompatible/go.mod h1:eEKB0N0r5NX/I1kEveEz05bcu8tLC/8azJZsviup8Sk= +github.com/docker/go-connections v0.5.0 h1:USnMq7hx7gwdVZq1L49hLXaFtUdTADjXGp+uj1Br63c= +github.com/docker/go-connections v0.5.0/go.mod h1:ov60Kzw0kKElRwhNs9UlUHAE/F9Fe6GLaXnqyDdmEXc= github.com/docker/go-units v0.5.0 h1:69rxXcBk27SvSaaxTtLh/8llcHD8vYHT7WSdRZ/jvr4= github.com/docker/go-units v0.5.0/go.mod h1:fgPhTUdO+D/Jk86RDLlptpiXQzgHJF7gydDDbaIK4Dk= github.com/dustin/go-humanize v1.0.0/go.mod h1:HtrtbFcZ19U5GC7JDqmcUSB87Iq5E25KnS6fMYU6eOk= @@ -234,8 +236,8 @@ github.com/gogo/protobuf v1.3.2 h1:Ov1cvc58UF3b5XjBnZv7+opcTcQFZebYjWzi34vdm4Q= github.com/gogo/protobuf v1.3.2/go.mod h1:P1XiOD3dCwIKUDQYPy72D8LYyHL2YPYrpS2s69NZV8Q= github.com/golang-jwt/jwt/v5 v5.2.1 h1:OuVbFODueb089Lh128TAcimifWaLhJwVflnrgM17wHk= github.com/golang-jwt/jwt/v5 v5.2.1/go.mod h1:pqrtFR0X4osieyHYxtmOUWsAWrfe1Q5UVIyoH402zdk= -github.com/golang-migrate/migrate/v4 v4.17.1 h1:4zQ6iqL6t6AiItphxJctQb3cFqWiSpMnX7wLTPnnYO4= -github.com/golang-migrate/migrate/v4 v4.17.1/go.mod h1:m8hinFyWBn0SA4QKHuKh175Pm9wjmxj3S2Mia7dbXzM= +github.com/golang-migrate/migrate/v4 v4.18.1 h1:JML/k+t4tpHCpQTCAD62Nu43NUFzHY4CV3uAuvHGC+Y= +github.com/golang-migrate/migrate/v4 v4.18.1/go.mod h1:HAX6m3sQgcdO81tdjn5exv20+3Kb13cmGli1hrD6hks= github.com/golang-sql/civil v0.0.0-20220223132316-b832511892a9 h1:au07oEsX2xN0ktxqI+Sida1w446QrXBRJ0nee3SNZlA= github.com/golang-sql/civil v0.0.0-20220223132316-b832511892a9/go.mod h1:8vg3r2VgvsThLBIFL93Qb5yWzgyZWhEmBwUJWevAkK0= github.com/golang-sql/sqlexp v0.1.0 h1:ZCD6MBpcuOVfGVqsEmY5/4FtYiKz6tSyUv9LPEDei6A= @@ -463,8 +465,8 @@ github.com/mattn/go-isatty v0.0.16/go.mod h1:kYGgaQfpe5nmfYZH+SKPsOc2e4SrIfOl2e/ github.com/mattn/go-isatty v0.0.19/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D7dTCTo3Y= github.com/mattn/go-isatty v0.0.20 h1:xfD0iDuEKnDkl03q4limB+vH+GxLEtL/jb4xVJSWWEY= github.com/mattn/go-isatty v0.0.20/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D7dTCTo3Y= -github.com/mattn/go-sqlite3 v1.14.16 h1:yOQRA0RpS5PFz/oikGwBEqvAWhWg5ufRz4ETLjwpU1Y= -github.com/mattn/go-sqlite3 v1.14.16/go.mod h1:2eHXhiwb8IkHr+BDWZGa96P6+rkvnG63S2DGjv9HUNg= +github.com/mattn/go-sqlite3 v1.14.22 h1:2gZY6PC6kBnID23Tichd1K+Z0oS6nE/XwU+Vz/5o4kU= +github.com/mattn/go-sqlite3 v1.14.22/go.mod h1:Uh1q+B4BYcTPb+yiD3kU8Ct7aC0hY9fxUwlHK0RXw+Y= github.com/matttproud/golang_protobuf_extensions v1.0.1/go.mod h1:D8He9yQNgCq6Z5Ld7szi9bcBfOoFv/3dc6xSMkL2PC0= github.com/microsoft/go-mssqldb v1.7.2 h1:CHkFJiObW7ItKTJfHo1QX7QBBD1iV+mn1eOyRP3b/PA= github.com/microsoft/go-mssqldb v1.7.2/go.mod h1:kOvZKUdrhhFQmxLZqbwUV0rHkNkZpthMITIb2Ko1IoA= @@ -504,6 +506,8 @@ github.com/mitchellh/mapstructure v1.5.0/go.mod h1:bFUtVrKA4DC2yAKiSyO/QUcy7e+RR github.com/mitchellh/reflectwalk v1.0.0/go.mod h1:mSTlrgnPZtwu0c4WaC2kGObEpuNDbx0jmZXqmk4esnw= github.com/mitchellh/reflectwalk v1.0.2 h1:G2LzWKi524PWgd3mLHV8Y5k7s6XUvT0Gef6zxSIeXaQ= github.com/mitchellh/reflectwalk v1.0.2/go.mod h1:mSTlrgnPZtwu0c4WaC2kGObEpuNDbx0jmZXqmk4esnw= +github.com/moby/docker-image-spec v1.3.1 h1:jMKff3w6PgbfSa69GfNg+zN/XLhfXJGnEx3Nl2EsFP0= +github.com/moby/docker-image-spec v1.3.1/go.mod h1:eKmb5VW8vQEh/BAr2yvVNvuiJuY6UIocYsFu/DxxRpo= github.com/moby/term v0.5.0 h1:xt8Q1nalod/v7BqbG21f8mQPqH+xAaC9C3N3wfWbVP0= github.com/moby/term v0.5.0/go.mod h1:8FzsFHVUBGZdbDsJw/ot+X+d5HLUbvklYLJ9uGfcI3Y= github.com/modern-go/concurrent v0.0.0-20180228061459-e0a39a4cb421/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q= @@ -527,8 +531,8 @@ github.com/npillmayer/nestext v0.1.3/go.mod h1:h2lrijH8jpicr25dFY+oAJLyzlya6jhnu github.com/oklog/run v1.0.0/go.mod h1:dlhp/R75TPv97u0XWUtDeV/lRKWPKSdTuV0TZvrmrQA= github.com/opencontainers/go-digest v1.0.0 h1:apOUWs51W5PlhuyGyz9FCeeBIOUDA/6nW8Oi/yOhh5U= github.com/opencontainers/go-digest v1.0.0/go.mod h1:0JzlMkj0TRzQZfJkVvzbP0HBR3IKzErnv2BNG4W4MAM= -github.com/opencontainers/image-spec v1.0.2 h1:9yCKha/T5XdGtO0q9Q9a6T5NUCsTn/DrBg0D7ufOcFM= -github.com/opencontainers/image-spec v1.0.2/go.mod h1:BtxoFyWECRxE4U/7sNtV5W15zMzWCbyJoFRP3s7yZA0= +github.com/opencontainers/image-spec v1.1.0 h1:8SG7/vwALn54lVB/0yZ/MMwhFrPYtpEHQb2IpWsCzug= +github.com/opencontainers/image-spec v1.1.0/go.mod h1:W4s4sFTMaBeK1BQLXbG4AdM2szdn85PY75RI83NrTrM= github.com/pact-foundation/pact-go/v2 v2.0.8 h1:j9s/tk46O5hpEEbYd0/QF9kQlQt/mu3HJrVJVeix54w= github.com/pact-foundation/pact-go/v2 v2.0.8/go.mod h1:/IAP9loNwPHWdZUrECAltM8p630NETHitNarJa/DkXU= github.com/pascaldekloe/goe v0.0.0-20180627143212-57f6aae5913c/go.mod h1:lzWF7FIEvWOWxwDKqyGYQf6ZUaNfKdP144TG7ZOy1lc= @@ -682,8 +686,8 @@ go.opentelemetry.io/otel v1.29.0 h1:PdomN/Al4q/lN6iBJEN3AwPvUiHPMlt93c8bqTG5Llw= go.opentelemetry.io/otel v1.29.0/go.mod h1:N/WtXPs1CNCUEx+Agz5uouwCba+i+bJGFicT8SR4NP8= go.opentelemetry.io/otel/metric v1.29.0 h1:vPf/HFWTNkPu1aYeIsc98l4ktOQaL6LeSoeV2g+8YLc= go.opentelemetry.io/otel/metric v1.29.0/go.mod h1:auu/QWieFVWx+DmQOUMgj0F8LHWdgalxXqvp7BII/W8= -go.opentelemetry.io/otel/sdk v1.28.0 h1:b9d7hIry8yZsgtbmM0DKyPWMMUMlK9NEKuIG4aBqWyE= -go.opentelemetry.io/otel/sdk v1.28.0/go.mod h1:oYj7ClPUA7Iw3m+r7GeEjz0qckQRJK2B8zjcZEfu7Pg= +go.opentelemetry.io/otel/sdk v1.29.0 h1:vkqKjk7gwhS8VaWb0POZKmIEDimRCMsopNYnriHyryo= +go.opentelemetry.io/otel/sdk v1.29.0/go.mod h1:pM8Dx5WKnvxLCb+8lG1PRNIDxu9g9b9g59Qr7hfAAok= go.opentelemetry.io/otel/trace v1.29.0 h1:J/8ZNK4XgR7a21DZUAsbF8pZ5Jcw1VhACmnYt39JTi4= go.opentelemetry.io/otel/trace v1.29.0/go.mod h1:eHl3w0sp3paPkYstJOmAimxhiFXPg+MMTlEh3nsQgWQ= go.uber.org/atomic v1.7.0/go.mod h1:fEN4uk6kAWBTFdckzkM89CLk9XfWZrxpCo0nPH17wJc= @@ -738,8 +742,8 @@ golang.org/x/mod v0.2.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.4.2/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4/go.mod h1:jJ57K6gSWd91VN4djpZkiMVwK6gcyfeH4XE8wZrZaV4= -golang.org/x/mod v0.20.0 h1:utOm6MM3R3dnawAiJgn0y+xvuYRsm1RKM/4giyfDgV0= -golang.org/x/mod v0.20.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= +golang.org/x/mod v0.21.0 h1:vvrHzRwRfVKSiLrG+d4FMl/Qi4ukBCE6kZlTUkDYRT0= +golang.org/x/mod v0.21.0/go.mod h1:6SkKJ3Xj0I0BrPOZoBy3bdMptDDU9oJrpohJ3eWZ1fY= golang.org/x/net v0.0.0-20180724234803-3673e40ba225/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20180826012351-8a410e7b638d/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20181114220301-adae6a3d119a/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= @@ -779,8 +783,8 @@ golang.org/x/net v0.0.0-20220127200216-cd36cc0744dd/go.mod h1:CfG3xpIq0wQ8r1q4Su golang.org/x/net v0.0.0-20220225172249-27dd8689420f/go.mod h1:CfG3xpIq0wQ8r1q4Su4UZFWDARRcnwPjda9FqA0JpMk= golang.org/x/net v0.0.0-20220722155237-a158d28d115b/go.mod h1:XRhObCWvk6IyKnWLug+ECip1KBveYUHfp+8e9klMJ9c= golang.org/x/net v0.7.0/go.mod h1:2Tu9+aMcznHK/AK1HMvgo6xiTLG5rD5rZLDS+rp2Bjs= -golang.org/x/net v0.28.0 h1:a9JDOJc5GMUJ0+UDqmLT86WiEy7iWyIhz8gz8E4e5hE= -golang.org/x/net v0.28.0/go.mod h1:yqtgsTWOOnlGLG9GFRrK3++bGOUEkNBoHZc8MEDWPNg= +golang.org/x/net v0.29.0 h1:5ORfpBpCs4HzDYoodCDBbwHzdR5UrLBZ3sOnUJmFoHo= +golang.org/x/net v0.29.0/go.mod h1:gLkgy8jTGERgjzMic6DS9+SP0ajcu6Xu3Orq/SpETg0= golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U= golang.org/x/oauth2 v0.0.0-20190226205417-e64efc72b421/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw= golang.org/x/oauth2 v0.0.0-20190604053449-0f29369cfe45/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw= diff --git a/sherlock/internal/api/sherlock/roles_v3.go b/sherlock/internal/api/sherlock/roles_v3.go index 2d44badf5..cf70bf0d8 100644 --- a/sherlock/internal/api/sherlock/roles_v3.go +++ b/sherlock/internal/api/sherlock/roles_v3.go @@ -14,35 +14,41 @@ type RoleV3 struct { } type RoleV3Edit struct { - Name *string `json:"name" form:"name"` - SuspendNonSuitableUsers *bool `json:"suspendNonSuitableUsers,omitempty" form:"suspendNonSuitableUsers"` // When true, the "suspended" field on role assignments will be computed by Sherlock based on suitability instead of being a mutable API field - AutoAssignAllUsers *bool `json:"autoAssignAllUsers,omitempty" form:"autoAssignAllUsers"` // When true, Sherlock will automatically assign all users to this role who do not already have a role assignment - CanBeGlassBrokenByRole *uint `json:"canBeGlassBrokenByRole,omitempty" form:"canBeGlassBrokenByRole"` - DefaultGlassBreakDuration *Duration `json:"defaultGlassBreakDuration,omitempty" swaggertype:"string" form:"defaultGlassBreakDuration"` - GrantsSherlockSuperAdmin *bool `json:"grantsSherlockSuperAdmin,omitempty" form:"grantsSherlockSuperAdmin"` - GrantsDevFirecloudGroup *string `json:"grantsDevFirecloudGroup,omitempty" form:"grantsDevFirecloudGroup"` - GrantsQaFirecloudGroup *string `json:"grantsQaFirecloudGroup,omitempty" form:"grantsQaFirecloudGroup"` - GrantsProdFirecloudGroup *string `json:"grantsProdFirecloudGroup,omitempty" form:"grantsProdFirecloudGroup"` - GrantsDevAzureGroup *string `json:"grantsDevAzureGroup,omitempty" form:"grantsDevAzureGroup"` - GrantsProdAzureGroup *string `json:"grantsProdAzureGroup,omitempty" form:"grantsProdAzureGroup"` - GrantsBroadInstituteGroup *string `json:"grantsBroadInstituteGroup,omitempty" form:"grantsBroadInstituteGroup"` + Name *string `json:"name" form:"name"` + SuspendNonSuitableUsers *bool `json:"suspendNonSuitableUsers,omitempty" form:"suspendNonSuitableUsers"` // When true, the "suspended" field on role assignments will be computed by Sherlock based on suitability instead of being a mutable API field + AutoAssignAllUsers *bool `json:"autoAssignAllUsers,omitempty" form:"autoAssignAllUsers"` // When true, Sherlock will automatically assign all users to this role who do not already have a role assignment + CanBeGlassBrokenByRole *uint `json:"canBeGlassBrokenByRole,omitempty" form:"canBeGlassBrokenByRole"` + DefaultGlassBreakDuration *Duration `json:"defaultGlassBreakDuration,omitempty" swaggertype:"string" form:"defaultGlassBreakDuration"` + GrantsSherlockSuperAdmin *bool `json:"grantsSherlockSuperAdmin,omitempty" form:"grantsSherlockSuperAdmin"` + GrantsDevFirecloudGroup *string `json:"grantsDevFirecloudGroup,omitempty" form:"grantsDevFirecloudGroup"` + GrantsQaFirecloudGroup *string `json:"grantsQaFirecloudGroup,omitempty" form:"grantsQaFirecloudGroup"` + GrantsProdFirecloudGroup *string `json:"grantsProdFirecloudGroup,omitempty" form:"grantsProdFirecloudGroup"` + GrantsDevFirecloudFolderOwner *string `json:"grantsDevFirecloudFolderOwner,omitempty" form:"grantsDevFirecloudFolderOwner"` + GrantsQaFirecloudFolderOwner *string `json:"grantsQaFirecloudFolderOwner,omitempty" form:"grantsQaFirecloudFolderOwner"` + GrantsProdFirecloudFolderOwner *string `json:"grantsProdFirecloudFolderOwner,omitempty" form:"grantsProdFirecloudFolderOwner"` + GrantsDevAzureGroup *string `json:"grantsDevAzureGroup,omitempty" form:"grantsDevAzureGroup"` + GrantsProdAzureGroup *string `json:"grantsProdAzureGroup,omitempty" form:"grantsProdAzureGroup"` + GrantsBroadInstituteGroup *string `json:"grantsBroadInstituteGroup,omitempty" form:"grantsBroadInstituteGroup"` } func (r RoleV3) toModel() models.Role { ret := models.Role{ Model: r.toGormModel(), RoleFields: models.RoleFields{ - Name: r.Name, - SuspendNonSuitableUsers: r.SuspendNonSuitableUsers, - AutoAssignAllUsers: r.AutoAssignAllUsers, - CanBeGlassBrokenByRoleID: r.CanBeGlassBrokenByRole, - GrantsSherlockSuperAdmin: r.GrantsSherlockSuperAdmin, - GrantsDevFirecloudGroup: r.GrantsDevFirecloudGroup, - GrantsQaFirecloudGroup: r.GrantsQaFirecloudGroup, - GrantsProdFirecloudGroup: r.GrantsProdFirecloudGroup, - GrantsDevAzureGroup: r.GrantsDevAzureGroup, - GrantsProdAzureGroup: r.GrantsProdAzureGroup, - GrantsBroadInstituteGroup: r.GrantsBroadInstituteGroup, + Name: r.Name, + SuspendNonSuitableUsers: r.SuspendNonSuitableUsers, + AutoAssignAllUsers: r.AutoAssignAllUsers, + CanBeGlassBrokenByRoleID: r.CanBeGlassBrokenByRole, + GrantsSherlockSuperAdmin: r.GrantsSherlockSuperAdmin, + GrantsDevFirecloudGroup: r.GrantsDevFirecloudGroup, + GrantsQaFirecloudGroup: r.GrantsQaFirecloudGroup, + GrantsProdFirecloudGroup: r.GrantsProdFirecloudGroup, + GrantsDevFirecloudFolderOwner: r.GrantsDevFirecloudFolderOwner, + GrantsQaFirecloudFolderOwner: r.GrantsQaFirecloudFolderOwner, + GrantsProdFirecloudFolderOwner: r.GrantsProdFirecloudFolderOwner, + GrantsDevAzureGroup: r.GrantsDevAzureGroup, + GrantsProdAzureGroup: r.GrantsProdAzureGroup, + GrantsBroadInstituteGroup: r.GrantsBroadInstituteGroup, }, } if r.DefaultGlassBreakDuration != nil { @@ -67,13 +73,16 @@ func roleFromModel(model models.Role) RoleV3 { DefaultGlassBreakDuration: utils.NilOrCall(func(nanoseconds int64) Duration { return Duration{time.Duration(nanoseconds)} }, model.DefaultGlassBreakDuration), - GrantsSherlockSuperAdmin: model.GrantsSherlockSuperAdmin, - GrantsDevFirecloudGroup: model.GrantsDevFirecloudGroup, - GrantsQaFirecloudGroup: model.GrantsQaFirecloudGroup, - GrantsProdFirecloudGroup: model.GrantsProdFirecloudGroup, - GrantsDevAzureGroup: model.GrantsDevAzureGroup, - GrantsProdAzureGroup: model.GrantsProdAzureGroup, - GrantsBroadInstituteGroup: model.GrantsBroadInstituteGroup, + GrantsSherlockSuperAdmin: model.GrantsSherlockSuperAdmin, + GrantsDevFirecloudGroup: model.GrantsDevFirecloudGroup, + GrantsQaFirecloudGroup: model.GrantsQaFirecloudGroup, + GrantsProdFirecloudGroup: model.GrantsProdFirecloudGroup, + GrantsDevFirecloudFolderOwner: model.GrantsDevFirecloudFolderOwner, + GrantsQaFirecloudFolderOwner: model.GrantsQaFirecloudFolderOwner, + GrantsProdFirecloudFolderOwner: model.GrantsProdFirecloudFolderOwner, + GrantsDevAzureGroup: model.GrantsDevAzureGroup, + GrantsProdAzureGroup: model.GrantsProdAzureGroup, + GrantsBroadInstituteGroup: model.GrantsBroadInstituteGroup, }, } if len(model.Assignments) > 0 { diff --git a/sherlock/internal/api/sherlock/roles_v3_test.go b/sherlock/internal/api/sherlock/roles_v3_test.go index 629b4a354..25f5feec1 100644 --- a/sherlock/internal/api/sherlock/roles_v3_test.go +++ b/sherlock/internal/api/sherlock/roles_v3_test.go @@ -15,18 +15,21 @@ func (s *handlerSuite) Test_roleFromModel() { UpdatedAt: model.UpdatedAt, }, RoleV3Edit: RoleV3Edit{ - Name: model.Name, - SuspendNonSuitableUsers: model.SuspendNonSuitableUsers, - AutoAssignAllUsers: model.AutoAssignAllUsers, - CanBeGlassBrokenByRole: model.CanBeGlassBrokenByRoleID, - DefaultGlassBreakDuration: nil, - GrantsSherlockSuperAdmin: model.GrantsSherlockSuperAdmin, - GrantsDevFirecloudGroup: model.GrantsDevFirecloudGroup, - GrantsQaFirecloudGroup: model.GrantsQaFirecloudGroup, - GrantsProdFirecloudGroup: model.GrantsProdFirecloudGroup, - GrantsDevAzureGroup: model.GrantsDevAzureGroup, - GrantsProdAzureGroup: model.GrantsProdAzureGroup, - GrantsBroadInstituteGroup: model.GrantsBroadInstituteGroup, + Name: model.Name, + SuspendNonSuitableUsers: model.SuspendNonSuitableUsers, + AutoAssignAllUsers: model.AutoAssignAllUsers, + CanBeGlassBrokenByRole: model.CanBeGlassBrokenByRoleID, + DefaultGlassBreakDuration: nil, + GrantsSherlockSuperAdmin: model.GrantsSherlockSuperAdmin, + GrantsDevFirecloudGroup: model.GrantsDevFirecloudGroup, + GrantsQaFirecloudGroup: model.GrantsQaFirecloudGroup, + GrantsProdFirecloudGroup: model.GrantsProdFirecloudGroup, + GrantsDevFirecloudFolderOwner: model.GrantsDevFirecloudFolderOwner, + GrantsQaFirecloudFolderOwner: model.GrantsQaFirecloudFolderOwner, + GrantsProdFirecloudFolderOwner: model.GrantsProdFirecloudFolderOwner, + GrantsDevAzureGroup: model.GrantsDevAzureGroup, + GrantsProdAzureGroup: model.GrantsProdAzureGroup, + GrantsBroadInstituteGroup: model.GrantsBroadInstituteGroup, }, } role := roleFromModel(model) diff --git a/sherlock/internal/models/role.go b/sherlock/internal/models/role.go index 7418e41ed..ffcc9ba69 100644 --- a/sherlock/internal/models/role.go +++ b/sherlock/internal/models/role.go @@ -43,23 +43,48 @@ type RoleFields struct { // GrantsDevFirecloudGroup, when not null, indicates that a User with an unsuspended RoleAssignment to this // Role should have their dev Firecloud account (if they have one) added to this group. + // Multiple values can be comma separated and whitespace will be trimmed. GrantsDevFirecloudGroup *string // GrantsQaFirecloudGroup, when not null, indicates that a User with an unsuspended RoleAssignment to this // Role should have their qa Firecloud account (if they have one) added to this group. + // Multiple values can be comma separated and whitespace will be trimmed. GrantsQaFirecloudGroup *string // GrantsProdFirecloudGroup, when not null, indicates that a User with an unsuspended RoleAssignment to this // Role should have their prod Firecloud account (if they have one) added to this group. + // Multiple values can be comma separated and whitespace will be trimmed. GrantsProdFirecloudGroup *string + // GrantsDevFirecloudFolderOwner, when not null, indicates that a User with an unsuspended RoleAssignment to this + // Role should have their dev Firecloud account (if they have one) granted the GCP IAM Owner role on this folder. + // The folder doesn't need to actually be within the dev Firecloud organization -- that just indicates what account + // will get the access. + // Multiple values can be comma separated and whitespace will be trimmed. + GrantsDevFirecloudFolderOwner *string + // GrantsQaFirecloudFolderOwner, when not null, indicates that a User with an unsuspended RoleAssignment to this + // Role should have their qa Firecloud account (if they have one) granted the GCP IAM Owner role on this folder. + // The folder doesn't need to actually be within the qa Firecloud organization -- that just indicates what account + // will get the access. + // Multiple values can be comma separated and whitespace will be trimmed. + GrantsQaFirecloudFolderOwner *string + // GrantsProdFirecloudFolderOwner, when not null, indicates that a User with an unsuspended RoleAssignment to this + // Role should have their prod Firecloud account (if they have one) granted the GCP IAM Owner role on this folder. + // The folder doesn't need to actually be within the prod Firecloud organization -- that just indicates what account + // will get the access. + // Multiple values can be comma separated and whitespace will be trimmed. + GrantsProdFirecloudFolderOwner *string + // 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. GrantsDevAzureGroup *string // GrantsProdAzureGroup, 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. GrantsProdAzureGroup *string // 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. GrantsBroadInstituteGroup *string } diff --git a/sherlock/internal/models/test_data.go b/sherlock/internal/models/test_data.go index 4bfdfacae..e32aa1f82 100644 --- a/sherlock/internal/models/test_data.go +++ b/sherlock/internal/models/test_data.go @@ -357,15 +357,18 @@ func (td *testDataImpl) Role_TerraSuitableEngineer() Role { if td.role_terraSuitableEngineer.ID == 0 { td.role_terraSuitableEngineer = Role{ RoleFields: RoleFields{ - Name: utils.PointerTo("terra-suitable-engineer"), - SuspendNonSuitableUsers: utils.PointerTo(true), - AutoAssignAllUsers: utils.PointerTo(false), - GrantsDevFirecloudGroup: utils.PointerTo("terra-suitable-engineer-dev"), - GrantsQaFirecloudGroup: utils.PointerTo("terra-suitable-engineer-qa"), - GrantsProdFirecloudGroup: utils.PointerTo("terra-suitable-engineer-prod"), - GrantsDevAzureGroup: utils.PointerTo("00000000-0000-0000-0000-000000000001"), - GrantsProdAzureGroup: utils.PointerTo("00000000-0000-0000-0000-000000000002"), - GrantsBroadInstituteGroup: utils.PointerTo("terra-suitable-engineer-broadinstitute"), + Name: utils.PointerTo("terra-suitable-engineer"), + SuspendNonSuitableUsers: utils.PointerTo(true), + AutoAssignAllUsers: utils.PointerTo(false), + GrantsDevFirecloudGroup: utils.PointerTo("terra-suitable-engineer-dev"), + GrantsQaFirecloudGroup: utils.PointerTo("terra-suitable-engineer-qa"), + GrantsProdFirecloudGroup: utils.PointerTo("terra-suitable-engineer-prod"), + GrantsDevFirecloudFolderOwner: utils.PointerTo("folders/dev123"), + GrantsQaFirecloudFolderOwner: utils.PointerTo("folders/qa456"), + GrantsProdFirecloudFolderOwner: utils.PointerTo("folders/prod789"), + GrantsDevAzureGroup: utils.PointerTo("00000000-0000-0000-0000-000000000001"), + GrantsProdAzureGroup: utils.PointerTo("00000000-0000-0000-0000-000000000002"), + GrantsBroadInstituteGroup: utils.PointerTo("terra-suitable-engineer-broadinstitute"), }, } td.h.SetSelfSuperAdminForDB() diff --git a/sherlock/internal/role_propagation/boot.go b/sherlock/internal/role_propagation/boot.go index e4806c4c4..30157768f 100644 --- a/sherlock/internal/role_propagation/boot.go +++ b/sherlock/internal/role_propagation/boot.go @@ -6,6 +6,8 @@ import ( "github.com/broadinstitute/sherlock/sherlock/internal/role_propagation/propagation_engines" ) +// propagators will run in sequence. Use parallelizingPropagator instances to +// make parallel steps. var propagators []propagator // Init sets up the propagators to be used during normal operation. They will @@ -14,37 +16,64 @@ var propagators []propagator func Init(ctx context.Context) error { propagators = []propagator{ - &propagatorImpl[string, propagation_engines.GoogleWorkspaceGroupIdentifier, propagation_engines.GoogleWorkspaceGroupFields]{ - configKey: "devFirecloudGroup", - getGrant: func(role models.Role) *string { return role.GrantsDevFirecloudGroup }, - engine: &propagation_engines.GoogleWorkspaceGroupEngine{}, - }, - &propagatorImpl[string, propagation_engines.GoogleWorkspaceGroupIdentifier, propagation_engines.GoogleWorkspaceGroupFields]{ - configKey: "qaFirecloudGroup", - getGrant: func(role models.Role) *string { return role.GrantsQaFirecloudGroup }, - engine: &propagation_engines.GoogleWorkspaceGroupEngine{}, - }, - &propagatorImpl[string, propagation_engines.GoogleWorkspaceGroupIdentifier, propagation_engines.GoogleWorkspaceGroupFields]{ - configKey: "prodFirecloudGroup", - getGrant: func(role models.Role) *string { return role.GrantsProdFirecloudGroup }, - engine: &propagation_engines.GoogleWorkspaceGroupEngine{}, - }, + // 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. + ¶llelizingPropagator{ + parallelPropagators: []propagator{ + &propagatorImpl[string, propagation_engines.GoogleWorkspaceGroupIdentifier, propagation_engines.GoogleWorkspaceGroupFields]{ + configKey: "devFirecloudGroup", + getGrants: func(role models.Role) []*string { return splitStringPointerOnCommas(role.GrantsDevFirecloudGroup) }, + engine: &propagation_engines.GoogleWorkspaceGroupEngine{}, + }, + &propagatorImpl[string, propagation_engines.GoogleWorkspaceGroupIdentifier, propagation_engines.GoogleWorkspaceGroupFields]{ + configKey: "qaFirecloudGroup", + getGrants: func(role models.Role) []*string { return splitStringPointerOnCommas(role.GrantsQaFirecloudGroup) }, + engine: &propagation_engines.GoogleWorkspaceGroupEngine{}, + }, + &propagatorImpl[string, propagation_engines.GoogleWorkspaceGroupIdentifier, propagation_engines.GoogleWorkspaceGroupFields]{ + configKey: "prodFirecloudGroup", + getGrants: func(role models.Role) []*string { return splitStringPointerOnCommas(role.GrantsProdFirecloudGroup) }, + engine: &propagation_engines.GoogleWorkspaceGroupEngine{}, + }, - &propagatorImpl[string, propagation_engines.AzureGroupIdentifier, propagation_engines.AzureGroupFields]{ - configKey: "devAzureGroup", - getGrant: func(role models.Role) *string { return role.GrantsDevAzureGroup }, - engine: &propagation_engines.AzureGroupEngine{}, - }, - &propagatorImpl[string, propagation_engines.AzureGroupIdentifier, propagation_engines.AzureGroupFields]{ - configKey: "prodAzureGroup", - getGrant: func(role models.Role) *string { return role.GrantsProdAzureGroup }, - engine: &propagation_engines.AzureGroupEngine{}, - }, + &propagatorImpl[string, propagation_engines.GoogleWorkspaceFolderOwnerIdentifier, propagation_engines.GoogleWorkspaceFolderOwnerFields]{ + configKey: "devFirecloudFolderOwner", + getGrants: func(role models.Role) []*string { + return splitStringPointerOnCommas(role.GrantsDevFirecloudFolderOwner) + }, + engine: &propagation_engines.GoogleWorkspaceFolderOwnerEngine{}, + }, + &propagatorImpl[string, propagation_engines.GoogleWorkspaceFolderOwnerIdentifier, propagation_engines.GoogleWorkspaceFolderOwnerFields]{ + configKey: "qaFirecloudFolderOwner", + getGrants: func(role models.Role) []*string { return splitStringPointerOnCommas(role.GrantsQaFirecloudFolderOwner) }, + engine: &propagation_engines.GoogleWorkspaceFolderOwnerEngine{}, + }, + &propagatorImpl[string, propagation_engines.GoogleWorkspaceFolderOwnerIdentifier, propagation_engines.GoogleWorkspaceFolderOwnerFields]{ + configKey: "prodFirecloudFolderOwner", + getGrants: func(role models.Role) []*string { + return splitStringPointerOnCommas(role.GrantsProdFirecloudFolderOwner) + }, + engine: &propagation_engines.GoogleWorkspaceFolderOwnerEngine{}, + }, + + &propagatorImpl[string, propagation_engines.AzureGroupIdentifier, propagation_engines.AzureGroupFields]{ + configKey: "devAzureGroup", + getGrants: func(role models.Role) []*string { return splitStringPointerOnCommas(role.GrantsDevAzureGroup) }, + engine: &propagation_engines.AzureGroupEngine{}, + }, + &propagatorImpl[string, propagation_engines.AzureGroupIdentifier, propagation_engines.AzureGroupFields]{ + configKey: "prodAzureGroup", + getGrants: func(role models.Role) []*string { return splitStringPointerOnCommas(role.GrantsProdAzureGroup) }, + engine: &propagation_engines.AzureGroupEngine{}, + }, - &propagatorImpl[string, propagation_engines.NonAdminGoogleGroupIdentifier, propagation_engines.NonAdminGoogleGroupFields]{ - configKey: "broadInstituteGroup", - getGrant: func(role models.Role) *string { return role.GrantsBroadInstituteGroup }, - engine: &propagation_engines.NonAdminGoogleGroupEngine{}, + &propagatorImpl[string, propagation_engines.NonAdminGoogleGroupIdentifier, propagation_engines.NonAdminGoogleGroupFields]{ + configKey: "broadInstituteGroup", + getGrants: func(role models.Role) []*string { return splitStringPointerOnCommas(role.GrantsBroadInstituteGroup) }, + engine: &propagation_engines.NonAdminGoogleGroupEngine{}, + }, + }, }, } for _, p := range propagators { diff --git a/sherlock/internal/role_propagation/parallelizing_propagator.go b/sherlock/internal/role_propagation/parallelizing_propagator.go new file mode 100644 index 000000000..50e976f4d --- /dev/null +++ b/sherlock/internal/role_propagation/parallelizing_propagator.go @@ -0,0 +1,75 @@ +package role_propagation + +import ( + "context" + "fmt" + "github.com/broadinstitute/sherlock/go-shared/pkg/utils" + "github.com/broadinstitute/sherlock/sherlock/internal/models" + "sync" +) + +var _ propagator = ¶llelizingPropagator{} + +// parallelizingPropagator wraps any number of other propagator instances and allows them to run in parallel. +// The base-level behavior of this package is to run the propagators in order, sequentially. This is helpful +// because one propagator might make an account that a later propagator might grant other permissions to (or +// something like that). +// +// In other cases, though, it's perfectly safe to grant different kinds of permissions at once. To help with +// propagation delay, this special propagator creates a sort of concurrent step in the process. +// +// This is indeed an extra layer of indirection, and spawning goroutines etc. isn't free. I (Jack) am +// operating on the notion that a Go codebase is going to be orders of magnitude faster doing those things +// than the average return time of a cloud provider API call. +type parallelizingPropagator struct { + parallelPropagators []propagator +} + +// LogPrefix for parallelizingPropagator returns an empty string because our own Propagate method is what +// gets the real log prefixes from the real inner propagators. +func (p *parallelizingPropagator) LogPrefix() string { + return "" +} + +// Init for parallelizingPropagator runs sequentially for simplicity and predictability (because it only +// runs once at startup). +func (p *parallelizingPropagator) Init(ctx context.Context) error { + for _, inner := range p.parallelPropagators { + if err := inner.Init(ctx); err != nil { + return err + } + } + return nil +} + +// Propagate for parallelizingPropagator is very similar to doNonConcurrentPropagation except that it +// runs the inner propagators in parallel goroutines. +// +// There's a conscious choice to not bother with stable ordering for the return values -- it'll +// kinda be sorted by whichever inner propagators finish first. That's fine enough and potentially +// helpful for debugging. Easier to live with that than write a bunch of code to output them in the +// order the goroutines were kicked off in. +func (p *parallelizingPropagator) Propagate(ctx context.Context, role models.Role) (results []string, errors []error) { + results = make([]string, 0) + errors = make([]error, 0) + var returnValueMutex sync.Mutex + var wg sync.WaitGroup + for _, unsafeInner := range p.parallelPropagators { + inner := unsafeInner + wg.Add(1) + go func() { + defer wg.Done() + additionalResults, additionalErrors := inner.Propagate(ctx, role) + returnValueMutex.Lock() + results = append(results, utils.Map(additionalResults, func(result string) string { + return inner.LogPrefix() + result + })...) + errors = append(errors, utils.Map(additionalErrors, func(err error) error { + return fmt.Errorf("%s%w", inner.LogPrefix(), err) + })...) + returnValueMutex.Unlock() + }() + } + wg.Wait() + return results, errors +} diff --git a/sherlock/internal/role_propagation/parallelizing_propagator_test.go b/sherlock/internal/role_propagation/parallelizing_propagator_test.go new file mode 100644 index 000000000..b083c9364 --- /dev/null +++ b/sherlock/internal/role_propagation/parallelizing_propagator_test.go @@ -0,0 +1,49 @@ +package role_propagation + +import ( + "context" + "fmt" + "github.com/broadinstitute/sherlock/go-shared/pkg/utils" + "github.com/broadinstitute/sherlock/sherlock/internal/models" + "github.com/broadinstitute/sherlock/sherlock/internal/role_propagation/role_propagation_mocks" +) + +func (s *propagateSuite) Test_parallelizingPropagator_LogPrefix() { + p := ¶llelizingPropagator{ + parallelPropagators: []propagator{ + role_propagation_mocks.NewMockPropagator(s.T()), + role_propagation_mocks.NewMockPropagator(s.T()), + }, + } + s.Equal("", p.LogPrefix()) +} + +func (s *propagateSuite) Test_parallelizingPropagator_Init() { + ctx := context.Background() + prop1 := role_propagation_mocks.NewMockPropagator(s.T()) + prop1.EXPECT().Init(ctx).Return(nil).Once() + prop2 := role_propagation_mocks.NewMockPropagator(s.T()) + prop2.EXPECT().Init(ctx).Return(nil).Once() + p := ¶llelizingPropagator{ + parallelPropagators: []propagator{prop1, prop2}, + } + s.NoError(p.Init(ctx)) +} + +func (s *propagateSuite) Test_parallelizingPropagator_Propagate() { + ctx := context.Background() + role := models.Role{} + prop1 := role_propagation_mocks.NewMockPropagator(s.T()) + prop1.EXPECT().Propagate(ctx, role).Return([]string{"result1"}, []error{fmt.Errorf("error1")}).Once() + prop1.EXPECT().LogPrefix().Return("prefix1: ") + prop2 := role_propagation_mocks.NewMockPropagator(s.T()) + prop2.EXPECT().Propagate(ctx, role).Return([]string{"result2"}, []error{fmt.Errorf("error2")}).Once() + prop2.EXPECT().LogPrefix().Return("prefix2: ") + p := ¶llelizingPropagator{ + parallelPropagators: []propagator{prop1, prop2}, + } + results, errors := p.Propagate(ctx, role) + errorStrings := utils.Map(errors, func(e error) string { return e.Error() }) + s.ElementsMatch([]string{"prefix1: result1", "prefix2: result2"}, results) + s.ElementsMatch([]string{"prefix1: error1", "prefix2: error2"}, errorStrings) +} diff --git a/sherlock/internal/role_propagation/propagate.go b/sherlock/internal/role_propagation/propagate.go index 7af1bd6ab..93eaba1d7 100644 --- a/sherlock/internal/role_propagation/propagate.go +++ b/sherlock/internal/role_propagation/propagate.go @@ -3,6 +3,7 @@ package role_propagation import ( "context" "fmt" + "github.com/broadinstitute/sherlock/go-shared/pkg/utils" "github.com/broadinstitute/sherlock/sherlock/internal/clients/slack" "github.com/broadinstitute/sherlock/sherlock/internal/config" "github.com/broadinstitute/sherlock/sherlock/internal/models" @@ -134,8 +135,12 @@ func doNonConcurrentPropagation(ctx context.Context, role models.Role) { errors := make([]error, 0) for _, p := range propagators { additionalResults, additionalErrors := p.Propagate(ctx, role) - results = append(results, additionalResults...) - errors = append(errors, additionalErrors...) + results = append(results, utils.Map(additionalResults, func(result string) string { + return p.LogPrefix() + result + })...) + errors = append(errors, utils.Map(additionalErrors, func(err error) error { + return fmt.Errorf("%s%w", p.LogPrefix(), err) + })...) } if len(results) > 0 || len(errors) > 0 { slack.SendPermissionChangeNotification(ctx, models.SelfUser.SlackReference(true), slack.PermissionChangeNotificationInputs{ diff --git a/sherlock/internal/role_propagation/propagation_engines/google_workspace_folder_owner.go b/sherlock/internal/role_propagation/propagation_engines/google_workspace_folder_owner.go new file mode 100644 index 000000000..523bd315a --- /dev/null +++ b/sherlock/internal/role_propagation/propagation_engines/google_workspace_folder_owner.go @@ -0,0 +1,190 @@ +package propagation_engines + +import ( + "cloud.google.com/go/iam/apiv1/iampb" + resourcemanager "cloud.google.com/go/resourcemanager/apiv3" + "context" + "fmt" + "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" + admin "google.golang.org/api/admin/directory/v1" + "google.golang.org/api/option" + "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" + +type GoogleWorkspaceFolderOwnerIdentifier struct { + Email string `koanf:"email"` +} + +func (g GoogleWorkspaceFolderOwnerIdentifier) EqualTo(other intermediary_user.Identifier) bool { + switch other := other.(type) { + case GoogleWorkspaceFolderOwnerIdentifier: + return g.Email == other.Email + default: + return false + } +} + +type GoogleWorkspaceFolderOwnerFields struct{} + +func (g GoogleWorkspaceFolderOwnerFields) EqualTo(other intermediary_user.Fields) bool { + switch other.(type) { + case GoogleWorkspaceFolderOwnerFields: + return true + default: + return false + } +} + +var _ PropagationEngine[string, GoogleWorkspaceFolderOwnerIdentifier, GoogleWorkspaceFolderOwnerFields] = &GoogleWorkspaceFolderOwnerEngine{} + +// GoogleWorkspaceFolderOwnerEngine 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 { + workspaceDomain string + userEmailSuffixesToReplace []string + adminService *admin.Service + foldersClient *resourcemanager.FoldersClient +} + +func (g *GoogleWorkspaceFolderOwnerEngine) Init(ctx context.Context, k *koanf.Koanf) error { + g.workspaceDomain = k.String("workspaceDomain") + g.userEmailSuffixesToReplace = k.Strings("userEmailSuffixesToReplace") + var err error + g.adminService, err = admin.NewService(ctx, option.WithScopes(admin.AdminDirectoryUserReadonlyScope)) + if err != nil { + return err + } + g.foldersClient, err = resourcemanager.NewFoldersClient(ctx) + return err +} + +func (g *GoogleWorkspaceFolderOwnerEngine) LoadCurrentState(ctx context.Context, grant string) ([]intermediary_user.IntermediaryUser[GoogleWorkspaceFolderOwnerIdentifier, GoogleWorkspaceFolderOwnerFields], 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) + for _, binding := range policy.Bindings { + if binding.Role == ownerRole { + 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{}, + }) + } + } + } + } + return currentState, nil + } +} + +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]) + 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 + continue + } + + email := utils.SubstituteSuffix(roleAssignment.User.Email, g.userEmailSuffixesToReplace, "@"+g.workspaceDomain) + if !strings.HasSuffix(email, "@"+g.workspaceDomain) { + // We can short-circuit here, we know that the user is not in the workspace domain so we won't bother looking + continue + } + + err := g.adminService.Users.List(). + Domain(g.workspaceDomain). + Query("email="+email). + Fields("users(primaryEmail)"). + MaxResults(1). + 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{}, + } + } + } + return nil + }) + if err != nil { + return nil, err + } + } + return desiredState, nil +} + +func (g *GoogleWorkspaceFolderOwnerEngine) Add(ctx context.Context, grant string, identifier GoogleWorkspaceFolderOwnerIdentifier, fields GoogleWorkspaceFolderOwnerFields) (string, error) { + policy, err := g.foldersClient.GetIamPolicy(ctx, &iampb.GetIamPolicyRequest{ + Resource: grant, + }) + if err != nil { + return "", err + } + + for _, binding := range policy.Bindings { + if binding.Role == ownerRole && binding.Condition == nil { + binding.Members = append(binding.Members, "user:"+identifier.Email) + } + } + + _, err = g.foldersClient.SetIamPolicy(ctx, &iampb.SetIamPolicyRequest{ + Resource: grant, + Policy: policy, + }) + if err != nil { + return "", fmt.Errorf("failed to grant %s %s on %s: %w", identifier.Email, ownerRole, grant, err) + } else { + return fmt.Sprintf("granted %s %s on %s", identifier.Email, ownerRole, 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 *GoogleWorkspaceFolderOwnerEngine) Remove(ctx context.Context, grant string, identifier GoogleWorkspaceFolderOwnerIdentifier) (string, error) { + policy, err := g.foldersClient.GetIamPolicy(ctx, &iampb.GetIamPolicyRequest{ + Resource: grant, + }) + if err != nil { + return "", err + } + + for _, binding := range policy.Bindings { + if binding.Role == ownerRole && binding.Condition == nil { + members := make([]string, 0, len(binding.Members)-1) + for _, member := range binding.Members { + if member != "user:"+identifier.Email { + members = append(members, member) + } + } + binding.Members = members + } + } + + _, err = g.foldersClient.SetIamPolicy(ctx, &iampb.SetIamPolicyRequest{ + Resource: grant, + Policy: policy, + }) + if err != nil { + return "", fmt.Errorf("failed to revoke %s's %s on %s: %w", identifier.Email, ownerRole, grant, err) + } else { + return fmt.Sprintf("revoke %s's %s on %s", identifier.Email, ownerRole, 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_workspace_folder_owner_test.go new file mode 100644 index 000000000..cabb095b8 --- /dev/null +++ b/sherlock/internal/role_propagation/propagation_engines/google_workspace_folder_owner_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 TestGoogleWorkspaceFolderOwnerIdentifier_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: GoogleWorkspaceFolderOwnerIdentifier{ + Email: "foo", + }, + }, + want: true, + }, + { + name: "not equal", + fields: fields{ + Email: "foo", + }, + args: args{ + other: GoogleWorkspaceFolderOwnerIdentifier{ + Email: "bar", + }, + }, + want: false, + }, + { + name: "different type", + fields: fields{ + Email: "foo", + }, + args: args{ + other: AzureGroupIdentifier{ + ID: "foo", + }, + }, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + a := GoogleWorkspaceFolderOwnerIdentifier{ + Email: tt.fields.Email, + } + assert.Equalf(t, tt.want, a.EqualTo(tt.args.other), "EqualTo(%v)", tt.args.other) + }) + } +} + +func TestGoogleWorkspaceFolderOwnerFields_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: GoogleWorkspaceFolderOwnerFields{}, + }, + want: true, + }, + { + name: "different type", + args: args{ + other: AzureGroupFields{}, + }, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := GoogleWorkspaceFolderOwnerFields{} + assert.Equalf(t, tt.want, f.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 TestGoogleWorkspaceFolderOwnerEngine_GenerateDesiredState_isActiveShortCircuit(t *testing.T) { + engine := &GoogleWorkspaceFolderOwnerEngine{} + 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 TestGoogleWorkspaceFolderOwnerEngine_GenerateDesiredState_emailShortCircuit(t *testing.T) { + engine := &GoogleWorkspaceFolderOwnerEngine{ + workspaceDomain: "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 TestGoogleWorkspaceFolderOwnerEngine_Update_errors(t *testing.T) { + engine := &GoogleWorkspaceFolderOwnerEngine{} + _, err := engine.Update(context.Background(), "", GoogleWorkspaceFolderOwnerIdentifier{}, GoogleWorkspaceFolderOwnerFields{}, GoogleWorkspaceFolderOwnerFields{}) + assert.Error(t, err) +} diff --git a/sherlock/internal/role_propagation/propagator.go b/sherlock/internal/role_propagation/propagator.go index 313ae5cdb..7a3ddd6ef 100644 --- a/sherlock/internal/role_propagation/propagator.go +++ b/sherlock/internal/role_propagation/propagator.go @@ -13,8 +13,9 @@ import ( // use a `var propagators []propagatorImpl[any, any]` because Go doesn't support covariance like that -- we call // it a `var propagators []propagator` instead. type propagator interface { - // Name is how the propagator should be referenced in logs and alerts. - Name() string + // LogPrefix will be added before any individual operation results or errors when written to logs or alerts. + // This is helpful to indicate which propagator did what. + LogPrefix() string // Init loads configuration and initializes the engine (assuming the configuration doesn't say this // propagator is disabled). It should be called once at startup, and an error here should abort startup. Init(ctx context.Context) error @@ -29,6 +30,8 @@ type propagatorImpl[ // likely case is that it could be a boolean, if the propagator were meant to grant an actual account or something. // // A good rule of thumb for understanding this type is that it's however the grant is stored on the models.Role. + // It's okay for a models.Role to store multiple grants (like a list of groups rather than just one) -- the + // propagation system will be called individually for each grant. Grant any, // Identifier is a struct containing how the engine identifies users on the cloud provider. The key thing is that // the engine should be able to read this from the cloud provider, so that means "Sherlock user ID" should almost @@ -49,11 +52,14 @@ type propagatorImpl[ // configKey is used in rolePropagation.propagators. to load configuration for this propagatorImpl. configKey string - // getGrant reads the models.Role to tell us what we're trying to grant. See the Grant generic type for more info. - // A nil or empty return value means the models.Role doesn't have anything for us to grant. - getGrant func(role models.Role) *Grant + // getGrants reads the models.Role to tell us what we're trying to grant. See the Grant generic type for more info. + // + // Nil or empty values in the return list will be filtered out, and then an empty list will indicate that we have + // nothing to grant. + getGrants func(role models.Role) []*Grant // engine is the implementation of the cloud-specific logic for introspecting and adjusting the grant's state. + // If getGrants returns multiple grants, they'll each be given individually to the engine. engine propagation_engines.PropagationEngine[Grant, Identifier, Fields] // Fields below this point are used as state for the propagatorImpl, you're not meant to set them yourself. @@ -80,6 +86,6 @@ type propagatorImpl[ _toleratedUsers []Identifier } -func (p propagatorImpl[Grant, Identifier, Fields]) Name() string { - return p.configKey +func (p *propagatorImpl[Grant, Identifier, Fields]) LogPrefix() string { + return p.configKey + ": " } diff --git a/sherlock/internal/role_propagation/propagator_consume_states_to_diff.go b/sherlock/internal/role_propagation/propagator_calculate_alignment_operations.go similarity index 69% rename from sherlock/internal/role_propagation/propagator_consume_states_to_diff.go rename to sherlock/internal/role_propagation/propagator_calculate_alignment_operations.go index b7fcc8399..d39916318 100644 --- a/sherlock/internal/role_propagation/propagator_consume_states_to_diff.go +++ b/sherlock/internal/role_propagation/propagator_calculate_alignment_operations.go @@ -5,46 +5,52 @@ import ( "github.com/broadinstitute/sherlock/sherlock/internal/role_propagation/intermediary_user" ) -// consumeStatesToDiff returns a list of functions that, when called, will align the currentState with the -// desiredState. This function uses the state inputs as accumulators, so the inputs given to this function should not -// be used afterward. +// calculateAlignmentOperations returns a list of functions that, when called, will align the currentState with the +// desiredState. // // This function has a lot of loops that generate functions that will be called later. We explicitly initialize new // variables to store the values of the loop variables so that the functions generated will capture the correct values. // We just do this pattern everywhere in this function because it's always correct and it's easier to reason about. -func (p *propagatorImpl[Grant, Identifier, Fields]) consumeStatesToDiff( +func (p *propagatorImpl[Grant, Identifier, Fields]) calculateAlignmentOperations( ctx context.Context, grant Grant, currentState []intermediary_user.IntermediaryUser[Identifier, Fields], desiredState map[uint]intermediary_user.IntermediaryUser[Identifier, Fields], ) (alignmentOperations []func() (string, error)) { + // We don't want to mutate the state inputs we were given, so we assemble a copy of the desired state map to mess + // with as we go. + copyOfDesiredState := make(map[uint]intermediary_user.IntermediaryUser[Identifier, Fields]) + for userID, intermediaryUser := range desiredState { + copyOfDesiredState[userID] = intermediaryUser + } + currentlyGrantedUserLoop: for _, unsafeCurrentlyGrantedUser := range currentState { currentlyGrantedUser := unsafeCurrentlyGrantedUser - // Seek match from desiredState - for unsafeDesiredSherlockUserID, unsafeDesiredUser := range desiredState { + // Seek match from copyOfDesiredState + for unsafeDesiredSherlockUserID, unsafeDesiredUser := range copyOfDesiredState { desiredSherlockUserID := unsafeDesiredSherlockUserID desiredUser := unsafeDesiredUser if currentlyGrantedUser.Identifier.EqualTo(desiredUser.Identifier) { // Match! If fields are different we update; either way we move on to the next currently granted user. - // We actually remove the entry from desiredState so we know what's left over and needs to be added - // at the end. + // We actually remove the entry from copyOfDesiredState so we know what's left over and needs to be + // 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) }) } - delete(desiredState, desiredSherlockUserID) + delete(copyOfDesiredState, desiredSherlockUserID) continue currentlyGrantedUserLoop } } - // No match from desiredState! Let's seek a match in the users we are configured to tolerate. + // No match from copyOfDesiredState! Let's seek a match in the users we are configured to tolerate. for _, toleratedUser := range p._toleratedUsers { if currentlyGrantedUser.Identifier.EqualTo(toleratedUser) { // Match! Let's move on to the next currently granted user, we'll leave this one alone. @@ -59,7 +65,7 @@ currentlyGrantedUserLoop: } // If there are any desired users left, add them. - for _, unsafeDesiredUser := range desiredState { + for _, unsafeDesiredUser := range copyOfDesiredState { desiredUser := unsafeDesiredUser alignmentOperations = append(alignmentOperations, func() (string, error) { return p.engine.Add(ctx, grant, desiredUser.Identifier, desiredUser.Fields) diff --git a/sherlock/internal/role_propagation/propagator_consume_states_to_diff_test.go b/sherlock/internal/role_propagation/propagator_calculate_alignment_operations_test.go similarity index 85% rename from sherlock/internal/role_propagation/propagator_consume_states_to_diff_test.go rename to sherlock/internal/role_propagation/propagator_calculate_alignment_operations_test.go index 57bb0bfd0..228d6d483 100644 --- a/sherlock/internal/role_propagation/propagator_consume_states_to_diff_test.go +++ b/sherlock/internal/role_propagation/propagator_calculate_alignment_operations_test.go @@ -4,6 +4,7 @@ import ( "context" "github.com/broadinstitute/sherlock/sherlock/internal/role_propagation/intermediary_user" "github.com/broadinstitute/sherlock/sherlock/internal/role_propagation/propagation_engines/propagation_engines_mocks" + "github.com/stretchr/testify/assert" "testing" ) @@ -25,7 +26,7 @@ func (t testFields) EqualTo(other intermediary_user.Fields) bool { type testIntermediaryUser = intermediary_user.IntermediaryUser[testIdentifier, testFields] -func Test_propagatorImpl_consumeStatesToDiff(t *testing.T) { +func Test_propagatorImpl_calculateAlignmentOperations(t *testing.T) { engine := propagation_engines_mocks.NewMockPropagationEngine[string, testIdentifier, testFields](t) p := &propagatorImpl[string, testIdentifier, testFields]{ engine: engine, @@ -72,17 +73,24 @@ func Test_propagatorImpl_consumeStatesToDiff(t *testing.T) { desiredState[6] = testIntermediaryUser{Identifier: identifier6, Fields: fields6} engine.EXPECT().Add(ctx, grant, identifier6, fields6).Return("", nil).Once() - alignmentOperations := p.consumeStatesToDiff(ctx, grant, currentState, desiredState) + currentStateLen := len(currentState) + desiredStateLen := len(desiredState) + + 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() } + // 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_consumeStatesToDiff_empty(t *testing.T) { +func Test_propagatorImpl_calculateAlignmentOperations_empty(t *testing.T) { engine := propagation_engines_mocks.NewMockPropagationEngine[string, testIdentifier, testFields](t) p := &propagatorImpl[string, testIdentifier, testFields]{ engine: engine, @@ -90,7 +98,7 @@ func Test_propagatorImpl_consumeStatesToDiff_empty(t *testing.T) { // This is a sorta dumb test but we're checking that we don't somehow fail on empty inputs -- everything else // is pretty thoroughly covered by the main test above - alignmentOperations := p.consumeStatesToDiff(context.Background(), "", nil, nil) + alignmentOperations := p.calculateAlignmentOperations(context.Background(), "", nil, nil) for _, alignmentOperation := range alignmentOperations { _, _ = alignmentOperation() } diff --git a/sherlock/internal/role_propagation/propagator_get_and_filter_grants.go b/sherlock/internal/role_propagation/propagator_get_and_filter_grants.go new file mode 100644 index 000000000..e07a7a5ce --- /dev/null +++ b/sherlock/internal/role_propagation/propagator_get_and_filter_grants.go @@ -0,0 +1,18 @@ +package role_propagation + +import ( + "github.com/broadinstitute/sherlock/sherlock/internal/models" + "reflect" +) + +func (p *propagatorImpl[Grant, Identifier, Fields]) getAndFilterGrants(role models.Role) []Grant { + filteredGrants := make([]Grant, 0) + if unfilteredGrants := p.getGrants(role); unfilteredGrants != nil { + for _, grantPointer := range unfilteredGrants { + if grantPointer != nil && !reflect.ValueOf(*grantPointer).IsZero() { + filteredGrants = append(filteredGrants, *grantPointer) + } + } + } + return filteredGrants +} diff --git a/sherlock/internal/role_propagation/propagator_get_and_filter_grants_test.go b/sherlock/internal/role_propagation/propagator_get_and_filter_grants_test.go new file mode 100644 index 000000000..531394f0b --- /dev/null +++ b/sherlock/internal/role_propagation/propagator_get_and_filter_grants_test.go @@ -0,0 +1,66 @@ +package role_propagation + +import ( + "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/intermediary_user_mocks" + "github.com/stretchr/testify/assert" + "testing" +) + +func Test_propagatorImpl_getAndFilterGrants(t *testing.T) { + type testCase struct { + name string + grantFromRole []*string + wantGrants []string + } + tests := []testCase{ + { + name: "nil", + grantFromRole: nil, + wantGrants: []string{}, + }, + { + name: "empty", + grantFromRole: []*string{}, + wantGrants: []string{}, + }, + { + name: "slice with nil", + grantFromRole: []*string{nil}, + wantGrants: []string{}, + }, + { + name: "slice with empty", + grantFromRole: []*string{utils.PointerTo("")}, + wantGrants: []string{}, + }, + { + name: "slice with non-empty", + grantFromRole: []*string{utils.PointerTo("string")}, + wantGrants: []string{"string"}, + }, + { + name: "slice with nil, empty, and non-empty", + grantFromRole: []*string{nil, utils.PointerTo(""), utils.PointerTo("string")}, + wantGrants: []string{"string"}, + }, + { + name: "preserves order", + grantFromRole: []*string{utils.PointerTo("string1"), nil, utils.PointerTo("string2"), utils.PointerTo(""), utils.PointerTo("string3")}, + wantGrants: []string{"string1", "string2", "string3"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + p := propagatorImpl[string, *intermediary_user_mocks.MockIdentifier, *intermediary_user_mocks.MockFields]{ + getGrants: func(_ models.Role) []*string { + return tt.grantFromRole + }, + } + gotGrants := p.getAndFilterGrants(models.Role{}) + assert.Equal(t, tt.wantGrants, gotGrants) + }) + } +} diff --git a/sherlock/internal/role_propagation/propagator_propagate.go b/sherlock/internal/role_propagation/propagator_propagate.go index cfec7dbd4..d36bccc01 100644 --- a/sherlock/internal/role_propagation/propagator_propagate.go +++ b/sherlock/internal/role_propagation/propagator_propagate.go @@ -7,6 +7,7 @@ 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" + "sync" ) func (p *propagatorImpl[Grant, Identifier, Fields]) Propagate(ctx context.Context, role models.Role) (results []string, errors []error) { @@ -20,37 +21,70 @@ func (p *propagatorImpl[Grant, Identifier, Fields]) Propagate(ctx context.Contex return nil, nil } - timeoutCtx, cancel := context.WithTimeout(ctx, p._timeout) - defer cancel() - - shouldPropagate, grant := p.shouldPropagate(role) - if !shouldPropagate { + grantsToPropagate := p.getAndFilterGrants(role) + if len(grantsToPropagate) == 0 { return nil, nil } - currentState, err := retry.DoWithData(func() ([]intermediary_user.IntermediaryUser[Identifier, Fields], error) { - return p.engine.LoadCurrentState(timeoutCtx, grant) - }, config.RetryOptions...) - if err != nil { - return nil, []error{fmt.Errorf("failed to load current state for grant %v: %w", grant, err)} - } + timeoutCtx, cancel := context.WithTimeout(ctx, p._timeout) + defer cancel() + retryOptionsWithTimeoutCtx := append(config.RetryOptions, retry.Context(timeoutCtx)) - desiredState, err := retry.DoWithData(func() (map[uint]intermediary_user.IntermediaryUser[Identifier, Fields], error) { + // The desired state will be the same across all grants, so we generate it once + desiredState, desiredStateErr := retry.DoWithData(func() (map[uint]intermediary_user.IntermediaryUser[Identifier, Fields], error) { return p.engine.GenerateDesiredState(timeoutCtx, role.AssignmentsMap()) - }, config.RetryOptions...) - if err != nil { - return nil, []error{fmt.Errorf("failed to generate desired state for grant %v: %w", grant, err)} + }, retryOptionsWithTimeoutCtx...) + if desiredStateErr != nil { + return nil, []error{fmt.Errorf("failed to generate desired state: %w", desiredStateErr)} } - alignmentOperations := p.consumeStatesToDiff(timeoutCtx, grant, currentState, desiredState) + // For each grant, the current state and the operations to match the desired state could be different, so + // we process them separately. We parallelize to avoid runtime scaling linearly with the number of grants + // (since we have a timeout in our context and there's nothing scaling that to our role that we're + // working with). + var wg sync.WaitGroup + resultsPerGrant := make([][]string, len(grantsToPropagate)) + errorsPerGrant := make([][]error, len(grantsToPropagate)) + for unsafeGrantIndex, unsafeGrant := range grantsToPropagate { + grantIndex := unsafeGrantIndex + grant := unsafeGrant + resultsPerGrant[grantIndex] = make([]string, 0) + errorsPerGrant[grantIndex] = make([]error, 0) + wg.Add(1) + go func() { + defer wg.Done() + defer func() { + if r := recover(); r != nil { + errorsPerGrant[grantIndex] = append(errorsPerGrant[grantIndex], fmt.Errorf("panic in %T during propagation for grant %v: %v", p, grant, r)) + } + }() + currentState, currentStateErr := retry.DoWithData(func() ([]intermediary_user.IntermediaryUser[Identifier, Fields], error) { + return p.engine.LoadCurrentState(timeoutCtx, grant) + }, retryOptionsWithTimeoutCtx...) + if currentStateErr != nil { + errorsPerGrant[grantIndex] = append(errorsPerGrant[grantIndex], fmt.Errorf("failed to load current state for grant %v: %w", grant, currentStateErr)) + return + } - for _, alignmentOperation := range alignmentOperations { - result, err := retry.DoWithData(alignmentOperation, config.RetryOptions...) - if err != nil { - errors = append(errors, fmt.Errorf("%s: %w", p.Name(), err)) - } else { - results = append(results, fmt.Sprintf("%s: %s", p.Name(), result)) - } + alignmentOperations := p.calculateAlignmentOperations(timeoutCtx, grant, currentState, desiredState) + + for _, alignmentOperation := range alignmentOperations { + operationResult, operationErr := retry.DoWithData(alignmentOperation, retryOptionsWithTimeoutCtx...) + if operationErr != nil { + errorsPerGrant[grantIndex] = append(errorsPerGrant[grantIndex], operationErr) + } else { + resultsPerGrant[grantIndex] = append(resultsPerGrant[grantIndex], operationResult) + } + } + }() + } + wg.Wait() + + for _, resultsForGrant := range resultsPerGrant { + results = append(results, resultsForGrant...) + } + for _, errorsForGrant := range errorsPerGrant { + errors = append(errors, errorsForGrant...) } return results, errors diff --git a/sherlock/internal/role_propagation/propagator_propagate_test.go b/sherlock/internal/role_propagation/propagator_propagate_test.go index b2b0b7b77..37c3a22d4 100644 --- a/sherlock/internal/role_propagation/propagator_propagate_test.go +++ b/sherlock/internal/role_propagation/propagator_propagate_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "slices" + "strings" "testing" "time" ) @@ -19,10 +20,38 @@ import ( func Test_propagatorImpl_Propagate_panic(t *testing.T) { config.LoadTestConfig() engine := propagation_engines_mocks.NewMockPropagationEngine[string, propagation_engines.GoogleWorkspaceGroupIdentifier, propagation_engines.GoogleWorkspaceGroupFields](t) + engine.EXPECT().GenerateDesiredState(mock.Anything, mock.Anything).Panic("panic").Once() + p := propagatorImpl[string, propagation_engines.GoogleWorkspaceGroupIdentifier, propagation_engines.GoogleWorkspaceGroupFields]{ + getGrants: func(role models.Role) []*string { + return []*string{role.GrantsDevFirecloudGroup} + }, + engine: engine, + _enable: true, + _timeout: time.Minute, + } + var results []string + var errors []error + assert.NotPanics(t, func() { + results, errors = p.Propagate(context.Background(), models.Role{ + RoleFields: models.RoleFields{ + GrantsDevFirecloudGroup: utils.PointerTo("string"), + }, + }) + }) + assert.Empty(t, results) + if assert.Len(t, errors, 1) { + assert.ErrorContains(t, errors[0], "panic") + } +} + +func Test_propagatorImpl_Propagate_panicOnGrant(t *testing.T) { + config.LoadTestConfig() + engine := propagation_engines_mocks.NewMockPropagationEngine[string, propagation_engines.GoogleWorkspaceGroupIdentifier, propagation_engines.GoogleWorkspaceGroupFields](t) + engine.EXPECT().GenerateDesiredState(mock.Anything, mock.Anything).Return(nil, nil).Once() engine.EXPECT().LoadCurrentState(mock.Anything, mock.Anything).Panic("panic").Once() p := propagatorImpl[string, propagation_engines.GoogleWorkspaceGroupIdentifier, propagation_engines.GoogleWorkspaceGroupFields]{ - getGrant: func(role models.Role) *string { - return role.GrantsDevFirecloudGroup + getGrants: func(role models.Role) []*string { + return []*string{role.GrantsDevFirecloudGroup} }, engine: engine, _enable: true, @@ -60,8 +89,8 @@ func Test_propagatorImpl_Propagate_notEnabled(t *testing.T) { func Test_propagatorImpl_Propagate_shouldNotPropagate(t *testing.T) { config.LoadTestConfig() p := propagatorImpl[string, propagation_engines.GoogleWorkspaceGroupIdentifier, propagation_engines.GoogleWorkspaceGroupFields]{ - getGrant: func(role models.Role) *string { - return role.GrantsDevFirecloudGroup + getGrants: func(role models.Role) []*string { + return []*string{role.GrantsDevFirecloudGroup} }, _enable: true, _timeout: time.Minute, @@ -95,11 +124,12 @@ func Test_propagatorImpl_Propagate_shouldNotPropagate(t *testing.T) { func Test_propagatorImpl_Propagate_failToLoadCurrent(t *testing.T) { config.LoadTestConfig() engine := propagation_engines_mocks.NewMockPropagationEngine[string, propagation_engines.GoogleWorkspaceGroupIdentifier, propagation_engines.GoogleWorkspaceGroupFields](t) + engine.EXPECT().GenerateDesiredState(mock.Anything, mock.Anything).Return(nil, nil).Once() engine.EXPECT().LoadCurrentState(mock.Anything, mock.Anything). Return(nil, assert.AnError).Once() p := propagatorImpl[string, propagation_engines.GoogleWorkspaceGroupIdentifier, propagation_engines.GoogleWorkspaceGroupFields]{ - getGrant: func(role models.Role) *string { - return role.GrantsDevFirecloudGroup + getGrants: func(role models.Role) []*string { + return []*string{role.GrantsDevFirecloudGroup} }, engine: engine, _enable: true, @@ -123,13 +153,11 @@ func Test_propagatorImpl_Propagate_failToLoadCurrent(t *testing.T) { func Test_propagatorImpl_Propagate_failToGenerateDesired(t *testing.T) { config.LoadTestConfig() engine := propagation_engines_mocks.NewMockPropagationEngine[string, propagation_engines.GoogleWorkspaceGroupIdentifier, propagation_engines.GoogleWorkspaceGroupFields](t) - engine.EXPECT().LoadCurrentState(mock.Anything, mock.Anything). - Return(nil, nil).Once() engine.EXPECT().GenerateDesiredState(mock.Anything, mock.Anything). Return(nil, assert.AnError).Once() p := propagatorImpl[string, propagation_engines.GoogleWorkspaceGroupIdentifier, propagation_engines.GoogleWorkspaceGroupFields]{ - getGrant: func(role models.Role) *string { - return role.GrantsDevFirecloudGroup + getGrants: func(role models.Role) []*string { + return []*string{role.GrantsDevFirecloudGroup} }, engine: engine, _enable: true, @@ -146,13 +174,13 @@ func Test_propagatorImpl_Propagate_failToGenerateDesired(t *testing.T) { }) assert.Empty(t, results) if assert.Len(t, errors, 1) { - assert.ErrorContains(t, errors[0], "failed to generate desired state for grant") + assert.ErrorContains(t, errors[0], "failed to generate desired state") } } func Test_propagatorImpl_Propagate(t *testing.T) { config.LoadTestConfig() - // consumeStatesToDiff is tested separately; we're not trying to exercise it here, just test that we + // calculateAlignmentOperations is tested separately; we're not trying to exercise it here, just test that we // call and handle it correctly engine := propagation_engines_mocks.NewMockPropagationEngine[string, propagation_engines.GoogleWorkspaceGroupIdentifier, propagation_engines.GoogleWorkspaceGroupFields](t) loadCurrentStateCalls := 0 @@ -213,14 +241,13 @@ func Test_propagatorImpl_Propagate(t *testing.T) { Return("oh no", fmt.Errorf("failed to add c")).Once() p := propagatorImpl[string, propagation_engines.GoogleWorkspaceGroupIdentifier, propagation_engines.GoogleWorkspaceGroupFields]{ configKey: "test-config-key", - getGrant: func(role models.Role) *string { - return role.GrantsDevFirecloudGroup + getGrants: func(role models.Role) []*string { + return []*string{role.GrantsDevFirecloudGroup} }, engine: engine, _enable: true, _timeout: time.Minute, } - p.Name() var results []string var errors []error assert.NotPanics(t, func() { @@ -231,6 +258,84 @@ func Test_propagatorImpl_Propagate(t *testing.T) { }) }) slices.Sort(results) - assert.Equal(t, []string{"test-config-key: added b", "test-config-key: removed a"}, results) - assert.Equal(t, []error{fmt.Errorf("test-config-key: %w", fmt.Errorf("failed to add c"))}, errors) + assert.Equal(t, []string{"added b", "removed a"}, results) + assert.Equal(t, []error{fmt.Errorf("failed to add c")}, errors) +} + +func Test_propagatorImpl_Propagate_multi(t *testing.T) { + + config.LoadTestConfig() + engine := propagation_engines_mocks.NewMockPropagationEngine[string, propagation_engines.GoogleWorkspaceGroupIdentifier, propagation_engines.GoogleWorkspaceGroupFields](t) + + engine.EXPECT().GenerateDesiredState(mock.Anything, mock.Anything).Return(map[uint]intermediary_user.IntermediaryUser[propagation_engines.GoogleWorkspaceGroupIdentifier, propagation_engines.GoogleWorkspaceGroupFields]{ + 1: { + Identifier: propagation_engines.GoogleWorkspaceGroupIdentifier{Email: "user@example.com"}, + Fields: propagation_engines.GoogleWorkspaceGroupFields{}, + }, + }, nil).Once() + + // group-that-user-isn't-in-but-should-be + engine.EXPECT().LoadCurrentState(mock.Anything, "group-that-user-isn't-in-but-should-be"). + Return([]intermediary_user.IntermediaryUser[propagation_engines.GoogleWorkspaceGroupIdentifier, propagation_engines.GoogleWorkspaceGroupFields]{}, nil). + Once() + engine.EXPECT().Add(mock.Anything, "group-that-user-isn't-in-but-should-be", + propagation_engines.GoogleWorkspaceGroupIdentifier{Email: "user@example.com"}, + propagation_engines.GoogleWorkspaceGroupFields{}). + Return("added", nil).Once() + + // group-that-user-is-already-in + engine.EXPECT().LoadCurrentState(mock.Anything, "group-that-user-is-already-in"). + Return([]intermediary_user.IntermediaryUser[propagation_engines.GoogleWorkspaceGroupIdentifier, propagation_engines.GoogleWorkspaceGroupFields]{ + { + Identifier: propagation_engines.GoogleWorkspaceGroupIdentifier{Email: "user@example.com"}, + Fields: propagation_engines.GoogleWorkspaceGroupFields{}, + }, + }, nil).Once() + + // group-that-another-user-needs-to-be-removed-from + engine.EXPECT().LoadCurrentState(mock.Anything, "group-that-another-user-needs-to-be-removed-from"). + Return([]intermediary_user.IntermediaryUser[propagation_engines.GoogleWorkspaceGroupIdentifier, propagation_engines.GoogleWorkspaceGroupFields]{ + { + Identifier: propagation_engines.GoogleWorkspaceGroupIdentifier{Email: "user@example.com"}, + Fields: propagation_engines.GoogleWorkspaceGroupFields{}, + }, + { + Identifier: propagation_engines.GoogleWorkspaceGroupIdentifier{Email: "user-2@example.com"}, + Fields: propagation_engines.GoogleWorkspaceGroupFields{}, + }, + }, nil).Once() + engine.EXPECT().Remove(mock.Anything, "group-that-another-user-needs-to-be-removed-from", + propagation_engines.GoogleWorkspaceGroupIdentifier{Email: "user-2@example.com"}). + Return("removed", nil).Once() + + p := propagatorImpl[string, propagation_engines.GoogleWorkspaceGroupIdentifier, propagation_engines.GoogleWorkspaceGroupFields]{ + configKey: "test-config-key", + getGrants: func(role models.Role) []*string { + if role.GrantsDevFirecloudGroup == nil { + return nil + } else { + return utils.Map(strings.Split(*role.GrantsDevFirecloudGroup, ", "), func(s string) *string { + return &s + }) + } + }, + engine: engine, + _enable: true, + _timeout: time.Minute, + } + + var results []string + var errors []error + + assert.NotPanics(t, func() { + results, errors = p.Propagate(context.Background(), models.Role{ + RoleFields: models.RoleFields{ + GrantsDevFirecloudGroup: utils.PointerTo("group-that-user-isn't-in-but-should-be, group-that-user-is-already-in, group-that-another-user-needs-to-be-removed-from"), + }, + }) + }) + + slices.Sort(results) + assert.Equal(t, []string{"added", "removed"}, results) + assert.Empty(t, errors) } diff --git a/sherlock/internal/role_propagation/propagator_should_propagate.go b/sherlock/internal/role_propagation/propagator_should_propagate.go deleted file mode 100644 index 31cc7918e..000000000 --- a/sherlock/internal/role_propagation/propagator_should_propagate.go +++ /dev/null @@ -1,21 +0,0 @@ -package role_propagation - -import ( - "github.com/broadinstitute/sherlock/sherlock/internal/models" - "reflect" -) - -// shouldPropagate determines whether this propagator should act on the given role. -// -// It uses a tiny bit of reflection: if the grant is nil or the zero value of its type, we will not propagate. -// This is because we often explicitly store Go zero values in the database to represent "unset", because that's -// way easier for us to work with than null itself -- Gorm will ignore nulls by default to try to fit with Go's -// zero value semantics. -func (p *propagatorImpl[Grant, Identifier, Fields]) shouldPropagate(role models.Role) (shouldPropagate bool, grant Grant) { - grantPointer := p.getGrant(role) - if grantPointer == nil || reflect.ValueOf(*grantPointer).IsZero() { - return false, grant - } else { - return true, *grantPointer - } -} diff --git a/sherlock/internal/role_propagation/propagator_should_propagate_test.go b/sherlock/internal/role_propagation/propagator_should_propagate_test.go deleted file mode 100644 index da0f2c600..000000000 --- a/sherlock/internal/role_propagation/propagator_should_propagate_test.go +++ /dev/null @@ -1,72 +0,0 @@ -package role_propagation - -import ( - "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/broadinstitute/sherlock/sherlock/internal/role_propagation/propagation_engines" - "github.com/stretchr/testify/assert" - "testing" -) - -func Test_propagatorImpl_shouldPropagate(t *testing.T) { - p := propagatorImpl[string, propagation_engines.GoogleWorkspaceGroupIdentifier, propagation_engines.GoogleWorkspaceGroupFields]{ - getGrant: func(role models.Role) *string { - return role.GrantsDevFirecloudGroup - }, - } - type args struct { - role models.Role - } - type testCase[Grant any, Identifier intermediary_user.Identifier, Fields intermediary_user.Fields] struct { - name string - args args - wantShouldPropagate bool - wantGrant Grant - } - tests := []testCase[string, propagation_engines.GoogleWorkspaceGroupIdentifier, propagation_engines.GoogleWorkspaceGroupFields]{ - { - name: "nil", - args: args{ - role: models.Role{ - RoleFields: models.RoleFields{ - GrantsDevFirecloudGroup: nil, - }, - }, - }, - wantShouldPropagate: false, - wantGrant: "", - }, - { - name: "empty", - args: args{ - role: models.Role{ - RoleFields: models.RoleFields{ - GrantsDevFirecloudGroup: utils.PointerTo(""), - }, - }, - }, - wantShouldPropagate: false, - wantGrant: "", - }, - { - name: "non-empty", - args: args{ - role: models.Role{ - RoleFields: models.RoleFields{ - GrantsDevFirecloudGroup: utils.PointerTo("string"), - }, - }, - }, - wantShouldPropagate: true, - wantGrant: "string", - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - gotShouldPropagate, gotGrant := p.shouldPropagate(tt.args.role) - assert.Equalf(t, tt.wantShouldPropagate, gotShouldPropagate, "shouldPropagate(%v)", tt.args.role) - assert.Equalf(t, tt.wantGrant, gotGrant, "shouldPropagate(%v)", tt.args.role) - }) - } -} diff --git a/sherlock/internal/role_propagation/role_propagation_mocks/mock_propagator.go b/sherlock/internal/role_propagation/role_propagation_mocks/mock_propagator.go index 2b9248263..f1048a8fd 100644 --- a/sherlock/internal/role_propagation/role_propagation_mocks/mock_propagator.go +++ b/sherlock/internal/role_propagation/role_propagation_mocks/mock_propagator.go @@ -64,8 +64,8 @@ func (_c *MockPropagator_Init_Call) RunAndReturn(run func(context.Context) error return _c } -// Name provides a mock function with given fields: -func (_m *MockPropagator) Name() string { +// LogPrefix provides a mock function with given fields: +func (_m *MockPropagator) LogPrefix() string { ret := _m.Called() var r0 string @@ -78,29 +78,29 @@ func (_m *MockPropagator) Name() string { return r0 } -// MockPropagator_Name_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Name' -type MockPropagator_Name_Call struct { +// MockPropagator_LogPrefix_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'LogPrefix' +type MockPropagator_LogPrefix_Call struct { *mock.Call } -// Name is a helper method to define mock.On call -func (_e *MockPropagator_Expecter) Name() *MockPropagator_Name_Call { - return &MockPropagator_Name_Call{Call: _e.mock.On("Name")} +// LogPrefix is a helper method to define mock.On call +func (_e *MockPropagator_Expecter) LogPrefix() *MockPropagator_LogPrefix_Call { + return &MockPropagator_LogPrefix_Call{Call: _e.mock.On("LogPrefix")} } -func (_c *MockPropagator_Name_Call) Run(run func()) *MockPropagator_Name_Call { +func (_c *MockPropagator_LogPrefix_Call) Run(run func()) *MockPropagator_LogPrefix_Call { _c.Call.Run(func(args mock.Arguments) { run() }) return _c } -func (_c *MockPropagator_Name_Call) Return(_a0 string) *MockPropagator_Name_Call { +func (_c *MockPropagator_LogPrefix_Call) Return(_a0 string) *MockPropagator_LogPrefix_Call { _c.Call.Return(_a0) return _c } -func (_c *MockPropagator_Name_Call) RunAndReturn(run func() string) *MockPropagator_Name_Call { +func (_c *MockPropagator_LogPrefix_Call) RunAndReturn(run func() string) *MockPropagator_LogPrefix_Call { _c.Call.Return(run) return _c } diff --git a/sherlock/internal/role_propagation/split_string_pointer_on_commas.go b/sherlock/internal/role_propagation/split_string_pointer_on_commas.go new file mode 100644 index 000000000..14003eb39 --- /dev/null +++ b/sherlock/internal/role_propagation/split_string_pointer_on_commas.go @@ -0,0 +1,17 @@ +package role_propagation + +import ( + "github.com/broadinstitute/sherlock/go-shared/pkg/utils" + "strings" +) + +// a convenience function helpful for handling grant fields on roles that we treat as comma-separated lists. +// This function doesn't need to handle filtering the list at all because that's generic behavior that +// propagatorImpl.getAndFilterGrants will handle. +func splitStringPointerOnCommas(s *string) []*string { + if s == nil { + return nil + } else { + return utils.Map(strings.Split(*s, ","), func(s string) *string { return utils.PointerTo(strings.TrimSpace(s)) }) + } +} diff --git a/sherlock/internal/role_propagation/split_string_pointer_on_commas_test.go b/sherlock/internal/role_propagation/split_string_pointer_on_commas_test.go new file mode 100644 index 000000000..7cc92f5d7 --- /dev/null +++ b/sherlock/internal/role_propagation/split_string_pointer_on_commas_test.go @@ -0,0 +1,49 @@ +package role_propagation + +import ( + "github.com/broadinstitute/sherlock/go-shared/pkg/utils" + "github.com/stretchr/testify/assert" + "testing" +) + +func Test_splitStringPointerOnCommas(t *testing.T) { + type args struct { + s *string + } + tests := []struct { + name string + args args + want []*string + }{ + { + name: "nil", + args: args{s: nil}, + want: nil, + }, + { + name: "empty", + args: args{s: utils.PointerTo("")}, + want: []*string{utils.PointerTo("")}, + }, + { + name: "one", + args: args{s: utils.PointerTo("one")}, + want: []*string{utils.PointerTo("one")}, + }, + { + name: "two", + args: args{s: utils.PointerTo("one,two")}, + want: []*string{utils.PointerTo("one"), utils.PointerTo("two")}, + }, + { + name: "with some whitespace", + args: args{s: utils.PointerTo("one, two, three")}, + want: []*string{utils.PointerTo("one"), utils.PointerTo("two"), utils.PointerTo("three")}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equalf(t, tt.want, splitStringPointerOnCommas(tt.args.s), "splitStringPointerOnCommas(%v)", tt.args.s) + }) + } +}