@okta/okta-react@1.2.1 #517
Conversation
vijetmahabaleshwar-okta
force-pushed
the
okta-react@1.2.1
branch
from
32708db
to
c73d5bb
Compare
aarongranick-okta
force-pushed
the
okta-react@1.2.1
branch
2 times, most recently
from
8e41fc9
to
a4b9b42
Compare
|
Requires updates to configuration-validation module: #529 |
aarongranick-okta
force-pushed
the
okta-react@1.2.1
branch
4 times, most recently
from
6c55ce6
to
bb58066
Compare
- PKCE support, @okta/okta-auth-js@2.6.3
aarongranick-okta
force-pushed
the
okta-react@1.2.1
branch
from
bb58066
to
d0bd4ca
Compare
| <Link to='/'>Home</Link><br/> | ||
| <Link to='/protected'>Protected</Link><br/> | ||
| <Link to='/sessionToken-login'>Session Token Login</Link><br/> |
There was a problem hiding this comment.
nit: Mixing hypenation and camelCase? ick
There was a problem hiding this comment.
this our test harness app.
There was a problem hiding this comment.
The URL already exists and is being used in the E2E tests. I just added a link on the page so we don't have to keep typing it in when we are testing manually.
| ReactDOM.render(<App />, document.getElementById('root')); | ||
| // To perform end-to-end PKCE flow we must be configured on both ends: when the login is initiated, and on the callback | ||
| // The login page is loaded with a query param. This will select a unique callback url | ||
| // On the callback load we detect PKCE by inspecting the pathname |
There was a problem hiding this comment.
Is this information devs using this SDK would need to know/implement? If not, why is this test different than our sample code?
There was a problem hiding this comment.
our samples use environment variable, you would have to restart the server to switch PKCE on or off.
There was a problem hiding this comment.
This method I am using here is more brittle, and would not be recommended for users. But to use environment variables I would need to run 2 completely separate test cycles and run the webpack again.
| - **issuer** (required) - The OpenId Connect `issuer` | ||
| - **client_id** (required) - The OpenId Connect `client_id` | ||
| - **redirect_uri** (required) - Where the callback handler is hosted | ||
| - **clientId** (required) - The OpenId Connect `client_id` |
There was a problem hiding this comment.
I'm 90% sure I know the answer, but what happens to users of the old standard?
If we're deprecating the old snake_case method, we should indicate that somewhere so users understand why their code works (per above) when it doesn't match the docs. The changelog would satisfy me on that if it shows up there (and thus not in this PR), but we need to make sure it does show up there in that case.
There was a problem hiding this comment.
I will add note to the changelog
| - **scope** *(optional)* - Reserved or custom claims to be returned in the tokens. Default: `['openid', 'email', 'profile']` | ||
| - **response_type** *(optional)* - Desired token types. Default: `['id_token', 'token']` | ||
| - **grantType** *(optional)* - Can be `implicit` (default) or `authorization_code` (for PKCE flow) |
There was a problem hiding this comment.
Was grantType ever published for okta-react, or is this leftover from earlier work in this same release?
There was a problem hiding this comment.
its left over, we have not released pkce support for react yet
| - **onAuthRequired** *(optional)* - callback function | ||
|
|
||
| Accepts a callback to make a decision when authentication is required. If this is not supplied, `okta-react` redirects to Okta. This callback will receive `auth` and `history` parameters. This is triggered when: | ||
| 1. `auth.login` is called | ||
| 2. SecureRoute is accessed without authentication | ||
|
|
||
| - **storage** *(optional)*: |
There was a problem hiding this comment.
These moved to the Auth config, right?
There was a problem hiding this comment.
These, and all other supported options are part of the "Configuration refernence" (I point them to okta-auth-js). pkce, secure, etc. are also located here.
| @@ -228,8 +219,17 @@ class App extends Component { | |||
| } | |||
| ``` | |||
|
|
|||
| #### Alternate configuration using `Auth` object | |||
There was a problem hiding this comment.
+1 on this section - very clear, very helpful
packages/okta-react/README.md
Outdated
|
|
||
| Assuming you have configured your application to allow the `Authorization code` grant type, simply pass `pkce=true` to the `Security` component. This will configure the `Auth` object to perform PKCE flow for both login and token refresh. | ||
|
|
||
| ```typescript |
There was a problem hiding this comment.
Why are these all flagged as TS instead JS?
There was a problem hiding this comment.
it makes the jsx tags prettier
There was a problem hiding this comment.
The dev blog supports jsx as a language type. Maybe the new docs site does too?
There was a problem hiding this comment.
packages/okta-react/README.md
Outdated
| <Router> | ||
| <Security issuer='https://{yourOktaDomain}.com/oauth2/default' | ||
| clientId='{clientId}' | ||
| pkce=true |
There was a problem hiding this comment.
Should this not be {true}? IIRC JSX props are either a braced expression or a string literal, no boolean.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Bump version
Does this PR introduce a breaking change?
Other information
See pending release: https://github.com/okta/okta-oidc-js/releases
Reviewers