Return correct loginname in credentials #21288
Conversation
|
This fixes issue #21285 but I cannot figure out how to add it to linked issue. I followed the github documentation at https://help.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue, but:
|
|
Thanks for debugging 👍
|
kesselb
added
3. to review
|
On Mon, Jun 08, 2020 at 12:01:04AM -0700, Christoph Wurst wrote:
@ChristophWurst requested changes on this pull request.
Fine from a functionality PoV, but a bit problematic in terms of code quality
I had started a bigger change, that would have added the loginname to
the associative array (key-value pair set) returned by
$this->session->get('login_credentials')
This entailed changing the event that sets login_credentials, namely
('OC_User', 'post_login'), and also ('\OC\User', 'postLogin') to add
this parameter. However, I wasn't sure if that would touch a
guaranteed stable public API (e.g. for apps) or not.
Changing the ('OC_User', 'post_login') looked rather safe, since it
gives an associative array; changing ('\OC\User', 'postLogin') looked
non-backwards compatible (since it gives multiple parameters, not a
single associative array) _but_ it looked like it could be a private
internal API, and changing all listeners within Nextcloud itself would
be sufficient? But I wasn't sure.
Then I saw this code pattern
$this->session->get('loginname')
elsewhere in the code and thought "ah, that's how they do it".
If you tell me my first idea was better, and that the "public API"
worries are unfounded, I will gladly submit a new pull request based
in that.
Please let me know.
> @@ -112,7 +112,7 @@ public function getLoginCredentials(): ICredentials {
if ($trySession && $this->session->exists('login_credentials')) {
$creds = json_decode($this->session->get('login_credentials'));
- return new Credentials($creds->uid, $creds->uid, $creds->password);
+ return new Credentials($creds->uid, $this->session->get('loginname'), $creds->password);
This is a bit of a red flag for me as this `loginname` value from
the session is set elsewhere. So this relies on a few assumptions,
making the code harder to understand and introducing undocumented
dependencies between this code and the code that sets the value.
In any case, can we maybe add a fallback to the UID when the session
value is not there? Like ``$this->session->get('loginname') ??
$this->uid``?
I would not put a fallback to a value that is a wrong one, and
systematically so in some scenarios (such as LDAP external
authentication with uid != loginname). That already created a subtle
and hard to track bug (app tokens would work for a few minutes and
then silently stop working...), I don't think keeping such things in
the codebase is a good idea. If we cannot get the loginname, then we
can return an _exception_, or maybe the value null?
|
Seems the encryption app run into a similar issue before. Adding the loginName to the event sounds good but is quite hard to backport. |
|
On the "hard to backport" part, we could imagine having this one-liner fix in the stable branches, and the event change in master? |
Fine by me |
My current patch backports easily to stable19 branch. After you are happy with the current patch, let me know what version you want on stable19 (event change or "dirty" one-liner). |
lmamane
force-pushed
the
master
branch
2 times, most recently
from
e23b754
to
60cf6dc
Compare
| @@ -47,9 +53,10 @@ class PostLoginEvent extends Event { | |||
| /** | |||
| * @since 18.0.0 | |||
| */ | |||
| public function __construct(IUser $user, string $password, bool $isTokenLogin) { | |||
| public function __construct(IUser $user, string $loginName, string $password, bool $isTokenLogin) { | |||
There was a problem hiding this comment.
FYI, we cannot backport this as it breaks public API.
The one liner. |
Stable19: #21779 |
even when token is invalid or has no password. Returning the uid as loginname is wrong, and leads to problems when these differ. E.g. the getapppassword API was creating app token with the uid as loginname. In a scenario with external authentication (such as LDAP), these tokens were then invalidated next time their underlying password was checked, and systematically ceased to function. Co-authored-by: kesselb <mail@danielkesselberg.de> for: switch to consistent camelCase Signed-off-by: Lionel Elie Mamane <lionel@mamane.lu>
… to uid != loginname Signed-off-by: Lionel Elie Mamane <lionel@mamane.lu>
|
Rebased to fix a conflict. |
Fix #21285