feat(core): add oidc auth code flow [release] #587

vegardok · 2021-06-22T11:56:19Z

BREAKING CHANGE: loginWithOAuth signature is changed

packages/core/src/baseCogniteClient.ts

codecov · 2021-06-23T14:27:53Z

Codecov Report

Merging #587 (e5fc767) into master (00c73e4) will decrease coverage by 22.96%.
The diff coverage is 85.58%.

@@             Coverage Diff             @@
##           master     #587       +/-   ##
===========================================
- Coverage   98.69%   75.72%   -22.97%     
===========================================
  Files          19       86       +67     
  Lines         764     3077     +2313     
  Branches       51      470      +419     
===========================================
+ Hits          754     2330     +1576     
- Misses         10      719      +709     
- Partials        0       28       +28

Impacted Files	Coverage Δ
packages/stable/src/api/sequences/sequencesApi.ts	`100.00% <ø> (ø)`
...able/src/api/serviceAccounts/serviceAccountsApi.ts	`100.00% <ø> (ø)`
...table/src/api/timeSeries/syntheticTimeSeriesApi.ts	`100.00% <ø> (ø)`
...ackages/stable/src/api/timeSeries/timeSeriesApi.ts	`100.00% <ø> (ø)`
packages/stable/src/cogniteClient.ts	`100.00% <ø> (ø)`
packages/stable/src/retryValidator.ts	`100.00% <ø> (ø)`
packages/stable/src/types.ts	`100.00% <ø> (ø)`
packages/template/src/__tests__/testUtils.ts	`100.00% <ø> (ø)`
packages/template/src/cogniteClient.ts	`100.00% <ø> (ø)`
packages/wells/src/__tests__/testUtils.ts	`50.00% <ø> (ø)`
... and 180 more

jarlah

looks good. Have some change requests. and/or comments

jarlah · 2021-06-24T14:42:24Z

packages/core/src/baseCogniteClient.ts

@@ -589,7 +631,7 @@ export default class BaseCogniteClient {
        user = null;
      }

-      if (!user || !user.access_token) {
+      if (!user || user.access_token) {


is this intentional leaving out the !in front of user.access_token? did it came from my branch 🙈

so if we don't have a user or a user WITH access token .... I think this is wrong

yea, looks like a typo to me, good catch

jarlah · 2021-06-24T14:46:31Z

samples/react/authentication-adfs/src/App.js

@@ -57,7 +57,7 @@ function App() {

  useEffect(() => {
    const login = async (client) => {
-      const result = await client.loginWithOAuth({
+      const result = await client.loginWithOAuth('ADFS_OAUTH', {


needs to be updated to send in flow object

jarlah · 2021-06-24T14:46:36Z

samples/react/authentication-aad/src/App.js

@@ -61,7 +61,7 @@ function App() {

  useEffect(() => {
    const login = async (client) => {
-      const result = await client.loginWithOAuth({
+      const result = await client.loginWithOAuth('AAD_OAUTH', {


needs to be updated to send in flow object

jarlah · 2021-06-24T15:07:22Z

packages/core/src/baseCogniteClient.ts

+   * }
+   * });
+   *
+   * // or with client credentials


this is not a part of this PR but will be a part of my PR. possibly remove.

jarlah · 2021-06-24T15:08:35Z

packages/core/src/baseCogniteClient.ts


+export type AuthFlowType =


so glad we did this. 🙌 much more readable and only by using a basic union with common fields

Agree — this approach looks great

jarlah · 2021-06-24T15:13:22Z

samples/react/authentication-auth-code-flow/src/App.js

+  const client = useMemo(() => new CogniteClient({ appId: 'sample-app-id' }), []);
+
+  useEffect(() => {
+    client.loginWithOAuth('OIDC_AUTHORIZATION_CODE_FLOW', {


needs to be updated to send in flow object

jarlah · 2021-06-25T09:34:17Z

I have been doing a lot of thinking on the flow manager approach in the code. So, we have a flow manager and a method inside the base client that handles the flow. So the logic is spread into two places, the flow manager and the loginWith[FlowType] method. I wonder, is it possible to merge the contents of the loginWith[FlowType] method into the flow manager? We could still continue to "manually" resolve the flow manager in the switch, but we wouldn't need to define a method in the base client. One way to avoid creating this method is to pass functions for setting bearer token and cluster.

This could be solved in the following manner:

      case 'OIDC_VENDOR_GENERIC_CLIENT_CREDENTIALS_FLOW': {
        this.vendorGenericFlowManager = new OidcVendorGeneric(flow.options, this.setCluster, this.setBearerToken);
        [authenticate, token] = await this.vendorGenericFlowManager.init();
        break;
      }

where this.setCluster and this.setBearerToken is lambda functions on the class (possible to pass around without lossing this context). Or even just pass them as (cluster: string) => this.httpClient.setCluster(cluster)or this.httpClient.setCluster(the latter if the setCluster in httpClient is a lambda.

the init method name might need to renamed to something else because some flow managers already have an init method. But the point remains.

packages/core/src/baseCogniteClient.ts

polomani

As I mentioned before, my biggest concern is about a breaking change in general.
We have working adfs/aad flow, and breaking the users just for a cleaner code's sake is not really a good excuse. I doubt we have resources to backport bugfixes/features anyway, so by releasing this we just make external developer's life harder.

I would keep the same method we had before, and just introduce an additional one.
Something like setLoginConfig(...) which wouldn't support API keys/legacy OAuth.
As I remember that was what we agreed on earlier.

vegardok · 2021-07-13T14:20:49Z

We have working adfs/aad flow, and breaking the users just for a cleaner code's sake is not really a good excuse.

The main purpose of the PR is to add OIDC support for Aize/Auth0. The API change is not motivated by clean code, but less brittle code. this is error prone, and the more flows you add, the more error prone it will be.

BREAKING CHANGE: stop silencing errors from aad

BREAKING CHANGE!: stop guessing which flow to use based on content of options.

polomani · 2021-07-13T14:49:56Z

and the more flows you add, the more error prone it will be

I completely agree, therefore I suggested adding a separate function to handle all these different flows (all of them except the legacy one)

CDF_OAUTH will be deprecated in a couple of months, another major?

vegardok · 2021-07-14T07:44:00Z

and the more flows you add, the more error prone it will be

I completely agree, therefore I suggested adding a separate function to handle all these different flows (all of them except the legacy one)

That would still be a breaking change for AAD/ADFS, so I don't see that as an alternative to the objection against frequent major version updates. I think the current setup is a good fit for changing flows in the future, e.g adding client credentials will be a minor change.

The way I see it, this PR will be breaking and is an improvement. I don't see why adding a new method instead of changing the API is better?

CDF_OAUTH will be deprecated in a couple of months, another major?

Q1 22 at the earliest, but that is optimistic. Or, it has been deprecated since Q1 21, but I don't think it will be shut down before the last customer has migrated. So who knows.

Something like setLoginConfig(...) which wouldn't support API keys/legacy OAuth.

This is also not correct, you are doing more than just setting the config (checking old tokens, trying to silently refresh them if possible). So the semantics of the existing method is a better fit.

jarlah self-assigned this Jun 22, 2021

jarlah force-pushed the add-oidc-auth-code-flow branch from 31d9290 to e44ed10 Compare June 23, 2021 11:08

jarlah reviewed Jun 23, 2021

View reviewed changes

packages/core/src/baseCogniteClient.ts Outdated Show resolved Hide resolved

vegardok force-pushed the add-oidc-auth-code-flow branch 3 times, most recently from 65ac887 to d9af8e5 Compare June 23, 2021 13:41

vegardok changed the title ~~Add oidc auth code flow~~ feat! add oidc auth code flow Jun 23, 2021

vegardok force-pushed the add-oidc-auth-code-flow branch from d9af8e5 to 224ebc5 Compare June 23, 2021 14:03

vegardok marked this pull request as ready for review June 23, 2021 14:03

vegardok requested a review from a team as a code owner June 23, 2021 14:03

vegardok requested review from a team and begozcan June 23, 2021 14:03

vegardok changed the title ~~feat! add oidc auth code flow~~ feat!: add oidc auth code flow Jun 23, 2021

vegardok force-pushed the add-oidc-auth-code-flow branch 2 times, most recently from c0776ae to 15f3a28 Compare June 23, 2021 14:23

vegardok requested a review from jarlah June 23, 2021 14:23

vegardok force-pushed the add-oidc-auth-code-flow branch from 15f3a28 to b018853 Compare June 24, 2021 14:05

jarlah suggested changes Jun 24, 2021

View reviewed changes

jarlah removed their assignment Jun 24, 2021

polomani reviewed Jun 25, 2021

View reviewed changes

packages/core/src/baseCogniteClient.ts Show resolved Hide resolved

polomani reviewed Jun 25, 2021

View reviewed changes

packages/core/src/baseCogniteClient.ts Outdated Show resolved Hide resolved

vegardok requested review from bdimitrijoski and peternic as code owners July 12, 2021 11:00

vegardok force-pushed the add-oidc-auth-code-flow branch 2 times, most recently from 067dcbe to 711f6c1 Compare July 13, 2021 10:13

vegardok requested review from polomani, jarlah and kimjoar July 13, 2021 10:18

polomani reviewed Jul 13, 2021

View reviewed changes

vegardok added 8 commits July 13, 2021 16:32

chore: move files around

6cb7803

fix: explicitly save which flow is used

73c083f

fix: relocate locale storage prefix string

d93f367

feat: add oidc authentication code flow

bce47db

chore: add build watch

571ec00

chore: add oidc auth flow example

99b0373

feat!: allow AAD errors to bubble

df0773f

BREAKING CHANGE: stop silencing errors from aad

feat!: change loginWithOAuth API signature

9c63176

BREAKING CHANGE!: stop guessing which flow to use based on content of options.

vegardok force-pushed the add-oidc-auth-code-flow branch from 711f6c1 to 9c63176 Compare July 13, 2021 14:32

vegardok requested a review from polomani July 13, 2021 14:38

vegardok changed the title ~~feat!: add oidc auth code flow~~ feat!: add oidc auth code flow [release] Jul 14, 2021

vegardok changed the title ~~feat!: add oidc auth code flow [release]~~ feat(core): add oidc auth code flow [release] Jul 14, 2021

feat: proforma changes to trigger major version bump

c782115

vegardok requested a review from a team July 14, 2021 12:55

vegardok requested a review from a team as a code owner July 14, 2021 12:55

chore: merge remote-tracking branch 'origin/master'

e5fc767

vegardok merged commit 0cc44aa into master Jul 15, 2021

vegardok deleted the add-oidc-auth-code-flow branch October 12, 2021 13:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): add oidc auth code flow [release] #587

feat(core): add oidc auth code flow [release] #587

vegardok commented Jun 22, 2021 •

edited by polomani

Loading

codecov bot commented Jun 23, 2021 •

edited

Loading

jarlah left a comment •

edited

Loading

jarlah Jun 24, 2021

jarlah Jun 24, 2021

vegardok Jul 13, 2021

jarlah Jun 24, 2021

vegardok Jul 13, 2021

jarlah Jun 24, 2021

vegardok Jul 13, 2021

jarlah Jun 24, 2021

vegardok Jul 13, 2021

jarlah Jun 24, 2021

kimjoar Jun 25, 2021

jarlah Jun 24, 2021

vegardok Jul 13, 2021

jarlah commented Jun 25, 2021 •

edited

Loading

polomani left a comment •

edited

Loading

vegardok commented Jul 13, 2021

polomani commented Jul 13, 2021 •

edited

Loading

vegardok commented Jul 14, 2021

feat(core): add oidc auth code flow [release] #587

feat(core): add oidc auth code flow [release] #587

Conversation

vegardok commented Jun 22, 2021 • edited by polomani Loading

codecov bot commented Jun 23, 2021 • edited Loading

Codecov Report

jarlah left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jarlah commented Jun 25, 2021 • edited Loading

polomani left a comment • edited Loading

Choose a reason for hiding this comment

vegardok commented Jul 13, 2021

polomani commented Jul 13, 2021 • edited Loading

vegardok commented Jul 14, 2021

vegardok commented Jun 22, 2021 •

edited by polomani

Loading

codecov bot commented Jun 23, 2021 •

edited

Loading

jarlah left a comment •

edited

Loading

jarlah commented Jun 25, 2021 •

edited

Loading

polomani left a comment •

edited

Loading

polomani commented Jul 13, 2021 •

edited

Loading