Indicate user attributes allowed to change in UI#21975
Indicate user attributes allowed to change in UI#21975NobodysNightmare wants to merge 1 commit intodevfrom
Conversation
app/contracts/users/base_contract.rb
Outdated
| end | ||
|
|
||
| def authenticates_externally? | ||
| model.uses_external_authentication? || @user.ldap_auth_source_id |
There was a problem hiding this comment.
why @user here? This is targeting the user calling the contract. I think it should be model
There was a problem hiding this comment.
Excellent catch. Pure copy & paste error.
(I'll make sure that specs cover this error)
app/contracts/users/base_contract.rb
Outdated
| attribute :firstname, writable: ->(*) { can_create_or_manage_users? || (editing_self? && !authenticates_externally?) } | ||
| attribute :lastname, writable: ->(*) { can_create_or_manage_users? || (editing_self? && !authenticates_externally?) } |
There was a problem hiding this comment.
Maybe personal taste, but this a || (b && c) construct needs a lot of brain waves to parse. I had to read it a few times.
WDYT about
attribute :firstname, writable: ->(*) { can_edit_name? }
def can_edit_name?
return false if authenticates_externally?
return true if can_create_or_manage_users?
editing_self?
end
This would also give a nice place to add the comments about why we are checking for a certain condition.
I know the method I wrote is not the same, but i the name is coming from an external source, would we even allow a user with the manage user permission to update the names?
There was a problem hiding this comment.
but i the name is coming from an external source, would we even allow a user with the manage user permission to update the names?
That's a good question. I wanted to change as little actual behaviour as possible (I changed a bit, though). For example by preventing everyone from changing names from an external auth source, I'd need to make sure that "privileged" code can still change it, but I am not sure whether that might use the same contracts. With privileged code I mean:
- The "external authentication" code that prevents those changes here
- LDAP
- SCIM
Since I am still in the scope of a bugfix that complained about UI misbehaviour, I thought that it would be better to keep the change scope lower than that (though I am willing to extend it, if pulling on another spaghetti forces me to change more).
There was a problem hiding this comment.
I extracted something, but different from your suggestion.
NobodysNightmare
force-pushed
the
user-edit-consistency
branch
5 times, most recently
from
01a68b0 to
cdba705
Compare
The decision on which fields are writable has been moved into the user's contract. Previously the fields in the My Account form were only disabled in the frontend, but when changing the browser form, the user could still update values. Now the contract has been updated to check the authentication method as well, denying changes to the own name and email address. Similarly when the contract prevents edits, this is also reflected in the admin UI for users. It's not pretty, because there seems to be no formatting for disabled text boxes in our old forms, but it still gives faster feedback than the old forms, where you had to submit to see that a change to your own login was rejected. In the My Accounts form, this change leads to the curious case, where an admin CAN change their own name in the My Account view, because admins could do it in other forms as well. It's surprising, but consistent. Additionally the behaviour has been harmonized across LDAP and other auth providers. Editing the email address is now also forbidden in all cases for the user themselves.
NobodysNightmare
force-pushed
the
user-edit-consistency
branch
from
cdba705 to
9c8837d
Compare
2 participants
Previously we had no UI feedback in some forms and rules that were inconsistent with the underlying contract in other forms (allowing to change things that were disabled in the UI).
This PR harmonizes the UI to reflect what's allowed according to the underlying contract.
Ticket
https://community.openproject.org/wp/71099