[SDK-4231] Implement initial structure of auth client #222
Conversation
| // UnmarshalJSON implements the json.Unmarshaler interface. | ||
| // | ||
| // It is required to handle the differences between error responses between the APIs. | ||
| func (a *authenticationError) UnmarshalJSON(b []byte) error { |
There was a problem hiding this comment.
The signature of the error returned from SignUp is different so we map it back in this unmarshal to return a consistent error
ewanharris
force-pushed
the
feat/SDK-4231-base-auth-client
branch
from
7a1e814 to
bbd484c
Compare
ewanharris
changed the title
| // Ignore the scheme if it was defined in the domain variable, then prefix | ||
| // with https as it's the only scheme supported by the Auth0 API. | ||
| if i := strings.Index(domain, "//"); i != -1 { | ||
| domain = domain[i+2:] | ||
| } | ||
| domain = "https://" + domain |
There was a problem hiding this comment.
Could we simply expect the domain to be without scheme, instead of trying to get it into shape? Unless this is implemented this way in other Management SDKs
There was a problem hiding this comment.
This is the same as what the management SDK already does. From a quick look all SDKs have an expectation that it should be without the scheme but not all enforce it, which can lead to errors.
Java does have some handling, I think PHP does, Node/Python don't touch the domain in any way
|
|
||
| var ( | ||
| domain = os.Getenv("AUTH0_DOMAIN") | ||
| clientID = os.Getenv("AUTH0_AUTH_CLIENT_ID") |
There was a problem hiding this comment.
Could the env variable be simply AUTH0_CLIENT_ID? Considering we have AUTH0_DOMAIN above.
There was a problem hiding this comment.
When I was testing my client id I used for the management tests wouldn't work here, I'm not sure if that was just down to me not having that client configure correctly to do auth and management calls or not. I'll investigate further and potentially stick to one env var as suggested
There was a problem hiding this comment.
I enabled all grants for the M2M app used for management tests and it doesn't seem to work for the TestOAuthLoginWithPassword test (however it does work for signup), I think for now it'll be best to keep the split and see if this can be improved in future.
| } | ||
|
|
||
| // LoginWithPassword is for logging in with information is typically received from a highly trusted | ||
| // public client like a SPA. |
There was a problem hiding this comment.
a highly trusted public client like a SPA. is this intended?
There was a problem hiding this comment.
I added this based on the docs in the API docs and node-auth0
There was a problem hiding this comment.
Just note that SPAs, being public clients, cannot hold a secret. So I'd suggest double-checking with @adamjmcgrath whether it's correct to call them "highly trusted".
There was a problem hiding this comment.
It's "typically received" which means the un/pw is received from a public client - the grant itself is sent from a confidential client
Although it does read a bit strange, let me take another look at it
There was a problem hiding this comment.
I think the API docs have changed since that node-auth0 docstring and they read a bit better - I would go with that
This is the OAuth 2.0 grant that highly-trusted apps use to access an API. In this flow, the end-user is asked to fill in credentials (username/password), typically using an interactive form in the user-agent (browser). This information is sent to the backend and from there to Auth0. It is therefore imperative that the application is absolutely trusted with this information. For [single-page applications and native/mobile apps](https://auth0.com/docs/flows/concepts/auth-code-pkce), we recommend using web flows instead.
There was a problem hiding this comment.
"Highly Trusted public client like a SPA" in this instance means, a browser based client you trust well enough to handle usernames and passwords (like lock.js) - but the updated API docs is better so I'd go with that
Co-authored-by: Rita Zerrizuela <zeta@widcket.com>
ewanharris
force-pushed
the
feat/SDK-4231-base-auth-client
branch
from
346675e to
7072976
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## beta #222 +/- ##
==========================================
- Coverage 94.99% 94.30% -0.69%
==========================================
Files 37 43 +6
Lines 7254 7469 +215
==========================================
+ Hits 6891 7044 +153
- Misses 295 338 +43
- Partials 68 87 +19
☔ View full report in Codecov by Sentry. |
🔧 Changes
Implements the initial base structure of the authentication client along with 2 methods in OAuth and Database in order to facilitate testing of JSON and form encoded requests.
This does not implement aspects such as ID token verification, telemetry and rate limiting onto the client which will be done in a future PR.
There is testing setup for these methods but currently it is dependent on tenant state (i.e. signed up users), randomising this data, recording management API interactions etc. proved difficult so I will file a ticket to ensure that this is dealt with as part of this effort. Current ideas are:
📚 References
🔬 Testing
Tested manually and covered by unit tests
📝 Checklist