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

Add option for using UUID client IDs (non-breaking) #168

Closed
wants to merge 11 commits into from
Closed

Add option for using UUID client IDs (non-breaking) #168

wants to merge 11 commits into from

Conversation

heisian
Copy link

@heisian heisian commented Nov 4, 2016

Introduce client UUIDs as a non-breaking change #14

On existing installations

  • Add Passport::useClientUUIDs() to app/Providers/AuthServiceProvider.php to enable
  • run php artisan passport:uuid to add a uuid column and generate UUIDs for existing clients.

New installations

  • Add Passport::useClientUUIDs() to app/Providers/AuthServiceProvider.php to enable

@heisian heisian mentioned this pull request Nov 6, 2016
@taylorotwell
Copy link
Member

I'm just not sure I see a big need for this at the moment.

@Modelizer
Copy link
Contributor

Modelizer commented Nov 8, 2016

It was a big step to add UUID. @heisian I recommend it can be discussed on stack @larachat #internals channel.

@phoenixgao
Copy link

@heisian really need this, thanks for saving my day

@heisian
Copy link
Author

heisian commented Nov 22, 2016

Sure, just be careful with this fork. Although unit tests all pass for it, I've not manually tested certain things like the authorization redirect. I mainly use OAuth for a stateless API, a la Github accesse tokens.

@MicroDroid
Copy link

I can't believe Taylor rejected a pull-request that adds an optional feature than many people want, just because he thinks it is not necessary.

@victorcastro
Copy link

@taylorotwell you have to consider. Please

@mosleim
Copy link

mosleim commented Apr 12, 2018

uuid is not working for new version?

@veelasky
Copy link

@mosleim it's not even merged.

@heisian
Copy link
Author

heisian commented Apr 13, 2018

You all may want to check out @ssmulders package https://github.com/ssmulders/hashed-passport

It essentially is middleware that will hash and unhash the client ID without changing the way it is stored in the database. It is a non-invasive method of handling what we want here and actually I think a better solution than what I wrote. See #14 .

Use caution as always, however, I haven't tested the package myself. Cheers all!


Additional thoughts:

I highly appreciate @taylorotwell 's stance of standing firm against feature creep.

Having worked primarily in node for the past year, I've come to see that a big problem in the npm community is that there are a lot of large and popular frameworks that work well, but not great/amazing.

Laravel is amazing and (almost) all-encompassing, and that's impossible to find, at least for web frameworks of this scale, in Node.

@mohamedsharaf
Copy link

i wish if uuid is supported

@billriess billriess mentioned this pull request Jan 7, 2019
@grEvenX
Copy link

grEvenX commented Jan 16, 2019

@heisian Even @ssmulders's package doesn't cover all cases well and as long as this feature is not built-in, it will be hard to keep everything working as expected for something that seems pretty trivial to support (it can be discussed if it even should be the default as most OAuth references advice against having this as a guessable sequential number).

One case this package doesn't support is when using HTTP BASIC auth for sending client-id/client-secret (used in machine-to-machine/application context to retrieve a non-personal access token for a client) because the package relies on intercepting the request as a middleware and looking for client_id parameter.

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.

10 participants