[BITAU-134] [BITAU-121] Create Shared CoreData Store #937
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #937 +/- ##
========================================
Coverage 88.67% 88.67%
========================================
Files 622 630 +8
Lines 39374 39602 +228
========================================
+ Hits 34916 35119 +203
- Misses 4458 4483 +25 ☔ View full report in Codecov by Sentry. |
|
No New Or Fixed Issues Found |
| #if SWIFT_PACKAGE | ||
| let modelURL = Bundle.module.url(forResource: "Bitwarden-Authenticator", withExtension: "momd")! | ||
| #else | ||
| let modelURL = Bundle(for: type(of: self)).url(forResource: "Bitwarden-Authenticator", withExtension: "momd")! | ||
| #endif |
There was a problem hiding this comment.
🎨 Could this be written as the next so not to duplicate the url code?
| #if SWIFT_PACKAGE | |
| let modelURL = Bundle.module.url(forResource: "Bitwarden-Authenticator", withExtension: "momd")! | |
| #else | |
| let modelURL = Bundle(for: type(of: self)).url(forResource: "Bitwarden-Authenticator", withExtension: "momd")! | |
| #endif | |
| #if SWIFT_PACKAGE | |
| let bundle = Bundle.module | |
| #else | |
| let bundle = Bundle(for: type(of: self)) | |
| #endif | |
| let modelURL = bundle.url(forResource: "Bitwarden-Authenticator", withExtension: "momd")! |
| public init( | ||
| storeType: AuthenticatorBridgeStoreType = .persisted, | ||
| groupIdentifier: String, | ||
| errorHandler: @escaping (Error) -> Void |
There was a problem hiding this comment.
🎨 Could we use a protocol here instead? Like the ErrorReporter of the apps. So then the apps can pass to the library the class that conforms to such protocol; we can just have our error reporters classes conform to this new library protocol and that's it.
This would be far easier to test, extend and hook things in the middle if we need to.
|
|
||
| let managedObjectModel = NSManagedObjectModel(contentsOf: modelURL)! | ||
| persistentContainer = NSPersistentContainer( | ||
| name: "Bitwarden-Authenticator", |
There was a problem hiding this comment.
🎨 I see this name being repeated several times in this file, could it be extracted to a constant to be reused?
| public func fetchAllForUserId(_ userId: String) async throws -> [AuthenticatorBridgeItemDataModel] { | ||
| let fetchRequest = AuthenticatorBridgeItemData.fetchByUserIdRequest(userId: userId) | ||
| let result = try persistentContainer.viewContext.fetch(fetchRequest) | ||
|
|
||
| return try result.map { data in | ||
| try data.model | ||
| } | ||
| } |
There was a problem hiding this comment.
🤔 Shouldn't this use the backgroundContext?
There was a problem hiding this comment.
🤔 What do you think about implementing a similar approach as the main app? I.e. the DataStore is a class that defines general behavior and doesn't have specific data models in there, however provide protocols for these specific data models and then the DataStore can have extensions that conform to such protocols for usage.
Specifically I'm referring to:
public func fetchAllForUserId(_ userId: String) async throws -> [AuthenticatorBridgeItemDataModel]
and
public func insertItems(_ items: [AuthenticatorBridgeItemDataModel], forUserId userId: String) async throws
which are specific for AuthenticatorBridgeItemDataModel.
This approach is more flexible if we need to add additional data models in the library to provide a clean separation between such models.
There was a problem hiding this comment.
See my comment below - I had pushed things up into the DataStore for a lot of this. But if we're ok with duplicating some of the CoreData extensions from the main app, I'll refactor this back to being more generic and add the convenience methods to a separate service. 👍
|
|
||
| /// A data model for persisting authenticator items into the shared CoreData store. | ||
| /// | ||
| public class AuthenticatorBridgeItemData: NSManagedObject { |
There was a problem hiding this comment.
🤔 Maybe it's worth to add the ManagedObject extension that we have on the main app. So when we create a share core library to use amongst swift apps we can quickly update/replace the code.
There was a problem hiding this comment.
In the POC, we just duplicated a couple of things (like ManagedObject, ManagedUserObject, etc). I had taken a slightly different tack of trying to minimize that and instead be more specific to just what we needed in the AuthenticatorBridgeKit.
But if we're OK with duplicating (for now, with an eye to refactoring to a shared library sometime in the future), I can move back to that. Then the DataStore would be much more generic (like the main app), and we'd add a wrapper (I liked your suggestion on the other PR of AuthenticatorBridgeItemService) to handle the convenience methods, encrypting, etc.
I'll refactor over to that here shortly.
There was a problem hiding this comment.
I guess that unless we know this data class is the only one we're going to use for now I'd go for the more general one. However I'd like to hear @KatherineInCode opinions as well.
There was a problem hiding this comment.
It does look like ManagedObject is duplicated already between BWA and PM. It would be nice if we could just declare conformance of this to ManagedObject and use it from both 🤔
Ultimately I'm okay with this duplication for the time being, but it does seem like a relatively easy target for sharing between the apps—though then everything that uses the ManagedObject in the PM will have to then import the Bridge library, which seems non-ideal from a dependency perspective.
Almost as though we actually need a separate library that's just "Core Data Components" 😂
There was a problem hiding this comment.
I've refactored to use the more generic version and pulled in a handful of files. These are all duplicates of what is in the main PM app (and in some cases BWA as well, as Katherine points out), but it does give us the more generic version of all of this and a possible path to consolidating them later.
I'm OK with either approach, just let me know which one you prefer. But I'm going to push up my latest commits which will bring in the more generic version so you all can see what that looks like in practice. If we choose to go the more specific route, we can always roll back to some of the earlier versions.
There was a problem hiding this comment.
I think ultimately I prefer the generic version, even though that's a bit more chrome and duplication here. But I also think that makes it easier to identify the duplication (because what's duplicated is a clearly-named separate file rather than inline) so in the future if/when we want to consolidate we can.
| static func fixture( | ||
| favorite: Bool = true, | ||
| id: String = UUID().uuidString, | ||
| name: String = "Name", | ||
| totpKey: String? = "TOTP Key", | ||
| username: String? = "Username" | ||
| ) -> AuthenticatorBridgeItemDataModel { |
There was a problem hiding this comment.
⛏️ On fixtures I think it's better to rely on default values and then customize as needed or provide a particular fixtureWhatever func for customization. This helps when using this amongst different tests where it's more intuitive to know the value of a default parameter and the expect behavior of such. What do you think?
| static func fixture( | |
| favorite: Bool = true, | |
| id: String = UUID().uuidString, | |
| name: String = "Name", | |
| totpKey: String? = "TOTP Key", | |
| username: String? = "Username" | |
| ) -> AuthenticatorBridgeItemDataModel { | |
| static func fixture( | |
| favorite: Bool = false, | |
| id: String = UUID().uuidString, | |
| name: String = "Name", | |
| totpKey: String? = nil, | |
| username: String? = nil | |
| ) -> AuthenticatorBridgeItemDataModel { |
There was a problem hiding this comment.
That sounds reasonable to me. 👍 I'll adjust this over in the next commit.
… Keep DataStore generic
…M app CoreData semantics. Refactor conveience methods to new service class
|
|
||
| /// A data model for persisting authenticator items into the shared CoreData store. | ||
| /// | ||
| public class AuthenticatorBridgeItemData: NSManagedObject { |
There was a problem hiding this comment.
It does look like ManagedObject is duplicated already between BWA and PM. It would be nice if we could just declare conformance of this to ManagedObject and use it from both 🤔
Ultimately I'm okay with this duplication for the time being, but it does seem like a relatively easy target for sharing between the apps—though then everything that uses the ManagedObject in the PM will have to then import the Bridge library, which seems non-ideal from a dependency perspective.
Almost as though we actually need a separate library that's just "Core Data Components" 😂
|
|
||
| /// A struct for storing information about items that are shared between the Bitwarden and Authenticator apps. | ||
| /// | ||
| public struct AuthenticatorBridgeItemDataModel: Codable, Equatable { |
There was a problem hiding this comment.
🤔 I feel like there was some reason I didn't make the AuthenticatorItemDataModel to conform to Equatable in the Authenticator, though I don't remember why, exactly. Is there a particular reason we want it here?
(I'm also finding myself wondering, in light of the ManagedObject conversation, if we actually might want to eventually consolidate the AuthenticatorItemDataModel and AuthenticatorBridgeItemDataModel into one type that's just used by both apps; it just so happens the BWA has local storage of those objects as well 🤔 Not sure how that would play with Core Data, though. On the other hand...would it potentially be possible to just...use the AuthenticatorItemDataModel and move it into this library, to be shared code to begin with? And update BWA to use that instead?)
There was a problem hiding this comment.
🤔 That's an interesting question. As far as CoreData, I'm not sure how it would work to have the local data store (writing to the app's container) and the shared data store (writing to the app's app group) using the exact same CoreData model. It's possible that's fine if we just reference everything properly. I could try doing a spike?
But also, that would mean that Authenticator immediately becomes wholly dependent on the Swift package for even its own local storage. The items are pretty much identical, but I wasn't sure if we want to jump all the way in on that.
There was a problem hiding this comment.
I think it's fine currently, despite them being identical. Probably not worth digging in on that too much at the moment, but something to keep in mind in the future.
| @@ -0,0 +1,52 @@ | |||
| import CoreData | |||
|
|
|||
| extension NSManagedObjectContext { | |||
There was a problem hiding this comment.
🤔 With this, we'll have the same file in three different places, as near as I can tell, since there's copies of this already in PM and BWA. Does it make sense to add it again?
There was a problem hiding this comment.
This one was the one piece I felt like there wasn't a good way around duplicating it. And as you mentioned above, I was hesitant to make everything depend on the Bridge library and move this exclusively in there. 🤔
🎟️ Tracking
[BITAU-134]
[BITAU-121]
📔 Objective
This PR adds the CoreData store, model class, and helpers to AuthenticatorBridgeKit. This extends the shared code to include a way to share data between the two apps. The CoreData store is protected by the App Group, so that only members of the App Group are able to load data from it. There's a number of convenience methods on the AuthenticatorBridgeDataStore that anticipate a few use-cases where we'll be calling this in the future, with a few more to be added.
Currently, this does not do any encryption as part of the store. I've pulled that code out into a separate branch which I will put up into a PR shortly after this one (PR for that branch). I felt like it was a bit much to review in two pieces, but the intention is to have that run through the DataStore so that nothing is stored without first being encrypted.
I also have not added code to instantiate/use any of this yet to the PM app. I'm trying to do as much as we can in the background before adding it behind the feature flag.
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes