X Tutup
Skip to content

Prohibit email/displayname change via API when not allowed#39353

Merged
JammingBen merged 5 commits intomasterfrom
issues/39332
Oct 14, 2021
Merged

Prohibit email/displayname change via API when not allowed#39353
JammingBen merged 5 commits intomasterfrom
issues/39332

Conversation

@JammingBen
Copy link
Contributor

@JammingBen JammingBen commented Oct 13, 2021 •
edited
Loading

Description

When the configs allow_user_to_change_mail_address or allow_user_to_change_display_name are set to false, changing the corresponding values via the provisioning API is no longer possible.

Related Issue

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

AlexAndBear
AlexAndBear approved these changes Oct 13, 2021
View reviewed changes
Copy link

@AlexAndBear AlexAndBear left a comment

Choose a reason for hiding this comment

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

LGTM, wondering if we should throw back a 405 instead of silently omitting the email address, if set in the request ?

AlexAndBear
AlexAndBear reviewed Oct 13, 2021
View reviewed changes
@JammingBen
Copy link
Contributor Author

JammingBen commented Oct 13, 2021

LGTM, wondering if we should throw back a 405 instead of silently omitting the email address, if set in the request ?

It actually throws a 997 👍 The logic for this already exists a few lines below my implementation.

@phil-davis phil-davis mentioned this pull request Oct 13, 2021
Closed
@phil-davis
Copy link
Contributor

phil-davis commented Oct 13, 2021

Note: allow_user_to_change_display_name seems to have the same issue. See #39354 which has test scenarios for both settings.

Maybe that can also be fixed in this PR, and cherry-pick the acceptance tests from #39354

@JammingBen JammingBen changed the title Prohibit email change via API when it's not allowed Prohibit email/displayname change via API when not allowed Oct 13, 2021
@JammingBen
Copy link
Contributor Author

JammingBen commented Oct 13, 2021 •
edited
Loading

@phil-davis Nice catch, I adjusted the PR to work with allow_user_to_change_display_name.

Edit: Also cherry-picked your two commits with the acceptance tests.

@AlexAndBear
Copy link

AlexAndBear commented Oct 13, 2021

Perfect one 👍

jvillafanez
jvillafanez approved these changes Oct 13, 2021
View reviewed changes
Copy link
Member

@jvillafanez jvillafanez left a comment

Choose a reason for hiding this comment

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

Changes look good

@phil-davis phil-davis mentioned this pull request Oct 13, 2021
Closed
11 tasks
@ownclouders
Copy link
Contributor

ownclouders commented Oct 13, 2021

💥 Acceptance tests pipeline webUIManageUsersGrps-chrome-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/33032/131/1

@ownclouders
Copy link
Contributor

ownclouders commented Oct 13, 2021

💥 Acceptance tests pipeline webUIManageUsersGrps-chrome-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/33035/131/1

@ownclouders
Copy link
Contributor

ownclouders commented Oct 13, 2021

💥 Acceptance tests pipeline webUIManageUsersGrps-chrome-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/33036/131/1

@phil-davis
Copy link
Contributor

phil-davis commented Oct 14, 2021

Note: PR #39357 was merged - that had a webUI test code change that should make some group management webUI tests more reliable. I see that this branch has been rebased - good.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 14, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@JammingBen JammingBen merged commit f9bfd2a into master Oct 14, 2021
@delete-merged-branch delete-merged-branch bot deleted the issues/39332 branch October 14, 2021 07:51
This was referenced Oct 20, 2021
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jvillafanez jvillafanez jvillafanez approved these changes

@phil-davis phil-davis phil-davis approved these changes

+1 more reviewer

@AlexAndBear AlexAndBear AlexAndBear approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

app:provisioning_api

Projects

None yet

Milestone

No milestone

6 participants

@JammingBen @phil-davis @AlexAndBear @ownclouders @jvillafanez @sakshamgurung
X Tutup