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

Issue#1686 YouTube cookie set to null by logout. #1694

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

zeratul47
Copy link

@zeratul47 zeratul47 commented Oct 30, 2024

Fixes #1686

  1. YouTube.cookie set to null by logout.
  2. Update settings.gradle.kts to auto download dependencies.

@gechoto
Copy link
Contributor

gechoto commented Oct 30, 2024

Hi, thanks for working on this.
I think there is room for improvement...

Your current version does not really clear the cookie in the saved preferences/settings.
It does only prevent the cookie from loading in on startup but it is still saved.
This means the cookie is still there and probably still in backups for example, which is not exactly great.

What do you think about moving this logic into LoginScreen.kt instead?
Setting the cookie there should also correctly save it.

I guess this is where the cookie is updated from the webview:

innerTubeCookie = CookieManager.getInstance().getCookie(url)

You could try to set the cookie to null in this place (if it does not contain SAPISID).
(you have to test, I'm currently only writing down an idea)

@zeratul47
Copy link
Author

Thank you for the feedback. You are right. I will check it too. And the stuff with visitorData.

@zeratul47
Copy link
Author

Hi, I have updated the code according to your suggestions. Now cookie and visitorData should be removed from preference storage by logout.

@gechoto
Copy link
Contributor

gechoto commented Nov 2, 2024

Hi @zeratul47 your current code still does not set the cookie to null in the settings. Now it is an empty string. This can be improved.

For example what do you think about this code in LoginScreen.kt:

val youtubeCookieString = CookieManager.getInstance().getCookie(url)
GlobalScope.launch {
    if ("SAPISID" in youtubeCookieString) { // if logged in
        innerTubeCookie = youtubeCookieString
    } else { // if logged out
        context.dataStore.edit { settings ->
            settings.remove(InnerTubeCookieKey)
        }
    }
    YouTube.accountInfo().onSuccess {
[...]

This should be much simpler. It does completely remove the cookie from the settings so it is really reset and read as null again (same as if the user never logged in).

With this you can remove your changes in App.kt.


As for visitorData I think it should not be set to an empty string. It should always be filled, just with something new and unrelated to the account.

You could just let it generate a new visitorData by finding a way to run this code again:

?: YouTube.visitorData().getOrNull()?.also { newVisitorData ->
dataStore.edit { settings ->
settings[VisitorDataKey] = newVisitorData
}

Should be easy since it already automatically re-runs if the visitorData is null or "null".
One way to solve this could be by also removing it from the settings:

fun onRetrieveVisitorData(newVisitorData: String?) {
    if (innerTubeCookie == "") { // clear visitorData after logout (this will be regenerated in App.kt)
        GlobalScope.launch {
            context.dataStore.edit { settings ->
                settings.remove(VisitorDataKey)
            }
        }
        return
    }
[...]

@zeratul47
Copy link
Author

I have updated code, by your advice.
But It seems that Home page updating only on close - reopen of the app. Should it be so?

@gechoto
Copy link
Contributor

gechoto commented Nov 22, 2024

But It seems that Home page updating only on close - reopen of the app. Should it be so?

oh well I forgot to think of that.
I can't test right now and unfortunately I currently don't know a great solution other than just restarting the app after logout

idk we have to test and see what works

For example in the App.kt there is this:

        GlobalScope.launch {
            dataStore.data
                .map { it[InnerTubeCookieKey] }
                .distinctUntilChanged()
                .collect { cookie ->
                    YouTube.cookie = cookie
                }
        }

which does automatically re-run when the cookie changes. Maybe something similar could be added to the home page.
But I really don't know if this is the best solution - maybe someone else has a better idea?

GlobalScope.launch {
if ("SAPISID" in parseCookieString(youtubeCookieString)) { // if logged in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you use parseCookieString here?

To my understanding this should be enough:
if ("SAPISID" in youtubeCookieString) { // if logged in

since youtubeCookieString is already a string and we can simply check if it contains the substring "SAPISID".

I do not see a reason to parse the cookies here as this would only take longer.
Was there a reason why you added it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if we do not parse the string, then how "SAPISID" in youtubeCookieString will work?
I probably can use youtubeCookieString.contains("SAPISID").

Anyway I will check it one time (and documentation too), and change if necessary.
Thank you for checking!

Copy link
Contributor

@gechoto gechoto Nov 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how "SAPISID" in youtubeCookieString will work?

it checks if youtubeCookieString contains "SAPISID"

From Kotlin docs: https://kotlinlang.org/docs/operator-overloading.html#in-operator

a in b translates into b.contains(a)

so you have both options:

  1. youtubeCookieString.contains("SAPISID")
  2. "SAPISID" in youtubeCookieString

use whatever you think looks better - they both work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Logout / option to delete cookie
2 participants