-
-
Notifications
You must be signed in to change notification settings - Fork 69
remove some direct references to the Keychain #22
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
Closed
Closed
Changes from all commits
Commits
Show all changes
46 commits
Select commit
Hold shift + click to select a range
286d038
add ACL tests with ACL fixes
cbaker6 f687de8
bump codecov
cbaker6 62d7947
Increase timeout
cbaker6 7cd68aa
reduce test threads
cbaker6 8fde3ce
Store ParseUser and default ACL to Keychain. Additional fixes:
cbaker6 82b8c09
Make logoutAsync thread test pass
cbaker6 c9d589e
Add more code coverage
cbaker6 e92ba7f
reduce codecov
cbaker6 656e8e3
Use BaseParseUser to get currentUser from Keychain
cbaker6 2f00074
Add initial documentation
cbaker6 b85cdfc
Bump minimum deployment for all OSs.
cbaker6 41285e5
Fix watchOS build
cbaker6 c9a841e
Really fix watchOS build
cbaker6 660e885
Bump OS versions in podspec
cbaker6 70347bb
Finish Installation support with unit tests.
cbaker6 92e800e
Add back old ParseUser threading test. Bump codecov
cbaker6 39ed8f1
Improve Keychain Installation tests
cbaker6 836f57e
Update installation in keychain whenever a badge update occurs
cbaker6 7a4f3fb
Only persist BaseParseUser and BaseInstallation values to keychain. A…
cbaker6 0544b04
Removed threadSafe SignUp, Login, and Logout since these are unrealis…
cbaker6 5dcefe7
Only use ParseInstall on main thread
cbaker6 70a26ad
Updates
cbaker6 e5a218f
update documentation
cbaker6 e07da5d
More updates to documentation
cbaker6 90dd572
Update README.md
cbaker6 cd2b6cd
Use Queryable protocol
cbaker6 25f966b
Merge remote-tracking branch 'refs/remotes/origin/acl'
cbaker6 82d22e4
Merge branch 'main'
cbaker6 2d03844
make links point to main branch
cbaker6 5b839ab
documentation fixes
TomWFox 91ff90d
Change source of logo image to show in docs
cbaker6 159cbf0
Updates with broken login
cbaker6 6364814
fixed URL components contruction along with adding body. Added improv…
cbaker6 799b543
Add Install example to playgrounds.
cbaker6 973c8a1
Improve playgrounds and add ACL example
cbaker6 0ae3711
Switch ACL value type name to ParseACL as it causes issues with parse…
cbaker6 da0b08a
Fix ACL saving to parse-server
cbaker6 a146f2b
Partially fixed decoding ParseError from ParseServer. Still doesn't d…
cbaker6 285fa07
Fixed decoding ParseError from server
cbaker6 2f3c557
Updates to Query
cbaker6 c65e2c1
Added constants enum
cbaker6 cd1dc1a
Update constants enum
cbaker6 17c1285
remove some references to Keychain
pranjalsatija b0d79a1
Merge branch 'acl' of https://github.com/parse-community/Parse-Swift
pranjalsatija 95042c4
update tests
pranjalsatija 6d6dc58
Merge branch 'main' of https://github.com/parse-community/Parse-Swift
pranjalsatija File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,27 +41,22 @@ struct CurrentUserContainer<T: ParseUser>: Codable { | |
// MARK: Current User Support | ||
extension ParseUser { | ||
static var currentUserContainer: CurrentUserContainer<Self>? { | ||
get { | ||
guard let currentUserInMemory: CurrentUserContainer<Self> | ||
= try? ParseStorage.shared.get(valueFor: ParseStorage.Keys.currentUser) else { | ||
return try? KeychainStore.shared.get(valueFor: ParseStorage.Keys.currentUser) | ||
} | ||
return currentUserInMemory | ||
} | ||
set { try? ParseStorage.shared.set(newValue, for: ParseStorage.Keys.currentUser) } | ||
get { try? ParseStorage.shared.secureStore.get(valueFor: ParseStorage.Keys.currentUser) } | ||
set { try? ParseStorage.shared.secureStore.set(newValue, for: ParseStorage.Keys.currentUser)} | ||
} | ||
|
||
internal static func saveCurrentContainerToKeychain() { | ||
//Only save the BaseParseUser to keep Keychain footprint finite | ||
guard let currentUserInMemory: CurrentUserContainer<BaseParseUser> | ||
= try? ParseStorage.shared.get(valueFor: ParseStorage.Keys.currentUser) else { | ||
internal static func saveCurrentUserContainer() { | ||
//Only save the BaseParseUser to keep memory footprint finite | ||
guard let currentUser: CurrentUserContainer<BaseParseUser> = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is similar to my previous comment, basically all changes are permanent before they are actually saved to the parse-server with no way to revert |
||
try? ParseStorage.shared.secureStore.get(valueFor: ParseStorage.Keys.currentUser) else { | ||
return | ||
} | ||
try? KeychainStore.shared.set(currentUserInMemory, for: ParseStorage.Keys.currentUser) | ||
|
||
try? ParseStorage.shared.secureStore.set(currentUser, for: ParseStorage.Keys.currentUser) | ||
} | ||
|
||
/** | ||
Gets the currently logged in user from the Keychain and returns an instance of it. | ||
Gets the currently logged in user from the shared secure store and returns an instance of it. | ||
|
||
- returns: Returns a `ParseUser` that is the currently logged in user. If there is none, returns `nil`. | ||
*/ | ||
|
@@ -140,7 +135,7 @@ extension ParseUser { | |
currentUser: user, | ||
sessionToken: response.sessionToken | ||
) | ||
Self.saveCurrentContainerToKeychain() | ||
Self.saveCurrentUserContainer() | ||
return user | ||
} | ||
} | ||
|
@@ -150,7 +145,7 @@ extension ParseUser { | |
extension ParseUser { | ||
|
||
/** | ||
Logs out the currently logged in user in Keychain *synchronously*. | ||
Logs out the currently logged in user in the shared secure store *synchronously*. | ||
*/ | ||
public static func logout() throws { | ||
_ = try logoutCommand().execute(options: []) | ||
|
@@ -159,7 +154,7 @@ extension ParseUser { | |
/** | ||
Logs out the currently logged in user *asynchronously*. | ||
|
||
This will also remove the session from the Keychain, log out of linked services | ||
This will also remove the session from the shared secure store, log out of linked services | ||
and all future calls to `current` will return `nil`. This is preferable to using `logout`, | ||
unless your code is already running from a background thread. | ||
|
||
|
@@ -176,7 +171,7 @@ extension ParseUser { | |
private static func logoutCommand() -> API.Command<NoBody, Void> { | ||
return API.Command(method: .POST, path: .logout) { (_) -> Void in | ||
currentUserContainer = nil | ||
try? KeychainStore.shared.delete(valueFor: ParseStorage.Keys.currentUser) | ||
try? ParseStorage.shared.secureStore.delete(valueFor: ParseStorage.Keys.currentUser) | ||
} | ||
} | ||
} | ||
|
@@ -263,7 +258,7 @@ extension ParseUser { | |
currentUser: user, | ||
sessionToken: response.sessionToken | ||
) | ||
Self.saveCurrentContainerToKeychain() | ||
Self.saveCurrentUserContainer() | ||
return user | ||
} | ||
} | ||
|
@@ -279,7 +274,7 @@ extension ParseUser { | |
currentUser: user, | ||
sessionToken: response.sessionToken | ||
) | ||
Self.saveCurrentContainerToKeychain() | ||
Self.saveCurrentUserContainer() | ||
return user | ||
} | ||
} | ||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I think this may change the original intent. Basically in this manner, any change the user makes to currentUser is automatically saved to the secureStore. Before this PR, all changes are made in memory and are only stored to the secureStore (Keychain) if the user calls .save or a variation of it.
It seems like there's no way for the user to revert a change if they accidentally changed a value, and that same changed would have to sync to the parse-server at some point.
Uh oh!
There was an error while loading. Please reload this page.
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.
I just want to clarify some stuff! I wasn't quite sure what the intent of the old code was, so I'll double check with you. The old code seems like it first tries to get the current installation from
ParseStorage.shared
and then tries to get it from the keychain, and if both fail, then it creates a new one.The old implementation was relying on the fact that
ParseStorage.shared
is an in-memory store, which it shouldn't do: it's 100% possible thatParseStorage.shared
(which is nowParseStorage.primitiveStore
) is backed by the keychain, a file on disk, etc. That being said, I updated it thinking that the fallback toKeychainStore
is only there in the event that the user didn't provide a shared store, which is no longer possible because the shared store isn't optional anymore.As far as reverting changes goes, I don't think it's correct to say that the old version provided a way to revert changes. It just so happens to be the case that the store it was reading / writing from first was in memory, but there was (and still is) nothing enforcing that that's true. If a user updates the value, I think we should store it persistently immediately. AFAIK, The old SDK and other SDKs like Firebase doesn't support this either; we can't be liable if the user accidentally sets the wrong value, and they should take care to write code that doesn't set the wrong value for the current user / installation.
As a matter of fact, should the current installation and user even be publicly settable? We can do
public(get)
andinternal(set)
so the property is read-only to end users. I understand updating the current user but again, I don't think that should be done by doingParseUser.current = ...
, it should be done through signing in, logging out, updating the object and saving it, etc.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.
This is true.
Kind of true... There wasn’t any persisted storage implemented. I used the in-memory ParseStore here on purpose so that changes to currentUser and currentInstallation happen in memory. When a “save” is successful, only then is it saved to the Keychain. This is done because there is currently no way to mark something is dirty and to determine when/how to sync with the parse-server. So the only way to confirm a change is as valid is after it’s committed to the parse-server.
The old version provides a way to revert because only saves to the parse-server are committed to the Keychain. If the user exits the app and iOS clears the memory, the last saved version that’s in the keychain (which was last synced to the parse-server) are returned. Memory is only committed after a save. In addition, if a user adds “bad” values to memory, they can simply pull the last version from the keychain (reverting). My point here, if there’s currently no way to tell if something is dirty, we shouldn’t be storing the data locally (which is what your changes will allow). It will make everything to get out sync. If a “bad” value is saved to the parse-server, then a “bad” value is stored in the Keychain
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.
It might have gotten lost in my responses, but I believe changes in the local storage should only be allowed when there are proper mechanisms in place to sync them to the parse-server. Until then, we shouldn’t persist changes locally until the parse-server says they have been committed.
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.
For the current version of the implementation. I didn’t take hints from the Obj-c SDK and I have no clue how Firebase works. In this situation, the local storage isn’t complete yet, so I’m not sure how they can be compared. If you have a SDK/app that relies on a centralized server (parse-server in this case), and you don’t have local storage, you should only commit after the centralized server tells you to. The obj-c has some form of local storage, so it doesn’t apply to this scenario
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.
Then what if we update
BaseParseUser
andBaseParseInstallation
to include anisConfirmed
field or something that only gets set totrue
when the server has confirmed a change? And regardless of that result, until we properly implement local storage, should I make the setters internal only?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.
This lets you know something has been changed locally, but how does it help synchronize the changes with the parse-server? I think syncing local changes need to be fully flushed out before allowing the user to change data locally. It will cause issues, for example, are the local changes older or newer than the parse-server? How do you know? In the current form (without full support of local storage and sync), the SDK works, with the limitation that data isn't changed until it's committed to the parse-server. This okay IMO for now until the completion of local storage.
Sounds reasonable to me. The one thing I've been doing is using the playground files to make sure everything is correct on the parse-server. The unit-tests are good, but they are mocking the parse-server. If you aren't already, you should make sure your changes work via playgrounds with a real parse-server. I posted some basic instructions and docker container (has parse-dashboard also) just for this here.