Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[IDP-1183, partial] Add cron-based sync of groups_external data from Google Sheet #360

Merged
merged 7 commits into from
Sep 9, 2024
110 changes: 110 additions & 0 deletions application/common/components/ExternalGroupsSync.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
<?php

namespace common\components;

use common\models\User;
use Webmozart\Assert\Assert;
use Yii;
use yii\base\Component;

class ExternalGroupsSync extends Component
{
public const MAX_SYNC_SETS = 20;

public static function syncAllSets(array $syncSetsParams)
{
for ($i = 1; $i <= self::MAX_SYNC_SETS; $i++) {
$appPrefixKey = sprintf('set%uAppPrefix', $i);
$googleSheetIdKey = sprintf('set%uGoogleSheetId', $i);

if (! array_key_exists($appPrefixKey, $syncSetsParams)) {
Yii::warning(sprintf(
'Finished syncing external groups after %s sync set(s).',
($i - 1)
));
break;
}
Comment on lines +16 to +26
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be my preference, just because it's more consistent with typical for loops. Leave the 1-based numbering for user-focused text. (Note: there's one more $i further down.)

Suggested change
for ($i = 1; $i <= self::MAX_SYNC_SETS; $i++) {
$appPrefixKey = sprintf('set%uAppPrefix', $i);
$googleSheetIdKey = sprintf('set%uGoogleSheetId', $i);
if (! array_key_exists($appPrefixKey, $syncSetsParams)) {
Yii::warning(sprintf(
'Finished syncing external groups after %s sync set(s).',
($i - 1)
));
break;
}
for ($i = 0; $i < self::MAX_SYNC_SETS; $i++) {
$appPrefixKey = sprintf('set%uAppPrefix', $i+1);
$googleSheetIdKey = sprintf('set%uGoogleSheetId', $i+1);
if (! array_key_exists($appPrefixKey, $syncSetsParams)) {
Yii::warning(sprintf(
'Finished syncing external groups after %s sync set(s).',
$i
));
break;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, but since this is also used for the names of the environment variables, starting with 1 seems more intuitive than starting with 0 in this case. I agree that it's more intuitive/expected that code-only for-loops start with 0, though.


$appPrefix = $syncSetsParams[$appPrefixKey] ?? null;
$googleSheetId = $syncSetsParams[$googleSheetIdKey] ?? null;

if (empty($appPrefix) || empty($googleSheetId)) {
Yii::error(sprintf(
'Unable to do external-groups sync set %s: '
. 'app-prefix (%s) or Google Sheet ID (%s) was empty.',
$i,
json_encode($appPrefix),
json_encode($googleSheetId),
));
} else {
Yii::warning(sprintf(
"Syncing '%s' external groups from Google Sheet (%s)...",
$appPrefix,
$googleSheetId
));
self::syncSet($appPrefix, $googleSheetId);
}
}
}

private static function syncSet(string $appPrefix, string $googleSheetId)
{
$desiredExternalGroups = self::getExternalGroupsFromGoogleSheet($googleSheetId);
$errors = User::updateUsersExternalGroups($appPrefix, $desiredExternalGroups);
Yii::warning(sprintf(
"Ran sync for '%s' external groups.",
$appPrefix
));

if (! empty($errors)) {
Yii::error(sprintf(
'Errors that occurred while syncing %s external groups: %s',
$appPrefix,
join(" / ", $errors)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's theoretically possible to accumulate thousands of errors, would it be good to limit the length of this log message?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I hadn't thought of that. Yes, I can limit that error message length.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do that in a follow-up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

));
}
}

/**
* Get the desired external-group values, indexed by email address, from the
* specified Google Sheet, from the tab named after this IDP's code name
* (i.e. the name used in this IDP's subdomain).
*
* @throws \Google\Service\Exception
*/
private static function getExternalGroupsFromGoogleSheet(string $googleSheetId): array
{
$googleSheetsClient = new Sheets([
'applicationName' => Yii::$app->params['google']['applicationName'],
'jsonAuthFilePath' => Yii::$app->params['google']['jsonAuthFilePath'],
'jsonAuthString' => Yii::$app->params['google']['jsonAuthString'],
'spreadsheetId' => $googleSheetId,
]);
$tabName = Yii::$app->params['idpName'];

$values = $googleSheetsClient->getValuesFromTab($tabName);
$columnLabels = $values[0];

Assert::eq($columnLabels[0], 'email', sprintf(
"The first column in the '%s' tab must be 'email'",
$tabName
));
Assert::eq($columnLabels[1], 'groups', sprintf(
"The second column in the '%s' tab must be 'groups'",
$tabName
));
Assert::eq(
count($columnLabels),
2,
'There should only be two columns with values'
);

$data = [];
for ($i = 1; $i < count($values); $i++) {
$email = trim($values[$i][0]);
$groups = trim($values[$i][1] ?? '');
$data[$email] = $groups;
}
return $data;
}
}
38 changes: 38 additions & 0 deletions application/common/components/Sheets.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace common\components;

use Google\Service\Exception;
use InvalidArgumentException;
use yii\base\Component;
use yii\helpers\Json;
Expand Down Expand Up @@ -127,6 +128,43 @@ public function getHeader()
return $header['values'][0];
}

/**
* Get all the values from the specified tab in this Google Sheet.
*
* @param string $tabName
* @param string $range
* @return array[]
* Example:
* ```
* [
* [
* "A1's value",
* "B1's value"
* ],
* [
* "A2's value",
* "B2's value"
* ],
* [
* "A3's value",
* "B3's value"
* ]
* ]
* ```
* @throws Exception
*/
public function getValuesFromTab(string $tabName, string $range = 'A:ZZ'): array
{
$client = $this->getGoogleClient();

$valueRange = $client->spreadsheets_values->get(
$this->spreadsheetId,
$tabName . '!' . $range
);

return $valueRange->values;
}

/**
* @param string[] $header
* @param array $records
Expand Down
1 change: 1 addition & 0 deletions application/common/config/main.php
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,7 @@
],
Env::getArrayFromPrefix('ABANDONED_USER_')
),
'externalGroupsSyncSets' => Env::getArrayFromPrefix('EXTERNAL_GROUPS_SYNC_'),
'googleAnalytics' => [
'apiSecret' => Env::get('GA_API_SECRET'),
'measurementId' => Env::get('GA_MEASUREMENT_ID'),
Expand Down
22 changes: 6 additions & 16 deletions application/common/models/User.php
Original file line number Diff line number Diff line change
Expand Up @@ -1062,9 +1062,9 @@ public static function updateUsersExternalGroups(
$successful = $user->updateExternalGroups($appPrefix, $groupsForPrefix);
if (! $successful) {
$errors[] = sprintf(
"Failed to update external groups for %s: \n%s",
'Failed to update external groups for %s: %s',
$email,
json_encode($user->getFirstErrors(), JSON_PRETTY_PRINT)
join(' / ', $user->getFirstErrors())
);
}
}
Expand All @@ -1084,9 +1084,9 @@ public function updateExternalGroups(string $appPrefix, string $csvAppExternalGr
if (! str_starts_with($appExternalGroup, $appPrefix . '-')) {
$this->addErrors([
'groups_external' => sprintf(
'The given group %s does not start with the given prefix (%s)',
json_encode($appExternalGroup),
json_encode($appPrefix)
'The given group (%s) does not start with the given prefix (%s)',
$appExternalGroup,
$appPrefix
),
]);
return false;
Expand All @@ -1096,17 +1096,7 @@ public function updateExternalGroups(string $appPrefix, string $csvAppExternalGr
$this->addInMemoryExternalGroups($appExternalGroups);

$this->scenario = self::SCENARIO_UPDATE_USER;
$saved = $this->save(true, ['groups_external']);
if ($saved) {
return true;
} else {
Yii::warning(sprintf(
'Failed to update external groups for %s: %s',
$this->email,
join(', ', $this->getFirstErrors())
));
return false;
}
return $this->save(true, ['groups_external']);
}

/**
Expand Down
11 changes: 11 additions & 0 deletions application/console/controllers/CronController.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace console\controllers;

use common\components\ExternalGroupsSync;
use common\helpers\Utils;
use common\models\Invite;
use common\models\Method;
Expand Down Expand Up @@ -137,6 +138,16 @@ public function actionExportToSheets()
User::exportToSheets();
}

/**
* Sync external groups from Google Sheets
*/
public function actionSyncExternalGroups()
{
ExternalGroupsSync::syncAllSets(
\Yii::$app->params['externalGroupsSyncSets'] ?? []
);
}

/**
* Run all cron tasks, catching and logging errors to allow remaining tasks to run after an exception
*/
Expand Down
8 changes: 8 additions & 0 deletions local.env.dist
Original file line number Diff line number Diff line change
Expand Up @@ -337,3 +337,11 @@ GOOGLE_delegatedAdmin=admin@example.com

# spreadsheet ID
GOOGLE_spreadsheetId=putAnActualSheetIDHerejD70xAjqPnOCHlDK3YomH

# === external groups sync sets === #

# EXTERNAL_GROUPS_SYNC_set1AppPrefix=
# EXTERNAL_GROUPS_SYNC_set1GoogleSheetId=
# EXTERNAL_GROUPS_SYNC_set2AppPrefix=
# EXTERNAL_GROUPS_SYNC_set2GoogleSheetId=
# ... with as many sets as you want.