Skip to content

feat: identities getting started #2103

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

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

christiannwamba
Copy link
Contributor

@christiannwamba christiannwamba commented Apr 8, 2025

Related Issue or Design Document

Checklist

  • I have read the contributing guidelines and signed the CLA.
  • I have referenced an issue containing the design document if my change introduces a new feature.
  • I have read the security policy.
  • I confirm that this pull request does not address a security vulnerability.
    If this pull request addresses a security vulnerability,
    I confirm that I got approval (please contact [email protected]) from the maintainers to push the changes.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added the necessary documentation within the code base (if appropriate).

Further comments

Organization after getting started

@christiannwamba
Copy link
Contributor Author

christiannwamba commented Apr 10, 2025

TODO:

  • Add a page for password reset/account recovery
  • Remove initialize sdk example from login page
  • Use brew (not npm) to install ory cli and show instructions for macos and linux
  • Use @ory/client-fetch (not @ory/client)
  • Set credentials to include instead of usine withCredentials
  • Use UI component for screenshots
  • Refine screenshots to be clear and annotated
  • Make UI texts bold
  • instead ui /ui/login please redirect to the route behind createBrowserLoginFlow
  • Add ory cli as alternative tab to Ory Console

@christiannwamba
Copy link
Contributor Author

christiannwamba commented Apr 10, 2025

Feedback from @piotrmsc

  • Refine local development
  • Print the session to console
  • Remove headers from code snippets
  • Extend the session and revoke the session using code
  • Use MS style guide for handling list in MFA page
  • Use a single image for the MFA config
  • Remove the list of provider and link to it
  • Fix social sign in image
  • Replace blur with template value
  • Link to session guide instead of configuring session

  • Bullet list might not be needed
  • Refresh and revoke session test
  • Consisteent basepath url
  • Image urls
  • Double check the mfa settings to see if they get an email
  • Consider whether or not to have the Next steps section
  • Remove the recommended approach section

Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Structurally it's going in the right direction, but I think we need a bit more work on the content side!


```tsx
export default function Page() {
const [session, setSession] = useState<Session | null>(null)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am following the guide "step by step" and it does not explain on which port my app should run, which modules beside ory client I should use and the config code example is broken.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am making some assumptions about the reader. If a reader is on this pages, they have a language/framework of choice which means they are familiar with that language/framework. Providing every context will bloat the example snippets. The only context that is required is the ory SDK stuff but language specific context are assumed about the reader.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a "What you'll need" section to set some expectation for the reader

@piotrmsc
Copy link
Collaborator

piotrmsc commented Apr 15, 2025

Following again blindly "step by step" : When I try logout I am getting 404 after logout call as well, in ory tunnel I see calls and redirects but in the end I end up with 404 from CF blocking rule, do I need to configure something extra?

Comment on lines +1 to +13
const requireAuth = async (req, res, next) => {
try {
const session = await ory.toSession({ cookie: req.header("cookie") })
req.session = session
next()
} catch (error) {
res.redirect(`${process.env.ORY_SDK_URL}/self-service/login/browser`)
}
}

app.get("/", requireAuth, (req, res) => {
res.json(req.session.identity.traits) // { email: '[email protected]' }
})
Copy link
Collaborator

@piotrmsc piotrmsc Apr 15, 2025

Choose a reason for hiding this comment

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

can this be re-used already in sign in /sign up example? you are also partially doing it in / handler for registration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The sign and sign up sections are intentionally flat so the reader can focus on a stripped down version of the code and just care about the core. Then they progress to higher levels of abstraction which is why I created the requireAuth middleware for protecting routes

To start the account recovery flow, send the user to the recovery page:

```html
<a href="$ORY_SDK_URL/self-service/recovery/browser">Recover your account</a>
Copy link
Collaborator

Choose a reason for hiding this comment

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

if we have node example shouldn't this be covered also with code example for consistency ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. This should be wrapped in a code example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Decided to leave this as is since it's straightforward. But happy to reconsider if it hurts the reader's experience

Copy link
Member

@vinckr vinckr left a comment

Choose a reason for hiding this comment

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

Huge PR!
I think we are very close to getting this one perfect and will be a great basis for getting started!

I did not test all the code examples but the ones I tested were straightforward enough - maybe you can create an issue to add automated tests for these examples later on? I think long-term would be good avoid drift.

I also noticed you added some neat new(?) components, love them!

Please track adding a "custom components" section to the README in a followup issue - so others know how to use these components? It is mostly clear from this example, but it could save us some time in the future - you don't need to do it in this PR though.

All in all I had mostly some minor style issues, nothing major. Let me know where you had followup questions, once we have this resolved let's :shipit:

<TabItem value="console">
```

**Enable and configure account recovery**
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
**Enable and configure account recovery**
#### Enable and configure account recovery

Please "Use bold only when writing about user interface (UI) elements." - see styleguide

Use h4 if you want a section that is "bolded" but does not show up in the TOC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was intentional because it messes up the table of content. Tabs repeat header content when those contents are repeated they are double in the table of content on the side bar

</BrowserWindow>
```

**Recovery strategy**
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
**Recovery strategy**
#### Recovery strategy

See above

<TabItem value="cli">
```

**Download your current configuration**
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
**Download your current configuration**
#### Download your current configuration

See above

</BrowserWindow>
```

**Adding a social provider**
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
**Adding a social provider**
#### Add a social provider


Ory offers pre-configured options for popular providers. Check our provider list [here](/docs/kratos/social-signin/overview).

**Configuring your provider**
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
**Configuring your provider**
#### Configure your provider

</Tabs>
```

## Initiating account recovery
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
## Initiating account recovery
## Initiate account recovery

@piotrmsc
Copy link
Collaborator

Huge PR! I think we are very close to getting this one perfect and will be a great basis for getting started!

I did not test all the code examples but the ones I tested were straightforward enough - maybe you can create an issue to add automated tests for these examples later on? I think long-term would be good avoid drift.

I agree with the idea of tests,however, to have them running and testing the code examples they would need to be code-complete and currently are not (see my previous review comments on them). Which for me would make sense to have getting-started code examples as code complete with option to collapse lines of code that are not ory-specific.

@christiannwamba what's your take on it?

Co-authored-by: Vincent <[email protected]>
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.

4 participants