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

[5.x] Support Laravel Passport #1521

Draft
wants to merge 22 commits into
base: 5.x
Choose a base branch
from

Conversation

hafezdivandari
Copy link
Contributor

@hafezdivandari hafezdivandari commented Jul 30, 2024

Todo

  • Add description for confidential checkbox.
  • Add "enable device flow" checkbox.
  • Fix Passport version on composer.json.
  • Add tests stubs.

Features

  • First-party integration with Laravel Passport.
  • New OAuth option during installation: jetstream:install --api --oauth
  • Support for both Jetstream's stacks: Inertia and Livewire.
  • Manage API tokens (PATs) via Passport: list, add, and delete.
  • New OAuth Features:
    • Manage OAuth connections with third-party apps and services: list and delete connections.
    • Manage OAuth apps: list, add, update, and delete apps.
    • Fully customizable: you may easily add more client metadata, e.g. Application logo
  • Authorize View (for auth code and implicit grants)
  • Device Views (for device auth grant)

OAuth Apps

oauth-apps

API Tokens

passport-api-tokens

Authorize View

laravel-jetstream

Copy link

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

Comment on lines +116 to +123
<!-- Confidential -->
<div class="col-span-6 sm:col-span-4">
<label for="confidential" class="flex items-center">
<Checkbox id="confidential" v-model:checked="createOAuthAppForm.confidential" />
<span class="ms-2 text-sm text-gray-600 dark:text-gray-400">Confidential</span>
</label>
<InputError :message="createOAuthAppForm.errors.confidential" class="mt-2" />
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

I would love a small explanation beneath the Checkbox Label, which explains what Confidential is needed for. Similar to https://tailwindui.com/components/application-ui/forms/checkboxes#component-f03fb959d6ba814eb987d39ae40961f0

Or simply radio buttons for selecting public or confidential with a corresponding explanation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

100%, any suggestion for the explanation?

An alternative approach is a dropdown for selecting the app type instead, like Google OAuth registration, e.g. "Web App", "Mobile and Desktop Apps", and "TVs and limited input devices":

image

Copy link
Contributor

Choose a reason for hiding this comment

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

I would keep it simple for now, with a radio button selection of one of the two options.

I would align with https://oauth.net/2/client-types/

Confidential clients are applications that are able to securely authenticate with the authorization server, for example being able to keep their registered client secret safe.

Public clients are unable to use registered client secrets, such as applications running in a browser or on a mobile device.

Copy link
Contributor

Choose a reason for hiding this comment

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

Confidential Client
Application that is able to securely authenticate with this application, without revealing the secret to an end-user.

Public Client
Unable to use registered clients secrets, such as applications running in a browser or on a mobile device.

Copy link
Contributor Author

@hafezdivandari hafezdivandari Aug 7, 2024

Choose a reason for hiding this comment

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

Thanks, I'll add this explanation for now. But Jetstream doesn't have pre-built radio button component.

Just keeping things simple for now but we may also need a way to enable device flow for laravel/passport#1750

Github example: https://github.com/settings/apps/new

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, there can be other options too.

You could use the same as the teams permissions options:
https://jetstream.laravel.com/features/teams.html

teams

@inmanturbo
Copy link
Contributor

Hey this is a great idea! I have maintained a package since the start of Jetstream which adds Passport support to Jetstream. It's possible you may find some of the code useful to your PR. Feel free to use what you need. First party support would be nice.

https://github.com/headerx/laravel-jetstream-passport

@hafezdivandari
Copy link
Contributor Author

Thank you, @inmanturbo! I appreciate the feedback and the offer to use your package as a reference. If you have any suggestions or insights from your experience maintaining that package, I'd love to hear them. This PR is still a work in progress, and I plan to add tests and implement the new device flow as soon as the required PRs on Passport merged.

@vincentleijen
Copy link

vincentleijen commented Sep 17, 2024

@hafezdivandari First of all: amazing work!

While clicking on the 'Authorize' button I encounter cors problems:
Screenshot 2024-09-17 at 18 47 54
(I've added https://server.dev to the client's allowed_origins just to be sure)
Network-tab reveals there is no Access-Control-Allow-Origin set while requesting.
When I manually visit the https://client.dev/oauth/callback?code=x&state=x link everything works as expected.

Edit: When I manually visit the callback link I see a preflight request with OPTIONS set (including the allowed origin header) in my network tab.
I don't see this preflight request showing up after clicking the 'Authorize' button.

@hafezdivandari
Copy link
Contributor Author

@vincentleijen You may check laravel/passport#213 (comment) and inertiajs/inertia-laravel#323 (comment) and inertiajs/inertia-laravel#303. The CORS issue seems to be totally unrelated to this PR.

@vincentleijen
Copy link

vincentleijen commented Sep 17, 2024

ou may check laravel/passport#213 (comment) and inertiajs/inertia-laravel#323 (comment) and inertiajs/inertia-laravel#303. The CORS issue seems to be totally unrelated to this PR.

@hafezdivandari
laravel/passport#213 (comment) This fixed indeed the issue. In that case please consider my previous message as unsent!

Copy link

@VincentLeijenProductHero VincentLeijenProductHero left a comment

Choose a reason for hiding this comment

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

Found a thingy

autofocus
autocomplete="off"
/>
<InputError :message="createOAuthAppForm.errors.name" class="mt-2" />
Copy link

Choose a reason for hiding this comment

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

Errors are wrapped into createClient
<InputError :message="createOAuthAppForm.errors?.createClient?.name" class="mt-2"/>

class="mt-1 block w-full"
autocomplete="off"
/>
<InputError :message="createOAuthAppForm.errors.redirect_uris" class="mt-2" />
Copy link

Choose a reason for hiding this comment

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

Same goes for this line:
<InputError :message="createOAuthAppForm.errors?.createClient?.redirect_uris" class="mt-2"/>

<Checkbox id="confidential" v-model:checked="createOAuthAppForm.confidential" />
<span class="ms-2 text-sm text-gray-600 dark:text-gray-400">Confidential</span>
</label>
<InputError :message="createOAuthAppForm.errors.confidential" class="mt-2" />
Copy link

Choose a reason for hiding this comment

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

And this one:
<InputError :message="createOAuthAppForm.errors?.createClient?.confidential" class="mt-2"/>

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.

5 participants