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

Don't allow the user to set fields they can't see #5223

Merged
merged 3 commits into from
Jun 6, 2017

Conversation

nickvergessen
Copy link
Member

Signed-off-by: Joas Schilling coding@schilljs.com

Signed-off-by: Joas Schilling <coding@schilljs.com>
Copy link
Member

@schiessle schiessle left a comment

Choose a reason for hiding this comment

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

looks good to me

@blizzz
Copy link
Member

blizzz commented Jun 2, 2017

the same issue exists in settings/Controller/UserController

Signed-off-by: Joas Schilling <coding@schilljs.com>
@codecov
Copy link

codecov bot commented Jun 2, 2017

Codecov Report

Merging #5223 into master will decrease coverage by 22.87%.
The diff coverage is 16.21%.

@@              Coverage Diff              @@
##             master    #5223       +/-   ##
=============================================
- Coverage     54.15%   31.27%   -22.88%     
- Complexity    22292    22305       +13     
=============================================
  Files          1379     1380        +1     
  Lines         85359    84813      -546     
  Branches       1321     1321               
=============================================
- Hits          46223    26526    -19697     
- Misses        39136    58287    +19151
Impacted Files Coverage Δ Complexity Δ
settings/Controller/UsersController.php 0% <0%> (-69.92%) 112 <0> (+3)
...rovisioning_api/lib/Controller/UsersController.php 81.21% <42.85%> (-1.89%) 116 <0> (+3)
...public/AppFramework/OCS/OCSBadRequestException.php 0% <0%> (-100%) 1% <0%> (ø)
apps/user_ldap/lib/Migration/UUIDFixUser.php 0% <0%> (-100%) 1% <0%> (ø)
...rivate/Authentication/Token/DefaultTokenMapper.php 0% <0%> (-100%) 11% <0%> (ø)
apps/files_sharing/lib/SharedPropagator.php 0% <0%> (-100%) 1% <0%> (ø)
apps/user_ldap/lib/Mapping/UserMapping.php 0% <0%> (-100%) 1% <0%> (ø)
apps/comments/lib/AppInfo/Application.php 0% <0%> (-100%) 1% <0%> (ø)
lib/private/Files/Mount/CacheMountProvider.php 0% <0%> (-100%) 4% <0%> (ø)
apps/files_trashbin/lib/AppInfo/Application.php 0% <0%> (-100%) 2% <0%> (ø)
... and 372 more

@nickvergessen
Copy link
Member Author

Done @blizzz

@blizzz
Copy link
Member

blizzz commented Jun 2, 2017

@nickvergessen thx, works!

@schiessle schiessle added 4. to release Ready to be released and/or waiting for tests to finish 2. developing Work in progress and removed 3. to review Waiting for reviews 4. to release Ready to be released and/or waiting for tests to finish labels Jun 2, 2017
@schiessle
Copy link
Member

@nickvergessen seems like you need to adjust some tests...

@MorrisJobke
Copy link
Member

@nickvergessen seems like you need to adjust some tests...

Ping

@nickvergessen
Copy link
Member Author

Yeah morris, sorry there was a long weekend in german, will fix now.

Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jun 6, 2017
@MorrisJobke
Copy link
Member

Failing tests (autoloader) fixed in another PR already.

@MorrisJobke MorrisJobke merged commit 15314b6 into master Jun 6, 2017
@MorrisJobke MorrisJobke deleted the do-not-allow-to-set-invisible-fields branch June 6, 2017 13:06
@MorrisJobke
Copy link
Member

Let me backport this.

@skjnldsv
Copy link
Member

Hey everyone!
Any reason we're forbidding the editing of email with this?
I think it should be more clear. The name of the config is allow_user_to_change_display_name it never mention email :)

Also, it now prevent admin to edit those fields if they're the current user, I think we should always let the admin edit those fields then. Thoughts? cc @rullzer

@nickvergessen
Copy link
Member Author

The name of the config is allow_user_to_change_display_name it never mention email :)

Yeah well, this is one of the stories with a long history even on OC already.

Also, it now prevent admin to edit those fields if they're the current user, I think we should always let the admin edit those fields then.

fine by me

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.

5 participants