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

UploaderStrategy still requires api_secret if parameter was already added externally #125

Open
amirulzin opened this issue May 7, 2018 · 4 comments
Assignees

Comments

@amirulzin
Copy link

amirulzin commented May 7, 2018

This issue affects all current http artifacts:

if (requiresSigning(action, options)) {
uploader.signRequestParams(params, options);
} else {
Util.clearEmpty(params);
}

Compared to the implementation on cloudinary-android:

if (requiresSigning(action, options)) {
    String apiKey = ObjectUtils.asString(options.get("api_key"), this.cloudinary().config.apiKey);
    if (apiKey == null)
        throw new IllegalArgumentException("Must supply api_key");
    if (options.containsKey("signature") && options.containsKey("timestamp")) {
        params.put("timestamp", options.get("timestamp"));
        params.put("signature", options.get("signature"));
        params.put("api_key", apiKey);
    } else {
        String apiSecret = ObjectUtils.asString(options.get("api_secret"), this.cloudinary().config.apiSecret);
        if (apiSecret == null)
            throw new IllegalArgumentException("Must supply api_secret");
        params.put("timestamp", Long.valueOf(System.currentTimeMillis() / 1000L).toString());
        params.put("signature", this.cloudinary().apiSignRequest(params, apiSecret));
        params.put("api_key", apiKey);
    }
}

Cloudinary Android Source: https://github.com/cloudinary/cloudinary_android/blob/f7a0b32cfd9f2b6507bb6461043e6c89d9478c03/lib/src/main/java/com/cloudinary/android/UploaderStrategy.java#L43-L59

My suggestion is to implement that for each http artifact or simply refactor the android implementation upwards to the core AbstractUploaderStrategy.

This allows cloudinary-java users to not have to enter api_secret if say the signature is generated via other microservices/authorization servers. This also allows easier server-side integration tests.

While I understand the audience for cloudinary-java is more towards server users where the secret key is most likely exposed within a monolithic service, the two reasons above are currently forcing us to mangle a separate UploaderStrategy which simply use the above Android part instead.

@yakirp yakirp self-assigned this May 7, 2018
@yakirp
Copy link
Contributor

yakirp commented May 7, 2018

Hi @amirulzin,

Thank you for pointing us to this issue, make a lot of sense.

Our Dev Team will review this for prioritization.

Thanks,
Yakir

@OleksandrYuvkoExt
Copy link

OleksandrYuvkoExt commented Aug 19, 2021

Hi, still no news? Java SDK don't allow to use pre-generated "signature", and trying to get "api_secret" even if "signature" is included. Is it allowed to create a pull request from side person(I will just move functionality from android repo cloudinary_android to cloudinary_java)?

@francistagbo
Copy link
Contributor

Hi @OleksandrYuvkoExt,

Thanks for your comment!
I am checking this with our team internally, I will update you once I have more insights.

Regards,
Francis

@francistagbo
Copy link
Contributor

Hi @OleksandrYuvkoExt

Yes, we do welcome users to create a PR.
We will review it once you have submitted it.

Thank you!

Regards,
Francis

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

No branches or pull requests

4 participants