-
Notifications
You must be signed in to change notification settings - Fork 372
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
Add MSC2604: Parameters for Login Fallback to the spec. #2709
Conversation
specification/client_server_api.rst
Outdated
@@ -1096,6 +1096,12 @@ This returns an HTML and JavaScript page which can perform the entire login | |||
process. The page will attempt to call the JavaScript function | |||
``window.onLogin`` when login has been successfully completed. | |||
|
|||
Non-credential related parameters of the login endpoint can be provided as query | |||
parameters, for example the ``device_id`` or ``initial_device_display_name``. |
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.
I would have liked to not list these out so we don't need to potentially update them in the future, but it seemed clearer to be explicit.
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.
otherwise looking good - thanks!
specification/client_server_api.rst
Outdated
Non-credential related parameters of the login endpoint can be provided as query | ||
parameters, for example the ``device_id`` or ``initial_device_display_name``. |
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.
Non-credential related parameters of the login endpoint can be provided as query | |
parameters, for example the ``device_id`` or ``initial_device_display_name``. | |
Non-credential parameters valid for the ``/login`` endpoint can be provided as query | |
string parameters here. For example:: |
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.
bonus points if /login
becomes a link.
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.
oh, rationale: we're not going to remember to update this so it's better to just leave it vague and open.
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.
Right. I couldn't figure out how to make it a link anyway. 😢
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.
Oh hmm, something got funky with accepting this one. Did you expect the "These are to be forwarded to the login endpoint during the login process." line to go away?
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.
I ended up removing that line as well, I think that's what you were going for!
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.
hmm, that line should have survived, I just forgot about it when writing the suggestion - sorry! I'll stuff it back in.
Co-authored-by: Travis Ralston <travpc@gmail.com>
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.
will merge manually
Thank you! 👍 |
This adds #2604 to the specification.
I feel like this should cross reference back to the normal login API, but I couldn't figure out how to do that.