vonH / plugin.video.iplayerwww

BBC iPlayer for Kodi
GNU General Public License v2.0
45 stars 24 forks source link

BBCiD is broken #128

Closed primaeval closed 7 years ago

primaeval commented 7 years ago

I received this message about the BBCiD functionality being broken:

The URLs that you use in your signin, signout, and status routines seem to all give http 302 errors. They have new locations: https://www.bbc.com/session, https://www.bbc.com/account.

The segment of code that fails is right here:

def CheckLogin(logged_in): if(logged_in == True or StatusBBCiD() == True): logged_in = True return True elif ADDON.getSetting('bbc_id_enabled') != 'true': xbmcgui.Dialog().ok(translation(30308), translation(30311)) else: attemptLogin = xbmcgui.Dialog().yesno(translation(30308), translation(30312)) if attemptLogin: SignInBBCiD() <----------------------- succeeds if(StatusBBCiD()): <--------------------- fails xbmcgui.Dialog().notification(translation(30308), translation(30309)) logged_in = True; return True; else: xbmcgui.Dialog().notification(translation(30308), translation(30310))

return False

CaptainTK commented 7 years ago

This had to happen on the first day of my holidays... It is not my code, BTW. This was all done by @ihurst, IIRC.

Long story short: This will stay broken for a week, sorry.

primaeval commented 7 years ago

The whole BBCiD mechanism is different now as far as I can see. I'd like to say just remove it all from the code until someone works out how to fix it, but this might be the only way we can keep the addon going in the future. So I think it should be fixed as soon as one of us works out how to do it.

Maybe just comment out the main menu entries until it gets fixed.

@ihurst Help! ;)

ihurst commented 7 years ago

I'll try and get my environment set up again tonight. It's been a while ;-)

primaeval commented 7 years ago

Thanks.

ihurst commented 7 years ago

Without testing and just looking at the code I think the issue may be quite simple. Think there is an error in the following:

def StatusBBCiD():
    status_url="https://ssl.bbc.co.uk/id/status"
    html=OpenURL(status_url)
    if("You are signed in" in html):
        return True
    return False

This function checks to see if the status url returns "You are signed in" within the returned HTML - and I don't think it does any more.

Regarding HTTP 302's I believe this is all fine and the libraries handle these redirections - following the location header.

I need to get a new media box built to test this, unless anyone else can check this quickly? Just change the string to "Lovely to see you here" (just to test). We need a better identifier mechanism to check logged in status.

ihurst commented 7 years ago

Just tested this, and no quick win. Further investigation required.

ihurst commented 7 years ago

Spent a lot of time on this last night and not got much further. Hopefully have another go tonight but then I'm unavailable for a week.

It looks like the login process is now using a new framework called AMOpen. I believe there are a lot more redirects (and during these redirects cookies are set which we cannot handle).

My plan is to turn our auto client redirects off (already happened on OpenURLPost) and processing each redirect myself.

If anyone else has any other ideas, I'd lve to hear. But I've got a working dev environment setup again :-)

primaeval commented 7 years ago

Thanks. That's much further than I got. It's not critical right now but the BBC have promised to make the BBCiD mandatory for iPlayer next year so it would be good to keep on top of this.

CaptainTK commented 7 years ago

Thanks a lot @ihurst! Your help is much appreciated. If you manage to reach some intermediate results, please do share them. I will have access to a dev environment from early next week again an plan to put in some work of my own then.

CaptainTK commented 7 years ago

The log-in mechanism seems to work. However, it will not be easy to create a new scraper. The Beeb have changed to a JS-based page which makes things a lot more difficult, especially in Kodi. I hate to say it, but this may really be the end of the add-on.

I am considering to remove the whole BBCiD from the add-on for the time being and get a release out to avoid people from using it and creating more errors in the Beeb's logs.

primaeval commented 7 years ago

If they get enough people to cough up for the License Fee they might leave it easy to get around on some low end devices, including ours. ;)

You could just comment out the settings and main menu items for now and see if @ihurst can defeat the BBC. :)

CaptainTK commented 7 years ago

Maybe my pessimism was a bit premature. I believe I got the authentication right. Stay tuned. :)

@ihurst, I think you were just missing a minor thing: You need collect initial cookies for two URLs.

CaptainTK commented 7 years ago

@primaeval can you please test the changes I applied in 46811ac075de5cb6cbe56d4bfd5d37bd543d0964?

If they work for you, all we are missing is a correct sign out procedure and we are done with this one.

ihurst commented 7 years ago

Just looking at code. Can't test or anything. But I'm not sure why you have two sessions setup? If its all working, all good. Test deleting the cookie file etc :-)

CaptainTK commented 7 years ago

I don't know if you need two sessions in requests, but you need to get the cookies for two URLs. Otherwise you don't get access to some data of the website, more specifically the markup which needs to be scraped.

I noticed that my browser holds separate cookies for .com and .co.uk, but the cookies file of the test script mostly only contained those for .com. After adding the second session I got all of the cookies for .co.uk as well. This did the trick.

I don't know much about proper handling of sessions in requests, so if there is better way of doing this, please let me know. Basically, what we need to assure is to get the cookies for .com and .co.uk. And a proper sign out mechanism.

primaeval commented 7 years ago

Video works fine for me. Nice work you two. :)

There is nothing in Radio. Did you do anything with that?

CaptainTK commented 7 years ago

I didn't touch radio, never use it. Is there a Listening section? If so, it neds to be adjusted as well.

primaeval commented 7 years ago

The Radio Favourites looks the same as the iPlayer My Programmes section now. I expect it is the same code underneath.

I wonder if anyone uses the Radio section. We could do with some analytics.

CaptainTK commented 7 years ago

@primaeval, do you want me to fix Radio as well, or do you want to have a go?

primaeval commented 7 years ago

I would rather you do it, please. I had a look the other day and it hurt my head trying to get back up to speed with all the json handling. ;)

ihurst commented 7 years ago

@CaptainTK The two sessions makes a little more sense now that you mention cookies for the two domains. I actually bumped into this by accident when making the change.

I'll look over it properly when I get back.

Great team work on this issue. Loving what you've been doing with the add-on!

ihurst commented 7 years ago

Another thing I looked at was the xbmc.log method that can be usd to write to kodi.log. we may want a wrapper function round this, so all our logs have some kind of prefix.

ihurst commented 7 years ago

Don't think we use any bbc.com url calls, so we could jus use the bbc.co.uk login?

CaptainTK commented 7 years ago

We need it to check the login state or we need to find an alternative way of checking that, @ihurst.

Edit: We also need it for radio.

CaptainTK commented 7 years ago

Actually, radio favourites looks the same, but it still uses regular HTML, @primaeval. Your very first step, breaking the code down in lines 404/405 fails. I'll need to look into this, but I'll do it later this week.

Edit: And the URL needs to be updated.

ihurst commented 7 years ago

@CaptainTK @primaeval thought you might be interested in this free book, the company have been providing a book a day for ages: https://www.packtpub.com/packt/offers/free-learning

Was not sure how else to send this apart from here :-)

primaeval commented 7 years ago

Thanks. :) I'll pm you my email in the kodi forum too.

ihurst commented 7 years ago

@CaptainTK regarding state check, this is a bit of a mess as we are only checking the login is valid for .com I would this we could use the same check with .co.uk But maybe we need two check state methods?

BBC are a pain not keeping the two domains consistent.

CaptainTK commented 7 years ago

@primaeval, I checked in a fixed version of Radio favourites in a8b7ec8f36337f8b218ad32cae85a97363830c7c.

  1. Can you please test if it also works for you?
  2. This fix only scrapes the "Added" programmes, not the "Following" programmes. Do you recall what the old code did?
CaptainTK commented 7 years ago

Thanks a lot for the link to the book, @ihurst. Very good idea, indeed! Glad you are back, working in a team makes things so much easier!

Regarding the state check: I would recommend to keep a single state check and just login with both domains. This is how the website also works. I found that on logout in the browser several cookies get deleted. I think we need to delete hem manually, or get the information that they need to be deleted from the server's response.

Do you have any clue how to check if the server side tells the client to delete a certain cookie?

primaeval commented 7 years ago

Radio Added works fine. Thanks. The BBC Radio site has sections for Overview, Listen List and Following. The Following section is quite broad. I just added Drama to it which doesn't show up in the two other lists. If it is easy please add it. I think any Radio fans would be grateful for anything.

CaptainTK commented 7 years ago

Overview shows both, but the HTML is tricky to scrape and more likely to break. From my point of view Following would be different from Added.

In the video section, we have Watching and Added, so I see no reason why we should not have Following and Added in radio. What do you think?

primaeval commented 7 years ago

I like to have everything. ;)

CaptainTK commented 7 years ago

Please try 1c1fd543166c3b83aeb8bd49f55d8cd873dfd4b0, @primaeval.

I can't get the icons to work in Following, though. The URL seems fine, but I always get timeouts. But then, the whole radio website is currently extremely slow for me.

primaeval commented 7 years ago

It works well. Thanks. I don't think they get that much traffic for the Radio shows so they might have to get the hamsters to spin up a couple of hard drives. ;) It has always been slow. The Following icons seem to be broken on the website right now. I don't think it is you.

CaptainTK commented 7 years ago

I think the main problem with the state check is our improper handling of cookies. As far as I understand @ihurst added the ignore_discard=True flag to store cookies although we did not have a session at that time. Now we need a session for the sign on procedure, but we only use it to get the cookies set. We don't actually use it as a session. Problem is: Because of the ignore_discard setting, we still keep all of the session cookies, which should have been deleted on sign out.

So why not change the whole thing to one consistent session? From my point of view, we wouldn't even need to store cookies in a file any more. They would simply be lost once Kodi is closed. The only downside is that users would need to sign in every time they start Kodi.

Any ideas? Any recommendations on how to change the code to have one session for everything?

ihurst commented 7 years ago

Can we store a state variable in the addon between calls? Alot of state is passed via the url parameters.

My preferred solution would be to use session but I'm not sure it's possible.

We should be able to delete cookies, just settling them to null should do this.

primaeval commented 7 years ago

As it's a single user environment in Kodi you could just write them to a hidden setting.

CaptainTK commented 7 years ago

I would like to avoid writing, if possible. One point we never considered is wear of the flash on some of the boxes Kodi runs on. So if we can avoid writing to file, we should.

I like the idea of the state variable. This should work, just like we pass other information. What I am not sure about is if this state variable is actually meaningful in the new context. I have no idea how Kodi actually executes the Python code.

ihurst commented 7 years ago

Agree with the file writing and wearing flash, but kodi writes logs etc that do this anyway.

I don't understand how the python code is run but I believed the script is executed in a new instance each time a menu is chosen etc? Hence you can't have a application wide variable. There must be away round this.

CaptainTK commented 7 years ago

Actually, I believe the add-on only creates new ListItems in the Kodi GUI. It does not execute a new instance. So it should be possible to just use a global variable. I guess it needs to be declared in default.py and then inherited to all the other scripts.

ihurst commented 7 years ago

Interesting. So if we have a single Session object that should work. I wasn't aware if how the plugin architecture worked.

primaeval commented 7 years ago

Please be careful if you are going to be adding things to the urls when people make shortcuts to programs or streams.

CaptainTK commented 7 years ago

:-/ Global variables don't seem to work as they should. I tried inserting a test variable in ipwww_common. In theory, I should be able to access and modify the variable in all namespaces that import ipwww_common. I manage to access it, but changes to the variable are not persistent. As soon as I leave a ListItem, it falls back to default. Strange.

CaptainTK commented 7 years ago

I think @ihurst is right: Kodi starts a new Python environment for every call. The only ways to pass data seems to be via file or by adding it to the URLs. Very annoying.

I understand your concerns regarding shortcuts, @primaeval. However, I believe having a working global session with requests lib is more important.

primaeval commented 7 years ago

This is how I used the Session object in requests to do a similar thing in the Internet Archive ROM Launcher addon: https://github.com/primaeval/plugin.program.iarl/commit/a23ecafe7cc2c4641f6afb1d8bfea172d7ccc341 That might help.

ihurst commented 7 years ago

How's this all going, anything outstanding that I could look at?

CaptainTK commented 7 years ago

Would be terrific if you could take another look at the sign out procedure. If there is no clean solution, I think we should just delete the cookies.

Other than that, I think we are set for a release.

primaeval commented 7 years ago

One thing that needs changing before a release is to change the "Behaviour for getting programs" setting to "by page". This addon for example is hammering the iplayer servers via our addon. http://forum.kodi.tv/showthread.php?tid=291038 It is slowing down Meta4Kodi too. I know I wanted it originally but now I have learnt the hard way. ;)

CaptainTK commented 7 years ago

That is not a proper fix, @primaeval. This problem needs to be solved in the other add-on, not in this. Even with changing the default behaviour, it still grabs way more pages than it should. I think we should notify @im85288 and make him aware of this problem.

As for Meta4Kodi: If I understand the purpose of this add-on correctly, it is not meant to scrape online add-ons. Is there a flag we can set in the add-on such that Meta4Kodi will ignore it?