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

Displayname nor email won't be editable from the profile page #218

Merged
merged 1 commit into from
Jun 21, 2018

Conversation

jvillafanez
Copy link
Member

Fix https://github.com/owncloud/enterprise/issues/2459 by making the profile read-only for the displayname and email.

Consider this PR as untested unless told otherwise.

@jvillafanez jvillafanez added this to the development milestone Mar 27, 2018
@codecov-io
Copy link

codecov-io commented Mar 27, 2018

Codecov Report

Merging #218 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #218      +/-   ##
============================================
+ Coverage     34.88%   34.89%   +<.01%     
+ Complexity     1358     1357       -1     
============================================
  Files            32       32              
  Lines          3927     3926       -1     
============================================
  Hits           1370     1370              
+ Misses         2557     2556       -1
Impacted Files Coverage Δ Complexity Δ
lib/User_LDAP.php 40.42% <ø> (ø) 36 <0> (ø) ⬇️
lib/User_Proxy.php 0% <ø> (ø) 41 <0> (-1) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a53c56...eb6a3fd. Read the comment docs.

@jvillafanez
Copy link
Member Author

Working in OC 10.0.7 + this branch.

@tomneedham
Copy link
Contributor

@butonic why did we have to add this in....?

@tomneedham
Copy link
Contributor

I think this might not be required with the sync changes in 10.0.8. I think it was an old bug that wouldnt sync the displayname unless this was set. Need to recheck

@jvillafanez
Copy link
Member Author

This is needed to bring back the same behaviour we have in 9.1.7. The problem isn't about syncing, it's about what are you editing there. Are you editing the user backend, or the account information? What will happen during the next sync if such value has been modified?

This is a quick fix in order to have the same behaviour we had before. There are a lot of discussions pending to move forward with a better and cleaner solution.

@jvillafanez
Copy link
Member Author

Rebased

@jvillafanez
Copy link
Member Author

bump!

Do we still want this?

@PVince81
Copy link
Contributor

Need careful review to make sure there are no side effects related to account table @tomneedham @DeepDiver1975

@butonic
Copy link
Member

butonic commented Jun 18, 2018

the line was introduced with e1b8f6d because of syncing, see reason in owncloud/core#29669 (comment)

provisioning ldap users via saml was broken, because the user_shibboleth app was trying to set the displayname via the user object: https://github.com/owncloud/user_shibboleth/pull/207/files#diff-eec818f31669ac6d6b4d1161cc38794bR447

at the time the immediate fix was to release a new user_ldap and user_shibboleth app. now the logic sync logic has moved to core and we can get rid of this. but two more things need to be done:

  • kill
    public function setDisplayName($uid, $displayName) {
    // ignored, function needed to make core store the display name in the account table
    }
    with 🔥
  • increase min oc version to 10.0.8, which at least for master already is the case

@jvillafanez
Copy link
Member Author

killed dead code and rebased

@PVince81 PVince81 requested a review from butonic June 18, 2018 13:32
Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍 approving as this was already agreed upon with @butonic

@PVince81 PVince81 merged commit e96673c into master Jun 21, 2018
@tomneedham
Copy link
Contributor

tomneedham commented Jun 21, 2018

Concerned this is just a revert of c88e91c#diff-4dae59834739e07c67b0df543693a485

@tomneedham
Copy link
Contributor

Ah, probably ok, since we now added a check in the sync to only update the backend if set is supported owncloud/core@05a82b5

This was not the case previously and so it was resolved by adding this hack

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