Dev 4.0 #8

aarongranick-okta · 2020-10-02T21:46:31Z

Sample update PR: okta/samples-js-react#144

Code changes:

Updates with new apis in auth-js@4.1
Removes AuthService
<Security>
- removes implicit oktaAuth instance initialization
- accepts oktaAuth, onAuthRequired from props
- implements default restoreOriginalUri callback
<SecureRoute>
- accepts onAuthRequired callback, prefer component props than root level props

jrwpatterson · 2020-10-12T02:38:19Z

How long for this to be implemented?

aarongranick-okta · 2020-11-05T00:32:53Z

src/SecureRoute.js

    // Only process logic if the route matches
    if (!match) {
      return;
    }
    // Start login if and only if app has decided it is not logged inn
    if(!authState.isAuthenticated && !authState.isPending) { 
-      authService.login();
+      handleLogin();


Is there a problem with calling an async function within a useEffect block? What if the component renders again while we are awaiting the onAuthRequired function? Would it make sense to use something like state to prevent multiple calls?

The isPending options should prevent multiple calls - at least, that was part of the role when it was in okta-react directly.

The default signInWithRedirect will take us to Okta and so unload the entire application.
But onAuthRequired may never resolve before it cases actions that make isAuthenticated to change to true. Then, something else happens which causes isAuthenticated to become false, and it will call onAuthRequired again, possibly before the first call has returned. It's an edge case, but imagine instead of a redirect flow we are doing an interact flow and we are waiting for a code from a mobile app. Meanwhile actions in other tabs cause the tokens to change which update auth state and toggle the value of isAuthenticated causing a re-render. Once we enter an auth flow and show the user a form, I don't think we'd want to re-render and erase the progress the user has made on that form, no matter what is happening under the hood.

Is there a problem with calling an async function within a useEffect block?

Yes, use async directly with useEffect will cause warning in console.
https://stackoverflow.com/a/53572588

Would it make sense to use something like state to prevent multiple calls?

Agree. We already implemented logic to prevent multiple calls for signInWithRedirect https://github.com/okta/okta-auth-js/blob/master/lib/browser/browser.ts#L264, the callback options should also be protected with similar logic.

Protected with pendingRef in SecureRoute

test/e2e/harness/src/Home.js

aarongranick-okta · 2020-11-05T00:36:16Z

test/e2e/harness/protractor.conf.js

@@ -14,6 +14,9 @@ const SpecReporter = require('jasmine-spec-reporter').SpecReporter;
 const JUnitXmlReporter = require('jasmine-reporters').JUnitXmlReporter;
 const TEST_RESULT_FILE_DIR = '../../../test-reports/e2e';

+// eslint-disable-next-line node/no-unpublished-require
+require('../../../env'); // will load environment vars from testenv file and set on process.env


will E2E no longer respect the testenv file?

It failed to bind the testenv when I tried yarn test:e2e and yarn protractor in the test folder.

maybe the testenv file is malformed or in the wrong location. message me and we can troubleshoot.

Only the environment variables used by the specs would be affected here: https://github.com/okta/okta-react/blob/master/test/e2e/harness/e2e/App.test.js#L40 USERNAME, PASSWORD

The harness app reads them when it webpacks: https://github.com/okta/okta-react/blob/master/test/e2e/harness/config-overrides.js#L4 ISSUER, CLIENT_ID

right, I think the protractor script should support locally e2e execution properly. With this line, it can wire user credentials from testenv.

Any concern for adding this line?

No concern, this is the right place to modify environment vars for protractor

README.md

aarongranick-okta · 2020-11-05T00:42:44Z

README.md


-Components can get this object as a passed prop using the [withOktaAuth](#withoktaauth) HOC or using the [useOktaAuth](#useoktaauth) React Hook.  The `authService` object provides methods for managing tokens and auth state. 
+From version 4.0, the Security component starts to explicitly accept [oktaAuth][Okta Auth SDK] instance as prop to replace the internal `authService` instance. You will need to replace the [Okta Auth SDK related configurations](https://github.com/okta/okta-auth-js#configuration-reference) with a pre-initialized [oktaAuth][Okta Auth SDK] instance.


maybe it's worth mentioning that you must add @okta/okta-auth-js as a dependency to your project and install it separately from okta-react

swiftone · 2020-11-05T17:54:25Z

README.md


 ```bash
 npm install --save @okta/okta-react
 ```

+Install peer dependency `@okta/okta-auth-js`


Note - npm 7 installs peer dependencies by default. I don't think that's a problem for us, but please take a moment to consider if I'm wrong.

I think a manual install will not hurt even if it's installed by default in npm7, as it can also handle cases for package managers < npm7 or yarn.

README.md

swiftone · 2020-11-05T17:57:10Z

README.md

 This example defines 3 routes:

 - **/** - Anyone can access the home page
 - **/protected** - Protected is only visible to authenticated users
- **/implicit/callback** - This is where auth is handled for you after redirection
+- **/login/callback** - This is where auth is handled for you after redirection


Doesn't the console still create /implicit/callback uris by default?

README.md

swiftone · 2020-11-05T18:00:48Z

README.md


-You may also configure an instance of the [Auth service][] directly and pass it to the Security component.
+- Initialize [oktaAuth](Okta Auth SDK) instance with `pkce=true` and pass it to the `Security` component.


pkce is the default now, we should rephrase this.

how about remove this sample? as initial pkce or not actually is the responsibility for auth-js instead of okta-react

swiftone · 2020-11-05T18:01:45Z

README.md


-You may also configure an instance of the [Auth service][] directly and pass it to the Security component.
+- Initialize [oktaAuth](Okta Auth SDK) instance with `pkce=true` and pass it to the `Security` component.
+- add `/login/callback` route with [LoginCallback](#logincallback) component to handle login redirect from OKTA.


In addition to the /implicit/callback vs /login/callback issue, we should be sure to mention that whatever they use needs to match the configuration of their app in Okta.

Agree! I don't really have a strong opinion on which one to use, probably use /login/callback and mention that they need to match config in okta app can save us from some future changes.

I think PKCE is worth a mention, but I agree that we can mention it...and point to the AuthJS docs.

Note, however, that I'm not very happy with the AuthJS docs. We should improve those before we start removing too much from elsewhere and leaning on AuthJS docs. An overhaul to focus on customer use-cases like Aaron did with the Widget is overdue, I think.

looks like with so many repo/doc depend on auth-js readme, we probably want to maintain proper versions of readme for auth-js, so we don't end up with broken links everywhere.

README.md

swiftone · 2020-11-05T18:05:01Z

README.md


-#### `authService.getAuthState()`
+- `@okta/okta-auth-js` has been changed as a peerDependency for this SDK. You must add `@okta/okta-auth-js` as a dependency to your project and install it separately from `@okta/okta-react`.
+- [onAuthRequired](#onauthrequired) is kept in Security's props.


is kept in Security's props

I'm not sure what this is saying. That it is now a prop?

README.md

swiftone · 2020-11-05T18:08:02Z

README.md


-(async method) Triggers a re-evaluation of whether a user is considered authenticated.  Might be need to be triggered manually for users that pass overrides to `onAuthRequired` to the Security component or to the `Auth` constructor.  Does NOT perform a login, simply re-evaluates the current authenticated status. An 'authStateChange' event is emitted once the re-evaluation is complete. 
+- `isAuthenticated` will be true if **both** accessToken **and** idToken are valid


suggest: "if and only if"

What if their scopes don't involve both tokens? Is that still a case that can happen?

What if their scopes don't involve both tokens? Is that still a case that can happen?

by default should be added. We may also want to add an explanation in auth-js#readme about how to use the transformAuthState callback to handle custom scopes cases.

README.md

swiftone · 2020-11-05T18:12:19Z

README.md


-#### Example with AuthService object
+*(optional)* Callback function. Called when authentication is required. If this is not supplied, `okta-react` redirects to Okta. This callback will receive [oktaAuth][Okta Auth SDK] instance as the first function parameter. This is triggered when a `SecureRoute` is accessed without authentication.


Can we give better advice on what this should be used for? We discuss when it is called, but not why. Our example below sends them to a location - is this instead of the normal redirection? We don't say what happens in such a case here.

aarongranick-okta · 2020-11-06T20:07:08Z

src/Security.js

 import OktaContext from './OktaContext';
 import OktaError from './OktaError';

 const Security = ({ oktaAuth, onAuthRequired, children }) => { 
  const history = useHistory();
-  const [authState, setAuthState] = useState(oktaAuth.authStateManager.getAuthState());
+  const [authState, setAuthState] = useState(() => {
+    if (!oktaAuth) {


Wow are we actually allowing it to work without okta-auth-js What is the concept? That<Security> can be created first, and then later re-rendered with an oktaAuth instance?

it's a workaround to make the <Security> render error when oktaAuth is missing. The linter does not allow conditional hooks (render error before the hook)

https://github.com/okta/okta-react/pull/8/files/5c3aee19c504c57e3203ca802079d560e0e6b20c#diff-b511f09482238b22139073ef766c2326566a2d86519cc15b992399092c344cddR56

Worthy of a comment then, methinks

aarongranick-okta · 2020-11-10T01:47:38Z

package.json

@@ -38,10 +38,10 @@
  },
  "dependencies": {
    "@babel/runtime": "^7.11.2",
-    "@okta/configuration-validation": "^0.4.1",
-    "@okta/okta-auth-js": "^3.2.5"
+    "babel-jest": "^26.3.0"


can probably be a dev dependency?

aarongranick-okta · 2020-11-20T16:44:38Z

@shuowu When you are ready for final review, close this PR and open a new one. I can't approve this PR because I opened it. Also squash the commits down to just the unique items which we want to add in the CHANGELOG

shuowu · 2020-11-20T16:52:33Z

@aarongranick-okta How about a new empty dev-4.0 branch with barely the version bump, then we keep merging the pending PRs into it? It would be easier to merge the whole 4.0 changes into master.

shuowu-okta force-pushed the dev-4.0 branch from 6576051 to ad9b194 Compare October 30, 2020 17:43

shuowu mentioned this pull request Nov 4, 2020

[okta-react] updates for okta-auth-js@4.0.0 okta/okta-oidc-js#866

Closed

14 tasks

aarongranick-okta and others added 8 commits November 4, 2020 16:48

bump package version to 4.0.0

edd851a

Upgrade to okta-auth-js@4.0

46af5cb

feat: update with auth-js v4.1

9678dec

fix: toRelativeUrl

49a3709

update with authjs 4.1

10eb39a

readme and changelog update

876324e

update migration guide and changelog

bc5cc07

update e2e harness app

97ed0ae

shuowu-okta force-pushed the dev-4.0 branch from db87731 to 97ed0ae Compare November 4, 2020 22:24

update unit test

14cd804

shuowu marked this pull request as ready for review November 4, 2020 23:27

shuowu requested a review from swiftone November 4, 2020 23:27

aarongranick-okta commented Nov 5, 2020

View reviewed changes

test/e2e/harness/src/Home.js Outdated Show resolved Hide resolved

aarongranick-okta commented Nov 5, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

aarongranick-okta commented Nov 5, 2020

View reviewed changes

resolve comments

c7421c9