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(federation): auto-accept shares from trusted servers #49973

Merged
merged 7 commits into from
Jan 9, 2025

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Dec 26, 2024

Needs #49974
Fix #34550

image

@skjnldsv

This comment was marked as resolved.

@skjnldsv skjnldsv self-assigned this Dec 26, 2024
@skjnldsv skjnldsv added this to the Nextcloud 31 milestone Dec 26, 2024
@skjnldsv skjnldsv force-pushed the feat/auto-accept-trusted-server branch 2 times, most recently from ff6f6e6 to 5d4c7b7 Compare December 26, 2024 16:08
@skjnldsv skjnldsv changed the base branch from master to backport/49973/master December 26, 2024 16:08
@skjnldsv skjnldsv force-pushed the feat/auto-accept-trusted-server branch 2 times, most recently from e29a165 to c78d1fe Compare December 26, 2024 17:47
@skjnldsv skjnldsv force-pushed the backport/49973/master branch from 1d4cadb to a710e0c Compare December 27, 2024 09:08
@skjnldsv skjnldsv force-pushed the feat/auto-accept-trusted-server branch 9 times, most recently from 0ba688c to 9e36462 Compare December 28, 2024 07:48
@skjnldsv skjnldsv marked this pull request as ready for review December 28, 2024 07:48
@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Dec 28, 2024
@skjnldsv skjnldsv requested a review from a team December 28, 2024 07:49
Base automatically changed from backport/49973/master to master January 2, 2025 10:50
@skjnldsv skjnldsv force-pushed the feat/auto-accept-trusted-server branch from 8b3c276 to 2467a25 Compare January 2, 2025 12:13
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

A bit of nitpick, apart from that backend code looks good.

apps/federatedfilesharing/lib/FederatedShareProvider.php Outdated Show resolved Hide resolved
apps/federation/lib/Controller/SettingsController.php Outdated Show resolved Hide resolved
Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

Some things that could be improved over the old code, if you're already migrating it anyway 🙈

apps/federatedfilesharing/lib/FederatedShareProvider.php Outdated Show resolved Hide resolved
apps/federation/appinfo/routes.php Outdated Show resolved Hide resolved
apps/federation/js/settings-admin.js Show resolved Hide resolved
apps/federation/lib/Controller/SettingsController.php Outdated Show resolved Hide resolved
apps/federation/lib/Controller/SettingsController.php Outdated Show resolved Hide resolved
@skjnldsv skjnldsv force-pushed the feat/auto-accept-trusted-server branch 4 times, most recently from 6bf6515 to 968be50 Compare January 9, 2025 14:09
@skjnldsv skjnldsv enabled auto-merge January 9, 2025 14:22
@skjnldsv skjnldsv force-pushed the feat/auto-accept-trusted-server branch from 968be50 to 669e6ca Compare January 9, 2025 14:39
@skjnldsv skjnldsv requested a review from Altahrim January 9, 2025 15:05
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Looks sane, didn't test

margin-top: 20px;
}

.settings-subsection__name {
Copy link
Member

Choose a reason for hiding this comment

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

nit: use scss and move this into .settings-subsection?

$servers = $this->trustedServers->getServers();

// obfuscate the shared secret
$servers = array_map(function ($server) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: introduce a new variable instead of repurposing the old one.

Suggested change
$servers = array_map(function ($server) {
$redactedServers = array_map(function ($server) {

try {
$this->trustedServers->getServer($id);
} catch (\Exception $e) {
throw new OCSNotFoundException($this->l->t('No server found with ID: %s', [$id]));
Copy link
Member

Choose a reason for hiding this comment

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

It could be something else as well, other than server not found. Log the exception at debug level.

@skjnldsv skjnldsv merged commit e346cf6 into master Jan 9, 2025
188 checks passed
@skjnldsv skjnldsv deleted the feat/auto-accept-trusted-server branch January 9, 2025 15:39
@ChristophWurst
Copy link
Member

I did not expect the fast merge :x

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.

Auto accept federated shares from trusted servers
5 participants