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

Credentials helper if config not present or unparseable #44

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
2 participants
@medwards
Copy link
Contributor

commented Mar 24, 2019

Fixes #1 but requires librespot-org/librespot#313 (though I can probably make a workaround)

This PR introduces a login helper interface (authentication.rs) that is invoked if there is no config file or the config file fails to parse. It proposes a breaking change to the config file format to match librespot credentials. Note: it also writes to the config file, so back it up if this is a concern.

It requires a bit of work before I'm happy with it:

  • improve login interface caller process (it's a mess right now, and doesn't verify the credentials before fully launching ncspot)
    • probably allows the login helper to return Credentials instead of its serialization
  • Login vs Login /w Facebook have different layouts (to allow URL copy/pasta)
  • polling should probably be done with a future instead
  • winapi dependency is copy/pasta, probably there are no ncurses backends that support windows so I can remove it

At this point my biggest feedback request is:

  • Are we ok changing the config file?
  • Is this way of doing logins OK, or would we rather have the login interface part of the main cursive application?

@medwards medwards force-pushed the medwards:master branch 2 times, most recently from 689af21 to de8bc56 Mar 24, 2019

@medwards

This comment has been minimized.

Copy link
Contributor Author

commented Mar 24, 2019

So the config struct got extended while I worked on this. I've rebased so everything is up-to-date at least, but the login helper will wipe out the new settings... (actually maybe every execution, I need to look into it)

Speaking of theming thats the other piece of feedback I'd like on this PR. The login interface's style isn't really in line with the rest of ncspot.

@hrkfdn

This comment has been minimized.

Copy link
Owner

commented Mar 25, 2019

Hey there and thanks a lot for your work on this!

  • Are we ok changing the config file?

Yes, I think we are. Especially, because this simplifies the configuration process quite a bit! Maybe it'd make sense to store the login data in a separate file? I was already thinking about creating a folder for configuration files (~/.config/ncspot/*) instead of a single file in ~/.config, which also seems to be more in line with XDG.

  • Is this way of doing logins OK, or would we rather have the login interface part of the main cursive application?

Mhh, if keeping it a separate application simplifies it, I think I'm okay with it. In the end, we don't want to run the main application if we can't login and I don't believe the authentication application will need any of the views/features of the main application, right?

@medwards

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

@hrkfdn

This comment has been minimized.

Copy link
Owner

commented Mar 26, 2019

@medwards: Small heads up, the config file now resides in ~/.config/ncspot/config.toml.

@medwards medwards force-pushed the medwards:master branch from de8bc56 to d97e633 Mar 27, 2019

@medwards

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

  • separated config.toml and credentials.toml
  • introduced some config file helpers (write some defaults if the files don't exist, variable callback logic)

Spotify auth endpoint polling is still done without futures and there is no verification of the credentials so entering a bad username/pass still results in ungraceful failure later.

On an unrelated note: I'm a couple of issues and I'm not sure if they're ncspot or upstream projects (ie librespot or rodio). Can I spam the issue tracker with them until they're nailed down?

@hrkfdn

This comment has been minimized.

Copy link
Owner

commented Mar 28, 2019

On an unrelated note: I'm a couple of issues and I'm not sure if they're ncspot or upstream projects (ie librespot or rodio). Can I spam the issue tracker with them until they're nailed down?

Yeah, go ahead.

Do you still want to polish this in your branch or should we get this in tree?

@medwards

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

@medwards

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

I might have some sort of obscure bug with this change. On one machine, everything works fine, on the other I get

thread '<unnamed>' panicked at 'Authentication failed with reason: BadCredentials', /home/medwards/.cargo/git/checkouts/librespot-06fda9f186b35c32/daeeeaa/core/src/connection/mod.rs:93:21
note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Canceled', src/libcore/result.rs:997:5

then everything launches and I can play music but the output is some garbled garbage.

I'm going to try to press gang some people to see if its just something weird on my one machine or a broader issue. I would be v happy if you verified this change locally before merging as well XD

@hrkfdn

This comment has been minimized.

Copy link
Owner

commented Mar 29, 2019

I tested this after your initial submission and it worked fine. No rush, though. We should probably investigate the issue you're encountering. I might be able to work on the polling and credential checking this week, too. I wanted to finish that first before merging it.

@medwards medwards force-pushed the medwards:master branch from d97e633 to 25542f8 Mar 29, 2019

@medwards

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

Eh, the broken audio looks like just a library behind the scenes getting updated now or a file handles issue. All good now after a reboot.

@hrkfdn

This comment has been minimized.

Copy link
Owner

commented Mar 30, 2019

I have merged this with some minor wording changes ;)

We can keep working on this in-tree. Thanks again!

@hrkfdn hrkfdn closed this Mar 30, 2019

@medwards

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2019

LOL I completely forgot it was still called crappy_poller XD

tyvm for the wording changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.