Skip to content
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

Implementing SaveCookies and RestoreCookies does not prevent unnecessary logins #186

Closed
DuncWatts opened this issue Aug 7, 2023 · 3 comments · Fixed by #193
Closed

Implementing SaveCookies and RestoreCookies does not prevent unnecessary logins #186

DuncWatts opened this issue Aug 7, 2023 · 3 comments · Fixed by #193
Assignees

Comments

@DuncWatts
Copy link
Contributor

As per #185 I'm implementing this library within an Azure function application.

Each call to an endpoint results in a new instance of the function class, and therefore a fresh DataClient instance. To prevent this from then reauthenticating on every invocation I have implemented the SaveCookies and RestoreCookies Funcs on the iRacingDataClientOptions property.

With these specified the LoginInternalAsync method will restore the CookieContainer, however it still runs through the auth process, leading to unnecessary calls to the auth endpoint.

I've added a workaround to mark it as logged in if a CookieContainer has been restored. Happy to create a PR if this is an acceptable approach.

            if (options.RestoreCookies is not null
                && options.RestoreCookies() is CookieCollection savedCookies)
            {
                cookieContainer.Add(savedCookies);

                IsLoggedIn = true;
                return;
            }
@AdrianJSClark
Copy link
Owner

Thanks for lodging this issue! Your suggested fix should be good as a first step to properly resolve this problem.

I think there will need to be additional consideration given to error handling, however, to ascertain if an error indicating authentication has failed needs to also alter the IsLoggedIn property.

I'll probably have some time to look at this this week.

@AdrianJSClark AdrianJSClark self-assigned this Aug 14, 2023
@tobiaszuercher
Copy link
Contributor

tobiaszuercher commented Aug 27, 2023

@AdrianJSClark any update? do you need any help on this?

a lot of things are internal - don't understand why. makes it hard to extend / use. e.g DataClient

@AdrianJSClark AdrianJSClark removed their assignment Aug 28, 2023
@AdrianJSClark
Copy link
Owner

Apologies, had some busy stuff going on in my home life and work life, so haven't got to this yet.

@tobiaszuercher, the thinking around things being internal was to guide people to the interface and make it obvious that I wasn't planning for extending. That was probably a bit selfish to be honest, because in the back of my mind I knew I could always either change it or just put my extension internal as well. I'd happily accept any changes to open the library up to make it easier to extend and use. Just open a new issue or PR (so we don't hijack Duncan's issue here).

Help is also welcome on this issue. I've un-assigned myself so feel free to do some work. I definitely planned to get things underway a couple of weeks ago but, as I said above, life interrupted.

@AdrianJSClark AdrianJSClark self-assigned this Nov 26, 2023
AdrianJSClark added a commit that referenced this issue Nov 26, 2023
If a "RestoreCookies" method is supplied and the cookies returned
include any which apply to our base URL, then consider the client
already authenticated so it doesn't have to call the login endpoint
again.

Fixes #186
@AdrianJSClark AdrianJSClark linked a pull request Nov 26, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants