-
Notifications
You must be signed in to change notification settings - Fork 0
GER-1260 - make authenticator private and expose methods for passthrough and initial capture #2
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
Conversation
| loginStorage: .init( | ||
| retrieveLogin: { @MainActor in | ||
| login | ||
| }, | ||
| storeLogin: { @MainActor newLogin in | ||
| login = newLogin | ||
| } | ||
| ) |
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 all temporary until the authenticate() method returns the session, right?
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.
Yes. And it’s my assertion/belief that once authenticate returns we should have no further need for the authenticator and the (local) login storage. The resulting token gets passed to another object which spawns its own authenticator object(s)
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.
Alternatively, we could define a login storage that handles both initial and ongoing storage, that gets handed off from in-memory storage to persistent storage. That seems more complex, but could handle refresh calls that happen after authenticate() returns
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.
Yes, refresh could be required immediately after authenticate() in some certain scenarios.
ChimeHQ/OAuthenticator#46 — here's the change to return the current session from the authenticate method.
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.
That seems more complex, but could handle refresh calls that happen after authenticate() returns
It is slightly more complex, but guarantees safe handling everywhere, and prevents leaking the Login, so maybe with that we wouldn't need #46 above.
ThisIsMissEm
left a comment
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 will work, but it looks a little weird
The sendAuthenticatedRequest in ATProtoLiteClient will trigger calls to loginStorage to store changes to the tokens, or even clear the current login. So it'd probably make sense to have a single shared LoginStorage implementation that stores the data correctly and securely, then share that across all authenticators.
In this case, until we have authenticate() returning the Login we'd need to use a custom loginStorage here, but it should probably still interact with the global implementation, I think.
75a6d3b to
bdcbba1
Compare
|
To properly hide the implementation in tests, ended up defining an ATProtoInterface, behind which are a struct that encapsulates the online calls, and an actor that mocks out ATProtocol for multiple test apps to interact with |
01c5759 to
9c4481c
Compare
…ugh authenticate() and a capture login method
9c4481c to
62e8f6e
Compare
|
rebased atop PR #1 and added a similar interface for load Protected Metadata so we can run offline tests |
This hides the implementation of ATProtoAuthenticator wrapping an Authenticator object