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.

Sign up for GitHub

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

Jump to bottom

🎫 DSAT-123 Workaround for policy update issues #91

Merged
merged 7 commits into from
Aug 4, 2019
Merged

🎫 DSAT-123 Workaround for policy update issues #91

merged 7 commits into from
Aug 4, 2019

Conversation

dmihalcik-virtru
Copy link
Collaborator

@dmihalcik-virtru dmihalcik-virtru commented Aug 3, 2019 •
edited
Loading

policy.builder() fails when policy.id is undefined, so always set it.
image

@dmihalcik-virtru dmihalcik-virtru changed the title 🎫 Workaround for policy update issues 🎫 DSAT-123 Workaround for policy update issues Aug 3, 2019
NathanAB
NathanAB reviewed Aug 3, 2019
View reviewed changes
NathanAB
NathanAB reviewed Aug 3, 2019
View reviewed changes
src/scenes/Document/Document.js
@@ -404,8 +408,23 @@ const actions = {
return { alert: value };
},
setPolicyId: (state, value) => {
Copy link

Choose a reason for hiding this comment

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

Not necessary to change now but I'm starting to regret using value for all our actions - it would probably be easier to follow with params like newPolicyId etc.

NathanAB
NathanAB reviewed Aug 3, 2019
View reviewed changes
src/scenes/Document/Document.js Outdated
@@ -65,13 +65,17 @@ function Document({

const encrypt = async () => {
setEncryptState(ENCRYPT_STATES.PROTECTING);
const cleanPolicy = policy
.builder()
.withPolicyId(null)
Copy link

Choose a reason for hiding this comment

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

damn this feels so wrong

@NathanAB
Copy link

NathanAB commented Aug 3, 2019

According to @jherwitz the better way to fix this issue is to build encrypt params with an empty policy, add the users, and THEN grab the policy id. See example: https://github.com/virtru/virtru-tdf3-js-integration/blob/master/tests/multi-user.js#L63

@dmihalcik-virtru
Copy link
Collaborator Author

Looking at it further, why not just generate our own uuid? All it is doing is calling uuid.v4() when there isn't a policyId present already - which why would there be if there isn't a copy constructor? I'll do that

@dmihalcik-virtru
Copy link
Collaborator Author

Okay I'm generating a uuid.v4() since that is what the current implementation is anyway. I can't easily use TDF.generateUuid since it is marked as async.

jherwitz
jherwitz approved these changes Aug 4, 2019
View reviewed changes
Copy link

@jherwitz jherwitz left a comment

Choose a reason for hiding this comment

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

Approved to unblock testing - let's circle back later today and make sure we're not using undocumented APIs to do this, though.

@dmihalcik-virtru dmihalcik-virtru merged commit 94197cf into develop Aug 4, 2019
@pnancarrow pnancarrow deleted the feature/fake-id branch March 29, 2022 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jherwitz jherwitz jherwitz approved these changes

@biscoe916 biscoe916 Awaiting requested review from biscoe916

@ivanovSPvirtru ivanovSPvirtru Awaiting requested review from ivanovSPvirtru

@NathanAB NathanAB Awaiting requested review from NathanAB

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dmihalcik-virtru @NathanAB @jherwitz