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

Saving credentials #43

Closed
wants to merge 1 commit into from

Conversation

ssttevee
Copy link

  • Added the ability to save credentials with --save-credentials

Reads the c_userid and c_userkey cookie values of a successfully authenticated session and saves them to $HOME/.crunchyrc
OR
Uses user provided c_userid and c_userkey values using --user-id and --user-key.

The --user-id and --user-key take priority over saved credentials.

The --user and --pass values take priority over user provided c_userid and c_userkey values.

When --save-credentials is passed with --user-id and --user-key, the previously saved credentials are overwritten by the credentials from new session.

@Godzil
Copy link
Owner

Godzil commented Aug 31, 2017

Hi,
Thanks for idea and proposal for saving credentials!

I'm not really fond of giving as parameter data that come from a browser's cookies, they can change at anytime as CR can change all of that mechanism without any warning and would break the thing and use for these parameters. They also can add some clever way to prevent cookies value to be used from one browser to another (will not tell how to not give them ideas) making that useless even if the page/cookies are not changed.

For me giving login / password should be the only way to do a login, but I'm happy to discuss about it of course!

Also instead of getting some specific cookies, it would be better, I think, to store the full cookie jar when we log than doing it this way.

For the storage file, I would prefer to be stored in the same folder as for the .crpersistent: the current folder. The idea is we may have different folder for different purpose and thing will not clash this way. (also the "UNIX way" of cluttering your HOME folder with dot file is a real pain. The .config folder we see these days is a bit better, but yet I think in this case this folder should not be hidden, but I digress here.)

So could you please, at least change the path for your config file and it's name I thing as it will only be used for credential storage ".crcred" is probably a better name, or a ".crcookies"

And of course I will not accept a pull request until Jenkins says it is all clear ;)

@ssttevee
Copy link
Author

ssttevee commented Sep 1, 2017

I was thinking about saving all the cookies in the jar as well, but there would be a lot of unnecessary data from cloudflare, etc. from looking at the cookies gathered in incognito mode. However I do agree that it would be the best way to make sure it keeps working.

Storage of the cookies, I believe, should be global (either user-wide or system-wide) instead of only .crpersistent, thus I chose to save it to$HOME or /etc. I find it hard to believe that one would have more than one premium crunchyroll account, and even if someone did, it doesn't make any difference when using one vs the other. Also, it makes purging of credentials a lot easier if they are stored in a global location, which is important when dealing with credentials.

I'll also have to update dependencies and typings in another PR before I make changes to this because the installed version of request does not support custom cookie storage and also because using the typings package is deprecated.

@Godzil
Copy link
Owner

Godzil commented Aug 1, 2018

Added similar authentication method in the latest set of changes. I will close the pull request as it is not possible to merge it anymore (too many changes) but this method of login is now in Crunchy code!

Thanks @ssttevee, and sorry to took so long to integrate it.

@Godzil Godzil closed this Aug 1, 2018
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.

None yet

2 participants