-
Notifications
You must be signed in to change notification settings - Fork 5
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
SuperTokens.doesSessionExist() blocking caller thread #60
Comments
The issue makes sense. Thank you for pointing this out. Is there a way in Swift where you can call the doesSessionExist in a non main UI thread and then update a state that then updates the UI? Do you think instead that we should make our functions async and run them in a separate thread internally, or rely on devs using our SDK to call the functions in a separate thread (of course, in this case we will have to update our docs to mention this everywhere)? |
It should never be the responsibility of the caller to execute 3rd party library code on a specific thread as that's an implementation detail of the library. If Here is a minimal example that wouldn't break the API contract (not tested): public static func doesSessionExist(completion: (Bool)-> Void) {
let tokenInfo = FrontToken.getToken()
if tokenInfo == nil {
completion(false)
return
}
let currentTimeInMillis: Int = Int(Date().timeIntervalSince1970 * 1000)
if let accessTokenExpiry: Int = tokenInfo!["ate"] as? Int, accessTokenExpiry < currentTimeInMillis {
let preRequestLocalSessionState = Utils.getLocalSessionState()
SuperTokensURLProtocol.onUnauthorisedResponse(preRequestLocalSessionState: preRequestLocalSessionState, callback: { unauthResponse in
var error: Error?
if unauthResponse.status == .API_ERROR {
error = unauthResponse.error
}
let shouldRetry = unauthResponse.status == .RETRY
// Here we dont throw the error and instead return false, because
// otherwise users would have to use a try catch just to call doesSessionExist
if error != nil {
completion(false)
return
}
completion(shouldRetry)
})
}
completion(true)
}
@available(*, deprecated, message: "This method is deprecated. Use doesSessionExist(completion:) instead.")
public static func doesSessionExist() -> Bool {
let executionSemaphore = DispatchSemaphore(value: 0)
var result: Bool = false
doesSessionExist {
result = $0
executionSemaphore.signal()
}
executionSemaphore.wait()
return result
} In my above suggestion we migrate the existing function to a closure-based approach, and then we consume this function in the old function and continue to use a semaphore blocked-based approach for API compatibility but we deprecate the function. Here is an example of how the import SwiftUI
struct MyView: View {
@State var isLoggedIn: Bool = false
var body: some View {
Group {
if isLoggedIn {
Text("Welcome to the app")
} else {
Text("Please log in")
}
}
.onAppear {
SuperTokens.doesSessionExist { @MainActor result in
self.isLoggedIn = result
}
}
}
} Of course, it would be better if this library supported Swift Concurrency and async-await but I'm assuming that is probably out of scope for now hence why I'm suggesting a closure-based approach. In all honestly and I don't mean this in a mean way the entire library is quite fragile and doesn't apply modern Swift conventions/standards and could be greatly improved ideally re-written. I'm tempted to give it a go haha. |
Thank you for the valuable feedback! We will add this to our list of tasks and work on this soonish (though we have other priorities). In the meantime though, you need not use our frontend sdk at all and instead manage the tokens yourself: https://supertokens.com/docs/session/quick-setup/handling-session-tokens#if-not-using-our-frontend-sdk (see the method of Header based and not cookie based).
That would be awesome if you could make PR(s) for these kind of fixes! Really appreciate it. |
I've gone further and started to rebuild the library from scratch though I've done it in an opinionated way so I don't feel I can raise PRs against this library. However, once I'm complete I'm happy to provide it as a modern alternative or you're welcome to endorse/own it if you are happy with the solution. I think it can be provided as a modern version of the iOS SDK that consumers can choose to use if they can and want to. It won't support Cookie-based auth as I feel this doesn't make much sense to native apps, header based auth is my recommended approach. Also, it won't likey use My view is that either the library should actually perform the sign-in operation in the same way it handles sign-out, or the consumer will have to integrate directly with their own URLSession. I'm still working out how that might work, it will be a little less magic but far more reliable and stable and probably actually clearer to consumers what is actually going on. I would like to understand a few things: If the frontend token comes back as "remove" instead of a token is this a documented API feature is just a workaround, just wondering if I would need to support this and what the use-case would actually be. Usually, tokens would be invalidated remotely so doesn't matter if client tokens are out-of-date as the API resource that requires them should be validating tokens. Why doesn't the library just handle sign-in instead of requiring consumers to perform their own networking, given it already provides a sign-out? Currently, the consumer has to provide their own API client for sign-in and then this library automagiclly intercepts the tokens on the response. I'm guessing there is a reason it was done this way but it's not clear to me. |
The problem for me to be able to use this current library is I have to evaluate the code and make sure it's something I can trust or would have written that way myself. In this case, I can't because there are too many issues in the Swift code as it currently stands for me to be able to use. It looks like a direct port of the Java library but in a Java kind of way if that makes sense. I almost started looking at alternative auth platforms but instead decided to double down here for now and see if I can make something work. I hope this makes sense. |
This means that you should clear the locally stored tokens.
We want to add more helper functions for sure. The only reason it's not done for the mobile SDKs is cause we are prioritising web SDKs as we have way more users there, and existing mobile users haven't not used us cause we don't have more helper functions. It's just a function of our priority, and nothing else.
That is perfectly fine. In fact, the default for non web clients is header based! Your help here is much appreciated. Im curious to see how you design the lib, and if it's something that works well, we'd be happy to merge it into our library. As a follow up question, will you be going with the request interception way? One thing to keep in mind is that the backend can update the session's access token payload in any API (for example, if the dev calls |
Yeah, a bit of a chicken-and-egg scenario. Well, hopefully, I can make using SuperTokens on Apple platforms more desirable and accessible. I know many authentication platforms that work out-of-the-box with native apps like Firebase and AWS so maybe that is another reason why it hasn't gained much adoption. For me personally, I'm building my own backend and need auth but didn't want to depend on a 3rd party SaSS to keep the costs down, so that's why I've been looking at the self-hosting option of SuperTokens. AWS Cognito and Firebase get very expensive when you ramp up the number of users.
That's interesting, I can see the complexity of the issue here and I did speculate that might be the reason, what would be the use case for APIs in the business domain dealing out tokens? |
Looking at the current What I'm trying to avoid is introducing a single point of failure by trying to work out when to intercept from all traffic, we know which out-of-the-box APIs need to be intercepted so that's easy to handle if we add all the FDI APIs to the iOS SDK, when it comes to custom API I guess we can expose the tools for the developer to use to capture the tokens and ensure they get stored through the SuperTokens SDK. I'm going to have a play with some ideas, essentially I'm trying to avoid trying to be too clever and move the responsibility to the client developer to hook up at the appropriate points that are not covered by the FDI apis. |
Maybe we can allow the custom APIs to be registered through the SuperTokens SDK in someway that allows them to then make requests through the SDK, essentially using the SuperTokens SDK as a kind of middleware. I'm kind of thinking out loud now, will play around with some ideas when I get some more free time. I've done most of the other implementation work just need to model the capture of tokens in the most effective way that works for all use-cases. |
For example, users may a role claim in the session which they want to update in an API call.
Yes and no. One can also have their application API in a sub domain like api.example.com and the auth api on auth.example.com, and in this case, if you set the
Well, that could be true, but it does add a bunch of friction points. For example, it's very easy for devs to miss out doing this since often times, the frontend and backend teams are different. But this is subjective for sure.
Could work. One issue with this is that as the FDI APIs expand (cause of a new feature), we would have to always also release a new version of the frontend SDK with an updated list of the APIs, which is not ideal. |
Thanks for the very insightful information. It might be I just release this
as an opinionated alternative where it might require more discipline to use
but with that comes safety and transparency on what it’s doing to your API
requests. Currently I’m building this for me but I guess you guys can
decide if you want to endorse or own this moving forward.
I’m keen on adding the FDI methods directly to the library as this will
significantly reduce the barrier to entry and makes sense give its
responsibilities.
That said i’m going to think about the use-cases you’ve put forward as they
are good ones and see if there is a way to have best of both worlds. Maybe
a decoupled approach as I’ve designed but in addition developers could
opt-in to an auto-inception based approach. With this approach even if it
doesn’t support newer FDI APIs immediately developers could still use the
opt-in approach which would work basically how it functions today using a
singleton.
Cameron.
…On Sun, 25 Feb 2024 at 06:30, Rishabh Poddar ***@***.***> wrote:
I can see the complexity of the issue here and I did speculate that might
be the reason, what would be the use case for APIs in the business domain
dealing out tokens?
For example, users may a role claim in the session which they want to
update in an API call.
it only works when the URL response comes from the Auth API domain right,
so I assume the use-case you're suggesting is basically where someone would
want to extend the auth API with their own functionality in addition to the
out-of-the-box APIs provided by the middleware?
Yes and no. One can also have their application API in a sub domain like
api.example.com and the auth api on auth.example.com, and in this case,
if you set the sessionTokenBackendDomain config on the frontend to .
example.com, the interception would happen even when querying
api.example.com. The idea is that we want the interception to happen for
both, the auth apis as well as the application apis.
I'm not sure it's the responsibility of this library to try and guess
which APIs it needs to intercept.
Well, that could be true, but it does add a bunch of friction points. For
example, it's very easy for devs to miss out doing this since often times,
the frontend and backend teams are different. But this is subjective for
sure.
What I'm trying to avoid is introducing a single point of failure by
trying to work out when to intercept from all traffic, we know which
out-of-the-box APIs need to be intercepted so that's easy to handle if we
add all the FDI APIs to the iOS SDK, when it comes to custom API I guess we
can expose the tools for the developer to use to capture the tokens and
ensure they get stored through the SuperTokens SDK.
Could work. One issue with this is that as the FDI APIs expand (cause of a
new feature), we would have to always also release a new version of the
frontend SDK with an updated list of the APIs, which is not ideal.
—
Reply to this email directly, view it on GitHub
<#60 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEZ6SPIUVYHJAYRCN73IPDYVLLADAVCNFSM6AAAAABDU76I4CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRSHAZTANJWGY>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Looking at this code, the synchronous function performs async work on session expiry and blocks the caller thread, given apps will likely call this method from the Main UI thread this seems like a red flag. If the API needs to do optional async work then the method should be async to avoid blocking the callers thread.
supertokens-ios/SuperTokensIOS/Classes/SuperTokens.swift
Line 92 in 9ec7023
Am I missing something here?
The text was updated successfully, but these errors were encountered: