-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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 custom auth endpoint #1294
Conversation
Deploy preview for cms-demo ready! Built with commit 55ff84f |
Deploy preview for netlify-cms-www ready! Built with commit 55ff84f |
Deploy preview for netlify-cms-www ready! Built with commit cfb2907 |
Deploy preview for cms-demo ready! Built with commit cfb2907 |
Deploy preview for netlify-cms-www ready! Built with commit 8cc6bb3 |
Deploy preview for cms-demo ready! Built with commit 8cc6bb3 |
Deploy preview for netlify-cms-www ready! Built with commit fbe49b0 |
Deploy preview for cms-demo ready! Built with commit fbe49b0 |
@marksteele please test and confirm this works for your use case. |
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 can confirm that the fix addresses my issue.
@erquhart I'm good with this. I do wonder, however, if it would just be better to have a new setting like |
I considered that, but base_url is used in a few places. We'd need to make sure it's okay to get rid of. Sent with GitHawk |
Hmm, that may be necessary, but I only see it used with |
I believe we're using it to test against the I'm inclined to go with what's in this PR since it's backward compatible, and maybe provide the more ideal solution you're describing in a 2.0 PR. What do you think? If you'd really rather us go for it, it's a matter of someone having time. |
2.0 is fine, go for it. |
@@ -84,6 +84,7 @@ class App extends React.Component { | |||
isFetching: auth && auth.get('isFetching'), | |||
siteId: this.props.config.getIn(["backend", "site_domain"]), | |||
base_url: this.props.config.getIn(["backend", "base_url"], null), | |||
authEndpoint: this.props.config.getIn(["backend", "auth_endpoint"]), |
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 wonder if we should make backends get the value themselves, instead of passing them all in here. Maybe a 2.0 thing?
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.
2.0, but yeah that definitely makes sense. @Benaiah what do you think?
- Summary
Currently the path "/auth" is enforced for authentication, even with a custom
base_url
. Developers should be able to override the default endpoint.Fixes #1285.
- Description for the changelog
allow custom auth endpoint