Juan Hernandez has posted comments on this change. Change subject: core, webadmin, restapi: Detach password from VdcUser ......................................................................
Patch Set 3: (1 comment) .................................................... Commit Message Line 12: passwords, and that is not always true, as some authentication Line 13: mechanisms don't use passwords for authentication. This change detaches Line 14: the password from the VdcUser class, storing it independently in the Line 15: session, so that it will be easier later to use other types of Line 16: authentication mechanisms. I agree that it is natural and logical to have the password attached to the user concept, as it is very usual to have that in the real world that we are trying to mdel. But doing so introduces several technical problems. We have two classes to represent user data (assuming that we accept to merge VdcUser and DbUser): DbUser and LdapUser (that will become DirectoryUser in the future). Which one of these should contain the password? The password isn't stored in the database, and I think that we all will agree that it shouldn't, so it doesn't belong there. Same for the DirectoryUser, if we want to make that generic then the password doesn't belong there either, as not all directories provide the password of the user (rather the opposite, most don't). Also, having the password attached to the user class means that we have the password potentially traveling all over the system, even when it isn't needed at all. Not saying that this happens today, but imagine that somehow we send ! a serialized version of an VdcUser containing a password to the GUI. That GUI instance may end up having in memory the passwords of many users. The suggestion to have a new LoginContext object, attached to the session, makes sense to me. It could be a good place for things like a generic set of credentials of the user (not only one password, but maybe several, or Kerberos tickets, or Keystore authentication tokens, etc). But I really don't want to get there yet, not in this set of patches, otherwise it will be even harder to rebase and review, I just want to make sure that there is no password attribute in the DbUser or DirectoryUser classes. Line 17: Line 18: Change-Id: I0ab9321816ee28355a4118910086891f5a552014 -- To view, visit http://gerrit.ovirt.org/17096 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0ab9321816ee28355a4118910086891f5a552014 Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Juan Hernandez <[email protected]> Gerrit-Reviewer: Juan Hernandez <[email protected]> Gerrit-Reviewer: Michael Pasternak <[email protected]> Gerrit-Reviewer: Omer Frenkel <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: Vojtech Szocs <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
