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.

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: add myaccount apis #1158

Closed
wants to merge 16 commits into from
Closed

Conversation

shuowu
Copy link
Contributor

@shuowu shuowu commented Mar 29, 2022

In this PR:

  • adds myaccount API methods to SDK layer, entry point is @okta/okta-auth-js/myaccount
  • adds integration tests to verify responses via snapshots
  • adds unit tests
  • generate myaccount docs via typedoc
  • moves react-oie test app to samples/generated/react-embedded-auth-with-sdk
  • add lintstaged to run scripts against staged files (lint, update docs), it's much faster than standalone lint command as it only run against the changes.


if (sdk.options.httpRequestInterceptors) {
for (const interceptor of sdk.options.httpRequestInterceptors) {
interceptor(options);
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of the interceptors? To allow for manipulation of the RequestOptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, seems like there is no proper way to manipulate the url (RequestOptions) after moving the logic to sdk layer. Before ciamx fix the browser cors issue, we still need a way to proxy the request. We can keep it as an internal feature first.

@shuowu-okta shuowu-okta force-pushed the sw-move-myaccount-api-to-sdk-OKTA-479699 branch from 7adf860 to ee92e70 Compare March 30, 2022 14:52
@shuowu-okta shuowu-okta changed the base branch from sw-add-cucumber-tests-for-my-account-sample-OKTA-479691 to master March 30, 2022 14:52
@shuowu shuowu marked this pull request as draft March 30, 2022 14:53
@shuowu-okta shuowu-okta force-pushed the sw-move-myaccount-api-to-sdk-OKTA-479699 branch from 22fc103 to f012819 Compare April 4, 2022 02:38
@codecov-commenter
Copy link

codecov-commenter commented Apr 4, 2022

Codecov Report

Merging #1158 (e11442f) into master (74cd01c) will decrease coverage by 1.54%.
The diff coverage is 66.19%.

@@            Coverage Diff             @@
##           master    #1158      +/-   ##
==========================================
- Coverage   93.46%   91.91%   -1.55%     
==========================================
  Files         159      173      +14     
  Lines        4679     4957     +278     
  Branches     1081     1116      +35     
==========================================
+ Hits         4373     4556     +183     
- Misses        288      378      +90     
- Partials       18       23       +5     
Impacted Files Coverage Δ
lib/options/index.ts 100.00% <ø> (ø)
lib/myaccount/transactions/Base.ts 9.09% <9.09%> (ø)
lib/myaccount/transactions/EmailTransaction.ts 10.71% <10.71%> (ø)
lib/myaccount/transactions/PhoneTransaction.ts 13.04% <13.04%> (ø)
...yaccount/transactions/EmailChallengeTransaction.ts 17.64% <17.64%> (ø)
...b/myaccount/transactions/EmailStatusTransaction.ts 20.00% <20.00%> (ø)
lib/myaccount/transactions/ProfileTransaction.ts 25.00% <25.00%> (ø)
...myaccount/transactions/ProfileSchemaTransaction.ts 40.00% <40.00%> (ø)
lib/OktaAuth.ts 89.24% <75.00%> (-0.22%) ⬇️
lib/http/request.ts 96.34% <85.71%> (-2.21%) ⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 74cd01c...e11442f. Read the comment docs.

@shuowu-okta shuowu-okta force-pushed the sw-move-myaccount-api-to-sdk-OKTA-479699 branch from 5d984a5 to bc57586 Compare April 4, 2022 15:32
@shuowu-okta shuowu-okta marked this pull request as ready for review April 6, 2022 15:39
@@ -85,7 +85,8 @@ describe('E2E login', () => {
await TestApp.logoutRedirect();
});

it('can login to social idp using signin widget (with redirect)', async () => {
// eslint-disable-next-line jasmine/no-disabled-tests
xit('can login to social idp using signin widget (with redirect)', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This there a ticket to track this and re-enable the test?

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -99,7 +100,8 @@ describe('E2E login', () => {
await TestApp.logoutRedirect();
});

it('can login to social idp using signin widget (no redirect)', async () => {
// eslint-disable-next-line jasmine/no-disabled-tests
xit('can login to social idp using signin widget (no redirect)', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This there a ticket to track this and re-enable the test?

Copy link
Contributor

Choose a reason for hiding this comment

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

@shuowu-okta shuowu-okta force-pushed the sw-move-myaccount-api-to-sdk-OKTA-479699 branch 2 times, most recently from dc5b7ea to 8b9ea19 Compare April 6, 2022 22:15
if (error?.error?.includes('insufficient_authentication_context')) {
const scopes = decodeToken(accessToken).payload.scp!;
// reauthentication - this call triggers redirect
await getWithRedirect(
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems incorrect. According to this JIRA - https://oktainc.atlassian.net/browse/OKTA-460659,

When a response includes insufficient_authentication_context that should trigger a new interact/authorize request including the max_age parameter as defined in the response

In our case, we need to call interact again instead of the getWithRedirect
The current experience redirects the user to okta hosted page which is undesired behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

Updated to throw error from SDK layer, and handle it via different re-authentication approaches at sample app layer.

@shuowu-okta shuowu-okta force-pushed the sw-move-myaccount-api-to-sdk-OKTA-479699 branch 2 times, most recently from 653f3f7 to 9876a83 Compare May 10, 2022 20:13
@shuowu-okta shuowu-okta force-pushed the sw-move-myaccount-api-to-sdk-OKTA-479699 branch 2 times, most recently from 80e63ac to c6a97d0 Compare May 20, 2022 19:57
@shuowu-okta shuowu-okta force-pushed the sw-move-myaccount-api-to-sdk-OKTA-479699 branch from e15ab14 to 9dd11c1 Compare June 10, 2022 18:17
@shuowu-okta shuowu-okta force-pushed the sw-move-myaccount-api-to-sdk-OKTA-479699 branch from 9dd11c1 to 91f08a3 Compare June 10, 2022 19:48

## Introduction

MyAccount APIs enables everything needed for the customer's end user to manage one's account. The API assumes the access token is obtained through the OAuth2 either by the classic or IDX flow.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
MyAccount APIs enables everything needed for the customer's end user to manage one's account. The API assumes the access token is obtained through the OAuth2 either by the classic or IDX flow.
MyAccount APIs enables end user account management in SPA applications. The API requires an access token obtained via OAuth flows with additional [scopes](#scopes)


## Scopes

Certain token scopes will be needed to gain the permission to read/manage the account resources:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Certain token scopes will be needed to gain the permission to read/manage the account resources:
The following scopes are required for permission to read/write the resources exposed by the MyAccount API:

<script src="https://global.oktacdn.com/okta-auth-js/6.7.0/okta-auth-js.min.js" type="text/javascript"></script>
```

> :warning: The version shown in this sample may be older than the current version. We recommend using the highest version available
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this warning into the html snippet above as a comment, or move it above the snippet. I think if it's below it's easy to miss


## Handling `insufficient_authentication_context` error

Re-Authentication will be needed when request requires higher assurance than provided by the access token. MyAccount API methods throws `AuthApiError` with `insufficient_authentication_context` in `errorSummary` and `max_age` in `meta` field to indicate the downstream applications to start a re-authentication flow.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Re-Authentication will be needed when request requires higher assurance than provided by the access token. MyAccount API methods throws `AuthApiError` with `insufficient_authentication_context` in `errorSummary` and `max_age` in `meta` field to indicate the downstream applications to start a re-authentication flow.
For additional security, the MyAccount API requires a higher assurance level to protect the end user's account from being manipulated by malicious actors. If the `access token` provided does not meet the required assurance level, an error will be thrown to prompt the user to re-authenticate. Applications consuming the MyAccount API will need to handle this error condition. When this occurs, `myaccount` methods will throw an `AuthApiError` with `insufficient_authentication_context` in `errorSummary` and `max_age` in `meta` field, like so:


Re-Authentication will be needed when request requires higher assurance than provided by the access token. MyAccount API methods throws `AuthApiError` with `insufficient_authentication_context` in `errorSummary` and `max_age` in `meta` field to indicate the downstream applications to start a re-authentication flow.

### AuthApiError example:
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this header

}
```

Re-authentication Approaches:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Re-authentication Approaches:
## Re-authentication Approaches:


`App.jsx` is the entry point of the whole app, it manages the transaction state of the whole app and shares it via React context. Based on the current transaction status, it also redirects the app to the proper routes.
This sample app demostrates the best practices for integrating Authentication into your React SAP via the embedded Auth SDK (@okta/okta-auth-js). It dynamically renders the form by following responses from [Okta's Identity Engine][]. Policy changes, like adding extra authenticator for MFA, can be reflected in this sample app with no code change.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This sample app demostrates the best practices for integrating Authentication into your React SAP via the embedded Auth SDK (@okta/okta-auth-js). It dynamically renders the form by following responses from [Okta's Identity Engine][]. Policy changes, like adding extra authenticator for MFA, can be reflected in this sample app with no code change.
This sample app demonstrates the best practices for integrating Authentication into your React SPA via the embedded Auth SDK (@okta/okta-auth-js). It dynamically renders the form by following responses from [Okta's Identity Engine][]. Policy changes, like adding extra authenticator for MFA, can be reflected in this sample app with no code change.

#!/bin/sh
. "$(dirname "$0")/_/husky.sh"

npx lint-staged
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we adding this with this PR? Is this required for myaccount? I'd prefer to have a separate conversation about pre-commit hooks and handle it in a separate PR. Also why npx instead of yarn?

Copy link
Contributor Author

@shuowu shuowu Jun 16, 2022

Choose a reason for hiding this comment

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

I feel we will want to take doc/code consistency as the first priority. In this PR, docs for myaccount is generated from code/types, and it's easy to miss to run doc update script when code changes happen. This pre-commit hook here is a solution to keep things in sync.

I understand it may affect a bit devex, but here I would prefer to take consistency over devex.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be done as a pre-push step instead of pre-commit? When rebasing we do a lot of commits, but there is only one push at the end.

Copy link
Contributor

Choose a reason for hiding this comment

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

pre-push cannot just handle staged files, this lint staged won't run scripts when no file matches the regex.

@@ -6,7 +6,7 @@ export TEST_SUITE_TYPE="junit"
export TEST_RESULT_FILE_DIR="${REPO}/build2/reports/unit"

export CI=true
export ISSUER=https://oie-signin-widget.okta.com/oauth2/default
export ISSUER=https://oie-signin-widget.okta.com
Copy link
Contributor

Choose a reason for hiding this comment

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

does myaccount only work with org auth server?

what about our other integration tests? Are we introducing a gap by not testing against custom auth server?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

does myaccount only work with org auth server?

Yes, sadly it is what current how myaccount api works

what about our other integration tests? Are we introducing a gap by not testing against custom auth server?

I don't think we are testing any custom AS specific features, @vijetmahabaleshwar-okta maybe you can share some insights

Copy link
Contributor

Choose a reason for hiding this comment

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

No, we aren't testing any custom AS specific features. Org AS should work for our tests.

@@ -0,0 +1,3 @@
export * from './profileApi';
export * from './emailApi';
export * from './phoneApi';
Copy link
Contributor

Choose a reason for hiding this comment

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

should also export types here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks like types have been generated under build/lib/myaccount

Screen Shot 2022-06-16 at 10 51 07 AM

I will add some assertion in test/types with importing from @okta/okta-auth-js (build package)

Copy link
Contributor

Choose a reason for hiding this comment

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

yes they are build there from the source file "types.ts". Don't you want export * from './types' in the index file?

Copy link
Contributor

Choose a reason for hiding this comment

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

added, also added tsd to verify types

jest.esm.mjs Outdated
@@ -15,6 +15,9 @@
const OktaAuth = process.env.BUNDLE_ENV === 'browser' ?
`<rootDir>/build/esm/browser/index.js` :
`<rootDir>/build/esm/node/index.js`;
const MyaccountEntry = process.env.BUNDLE_ENV === 'browser' ?
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: prefer "MyAccountEntry"

lib/.eslintrc.js Outdated
@@ -18,6 +18,8 @@ module.exports = {
},
rules: {
"node/no-unsupported-features/es-syntax": 0,
"@typescript-eslint/no-non-null-assertion": 0,
"no-use-before-define": 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

are there many instances of this? we are currently disabling the rule on a case by case basis since this could be hiding an error. particularly the non-null assertion could be better handled with some code refactor, so I see it as mostly a debt item to refactor the code to have early return or type guard functions so that TS knows whether a variable can be null or not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call, will take a closer look

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed, and replaced with comment in code

@@ -87,6 +88,7 @@ export function buildOptions(args: OktaAuthOptions = {}): OktaAuthOptions {
ignoreSignature: !!args.ignoreSignature,

// Server-side web applications
clientSecret: args.clientSecret
clientSecret: args.clientSecret,
setLocation: args.setLocation
Copy link
Contributor

Choose a reason for hiding this comment

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

should add documentation in README for these new options

Copy link
Contributor Author

Choose a reason for hiding this comment

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

readme option added

@@ -201,7 +201,7 @@ class OktaAuth implements OktaAuthInterface, SigninAPI, SignoutAPI {
};

// Add shims for compatibility, these will be removed in next major version. OKTA-362589
Object.assign(this.options.storageUtil, {
Object.assign(this.options.storageUtil || {}, {
Copy link
Contributor

Choose a reason for hiding this comment

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

under what condition can options.storageUtil be undefined?

Copy link
Contributor

Choose a reason for hiding this comment

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

required after typescript upgrade

@@ -69,7 +76,13 @@ export function httpRequest(sdk: OktaAuthHttpInterface, options: RequestOptions)
if (res && isString(res)) {
res = JSON.parse(res);
if (res && typeof res === 'object' && !res.headers) {
res.headers = resp.headers;
if (Array.isArray(res)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Which response is returning an array at the top-level?

Copy link
Contributor

Choose a reason for hiding this comment

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

/idp/myaccount/emails and /idp/myaccount/phones

@@ -23,6 +23,8 @@ export function getNativeConsole() {

export function getConsole() {
var nativeConsole = getNativeConsole();
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

why needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

after typescript upgrade

@@ -46,7 +64,7 @@
"test:server": "jest --config ./jest.server.js",
"test:report": "yarn test --ci --silent || true",
"test:samples": "yarn workspace @okta/test.e2e.samples start",
"test:integration": "jest --config ./jest.integration.js",
"test:integration": "jest --runInBand --config ./jest.integration.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason these tests cannot run in parallel?

Copy link
Contributor

Choose a reason for hiding this comment

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

myaccount api need to be tested in a crud flow, run it in parallel causes flakiness.

eng-prod-CI-bot-okta pushed a commit that referenced this pull request Jun 21, 2022
OKTA-486697
<<<Jenkins Check-In of Tested SHA: e8ddce4 for [email protected]>>>
Artifact: okta-auth-js
Files changed count: 208
PR Link: #1158
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants