Skip to content
This repository has been archived by the owner on Feb 1, 2024. It is now read-only.

Auth0 integration_V2 #703

Merged
merged 24 commits into from
Jun 30, 2021
Merged

Conversation

ymatienzo
Copy link
Contributor

Updates based on Nikhil previous comments

@ymatienzo ymatienzo requested a review from nikhilsaraf as a code owner June 19, 2021 12:14
Copy link
Contributor

@nikhilsaraf nikhilsaraf left a 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.
Copy link
Contributor

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]
Copy link
Contributor

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)
Copy link
Contributor

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)
Copy link
Contributor

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,
Copy link
Contributor

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
Copy link
Contributor

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 => {
Copy link
Contributor

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!

Copy link
Contributor

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.

Copy link
Contributor

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>
Copy link
Contributor

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,
Copy link
Contributor

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! 🎉

Copy link
Contributor

@nikhilsaraf nikhilsaraf left a 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 => {
Copy link
Contributor

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?

@nikhilsaraf
Copy link
Contributor

nikhilsaraf commented Jun 28, 2021

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
Copy link
Contributor

@nikhilsaraf nikhilsaraf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! 🎉

@nikhilsaraf nikhilsaraf merged commit 6b30fa2 into stellar-deprecated:master Jun 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants