-
Notifications
You must be signed in to change notification settings - Fork 364
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
Update docs for returnTo and client_id params on logout #484
Conversation
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.
LGTM. I left a suggestion on the returnTo
server behavior explanation.
For the next time, please use separate commits as it was difficult finding the actual changes introduced in this PR among all the generated docs.
src/global.ts
Outdated
* `returnTo` URL that is provided must be listed in the | ||
* Application's "Allowed Logout URLs" in the Auth0 dashboard. | ||
* However, if the `client_id` parameter is not included, the | ||
* `returnTo` URL must be listed in the "Allowed Logout URLs" at | ||
* the account level in the Auth0 dashboard. | ||
* | ||
* If the `returnTo` URL is **not** set and `client_id` is **not** `null`, the server returns the user to the first Allowed Logout URLs set in the Dashboard. |
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.
Documenting the behavior of sending/not sending client_id
is OK. But how this line is worded, returnTo
feels like a required parameter (because of the drawbacks you explain) and I see below that it's not. The more we explain a property the more we risk not keeping these docs up to date with the official docs. Also, I could think, why would I care reading these docs if I'm not setting this property at all? I think we should word it assuming this returnTo
value is present by now. This may be translated to, this line "both not null - scenario" line not necessary.
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.
Good shout - I've removed that line and just linked off to further info.
311a384
to
df6c577
Compare
df6c577
to
8419a4c
Compare
@lbalmaceda Thanks - committing the docs was a mistake, I've removed them from the diff. |
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.
Lgtm
Description
This PR updates the docs for the
returnTo
andclient_id
params on logout, to make it more clear what happens when certain params are omitted.Testing
Checklist
master