Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(core): move authentication out of CogniteClient #687

Merged
merged 9 commits into from
Oct 12, 2021
Merged

feat(core): move authentication out of CogniteClient #687

merged 9 commits into from
Oct 12, 2021

Conversation

vegardok
Copy link
Contributor

@vegardok vegardok commented Oct 7, 2021 •
edited
Loading

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 mandatory getToken argument to the CogniteClient constructor
  • setProject/setBaseUrl removed
  • authenticate 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:

  • The application is the entity that is logging in in a OAuth perspective, not the SDK
  • The SDK only cares about access tokens valid for CDF, but an app could care about access to other resources (e.g a separate app backend or IDP graph API). It is difficult/impossible to handle this with the current "internal auth" handling
  • CDF have different access scopes and only the app can know what is the appropriate scope for a certain feature/context. Handling tokens outside the SDK will enable apps to control this

@andersfylling
Copy link
Contributor

andersfylling commented Oct 8, 2021 •
edited
Loading

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

codecov bot commented Oct 8, 2021 •
edited
Loading

Codecov Report

Merging #687 (8d4dd99) into master (de3557f) will increase coverage by 1.86%.
The diff coverage is 82.41%.

@@            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
Impacted Files Coverage Δ
packages/beta/src/cogniteClient.ts 100.00% <ø> (ø)
packages/core/src/testUtils.ts 35.59% <0.00%> (-19.58%) ⬇️
...stable/src/api/entityMatching/entityMatchingApi.ts 37.14% <ø> (ø)
packages/stable/src/retryValidator.ts 100.00% <ø> (ø)
packages/stable/src/api/3d/assetMappings3DApi.ts 79.16% <16.66%> (-20.84%) ⬇️
packages/stable/src/api/3d/nodes3DApi.ts 81.81% <20.00%> (-18.19%) ⬇️
packages/stable/src/api/projects/projectsApi.ts 78.94% <20.00%> (-21.06%) ⬇️
packages/core/src/baseResourceApi.ts 34.00% <25.00%> (-5.07%) ⬇️
packages/stable/src/api/3d/revisions3DApi.ts 95.65% <33.33%> (-4.35%) ⬇️
...ges/stable/src/api/templates/templateGraphQlApi.ts 40.00% <40.00%> (ø)
... and 83 more

@vegardok vegardok marked this pull request as ready for review October 8, 2021 20:12
@vegardok vegardok requested review from a team and agadacz October 8, 2021 20:12
andersfylling
andersfylling approved these changes Oct 11, 2021
View reviewed changes
Copy link
Contributor

@andersfylling andersfylling left a comment

Choose a reason for hiding this comment

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

LGTM!

jarlah
jarlah reviewed Oct 12, 2021
View reviewed changes
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
Copy link
Contributor

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"?

Copy link
Contributor

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?

jarlah
jarlah reviewed Oct 12, 2021
View reviewed changes
packages/stable/README.md
apiKey: 'YOUR_SECRET_API_KEY',
const client = new CogniteClient({
appId: 'YOUR APPLICATION NAME',
apiKeyMode: true,
Copy link
Contributor

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.

jarlah
jarlah approved these changes Oct 12, 2021
View reviewed changes
Copy link
Contributor

@jarlah jarlah left a 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

@vegardok vegardok merged commit 879ed31 into master Oct 12, 2021
@vegardok vegardok deleted the v6 branch October 12, 2021 11:51
vegardok added a commit that referenced this pull request Oct 12, 2021
@vegardok
fix(release): undo major version release without major version bump
7b12241 
Revert "feat(core): move authentication out of CogniteClient (#687)"

This reverts commit 879ed31.
@vegardok vegardok mentioned this pull request Oct 12, 2021
Merged
vegardok added a commit that referenced this pull request Oct 12, 2021
@vegardok
fix(release): undo major version release without major version bump
9451565 
Revert "feat(core): move authentication out of CogniteClient (#687)"

This reverts commit 879ed31.
vegardok added a commit that referenced this pull request Oct 12, 2021
@vegardok
fix(release): undo major version release without major version bump (#…
72e1ecb 
…697)

Revert "feat(core): move authentication out of CogniteClient (#687)"

This reverts commit 879ed31.

Co-authored-by: Vegard Økland <vegard.okland@cognite.com>
vegardok added a commit that referenced this pull request Oct 12, 2021
@vegardok
feat(auth): re-release auth patch
81d65c3 
re-release (revert reversion) of "feat(core): move authentication out of CogniteClient"
#687

This reverts commit 72e1ecb.
This was referenced Oct 12, 2021
Closed
Merged
vegardok added a commit that referenced this pull request Oct 12, 2021
@vegardok
feat(auth): re-release auth patch (#700)
a53c40d 
BREAKING CHANGE: release v6

re-release (revert reversion) of "feat(core): move authentication out of CogniteClient"
#687

This reverts commit 72e1ecb.

Co-authored-by: Vegard Økland <vegard.okland@cognite.com>
@bitoverflow
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jarlah jarlah jarlah approved these changes

@andersfylling andersfylling andersfylling approved these changes

@bdimitrijoski bdimitrijoski Awaiting requested review from bdimitrijoski

@ebrattli ebrattli Awaiting requested review from ebrattli

@peternic peternic Awaiting requested review from peternic

@agadacz agadacz Awaiting requested review from agadacz agadacz is a code owner automatically assigned from cognitedata/cdf-user-experience

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vegardok @andersfylling @bitoverflow @jarlah