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

SAFE Authenticator Migration to the new data types #960

Merged
merged 2 commits into from Aug 8, 2019

Conversation

@lionel1704
Copy link
Member

commented Aug 6, 2019

Authored by @Yoga07
Closes #824

refactor/auth: refactor safe_authenticator to use new data types
- fix all safe_authenticator tests to the new impl
- overhauled recovery functions to the new types
- changed share_mdata functions and structs to the new type
- removed re-encryption while revocation
- revamped safe_core nfs which used old Mdata to the new type
- safe_app's object_cache adapted temporarily
Show resolved Hide resolved safe_authenticator/src/ipc.rs Outdated
Show resolved Hide resolved safe_authenticator/src/ipc.rs Outdated
Show resolved Hide resolved safe_core/src/nfs/dir.rs Outdated

@Yoga07 Yoga07 referenced this pull request Aug 7, 2019

Merged

MDataInfo Address #9

@nbaksalyar
Copy link
Member

left a comment

Looks good, great work guys! 🎉

I have some review comments but these mostly concern the code style and minor things which can be picked up in forthcoming PRs.

Show resolved Hide resolved safe_app/src/ffi/mutable_data/entries.rs Outdated
Show resolved Hide resolved safe_core/src/ipc/req/share_mdata.rs Outdated
Show resolved Hide resolved safe_authenticator/src/revocation.rs Outdated
Show resolved Hide resolved safe_authenticator/src/tests/revocation.rs Outdated
Show resolved Hide resolved safe_authenticator/src/tests/revocation.rs
Show resolved Hide resolved safe_core/src/client/recovery.rs Outdated
@@ -142,19 +143,19 @@ pub fn container_perms_from_repr_c(
}

/// Transforms a collection of container permissions into `routing::PermissionSet`
pub fn container_perms_into_permission_set<'a, Iter>(permissions: Iter) -> PermissionSet
pub fn container_perms_into_permission_set<'a, Iter>(permissions: Iter) -> MDataPermissionSet

This comment has been minimized.

Copy link
@nbaksalyar

nbaksalyar Aug 7, 2019

Member

On a general note (to be considered and implemented in one of the future PRs), I'm thinking that now the Permission type is redundant. We introduced it only for the purpose of distinguishing between the Mutable Data permissions and the container permissions. The latter is a higher-level construct referring to the apps permissions for accessing the user's containers (like music, videos, etc.) and read permissions referred to app having the encryption key to read the data. However, we didn't have the read perm for MutableData because it was implied it's always public: hence the distinct permission type.

Now that we have an opposite situation (MutableData is always unpublished), I'm wondering if we should just use MDataPermissionSet in this context without requiring any conversions?

This comment has been minimized.

Copy link
@lionel1704

lionel1704 Aug 8, 2019

Author Member

@Yoga07 since this is going into a separate PR can you raise an issue for this please?

This comment has been minimized.

Copy link
@Yoga07

Yoga07 Aug 8, 2019

Member

Raised now :)
#971

Show resolved Hide resolved safe_core/src/ipc/req/mod.rs Outdated
Show resolved Hide resolved safe_core/src/ipc/resp.rs Outdated
Show resolved Hide resolved safe_core/src/nfs/tests.rs Outdated
Show resolved Hide resolved safe_core/src/client/recovery.rs Outdated
Show resolved Hide resolved safe_core/src/client/recovery.rs Outdated
Show resolved Hide resolved safe_core/src/client/recovery.rs Outdated
Show resolved Hide resolved safe_core/src/client/recovery.rs Outdated
@m-cat
Copy link
Contributor

left a comment

Left review comments. They are non-exhaustive so do not cover all instances of a particular issue.

Also, MDataKind::from_flag has been merged and can be used now in conjunction with MDataAddress::from_kind.

feat/mdatainfo: add address getter function for MDataInfo
This commit also :
- Addresses a bug where the Account packet was initialized as an Unsequenced Mutable Data
- Replaces all instance of MDataAddress where the getter function could be used
- Fixes parameters names in recovery functions

The changes have been addressed.

@Yoga07

Yoga07 approved these changes Aug 8, 2019

Copy link
Member

left a comment

LGTM! 😉

@lionel1704 lionel1704 merged commit 06b4d55 into maidsafe:experimental Aug 8, 2019

2 of 3 checks passed

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

@lionel1704 lionel1704 deleted the lionel1704:adapt-auth branch Aug 8, 2019

@lionel1704 lionel1704 restored the lionel1704:adapt-auth branch Aug 8, 2019

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