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

Bugfix/check token for edit locally requests #5039

Merged
merged 3 commits into from
Oct 17, 2022

Conversation

mgallien
Copy link
Collaborator

No description provided.

src/gui/application.cpp Outdated Show resolved Hide resolved
src/gui/application.cpp Outdated Show resolved Hide resolved
src/gui/folderman.cpp Outdated Show resolved Hide resolved
const auto checkTokenForEditLocally = new SimpleApiJob(accountFound->account(), QStringLiteral("/ocs/v2.php/apps/files/api/v1/openlocaleditor/%1").arg(token));
checkTokenForEditLocally->setVerb(SimpleApiJob::Verb::Post);
checkTokenForEditLocally->setBody(QByteArray{"1path=/"}.append(relPath.toUtf8()));
constexpr auto HTTP_OK_CODE = 200;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgallien Do you really need that for just a single use? Would not it be better to just use 200 inside the if?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree
though indeed we could have a simple header that would define those constants globally


const auto checkTokenForEditLocally = new SimpleApiJob(accountFound->account(), QStringLiteral("/ocs/v2.php/apps/files/api/v1/openlocaleditor/%1").arg(token));
checkTokenForEditLocally->setVerb(SimpleApiJob::Verb::Post);
checkTokenForEditLocally->setBody(QByteArray{"1path=/"}.append(relPath.toUtf8()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgallien Can not you use something like QString Folder::remotePath() to construct the absolute remote path? Having this path=/ with a slash looks unreliable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will have a look
thanks for suggestion

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could not make it work

@mgallien mgallien force-pushed the bugfix/checkTokenForEditLocallyRequests branch from dd723c3 to 0a00b8d Compare October 13, 2022 12:17
@allexzander
Copy link
Contributor

@mgallien Please do not forget clean-up the history

Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>
Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>
check on server that the token received during a request to open a local
file is indeed a valid one

Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>
@mgallien mgallien force-pushed the bugfix/checkTokenForEditLocallyRequests branch from fcf112a to f9949ee Compare October 17, 2022 07:02
@nextcloud-desktop-bot
Copy link

AppImage file: nextcloud-PR-5039-f9949ee0de1b8a42412f528dc1f64e080eb44316-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

@sonarcloud
Copy link

sonarcloud bot commented Oct 17, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 9 Code Smells

46.2% 46.2% Coverage
0.0% 0.0% Duplication

@codecov
Copy link

codecov bot commented Oct 17, 2022

Codecov Report

Merging #5039 (f9949ee) into master (bde2f90) will increase coverage by 0.03%.
The diff coverage is 64.91%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5039      +/-   ##
==========================================
+ Coverage   57.34%   57.37%   +0.03%     
==========================================
  Files         138      138              
  Lines       17391    17418      +27     
==========================================
+ Hits         9972     9993      +21     
- Misses       7419     7425       +6     
Impacted Files Coverage Δ
src/libsync/networkjobs.h 4.76% <50.00%> (-0.24%) ⬇️
src/libsync/networkjobs.cpp 50.28% <65.45%> (+0.28%) ⬆️
src/libsync/discovery.cpp 84.05% <0.00%> (+0.28%) ⬆️
src/libsync/propagateremotemkdir.cpp 65.73% <0.00%> (+0.69%) ⬆️
src/libsync/propagatorjobs.cpp 73.23% <0.00%> (+1.40%) ⬆️

@mgallien mgallien merged commit 5baa254 into master Oct 17, 2022
@mgallien mgallien deleted the bugfix/checkTokenForEditLocallyRequests branch October 17, 2022 09:13
@allexzander
Copy link
Contributor

/backport to stable-3.6

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

3 participants