-
Notifications
You must be signed in to change notification settings - Fork 43
feat: change storages arguments to function instance #1789
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
Conversation
ElePT
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as I left on another PR: As a side note, I think that it would be good to also start documenting more internal-facing changes in the release notes, as we started doing with interface changes. I left some pointers in another PR on how to do this: #1788 (comment). If you don't agree, I am open for discussion on what should and shouldn't be part of the renos.
Thanks @ElePT . Change is done |
korgan00
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Just a minor thing.
…-serverless into feat-improve-storage-logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The release note looks very good, thanks @paaragon. I have not had time to go through the rest of the PR, but I see that you already have a good amount of reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a test trying to access Arguments storage with multiple users and have different contents in the files? The function creator with some arguments, one user with other arguments and a third one without a file. The three using the same function.
I think it should test #1789 (comment).
|
I've rolled back the changes in the ResultStorage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory, others users could access to jobs from other users: https://github.com/Qiskit/qiskit-serverless/blob/main/gateway/api/access_policies/jobs.py
But only the owner can save and read the results:
def can_read_result(user, job) -> bool:
return has_access = user.id == job.author.id
def can_save_result(user, job) -> bool:
return has_access = user.id == job.author.idSo, use user.username is in save_result and retrieve results is ok, so now it works. But if change the access in the future, it will fails. With these changes, we can be safe.
|
@avilches I'm not feeling very well today to follow all the comments here sorry, I will try to look at this tomorrow if I feel better 🙏 |
Too much text? TLDR = this PR includes the fix for two hidden bugs. Take care! |
Summary
This PR refactors the storages arguments so instead of receiving title and provider, they receive a function instance.
Details and comments