-
Notifications
You must be signed in to change notification settings - Fork 196
feat: exchange cloud ID #4417
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
base: main
Are you sure you want to change the base?
feat: exchange cloud ID #4417
Conversation
|
General layout plan:
Status:
|
ea81812 to
7c0eaa0
Compare
|
Hello there, 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.) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4417 +/- ##
===========================================
- Coverage 10.76% 1.84% -8.93%
===========================================
Files 128 113 -15
Lines 6512 6082 -430
Branches 1191 1270 +79
===========================================
- Hits 701 112 -589
- Misses 5690 5847 +157
- Partials 121 123 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks for the contribution, @redblom!
Is there any way this could work without introducing an app dependency? |
Short answer: No, that is to say, the actual cloud ID exchange is not a responsibility of the contacts app but the contacts app (the cloud ID field) is a logical place where to have this option (as per earlier discussion with the Design Team). |
0112a78 to
e7c2604
Compare
|
Thanks for the clarification |
76d6df3 to
ca42433
Compare
ca42433 to
62ce0c2
Compare
632955b to
5b7820f
Compare
We do not persist the optional personal message with the invite. So that's lost for a resend. But in order to correlate to the previously sent invite, what we can do is to use a new message like: Fwiw I prefer this to persisting the message. |
9f0bca4 to
55647b8
Compare
All the necessary plumbing is in Nextcloud 32.0.0.0rc4 so no external apps needed anymore. |
55647b8 to
20fc781
Compare
e51a605 to
153fdb1
Compare
3fba972 to
f9574de
Compare
26a76f8 to
2272c9f
Compare
|
@ArtificialOwl could you give it a go, the review. |
ArtificialOwl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quick review, not fully complete, but already some elements to rework
lib/WayfProvider.php
Outdated
| * @return string the WAYF login page endpoint | ||
| */ | ||
| public function getWayfEndpoint(): string { | ||
| $wayfEndpoint = 'https://' . $this->federatedInvitesService->getProviderFQDN() . '/apps/' . Application::APP_ID . self::WAYF_ROUTE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is a possibility that contacts is installed in /custom_apps/, not sure this endpoint would 100% work
lib/WayfProvider.php
Outdated
|
|
||
| public function getMeshProvidersFromCache(): array { | ||
| $json = $this->appConfig->getValueString(Application::APP_ID, 'federations_cache'); | ||
| $data = json_decode($json, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can use getValueArray() / setValueArray() and set the config value as lazy
lib/IWayfProvider.php
Outdated
| * Interface IWayfProvider. | ||
| * | ||
| */ | ||
| interface IWayfProvider { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it does not feel like an interface is really needed
lib/Command/DisableOcmInvites.php
Outdated
| } | ||
|
|
||
| protected function execute(InputInterface $input, OutputInterface $output): int { | ||
| $isAlreadyDisabled = $this->appConfig->getValueBool('contacts', 'ocm_invites_enabled') === false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getValueBool is already a boolean, no need for comparaison
lib/Command/DisableOcmInvites.php
Outdated
| } | ||
|
|
||
| $this->appConfig->setValueBool('contacts', 'ocm_invites_enabled', false); | ||
| $this->appConfig->deleteKey('core', 'ocm_invite_accept_dialog'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be better to use OC\Core\AppInfo\ConfigLexicon::OCM_INVITE_ACCEPT_DIALOG despite being from private OC\
|
|
||
| public function isOcmInvitesEnabled():bool { | ||
| $val = $this->appConfig->getValueBool(Application::APP_ID, 'ocm_invites_enabled', FederatedInvitesService::OCM_INVITES_ENABLED_BY_DEFAULT); | ||
| $boolval = (is_string($val) ? filter_var($val, FILTER_VALIDATE_BOOLEAN, FILTER_NULL_ON_FAILURE) : (bool)$val); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getValueBool() will always returns boolean
| * @return JSONResponse | ||
| */ | ||
| #[NoAdminRequired] | ||
| #[NoCSRFRequired] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why no CSRF ? is the endpoint to be requested from a remote instance ?
| #[NoAdminRequired] | ||
| #[NoCSRFRequired] | ||
| public function deleteInvite(string $token): JSONResponse { | ||
| if (!isset($token)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the feeling $token is always set
lib/Cron/UpdateOcmProviders.php
Outdated
| $data = $this->wayfProvider->getMeshProviders(); | ||
| $data['expires'] = time() + $this->expire_time; | ||
| $json = json_encode($data); | ||
| $this->appConfig->setValueString(Application::APP_ID, 'federations_cache', $json); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as said somewhere else, use setValueArray() and set it as lazy
| /** | ||
| * @template-extends QBMapper<FederatedInvite> | ||
| */ | ||
| class FederatedInviteMapper extends DbFederatedInviteMapper { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a huge fan of this,
if the DB is created and managed from cloud_federation_api, could this request be also integrated to the app, and calling an API from the cloud_federation_api to select/update/delete data from/to database ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I understand correctly that you propose to specify a db layer API/interface in cloud_federation_api and let the contacts app implement that? Meaning that cloud_federation_api can only access the db (and be functional) if there is an app implementing the API (the contacts app in this case)?
…ion workflow see https://github.com/cs3org/OCM-API/blob/v1.2.1/IETF-RFC.md Features: - Button to invite remote users to exchange cloudIDs. - Button to manually accept invite to exchange cloudIDs. - WAYF page allowing the receiver of the invite to open and accept the invitation. - Listing of open invitations. - Option to resend, revoke open invitations. Signed-off-by: antoonp <[email protected]> Co-authored-by: Micke Nordin <[email protected]>
IWayfProvider is redundant.
Use core config lexicon.
069d061 to
86027f9
Compare
Signed-off-by: Antoon P. <[email protected]>






This is a work in progressThis PR implements #4416