-
Notifications
You must be signed in to change notification settings - Fork 7
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
feat(core): move authentication out of CogniteClient #687
Conversation
6bc1b77
to
d5914cf
Compare
I haven't setup oidc yet, so might be we will see some unexpected 401. Uncertain if they enforced it for the api cluster yet. |
Codecov Report
@@ Coverage Diff @@
## master #687 +/- ##
==========================================
+ Coverage 84.06% 85.93% +1.86%
==========================================
Files 70 84 +14
Lines 1952 2502 +550
Branches 249 329 +80
==========================================
+ Hits 1641 2150 +509
- Misses 301 352 +51
+ Partials 10 0 -10
|
ebd530d
to
8f25873
Compare
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!
guides/derived_SDKs.md
Outdated
@@ -132,7 +132,8 @@ export class CoolThingAPI extends BaseResource<CoolThing> { | |||
} | |||
``` | |||
|
|||
The `CogniteClient` class makes instances of these classes in `initAPIs()`, which is called once credentials and project name are set. The accessor helper `accessApi()` reminds the user to set credentials before using APIs. | |||
The `CogniteClient` class makes instances of these classes in `initAPIs()`, which in the class |
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.
typo, dont understand. did you mean "which is called from the constructor"?
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.
or "which is in the class constructor". but then what is in the class constructor?
apiKey: 'YOUR_SECRET_API_KEY', | ||
const client = new CogniteClient({ | ||
appId: 'YOUR APPLICATION NAME', | ||
apiKeyMode: true, |
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.
it works i guess :) only two modes, either api key or bearer token.
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.
Added some comments but LGTM
When baseURL is provided to the constructor, getToken is not called. And authorization header is not added to requests. Testing on @cognite/sdk version 6.1.1 |
BREAKING CHANGE: remove authentication internals in
CogniteClient
and replace it with an improved api to allow Apps handle authentication.Change summarized:
loginWithMETHOD
have been removed and replaced with an mandatorygetToken
argument to theCogniteClient
constructorsetProject
/setBaseUrl
removedauthenticate
return type have been changed from() => Promise<boolean>
to() => Promise<string|undefined>
, returning the token if found.AzureAD example using MSAL
Legacy auth SPA example
API Key example
Motivation of change: