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

fix/vault: verify if the app is still authorised when reading data #948

Merged
merged 1 commit into from Aug 5, 2019

Conversation

@m-cat
Copy link
Contributor

commented Aug 2, 2019

Fixes #915

@m-cat m-cat requested a review from nbaksalyar as a code owner Aug 2, 2019

@m-cat m-cat requested a review from lionel1704 Aug 2, 2019

@m-cat m-cat force-pushed the m-cat:app-authorised-read branch 2 times, most recently from 4e17fd1 to 3065587 Aug 2, 2019

@nbaksalyar
Copy link
Member

left a comment

Thanks! 👍 Just a few suggestions & questions from me.


// For apps, check if its public key is listed as an auth key.
if is_app {
if let RequestType::GetForUnpub = request_type {

This comment has been minimized.

Copy link
@nbaksalyar

nbaksalyar Aug 2, 2019

Member

Doesn't it need to be checked for RequestType::NotGet too?

enum RequestType {
GetForPub,
GetForUnpub,
NotGet,

This comment has been minimized.

Copy link
@nbaksalyar

nbaksalyar Aug 2, 2019

Member

Probably better to call it Mutation to be consistent with our naming?

name: &XorName,
) -> (BTreeMap<PublicKey, AppPermissions>, u64) {
if self.get_account(&name).is_none() {
self.insert_account(*name);

This comment has been minimized.

Copy link
@nbaksalyar

nbaksalyar Aug 2, 2019

Member

I think it shouldn't create a new account implicitly during the get request (which is also not paid for).
If an account doesn't exist, just return the default value, so the entire function can be rewritten as e.g.

self.get_account(&name)
  .map(|account| (account.auth_keys().clone(), account.version()))
  .unwrap_or_else(Default::default)
@lionel1704
Copy link
Member

left a comment

There are some FIXMEs in the revocation tests that need to be addressed along with this PR. Apps could still access data after revocation so those tests were failing. Now with this fixed the tests need to be corrected.
Could you please address this as well?

@m-cat m-cat force-pushed the m-cat:app-authorised-read branch from 3065587 to 6f2dc09 Aug 4, 2019

@m-cat

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2019

There are some FIXMEs in the revocation tests that need to be addressed along with this PR. Apps could still access data after revocation so those tests were failing. Now with this fixed the tests need to be corrected.
Could you please address this as well?

Addressed this but it looks like the tests are failing, will debug this tomorrow.

fix/vault: verify if the app is still authorised when reading data
When reading data from the network, we should first check if an app still exists
at the list of auth'ed apps at the source elders. When this app is revoked,
there will be no re-encryption of data. The request will be blocked at the
source elders itself. This check was missing in the mock vault implementation.

@m-cat m-cat force-pushed the m-cat:app-authorised-read branch from 6f2dc09 to a9f83c3 Aug 5, 2019

@lionel1704
Copy link
Member

left a comment

Thanks!

These changes have been address.

@lionel1704 lionel1704 merged commit f9cb4e6 into maidsafe:experimental Aug 5, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details

@m-cat m-cat deleted the m-cat:app-authorised-read branch Aug 8, 2019

@m-cat m-cat referenced this pull request Aug 8, 2019

Open

Update revocation tests #972

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.