-
Notifications
You must be signed in to change notification settings - Fork 261
Auth0 integration_V2 #703
Auth0 integration_V2 #703
Conversation
…nto auth0-integration auth0-integration
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.
Thank you for updating the PR. Code looks good to me ! 🎉
Test was failing because of testnet reset and I've rerun it now and it's passing.
Going to download codebase once and run locally to ensure I haven't missed anything and we should be good to go!
@@ -184,6 +185,11 @@ You can find more details on the [CCXT_REST github page][ccxt-rest]. | |||
|
|||
[Postgres][postgres] v12.1 or later must be installed for Kelp to automatically write trades to a sql database along with updating the trader config file. | |||
|
|||
### Using Auth0 | |||
|
|||
A [auth0](https://auth0.com/) account is required. To use it, uncomment \[AUTH0] section in [Sample GUI config file](examples/configs/trader/sample_GUI_config.cfg) and enter your auth0 crendentials in required fields. |
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.
👍 thanks!
# Sample UI config file for the kelp bot | ||
|
||
# uncomment the AUTH0 section below to enable | ||
# [AUTH0] |
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.
👍
|
||
cert, err := getPemCert(token) | ||
if err != nil { | ||
return nil, fmt.Errorf("error when getting PEM certificate: %s", err) |
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.
👍
var router chi.Router = r | ||
if s.guiConfig.Auth0Config != nil && s.guiConfig.Auth0Config.Auth0Enabled { | ||
// setting the router to use the JWT middleware to handle auth0 style JWT tokens | ||
router = r.With(JWTMiddlewareVar.Handler) |
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.
much better, thank you!! 💯 🎉
} | ||
|
||
func (s *APIServer) serverMetadata(w http.ResponseWriter, r *http.Request) { | ||
metadata := ServerMetadataResponse{ | ||
DisablePubnet: s.disablePubnet, | ||
EnableKaas: s.enableKaas, | ||
GuiConfig: s.guiConfig, |
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.
perfect! 👍
@@ -59,6 +60,7 @@ type APIServer struct { | |||
kelpErrorsByUserLock *sync.Mutex | |||
|
|||
cachedOptionsMetadata metadata | |||
guiConfig guiconfig.GUIConfig |
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.
👍
@@ -1,5 +1,5 @@ | |||
export default (baseUrl) => { | |||
return fetch(baseUrl + "/api/v1/serverMetadata").then(resp => { | |||
return fetch(baseUrl + "/api/v1/serverMetadata", {method: "GET"}).then(resp => { |
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 thought the default is GET if left unspecified; we leave it unspecified when invoking other GET endpoints like /version
and /newSecretKey
. But this is ok, so can leave as-is. Thanks!
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.
so the thing with interceptor is, it overrides the header on every request, so when we make a fetch request it looks for header, but in this case, it was unable to find it so I deliberately added {method: "GET"} in fetch request to have a valid header to override.
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.
Ran into this issue while running in kaas mode in my local copy.
/newSecretKey
(newSecretKey.js
) fails in the interceptor because this config
isn't specified. Ideally we shouldn't have to specify this since it's optional in the fetch
API which defaults to using a GET method.
Can we try by adding a null check to config
in the interceptor rather than just checking if config.headers
is null?
@@ -372,7 +383,18 @@ class App extends Component { | |||
<Welcome quitFn={this.quit} showQuitButton={this.showQuitButton()}/> | |||
</div> | |||
); | |||
|
|||
return (<div> |
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.
glad this finally worked. thanks!
@@ -382,6 +413,7 @@ func init() { | |||
*options.noHeaders, | |||
quit, | |||
metricsTracker, | |||
auth0ConfigVar, |
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.
thanks for fitting into the existing serverMetadata pattern! 🎉
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.
Think I found only one issue while testing locally -- the issue with the {method: GET}
, commented inline.
Should be good to merge once we figure that out. This is coming together quite nicely!!
@@ -1,5 +1,5 @@ | |||
export default (baseUrl) => { | |||
return fetch(baseUrl + "/api/v1/serverMetadata").then(resp => { | |||
return fetch(baseUrl + "/api/v1/serverMetadata", {method: "GET"}).then(resp => { |
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.
Ran into this issue while running in kaas mode in my local copy.
/newSecretKey
(newSecretKey.js
) fails in the interceptor because this config
isn't specified. Ideally we shouldn't have to specify this since it's optional in the fetch
API which defaults to using a GET method.
Can we try by adding a null check to config
in the interceptor rather than just checking if config.headers
is null?
PS: created an issue for this task here: #706, for when we merge this in to the master branch tomorrow! |
updated interceptor to create a new header for a request which doesn't have one.
reverted back the changes to this file.
reverted this file back to its original state
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, thanks! 🎉
Updates based on Nikhil previous comments