[RAOP] Add support for password protected devices

[Schlaubischlump]

This pull request implements support for password protected RAOP devices. I'm ready to implement any changes / cleanups that are required.

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 7e61aade3212b5ccea86dbf36fb45d0f02042b49 into 809c5a384cd7762410919d1e6cdc53f3e973f93f - view on LGTM.com

new alerts:

• 1 for Use of a broken or weak cryptographic hashing algorithm on sensitive data

[lgtm-com[bot]]

This pull request introduces 1 alert when merging e195683d737b10e8534e118ac778a2a420cff415 into 809c5a384cd7762410919d1e6cdc53f3e973f93f - view on LGTM.com

new alerts:

• 1 for Use of a broken or weak cryptographic hashing algorithm on sensitive data

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 94f43fde42b554e9f0f2f6028efc3a19a4b038cd into 9be93f107a70b9b03826a330cfafe1250460b0ae - view on LGTM.com

new alerts:

• 1 for Use of a broken or weak cryptographic hashing algorithm on sensitive data

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 32155f2d62dc228586bc5a94394a86b512142b9e into 9be93f107a70b9b03826a330cfafe1250460b0ae - view on LGTM.com

new alerts:

• 1 for Use of a broken or weak cryptographic hashing algorithm on sensitive data

[Schlaubischlump]

I only implemented password support for RAOP. I'm not sure how passwords are handled for airplay (maybe the same with realm=Airplay?). Therefore I have not removed the section completely.

I have not implemented a setter in conf.AppleTV on purpose, because a setter such as set_password would suggest that it would also work for other protocols. A setter like set_raop_password must be removed in the future, if we add password support for other protocols. Maybe taking a look at the AirPlay protocol and how passwords are handled there is a good next step. Maybe we can merge this change first, and I'll take a look at the password stuff for the AirPlay protocol in the coming days / weeks. If I manage to support password there as well I'll add the setter.

If this approach is okay for you, I would add a disclaimer to the development/stream.md file, that the API is likely to change in the future.

[lgtm-com[bot]]

This pull request introduces 1 alert when merging f1ab3f01a5c58961241057f1181642ea915206b5 into 9be93f107a70b9b03826a330cfafe1250460b0ae - view on LGTM.com

new alerts:

• 1 for Use of a broken or weak cryptographic hashing algorithm on sensitive data

[postlund]

I only implemented password support for RAOP. I'm not sure how passwords are handled for airplay (maybe the same with realm=Airplay?). Therefore I have not removed the section completely.

I have not implemented a setter in conf.AppleTV on purpose, because a setter such as set_password would suggest that it would also work for other protocols. A setter like set_raop_password must be removed in the future, if we add password support for other protocols. Maybe taking a look at the AirPlay protocol and how passwords are handled there is a good next step. Maybe we can merge this change first, and I'll take a look at the password stuff for the AirPlay protocol in the coming days / weeks. If I manage to support password there as well I'll add the setter.

If this approach is okay for you, I would add a disclaimer to the development/stream.md file, that the API is likely to change in the future.

I believe what you have right now is good for now. Adding similar support for password in AirPlay would be great as well, there's a very old issue for that already (#556). You are more than welcome to explore and implement that as well!

For now I don't see a need for a set_password method, so we can probably skip that. I'm not entirely satisfied with set_credentials to be honest. So let's keep it simple.

[postlund]

Once checks pass I will merge this!

[postlund]

@Schlaubischlump There's a few minor issue to fix. Clean those up and I'll merge ASAP.

[lgtm-com[bot]]

This pull request introduces 1 alert when merging dc702ad444ab959ccfb062fa6841df728486faf6 into bdaf12a448daeb430013ca5a2702e35e0fbd3f35 - view on LGTM.com

new alerts:

• 1 for Use of a broken or weak cryptographic hashing algorithm on sensitive data

[Schlaubischlump]

Let me take a look.

Edit: I fix it tomorrow. It's already late and I have never used codespell. I have to read the documentation first.

[lgtm-com[bot]]

This pull request introduces 1 alert when merging ea077012143c39ef7aece1a9a0ea5fd1ca8224bf into bdaf12a448daeb430013ca5a2702e35e0fbd3f35 - view on LGTM.com

new alerts:

• 1 for Use of a broken or weak cryptographic hashing algorithm on sensitive data

[codecov[bot]]

Codecov Report

Merging #1212 (10a4b3d) into master (bdaf12a) will decrease coverage by 0.17%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1212      +/-   ##
==========================================
- Coverage   95.20%   95.03%   -0.18%     
==========================================
  Files          58       58              
  Lines        6111     6141      +30     
==========================================
+ Hits         5818     5836      +18     
- Misses        293      305      +12     

Impacted Files	Coverage Δ
pyatv/conf.py	99.21% <100.00%> (+<0.01%)	:arrow_up:
pyatv/raop/__init__.py	93.72% <100.00%> (+0.02%)	:arrow_up:
pyatv/raop/raop.py	85.99% <100.00%> (-3.34%)	:arrow_down:
pyatv/raop/rtsp.py	97.74% <100.00%> (+0.47%)	:arrow_up:
pyatv/support/http.py	93.62% <100.00%> (+0.08%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update bdaf12a...10a4b3d. Read the comment docs.

[postlund]

I think re-generating api docs is what's missing (that is only verified by python 3.8 now because of commit e1bc26f6010bf8ea4a598d9bbca0cdfb00df4be4.

[Schlaubischlump]

Okay, I renamed the remaining password variable to get around the insecure crypto warning and executed the api.py script (using python3.8 just to be sure) to generate the documentation. Hopefully it is working now.

[Schlaubischlump]

Strange... Why is it still failing ?

[postlund]

Strange... Why is it still failing ?

A few tests are a bit unstable (probably time sensitive). I haven't paid much attention to them in the past, just re-triggered and hoped that it worked next time... I'm gonna start writing them down now though and try to fix them.

[postlund]

Uhm, GitHub complains about conflict when rebasing, maybe you can try that manually? Not sure where that came from, didn't complain about it before.

[Schlaubischlump]

Do we have to fix the CodeQL error as well ? Because I'm out of ideas of to prevent it

[postlund]

Nah, ignore CodeQL.

[Schlaubischlump]

Okay this is getting out of hands :D I wanted to create a clean, easily mergeable state. I created a diff file, with the current changes, applied them on top of your latest master commit and created a new branch raop2 on my profile.

If you don't mind, I will close this pull request and open a new one. So the complete password support for raop devices is a single, easy to find commit. This makes it easier to track errors, if this feature should break something. Is this okay for you ?

[postlund]

You don't have to close the PR, just force-push your other branch (raop2) to this one and it will sort itself.

[Schlaubischlump]

I wasn't sure how github handles a force push, but it seems to work just fine. Can you run the test again ?

[postlund]

It works pretty ok fo force push (you need to do that if you don't use merge commits but rather rebase on top of master when catching up, like I prefer). But it can mess up open comments. Should be fine now though 👍

Restarted, let's hope it passes now!

[postlund]

Finally merged, thanks for the job and patience here 👍🎉 Hopefully, future PRs won't be as troublesome.

[Schlaubischlump]

Now that I know which linters etc. I have to run first, it should be easier the next time !

[postlund]

Yeah, it takes some time to get used to. But once you do, it's pretty ok. A pre-commit doing all that stuff might be an improvement for the future though...