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

Properly pass loginCookie in all requests #2053

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

malik1004x
Copy link

@malik1004x malik1004x commented Jul 3, 2024

Upon receiving the login cookie it is not updated in the jsonRequestOptions field and therefore not sent in subsequent requests. This causes permission issues on private wikis.

Fixed #2006

@kelson42
Copy link
Collaborator

kelson42 commented Jul 9, 2024

@malik1004x Thank you for your fix which at a first look is OK to me. We will do a proper review.

Does that mean that you use MWoffliner to scrape a wiki which needs to be logged in to have the article content visible? I ask because we have another ticket I'm not able to assess at #2006

Copy link

codecov bot commented Jul 9, 2024

Codecov Report

Attention: Patch coverage is 76.92308% with 6 lines in your changes missing coverage. Please review.

Project coverage is 71.98%. Comparing base (6a78f41) to head (556e816).

Files Patch % Lines
src/util/saveArticles.ts 33.33% 4 Missing ⚠️
src/Downloader.ts 85.71% 1 Missing ⚠️
src/MediaWiki.ts 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2053      +/-   ##
==========================================
- Coverage   74.45%   71.98%   -2.48%     
==========================================
  Files          41       41              
  Lines        3144     3148       +4     
  Branches      688      691       +3     
==========================================
- Hits         2341     2266      -75     
- Misses        683      748      +65     
- Partials      120      134      +14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@malik1004x
Copy link
Author

@malik1004x Thank you for your fix which at a first look is OK to me. We will do a proper review.

Does that mean that you use MWoffliner to scrape a wiki which needs to be logged in to have the article content visible? I ask because we have another ticket I'm not able to assess at #2006

Unfortunately not as of this commit – there is another almost-working branch in my repo. I'll PR when I have a moment.

@malik1004x
Copy link
Author

After looking at #2006 it appears to be the exact issue I was facing on my private wiki – I have managed to create a backup after some further bugfixes (the API check also needs the loginCookie passed in, while the current official version uses a blank login cookie). There's a separate branch with the fix on my repo, I'll re-check it and PR.

@kelson42
Copy link
Collaborator

kelson42 commented Jul 9, 2024

@malik1004x All great, please just update this PR with the rest of the code

@malik1004x
Copy link
Author

@malik1004x All great, please just update this PR with the rest of the code

I've merged the branch with the fix into main – PR should contain the full fix now. Account-only wikis should be scrapeable (mine is). Tests go through on my local (Debian 12).

@malik1004x malik1004x changed the title fix loginCookie not being updated in jsonRequestOptions Properly pass loginCookie in all requests Jul 9, 2024
@kelson42
Copy link
Collaborator

@malik1004x Thank you very much. @audiodude is the code reviewer, but to me we should not pass all these login info all over the function, they should just be used at a low level if specified.

@malik1004x
Copy link
Author

but to me we should not pass all these login info all over the function

It's not my preferred solution either, but there's not much we can do if the API check doesn't use the Downloader class, instead making requests "on its own".

@kelson42
Copy link
Collaborator

kelson42 commented Dec 3, 2024

but to me we should not pass all these login info all over the function

It's not my preferred solution either, but there's not much we can do if the API check doesn't use the Downloader class, instead making requests "on its own".

@malik1004x The MediaWiki class is AFAIK implemented as singleton. You can just rely on it to get the credentials.

@kelson42 kelson42 self-requested a review December 3, 2024 18:24
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.

mwoffliner 1.13.0 & OVM ubuntu 22.04.3 & mediawiki 1.17 - Status 404
2 participants