-
-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow using an app token to login with v2 flow auth #29531
Conversation
f12a745
to
558f6e0
Compare
/** | ||
* @PublicPage | ||
*/ | ||
public function apptokenRedirect(string $stateToken, string $user, string $password) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code looks good, didn't test
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 |
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>
ced51d7
to
cdda25a
Compare
@jancborchardt Updated: |
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: