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

drop zoom oauth refresh token #819

Open
wants to merge 8 commits into
base: rc-v0.5.0
Choose a base branch
from
18 changes: 18 additions & 0 deletions docs/sources/zoom/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -81,3 +81,21 @@ sufficient, but as of May 2024 are no longer available for newly created Zoom ap
Once the scopes are added, click on `Done` and then `Continue`.

6. Activate the app
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you can include an screenshot like this on this step too:

image


## Troubleshooting

### Zoom API Error : 400 invalid client

`{"reason":"Invalid client_id or client_secret","error":"invalid_client"}`

Causes:
- extra chars in Client ID; or incorrect Client ID

Confirm that the `Client ID` and `Client Secret` are correctly set in your secret store solution (AWS Parameter Store, Secrets Manager, or GCP Secret Manager).

### Zoom API Error : 400 Bad Request

`{"reason":"Bad Request","error":"invalid_request"}`

Causes:
- extra chars in Account ID; or incorrect Account ID
29 changes: 13 additions & 16 deletions infra/modules/aws-ssm-secrets/main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -13,21 +13,18 @@ locals {

externally_managed_secrets = { for k, spec in var.secrets : k => spec if !(spec.value_managed_by_tf) }
terraform_managed_secrets = { for k, spec in var.secrets : k => spec if spec.value_managed_by_tf }

tf_management_description_appendix = "Value managed by a Terraform configuration; changes outside Terraform may be overwritten by subsequent 'terraform apply' runs"
}

# see: https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/ssm_parameter
resource "aws_ssm_parameter" "secret" {
for_each = local.terraform_managed_secrets

name = "${local.path_prefix}${each.key}"
# Due https://github.com/hashicorp/terraform-provider-aws/issues/31267
# all are added as secureString
type = "SecureString"
description = each.value.description
value = sensitive(coalesce(each.value.value, local.PLACEHOLDER_VALUE))
key_id = coalesce(var.kms_key_id, "alias/aws/ssm")
name = "${local.path_prefix}${each.key}"
type = each.value.sensitive ? "SecureString" : "String"
description = each.value.description
value = each.value.sensitive ? sensitive(coalesce(each.value.value, local.PLACEHOLDER_VALUE)) : null
insecure_value = each.value.sensitive ? null : coalesce(each.value.value, local.PLACEHOLDER_VALUE)
key_id = coalesce(var.kms_key_id, "alias/aws/ssm")

lifecycle {
ignore_changes = [
Expand All @@ -41,17 +38,17 @@ resource "aws_ssm_parameter" "secret" {
resource "aws_ssm_parameter" "secret_with_externally_managed_value" {
for_each = local.externally_managed_secrets

name = "${local.path_prefix}${each.key}"
# Due https://github.com/hashicorp/terraform-provider-aws/issues/31267
# all are added as secureString
type = "SecureString"
description = each.value.description
value = sensitive(coalesce(each.value.value, local.PLACEHOLDER_VALUE))
key_id = coalesce(var.kms_key_id, "alias/aws/ssm")
name = "${local.path_prefix}${each.key}"
type = each.value.sensitive ? "SecureString" : "String"
description = each.value.description
value = each.value.sensitive ? sensitive(coalesce(each.value.value, local.PLACEHOLDER_VALUE)) : null
insecure_value = each.value.sensitive ? null : coalesce(each.value.value, local.PLACEHOLDER_VALUE)
key_id = coalesce(var.kms_key_id, "alias/aws/ssm")

lifecycle {
ignore_changes = [
value, # key difference here; we don't want to overwrite values filled by the external process
insecure_value,
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if for example client_id is wrongly provided if is not sensitive? Does value will update insecure_value on second attempt ?

Copy link
Member Author

Choose a reason for hiding this comment

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

if this is in lifecycle, then TF shouldn't change resource even if it believes changes are needed in EITHER value or insecure_value

tags
]
}
Expand Down
12 changes: 3 additions & 9 deletions infra/modules/worklytics-connector-specs/main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -971,14 +971,14 @@ EOT
{
name : "CLIENT_ID"
writable : false
sensitive : false
sensitive : false # zoom renders in clear in console
value_managed_by_tf : false
description : "Client ID of the Zoom 'Server-to-Server' OAuth App used by the Connector to retrieve Zoom data. Value should be obtained from your Zoom admin."
},
{
name : "ACCOUNT_ID"
writable : false
sensitive : true
sensitive : false # zoom renders in clear in console
value_managed_by_tf : false
description : "Account ID of the Zoom tenant from which the Connector will retrieve Zoom data. Value should be obtained from your Zoom admin."
},
Expand All @@ -989,13 +989,7 @@ EOT
value_managed_by_tf : false
description : "Short-lived Oauth access_token used by connector to retrieve Zoom data. Filled by Proxy instance."
},
{
name : "OAUTH_REFRESH_TOKEN"
writable : true
lockable : true
sensitive : true
value_managed_by_tf : false
}, # q: needed? per logic as of 9 June 2023, would be created
# used to have OAUTH_REFRESH_TOKEN; but as of Sept 2024, seeing that it's not being used
],
reserved_concurrent_executions : null # 1
example_api_calls_user_to_impersonate : null
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -212,11 +212,10 @@ public HttpEventResponse handle(HttpEventRequest request) {
sourceApiRequest = requestFactory.buildRequest(request.getHttpMethod(), new GenericUrl(targetForSourceApiRequest), content);
} catch (IOException e) {
builder.statusCode(HttpStatus.SC_INTERNAL_SERVER_ERROR);
builder.body("Failed to parse request; review logs");
builder.body("Failed to parse response; review logs");
builder.header(ResponseHeader.ERROR.getHttpHeader(), ErrorCauses.CONNECTION_SETUP.name());
log.log(Level.WARNING, e.getMessage(), e);
log.log(Level.WARNING, e.getMessage());
//something like "Error getting access token for service account: 401 Unauthorized POST https://oauth2.googleapis.com/token,"
log.log(Level.WARNING, "Confirm oauth scopes set in config.yaml match those granted in data source");
return builder.build();
} catch (java.util.NoSuchElementException e) {
// missing config, such as ACCESS_TOKEN
Expand All @@ -225,8 +224,6 @@ public HttpEventResponse handle(HttpEventRequest request) {
log.log(Level.WARNING, e.getMessage(), e);
return builder.build();
}

//TODO: what headers to forward???
populateHeadersFromSource(sourceApiRequest, request, targetForSourceApiRequest);

//setup request
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import com.google.api.client.http.UrlEncodedContent;
import lombok.Getter;
import lombok.NoArgsConstructor;
import org.apache.commons.lang3.StringUtils;

import javax.inject.Inject;
import java.nio.charset.StandardCharsets;
Expand Down Expand Up @@ -59,23 +60,26 @@ public enum ConfigProperty implements ConfigService.ConfigProperty {

@Override
public HttpContent buildPayload() {

//trim to avoid copy-paste errors
//q : check for non-printable chars or anything like that
String accountId = StringUtils.trim(config.getConfigPropertyAsOptional(ConfigProperty.ACCOUNT_ID)
.orElseGet(() -> secretStore.getConfigPropertyOrError(ConfigProperty.ACCOUNT_ID)));

// The documentation doesn't say anything to use POST data, but passes everything in the URL
// Tested manually and, for the moment, it is accepted as POST data
Map<String, String> data = new TreeMap<>();
data.put(PARAM_GRANT_TYPE, getGrantType());
//q: move to config? cost-benefit, mainly
data.put(PARAM_ACCOUNT_ID,
config.getConfigPropertyAsOptional(ConfigProperty.ACCOUNT_ID)
.orElseGet(() -> secretStore.getConfigPropertyOrError(ConfigProperty.ACCOUNT_ID)));
data.put(PARAM_ACCOUNT_ID, accountId);
return new UrlEncodedContent(data);
}

@Override
public void addHeaders(HttpHeaders httpHeaders) {
String clientId = config.getConfigPropertyAsOptional(ConfigProperty.CLIENT_ID)
.orElseGet(() -> secretStore.getConfigPropertyOrError(ConfigProperty.CLIENT_ID));
String clientId = StringUtils.trim(config.getConfigPropertyAsOptional(ConfigProperty.CLIENT_ID)
.orElseGet(() -> secretStore.getConfigPropertyOrError(ConfigProperty.CLIENT_ID)));

String clientSecret = secretStore.getConfigPropertyOrError(ConfigProperty.CLIENT_SECRET);
String clientSecret = StringUtils.trim(secretStore.getConfigPropertyOrError(ConfigProperty.CLIENT_SECRET));
String token = Base64.getEncoder()
.encodeToString(String.join(":", clientId, clientSecret).getBytes(StandardCharsets.UTF_8));
httpHeaders.setAuthorization("Basic " + token);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ <T> Optional<T> getConfigPropertyAsOptional(ConfigProperty property, Function<Ge

Optional<T> r;
if (Objects.equals(parameterResponse.parameter().value(), PLACEHOLDER_VALUE)) {
log.warning("Found placeholder value for " + paramName + "; this is either a misconfiguration, or a value that proxy itself should later fill.");
log.info("Found placeholder value for " + paramName + "; this is either a misconfiguration, or a value that proxy itself should later fill.");
r = Optional.empty();
} else {
if (envVarsConfig.isDevelopment()) {
Expand Down
Loading