Skip to content

Conversation

Alexander-Ger-Reich
Copy link
Contributor

I wrote an add-on that requires the client to provide the SHA-256 hash of the uploaded files and chunks.

-
-
@Alexander-Ger-Reich Alexander-Ger-Reich force-pushed the Alexander-Ger-Reich-patch-4 branch from 85c283e to 774eb29 Compare May 15, 2025 19:14
@Alexander-Ger-Reich Alexander-Ger-Reich force-pushed the Alexander-Ger-Reich-patch-4 branch from 2f73b59 to df8311e Compare May 15, 2025 19:36
Signed-off-by: Alexander-Ger-Reich <50119493+Alexander-Ger-Reich@users.noreply.github.com>
@Alexander-Ger-Reich Alexander-Ger-Reich force-pushed the Alexander-Ger-Reich-patch-4 branch from df8311e to 012b77b Compare May 15, 2025 19:39
@Alexander-Ger-Reich
Copy link
Contributor Author

DCO is happy

Signed-off-by: Alexander-Ger-Reich <50119493+Alexander-Ger-Reich@users.noreply.github.com>
Signed-off-by: Alexander-Ger-Reich <50119493+Alexander-Ger-Reich@users.noreply.github.com>
Signed-off-by: Alexander-Ger-Reich <50119493+Alexander-Ger-Reich@users.noreply.github.com>
Signed-off-by: Alexander-Ger-Reich <50119493+Alexander-Ger-Reich@users.noreply.github.com>
Signed-off-by: Alexander-Ger-Reich <50119493+Alexander-Ger-Reich@users.noreply.github.com>
Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

Copy link
Contributor

@alperozturk96 alperozturk96 left a comment

Choose a reason for hiding this comment

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

Hello

Thank you for the PR.

I've suggested a few naming changes to align with the naming conventions used throughout the project for consistency. I will also be testing the PR shortly.


if (transferred == totalToTransfer || transferEncoding) {
savedFile = true;
if (transferred == totalToTransfer || transferEncoding) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If possible, could you apply the fail fast here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by that?

Copy link
Contributor

@alperozturk96 alperozturk96 May 30, 2025

Choose a reason for hiding this comment

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

Instead of having nested if-else blocks, you can return early. However, I think the downloadFile() function is too complex for that. We can skip it, if it’s too much work to refactor.

e.g.

Before:

public void checkAge(int age) {
    if (age >= 0) {
        if (age >= 18) {
            System.out.println("User is an adult.");
        } else {
            System.out.println("User is a minor.");
        }
    } else {
        System.out.println("Invalid age.");
    }
}

After:

public void checkAge(int age) {
    if (age < 0) {
        System.out.println("Invalid age.");
        return;
    }

    if (age >= 18) {
        System.out.println("User is an adult.");
        return;
    }
    
    System.out.println("User is a minor.");
}

Alexander-Ger-Reich and others added 6 commits May 27, 2025 15:36
…ClientManagerFactory.java


Signed-off-by: Alexander-Ger-Reich <50119493+Alexander-Ger-Reich@users.noreply.github.com>

Co-authored-by: Alper Öztürk <67455295+alperozturk96@users.noreply.github.com>
Signed-off-by: Alexander-Ger-Reich <50119493+Alexander-Ger-Reich@users.noreply.github.com>
Signed-off-by: Alexander-Ger-Reich <50119493+Alexander-Ger-Reich@users.noreply.github.com>

Co-authored-by: Alper Öztürk <67455295+alperozturk96@users.noreply.github.com>
Signed-off-by: Alexander-Ger-Reich <50119493+Alexander-Ger-Reich@users.noreply.github.com>
@tobiasKaminsky
Copy link
Member

I am very late to this, sorry for that!
I see that this function setHashDownloadCheck is not being used inside library.
Thus I assume that this is within the customized files app?

Since we want to have only code that is actually used by the core Nextcloud system, what do you about this:

  • move sha check/generation to your customized app
  • enhance library to allow to pass/read any header
    --> this way the customized sha logic would be in your app, but the library will support it?

@Alexander-Ger-Reich
Copy link
Contributor Author

I am very late to this, sorry for that! I see that this function setHashDownloadCheck is not being used inside library. Thus I assume that this is within the customized files app?

Since we want to have only code that is actually used by the core Nextcloud system, what do you about this:

  • move sha check/generation to your customized app
  • enhance library to allow to pass/read any header
    --> this way the customized sha logic would be in your app, but the library will support it?

Do you know if there is already another method to check whether the file on the client is 100% the same as the file on the server during upload? Is this already happening? Is a hash already being generated somewhere else and communicated to the server? I only started doing this because I hadn't seen anything like it before.

@tobiasKaminsky
Copy link
Member

Do you know if there is already another method to check whether the file on the client is 100% the same as the file on the server during upload? Is this already happening? Is a hash already being generated somewhere else and communicated to the server? I only started doing this because I hadn't seen anything like it before.

No, we do not do this.
But we have for each and every upload/chunked upload a status code.
So it is assumed that if one transfer/chunk is working, then also the resulting file is correct.

@Alexander-Ger-Reich
Copy link
Contributor Author

Man

Do you know if there is already another method to check whether the file on the client is 100% the same as the file on the server during upload? Is this already happening? Is a hash already being generated somewhere else and communicated to the server? I only started doing this because I hadn't seen anything like it before.

No, we do not do this. But we have for each and every upload/chunked upload a status code. So it is assumed that if one transfer/chunk is working, then also the resulting file is correct.

I could add MD5 as an algorithm. To save power and time, it would only apply to the entire file, not individual chunks. The server would then have to enter this information into the file without changing it (metadata). All official changes to a file would change it, but any tampering would be immediately detected by a different hash.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants