Initial integration with TidepoolKit #3
Conversation
- Include Carthage build to lessen impact to build time (like DashKit)
|
|
||
| extension KeychainManager: SessionStorage { | ||
| public func setSession(_ session: TSession?, for service: String) throws { | ||
| try replaceGenericPassword(nil, forService: service) |
There was a problem hiding this comment.
Is there a reason this is using generic password storage rather than internet password storage?
There was a problem hiding this comment.
The TSession is a struct that contains an authentication token, user id, session trace, and environment. It is not an email and password that are implicit in the internet password storage. I felt it better fit into this, but I can switch it if you want. If so, what should go in the email field and what should go in the password field?
There was a problem hiding this comment.
Actually, I'm going to pull apart the TSession struct and put much of it into UserDefaults (because I have other related things to go in there) and only put the authentication token in the keychain. I'll still use the generic password since it is just the token (and not email/password).
There was a problem hiding this comment.
At the point the user enters a username/password, and if it is the same as the pair that the user enters on the web, then we should probably keep it as an internet password.
Storing the returned session token I agree should use the generic class, or some other secure storage.
(removed comment about credential sharing with safari; not sure that's possible)
There was a problem hiding this comment.
TidepoolKit and TidepoolService are intentionally not capturing the email and password. The authentication token should suffice here and provides better security.
There was a problem hiding this comment.
As long as we're careful about not expiring the session tokens and thus forcing the user to do password entry at times they haven't requested it (like overzealous/broken token refresh schemes), I think that makes sense.
|
@rickpasetto I just modeled committing the |
|
@darinkrauss but, it only slows the build down the first time, right? Are you talking about CI builds or user builds on their machine? |
|
@rickpasetto Correct, CI builds are the main concern. Considering the Locally, though, you'll only take the hit once (unless you clean your build folder). However, I think this discussion can be taken out of this PR. I'm going to leave it as-is, particularly since this is all in development. We can remove if we decide to when it is more mature. |
|
@darinkrauss sounds good to me. |
Updated translations from Lokalise on Thu Feb 9 13:30:18 CST 2023
This is just the initial integration and only adds the login UI to the TidepoolService creation workflow. It does not yet do anything after login.
You can probably ignore the files in
Carthage/Build. They are cached simply so as to not impact the build times.