Persist ParseUser, ParseInstallation, and default ACL in Keychain #19

cbaker6 · 2020-08-22T16:50:43Z

This takes a slightly different approach than its' Objective-c counterpart when storing important/private information related to the ParseUser, ParseInstallation, and default ACL. Instead of using some form of local storage such as CoreData or SQLite, it stores these items in the Keychain for safety. The Objective-c SDK does store sessionToken and some other items in the Keychain, but not whole objects as in this case.

Store ParseUser along with sessionToken in Keychain
Add ParseInstallation support and store ParseInstallation in Keychain
Fix bugs in ACL and store default ACL in Keychain
Bumped minimum OS requirements to .iOS(.v11), .macOS(.v10_13), .tvOS(.v11), .watchOS(.v4) due to ParseEncoder requirements. In the future may bump hire, check here for discussion.
Added Swift Package Index badges for compatibility and platforms to README
Ported some documentation from the Obj-c framework
Better organize file structure

codecov · 2020-08-22T16:52:00Z

Codecov Report

Merging #19 into main will increase coverage by 11.33%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             main      #19       +/-   ##
===========================================
+ Coverage   53.57%   64.91%   +11.33%     
===========================================
  Files          28       29        +1     
  Lines        1258     1485      +227     
===========================================
+ Hits          674      964      +290     
+ Misses        584      521       -63

Impacted Files	Coverage Δ
Sources/ParseSwift/API/API+Commands.swift	`81.90% <ø> (-5.10%)`	⬇️
Sources/ParseSwift/API/API.swift	`100.00% <ø> (ø)`
Sources/ParseSwift/API/URLSession+extensions.swift	`100.00% <ø> (ø)`
Sources/ParseSwift/Coding/Extensions.swift	`33.33% <ø> (ø)`
Sources/ParseSwift/Objects/ParseInstallation.swift	`93.51% <ø> (ø)`
Sources/ParseSwift/Objects/ParseObject.swift	`85.26% <ø> (+7.08%)`	⬆️
Sources/ParseSwift/Objects/ParseUser.swift	`79.51% <ø> (+1.90%)`	⬆️
...urces/ParseSwift/Objects/Protocols/Fetchable.swift	`0.00% <ø> (ø)`
...urces/ParseSwift/Objects/Protocols/Queryable.swift	`0.00% <ø> (ø)`
...ources/ParseSwift/Objects/Protocols/Saveable.swift	`0.00% <ø> (ø)`
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ff394d...cd1dc1a. Read the comment docs.

cbaker6 · 2020-08-23T17:30:52Z

@pranjalsatija can you review this? Also, you probably know better than I would on how best to write setDefaultACL as I think it will use ParseStorage.shared.use

pranjalsatija

This looks good to me. My only concern is the changes in concurrent iterations / timeouts, but those aren't big issues.

Tests/ParseSwiftTests/KeychainStoreTests.swift

Tests/ParseSwiftTests/ParseObjectBatchTests.swift

- Add updated badges - Bump minimus iOS support to iOS9 - Send codecov reports for all builds

Add initial version of ParseInstallation, still needs test cases

Also cleaned up file structure

cbaker6 · 2020-09-09T01:39:11Z

@pranjalsatija can you review this? The large changes in lines are due to documentation and restructuring.

Most of the updates are Installation (new), ParseUser (storing to Keychain and memory), and default ACL to Keychain, along with their respective test cases

…dded updated saving of User and Installation during fetch, find, first. Additional updates - updated some documentation (still more to do) - Deleted unnecessary files

…tic. For example, 100 signup threads won't be created to sign up a user

cbaker6 · 2020-09-12T17:24:09Z

@TomWFox a significant part of this PR is the API documentation since it essentially had no documentation before this. I ported the markdown comments from the obj-c framework to this one and attempted to tailor to this framework. This also switches to the "swift" style markdown (- parameter) from obj-c (@param). The jazzy docs should have no issues with the change.

I'm sure there are number of grammatical mistakes along with better documentation needed. Feel free to make any adjustments you think are necessary.

TomWFox · 2020-09-12T19:44:50Z

@cbaker6 Spell checked and some minor improvements.

…ed examples in playgrounds

Clean up Query

…-server JSON key "ACL". It also prevents you from using var ACL = ACL.defaultACL(). So we now use var ACL = ParseACL.defaultACL() instead.

…ecode message.

pranjalsatija · 2020-09-14T22:01:10Z

@cbaker6 sorry for taking so long; I noticed you were still slowly adding stuff so I figured I'd wait for at least a little while and then I got busy with school stuff. My main concern with this PR is the fact that it's directly using the Keychain from within the framework.

IIRC, Flo's idea was to make it so that users could "plug and play" different adaptors for different tasks, including the persistence of the current user, installation, etc. which is why I added ParseStorage.use; when users are initializing the SDK, they provide a storage provider and the SDK uses that storage provider to handle all its storage needs; if that's the case, then all storage work should go through ParseStorage.shared. If the user doesn't provide a storage provider, ParseStorage.shared should do it for them, or we should treat that situation as an unrecoverable error. Previously, I had it default to using an in memory Codable store and log a warning for good measure. If we don't want to use that approach anymore, that's also OK, I just wanted to point it out.

Other than that, I don't have any issues -- LGTM. I'll do another scan for typos and stuff.

Sources/ParseSwift/ParseConstants.swift

Sources/ParseSwift/Storage/PrimitiveObjectStore.swift

cbaker6 · 2020-09-14T22:11:52Z

@pranjalsatija thanks for reviewing!

IIRC, Flo's idea was to make it so that users could "plug and play" different adaptors for different tasks, including the persistence of the current user, installation, etc. which is why I added ParseStorage.use; when users are initializing the SDK, they provide a storage provider and the SDK uses that storage provider to handle all its storage needs; if that's the case, then all storage work should go through ParseStorage.shared. If the user doesn't provide a storage provider, ParseStorage.shared should do it for them, or we should treat that situation as an unrecoverable error. Previously, I had it default to using an in memory Codable store and log a warning for good measure. If we don't want to use that approach anymore, that's also OK, I just wanted to point it out.

I agree with you for the most part here. With the changes, I don't intend to store all or even most of the objects in the Keychain. The only items I store are the BaseParseUser, BaseInstallation, and the default ParseACL. This not only keeps these items safe, but also provides the local storage needed, which I don't think much else will need to be stored in Keychain. This differs slightly from the objective-c framework which just stored the sessionToken in the Keychain. My thinking is keep the user and Parse required items in a safe space (Keychain) and 3rd party storage can be used for everything else via a protocol and ParseStore.shared that you made

pranjalsatija · 2020-09-14T22:16:56Z

That's sensible! What if I update ParseStorage.shared a little bit then? I know we had a prior conversation about it taking multiple kinds of stores for different tasks. Right now, it takes a single PrimitiveObjectStore. What if I update it to take multiple? I'm thinking something like this:

struct ParseStorage {
    struct StorageConfig {
        let keyValueStore: PrimitiveObjectStore // defaults to in memory
        let secureStore: PrimitiveObjectStore // defaults to Keychain
    }
    
    mutating func use(_ config: StorageConfig) {
        self.backingStore = store
    }
}

And then I can also update the parts of this PR that reference the Keychain directly to reference the secure store?

cbaker6 · 2020-09-14T22:22:13Z

What if I update it to take multiple?

Sounds reasonable to me. It would make it more modular. My only concern is, do we want to let the user/developer to keep/choose the secureStore or should we just automatically keep this info in Keychain? Also, adding it your way would probably enable the framework to run on Linux as I don't think Keychain is in Foundation for Swift Linux

cbaker6 · 2020-09-14T22:23:25Z

If you want, it might be easier to merge this PR before you make the storage changes you mentioned

pranjalsatija · 2020-09-14T22:35:16Z

@cbaker6 I'll definitely merge this PR first! And I think letting them customize it is the way to go; you already pointed out the Linux situation and we / users might want to swap in an in-memory store for unit tests or something.

cbaker6 · 2020-09-14T22:46:41Z

@TomWFox can you approve this? @pranjalsatija has reviewed it

Sources/ParseSwift/ParseConstants.swift

add ACL tests with ACL fixes

286d038

cbaker6 added 3 commits August 22, 2020 13:00

bump codecov

f687de8

Increase timeout

62d7947

reduce test threads

7cd68aa

pranjalsatija approved these changes Aug 26, 2020

View reviewed changes

Tests/ParseSwiftTests/KeychainStoreTests.swift Outdated Show resolved Hide resolved

Tests/ParseSwiftTests/ParseObjectBatchTests.swift Outdated Show resolved Hide resolved

cbaker6 added 11 commits September 5, 2020 20:12

Store ParseUser and default ACL to Keychain. Additional fixes:

8fde3ce

- Add updated badges - Bump minimus iOS support to iOS9 - Send codecov reports for all builds

Make logoutAsync thread test pass

82b8c09

Add more code coverage

c9d589e

reduce codecov

e92ba7f

Use BaseParseUser to get currentUser from Keychain

656e8e3

Add initial documentation

2f00074

Bump minimum deployment for all OSs.

b85cdfc

Add initial version of ParseInstallation, still needs test cases

Fix watchOS build

41285e5

Really fix watchOS build

c9a841e

Bump OS versions in podspec

660e885

Finish Installation support with unit tests.

70347bb

Also cleaned up file structure

cbaker6 changed the title ~~add ACL tests with ACL fixes~~ Persist ParseUser, ParseInstallation, and default ACL in Keychain Sep 9, 2020

cbaker6 added 3 commits September 8, 2020 21:10

Add back old ParseUser threading test. Bump codecov

92e800e

Improve Keychain Installation tests

39ed8f1

Update installation in keychain whenever a badge update occurs

836f57e

cbaker6 added 7 commits September 9, 2020 16:36

Only persist BaseParseUser and BaseInstallation values to keychain. A…

7a4f3fb

…dded updated saving of User and Installation during fetch, find, first. Additional updates - updated some documentation (still more to do) - Deleted unnecessary files

Removed threadSafe SignUp, Login, and Logout since these are unrealis…

0544b04

…tic. For example, 100 signup threads won't be created to sign up a user

Only use ParseInstall on main thread

5dcefe7

Updates

70a26ad

update documentation

e5a218f

More updates to documentation

e07da5d

Update README.md

90dd572

cbaker6 changed the base branch from master to main September 12, 2020 16:40

cbaker6 added 2 commits September 12, 2020 13:10

Merge branch 'main'

82d22e4

make links point to main branch

2d03844

documentation fixes

5b839ab

cbaker6 added 10 commits September 12, 2020 16:30

Change source of logo image to show in docs

91ff90d

Updates with broken login

159cbf0

fixed URL components contruction along with adding body. Added improv…

6364814

…ed examples in playgrounds

Add Install example to playgrounds.

799b543

Clean up Query

Improve playgrounds and add ACL example

973c8a1

Switch ACL value type name to ParseACL as it causes issues with parse…

0ae3711

…-server JSON key "ACL". It also prevents you from using var ACL = ACL.defaultACL(). So we now use var ACL = ParseACL.defaultACL() instead.

Fix ACL saving to parse-server

da0b08a

Partially fixed decoding ParseError from ParseServer. Still doesn't d…

a146f2b

…ecode message.

Fixed decoding ParseError from server

285fa07

Updates to Query

2f3c557

pranjalsatija approved these changes Sep 14, 2020

View reviewed changes

Sources/ParseSwift/ParseConstants.swift Outdated Show resolved Hide resolved

Sources/ParseSwift/Storage/PrimitiveObjectStore.swift Show resolved Hide resolved

Added constants enum

c65e2c1

pranjalsatija approved these changes Sep 14, 2020

View reviewed changes

Sources/ParseSwift/ParseConstants.swift Outdated Show resolved Hide resolved

Sources/ParseSwift/ParseConstants.swift Outdated Show resolved Hide resolved

Update constants enum

cd1dc1a

TomWFox approved these changes Sep 15, 2020

View reviewed changes

cbaker6 merged commit 5d77060 into main Sep 15, 2020

cbaker6 deleted the acl branch September 15, 2020 12:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Persist ParseUser, ParseInstallation, and default ACL in Keychain #19

Persist ParseUser, ParseInstallation, and default ACL in Keychain #19

cbaker6 commented Aug 22, 2020 •

edited

Loading

codecov bot commented Aug 22, 2020 •

edited

Loading

cbaker6 commented Aug 23, 2020 •

edited

Loading

pranjalsatija left a comment

cbaker6 commented Sep 9, 2020

cbaker6 commented Sep 12, 2020

TomWFox commented Sep 12, 2020

pranjalsatija commented Sep 14, 2020

cbaker6 commented Sep 14, 2020

pranjalsatija commented Sep 14, 2020

cbaker6 commented Sep 14, 2020

cbaker6 commented Sep 14, 2020

pranjalsatija commented Sep 14, 2020

cbaker6 commented Sep 14, 2020

Persist ParseUser, ParseInstallation, and default ACL in Keychain #19

Persist ParseUser, ParseInstallation, and default ACL in Keychain #19

Conversation

cbaker6 commented Aug 22, 2020 • edited Loading

codecov bot commented Aug 22, 2020 • edited Loading

Codecov Report

cbaker6 commented Aug 23, 2020 • edited Loading

pranjalsatija left a comment

Choose a reason for hiding this comment

cbaker6 commented Sep 9, 2020

cbaker6 commented Sep 12, 2020

TomWFox commented Sep 12, 2020

pranjalsatija commented Sep 14, 2020

cbaker6 commented Sep 14, 2020

pranjalsatija commented Sep 14, 2020

cbaker6 commented Sep 14, 2020

cbaker6 commented Sep 14, 2020

pranjalsatija commented Sep 14, 2020

cbaker6 commented Sep 14, 2020

cbaker6 commented Aug 22, 2020 •

edited

Loading

codecov bot commented Aug 22, 2020 •

edited

Loading

cbaker6 commented Aug 23, 2020 •

edited

Loading