Skip to content

Stripping writeOnlyProperties when recording progress - #222

Merged
rjlohan merged 2 commits into
aws-cloudformation:masterfrom
johnttompkins:strip-write-only
Jan 7, 2020
Merged

Stripping writeOnlyProperties when recording progress#222
rjlohan merged 2 commits into
aws-cloudformation:masterfrom
johnttompkins:strip-write-only

Conversation

@johnttompkins

@johnttompkins johnttompkins commented Jan 2, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Issue #, if available: #221

Description of changes: This strips writeOnlyProperties from being logged and recorded via the RecordHandlerProgress API. Tested with a handler before and after adding the removal. writeOnlyProperty is "Password" in below logs:

Record Handler Progress with Request Id 9c65e2a3-7d38-4802-ae3a-48660215afb7 and Request: {RecordHandlerProgressRequest(BearerToken=db2cc6b1-e133-e975-c48c-ccf6b71f84bf, OperationStatus=IN_PROGRESS, CurrentOperationStatus=PENDING, ClientRequestToken=9f4dcd1c-98c5-4293-943f-017983f6f216)}
[CREATE] invoking handler...
[CREATE] handler invoked
Handler returned SUCCESS
Record Handler Progress with Request Id 831926a8-bf72-4eec-8fe7-b0c5df33d980 and Request: {RecordHandlerProgressRequest(BearerToken=db2cc6b1-e133-e975-c48c-ccf6b71f84bf, OperationStatus=SUCCESS, CurrentOperationStatus=IN_PROGRESS, ResourceModel={"Password":"SecretPassword","Identifier":"dd9898b4-15e8-4ab6-ad45-eee14bdbc95c"}, ClientRequestToken=21f3e86c-b659-45ce-9689-446c3dc695e3)}

After adding removal code:

Record Handler Progress with Request Id 14e196c9-4b32-476f-aae4-c35ce5398395 and Request: {RecordHandlerProgressRequest(BearerToken=6faf262c-7a42-063d-0ca8-f0f9d895e491, OperationStatus=IN_PROGRESS, CurrentOperationStatus=PENDING, ClientRequestToken=727952d0-796d-44d5-85d9-aeb523ed961c)}
[CREATE] invoking handler...
[CREATE] handler invoked
Handler returned SUCCESS
Record Handler Progress with Request Id 23b2729e-63f2-497d-b0af-57d9737c2aee and Request: {RecordHandlerProgressRequest(BearerToken=6faf262c-7a42-063d-0ca8-f0f9d895e491, OperationStatus=SUCCESS, CurrentOperationStatus=IN_PROGRESS, ResourceModel={"Identifier":"16670264-8fe5-44a3-9744-c79c0eae1d71"}, ClientRequestToken=ac65c03a-27e4-41a6-9162-74db750afa7a)}

This is pending #220 and aws-cloudformation/cloudformation-resource-schema#73

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@rjlohan

rjlohan commented Jan 6, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

I think we should probably redact the values of the writeOnlyProperties rather than strip the key-value pairs entirely. It achieves the same result whilst also allowing the user to see that those fields were (or were not) actually specified in the payload. Dropping them entirely could lead to confusion on certain errors.

Will approve for now, but should fix post merge I think.

@johnttompkins

johnttompkins commented Jan 6, 2020

Copy link
Copy Markdown
Contributor Author

I think we should probably redact the values of the writeOnlyProperties rather than strip the key-value pairs entirely. It achieves the same result whilst also allowing the user to see that those fields were (or were not) actually specified in the payload. Dropping them entirely could lead to confusion on certain errors.

Will approve for now, but should fix post merge I think.

This really only affects when progress is recorded for a handler. When reporting the model on progress updates, I would expect it to be the same model returned by the read handler. Should the read handler also return a model with redacted writeOnly properties? At least for redacting, we would just would have to think about how to handle the various types of things that need to be redacted.

@rjlohan

rjlohan commented Jan 6, 2020

Copy link
Copy Markdown
Contributor

Should the read handler also return a model with redacted writeOnly properties?

Yes, and that should be enforced via Contract Tests. That's the whole point of writeOnlyProperties. Redaction can just be replace value with **** for example. Indicates value is provided but not readable.

@johnttompkins

johnttompkins commented Jan 6, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Should the read handler also return a model with redacted writeOnly properties?

Yes, and that should be enforced via Contract Tests. That's the whole point of writeOnlyProperties. Redaction can just be replace value with **** for example. Indicates value is provided but not readable.

How would the read handler be aware of the write only properties? i feel like it would just leave them out

@johnttompkins johnttompkins mentioned this pull request Jan 6, 2020
Merged
@rjlohan
rjlohan merged commit 382291a into aws-cloudformation:master Jan 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tobywf tobywf Awaiting requested review from tobywf
2 more reviewers
@rjlohan rjlohan rjlohan approved these changes
@aygold92 aygold92 aygold92 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

See More

Successfully merging this pull request may close these issues.

3 participants

@johnttompkins @rjlohan @aygold92