Uh oh!
There was an error while loading. Please reload this page.
Return correct loginname in credentials - #21288
Conversation
lmamane
commented
Jun 7, 2020
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 👍
|
ChristophWurst
left a comment
There was a problem hiding this comment.
Fine from a functionality PoV, but a bit problematic in terms of code quality
Let's make it a bit less error-prone at least
Uh oh!
There was an error while loading. Please reload this page.
lmamane
commented
Jun 8, 2020
via email
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? |
kesselb
commented
Jun 8, 2020
Seems the encryption app run into a similar issue before. Adding the loginName to the event sounds good but is quite hard to backport. |
lmamane
commented
Jun 8, 2020
On the "hard to backport" part, we could imagine having this one-liner fix in the stable branches, and the event change in master? |
ChristophWurst
commented
Jun 9, 2020
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). |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
e23b754 to
60cf6dcCompare| * @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.
MorrisJobke
commented
Jul 6, 2020
The one liner. |
MorrisJobke
commented
Jul 9, 2020
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>
MorrisJobke
commented
Aug 20, 2020
Rebased to fix a conflict. |
Fix#21285