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

feat: electronic signing, add settings for eIDEasy #4328

Merged
merged 1 commit into from
Dec 21, 2024

Conversation

vmiklos
Copy link
Contributor

@vmiklos vmiklos commented Dec 10, 2024

Summary

Electronic signing needs to store settings as richdocuments settings. This involves the API URL, a client ID visible to the browser and a secret, which is only used during server-side requests.

The WOPI CheckFileInfo reply sends this information to the COOL server, similar to how it's done for digital signing (via PEM files).

Add the settings as admin settings, otherwise normal users would be able to use eIDEasy services outside richdocuments.

CollaboraOnline/online#10630 (comment) has instructions on what test data to use to try out the service in a test environment. Additionally, if the test CA is configured to be trusted as a user setting, then the green stamp icon will show up in the status bar.

Checklist

  • Code is properly formatted
  • Sign-off message is added to all commits
  • Documentation (manuals or wiki) has been updated or is not required

@vmiklos
Copy link
Contributor Author

vmiklos commented Dec 10, 2024

@juliusknorr could you please review this when you have time?

How to test, if you want: take the eideasy test url/clientid/secret from CollaboraOnline/online#10630 (comment), go to the richdocuments admin settings and configure the 3 values. Then open a PDF in COOL, Insert menu -> add electronic signature, you'll need to decide what provider to use, accept the default.

Then you need a test personal ID, use the value from the above GH comment. Once the popup is closed, you will notice a signature green sign / warning sign in the statusbar (depending on if the test CA is manually trusted), you can view the signature there.

#4311 (comment) has a screenshot on how this looks like on my end currently. Thanks.

@vmiklos
Copy link
Contributor Author

vmiklos commented Dec 11, 2024

Oh and forgot to mention: it seems to me that none of the 4 failing checks are related to the changes in this PR, but please let me know if I overlooked something. Thanks.

@vmiklos
Copy link
Contributor Author

vmiklos commented Dec 12, 2024

One more small thing: the properties used here are now documented at https://sdk.collaboraonline.com/docs/advanced_integration.html#electronic-signature-handling, so it's a bit safer to depend on them here.

@juliusknorr juliusknorr added enhancement New feature or request 3. to review Ready to be reviewed labels Dec 12, 2024
@juliusknorr
Copy link
Member

Yes, CI failures are unrelated 👍

<h2>{{ t('richdocuments', 'Electronic signature settings') }}</h2>
<SettingsInputText v-model="settings.esignature_base_url"
:label="t('richdocuments', 'URL for the electronic signature API')"
:hint="t('richdocuments', 'The production API URL is https://id.eideasy.com, the test API URL is https://test.eideasy.com.')"
Copy link
Member

Choose a reason for hiding this comment

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

Not a blocker, just curious, apart from development is there any reason to configure the test API URL in productive usage? If not I'd remove the input for this one and rather document how to set this value with the CLI for developers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is basically for development only, you're right. Where to document this, is docs/app_settings.md a good place to mention it?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that sounds good, there might be more undocumented ones, but that is the best place from my perspective 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, let me see, I think by now this is the only outstanding part of this PR.

@juliusknorr
Copy link
Member

Left some quick comments, but generally looks fine from my side. 👍

Have not tested yet.

@vmiklos vmiklos force-pushed the private/vmiklos/main branch from 67bd10e to 45875ff Compare December 12, 2024 14:53
@vmiklos
Copy link
Contributor Author

vmiklos commented Dec 12, 2024

Just pushing what I have so far, I still need to address the comment about making the API URL cmdline-only + moving to ServerPrivateInfo.

@vmiklos vmiklos force-pushed the private/vmiklos/main branch from 45875ff to b1ce7cd Compare December 16, 2024 14:27
Electronic signing needs to store settings as richdocuments settings.
This involves the API URL, a client ID visible to the browser and a
secret, which is only used during server-side requests.

The WOPI CheckFileInfo reply sends this information to the COOL server,
similar to how it's done for digital signing (via PEM files).

Add the settings as admin settings, otherwise normal users would be able
to use eIDEasy services outside richdocuments.

<CollaboraOnline/online#10630 (comment)>
has instructions on what test data to use to try out the service in a
test environment. Additionally, if the test CA is configured to be
trusted as a user setting, then the green stamp icon will show up in the
status bar.

Signed-off-by: Miklos Vajna <vmiklos@collabora.com>
@vmiklos vmiklos force-pushed the private/vmiklos/main branch from b1ce7cd to a9fa62c Compare December 16, 2024 14:28
@vmiklos
Copy link
Contributor Author

vmiklos commented Dec 16, 2024

@juliusknorr please take a look, I think I addressed your comments so far. Thanks!

@vmiklos vmiklos requested a review from juliusknorr December 16, 2024 15:19
@vmiklos vmiklos merged commit 681ae39 into main Dec 21, 2024
73 checks passed
@vmiklos vmiklos deleted the private/vmiklos/main branch December 21, 2024 18:23
@vmiklos
Copy link
Contributor Author

vmiklos commented Dec 21, 2024

@elzody thanks for the review :)

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.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Ready to be reviewed enhancement New feature or request feedback-requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Electronically sign documents via eIDEasy
3 participants