Allow using an app token to login with v2 flow auth #29531
Conversation
juliushaertl
force-pushed
the
bugfix/noid/flow-auth-v2-apptoken
branch
from
f12a745
to
558f6e0
Compare
juliushaertl
requested review from
tobiasKaminsky,
skjnldsv,
a team,
ArtificialOwl,
CarlSchwan and
blizzz
and removed request for
a team
| /** | ||
| * @PublicPage | ||
| */ | ||
| public function apptokenRedirect(string $stateToken, string $user, string $password) { |
There was a problem hiding this comment.
this is identical to the same method of ClientFlowLoginController.php, innit? Maybe moving this into a trait to avoid duplication?
There was a problem hiding this comment.
The only difference there is the method call to validate the sate token which is isValidStateToken vs isValidToken, but we can adjust those of course. Moving that into a trait makes sense 👍
There was a problem hiding this comment.
OK, so it requires some more restructuring and actually going for a base controller class seems more suitable, though in order to keep this a smaller backportable change I'd rather do this in a follow up for master only. What do you think? I'd push the refactoring then to a separate PR.
There was a problem hiding this comment.
OK, so it requires some more restructuring and actually going for a base controller class seems more suitable, though in order to keep this a smaller backportable change I'd rather do this in a follow up for master only. What do you think? I'd push the refactoring then to a separate PR.
Sane assessment, I agree with you.
|
Needs some further work since when using it in the client we'll need to update the poll result on v2 instead of performing the redirect |
juliushaertl
added
2. developing
juliushaertl
force-pushed
the
bugfix/noid/flow-auth-v2-apptoken
branch
from
558f6e0
to
ced51d7
Compare
|
Design-wise, instead of that separate section, what would be a bit more obvious is the text/link "Alternative login using app token" in the same big container, below the big "Log in" button, centered, and underlined. |
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
juliushaertl
force-pushed
the
bugfix/noid/flow-auth-v2-apptoken
branch
from
ced51d7
to
cdda25a
Compare
|
@jancborchardt Updated:
|
juliushaertl
added
3. to review
The possibility to authenticate using an existing app token was only present in v1 flow auth, however there are use cases for it with v2 as well, e.g. when SAML is setup as primary login method but there are some local users that might need to login with the desktop client.
This is the button that this is adding for v2: