Skip to content

CloudVariables access controls#2

Open
gsteinLTU wants to merge 16 commits intomainfrom
1-read-only-cloud-variables
Open

CloudVariables access controls#2
gsteinLTU wants to merge 16 commits intomainfrom
1-read-only-cloud-variables

Conversation

@gsteinLTU
Copy link
Copy Markdown
Member

Close #1
Adds a setVariableAccess method where access levels can be set for other users with/without password. Old variables and variables created by logged-out users will default to all users with password having full access and without password having no access (except for unlocking a lock your client owns, although it shouldn't come up).

Remaining questions:

  • Should the owner's access still be limited in any way? The old version did not have this limitation, but the owner always needs to have access to setVariableAccess so the password is kind of meaningless for them.
  • Should the levels be additive (i.e. withPassword is a superset of withoutPassword)?

@gsteinLTU gsteinLTU added the enhancement New feature or request label Dec 1, 2022
@gsteinLTU gsteinLTU self-assigned this Dec 1, 2022
@gsteinLTU gsteinLTU linked an issue Dec 1, 2022 that may be closed by this pull request
@gsteinLTU
Copy link
Copy Markdown
Member Author

gsteinLTU commented Dec 2, 2022

From today:

  • Password should apply to owner to make testing functionality easier
  • Access should be additive
  • Should accept list of letters and list of level names
  • Needs testing for when no password set
  • Needs appendToUserVariable (and fix in documentation referring to "appendVariable")


const {sharedVars} = getCollections();
const username = this.caller.username;
const variable = await sharedVars.findOne({name: name});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Calling findOne then updateOne can be prone to concurrency bugs. I would use $push instead: https://www.mongodb.com/docs/manual/reference/operator/update/push/. (Then return an error if it fails.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks, I forgot they would be stored as actual lists in the DB

@gsteinLTU
Copy link
Copy Markdown
Member Author

@brollb Something we didn't discuss that I just caught, should the _sendUpdate call when appending send the entire new list, or just the new value? Both seem like valid possibilities, but sending just the value appended is definitely simpler (and avoids bringing the concurrency issue back).

@brollb
Copy link
Copy Markdown
Contributor

brollb commented Dec 5, 2022

I would probably send the update with the entire list since there isn't currently a distinction between setVariable updates and appending updates. We should be able to use the findOneAndUpdate operation on MongoDB to avoid the concurrency issue.

@gsteinLTU
Copy link
Copy Markdown
Member Author

Thanks, that will work.

@gsteinLTU gsteinLTU marked this pull request as ready for review December 7, 2022 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Read-only Cloud Variables

2 participants