-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Add Sentry Auth + Driver #60
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
Conversation
Moved the auth register process into the app->booting call to access the config items.
|
Could you fix your broken indentation please? |
|
dont know why that happened... but it should be fixed now - but prefix to 'api' was set as null in the main repo, so I figured to use that. Override that per project, right? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
Do you have an example of how to implement this? #RushCode |
|
@philsturgeon your having trouble? What is the issue? |
|
@philsturgeon did you want me to fork a laravel install and set up all the dependencies or something? Sorry, i dont really know what your asking for. |
|
That sounds a little sarcastic. No I assume there is some sort of action other than just installing Sentry. I assume that if you are using this code, you have configured dingo to work with sentry. If so, could you share it? That config is required for docs at some point anyway. |
|
Wasn't being sarcastic on that last comment, i was honestly asking if that's what you wanted. You should be able to just override the dingo/api repo with my fork url, and if you have Sentry installed, I thought it would work as is. Ill set up a brand new project and see if im not crazy. |
|
@philsturgeon go ahead and clone this for the laravel install, it has what you need to test things. Uses Sentry 3.0 [EDIT] But until this morning I was using Sentry 2 - and im pretty sure it doesnt matter now that I changed one thing in the SentryProvider class. Let me know if that helps at all. There are 2 routes in there, one for public, and one for secured. |
|
Uh, 32 files changed? Looks like everything was changed to PSR-2 at some point. You may need to send another pull request. |
|
someone said something about reformatting the thing, so I just ran a 'reformat-code' through PHPStorm... im not seeing an option to do that with PSR-4, I only have a few options... any ideas? |
|
PSR4 is auto loading, typically done in composer. Your reformat is probably set to PSR 2 in PHP storm, which idk if @jassonlewis is using ? https://github.com/php-fig/fig-standards/blob/master/proposed/psr-4-autoloader/psr-4-autoloader.md |
|
Laravel use their own non-standard style guide, which is a little Jason, would you accept and push brackets around? Or maybe PSR-2
|
|
@ryanhungate looks like it got part way implementing that test stuff into mine. Public is fine, it will bounce me correctly for an invalid token, but when I have a valid token the cookies get screwy: Any ideas? If you can get this working, I'll pay you back by reformatting the code to make it L4-style and send a new PR with your commits in it, then we both win :) |
|
@philsturgeon, yeah, got something on my plate right this second, but I will get to this asap man, no problem. On May 22, 2014, at 11:10 AM, Phil Sturgeon [email protected] wrote:
|
|
@philsturgeon your using oauth, what package are you using? I could only test the basic auth, and that worked fine... |
|
I'll consider PSR-2 but not in this pull request. I'd like to be able to easily distinguish the actual changes. |
|
@jasonlewis - all I did is reformat the code inside phpstorm. My mistake. If it makes any difference, I can try this again from scratch. I actually did not really alter too many files at all of your core. @GrahamCampbell mentioned fixing the indents, and I just thought, ok, reformat code + push... sorry bout that. |
|
@ryanhungate Surely you don't need an automated tool to change the the cs of the entire project just to fix a bit of indentation. Most simple text editors can do that (even if you were to use find and replace). |
|
@GrahamCampbell slow your roll a little buddy, clearly @ryanhungate is not familiar with PSR-2 and the differences between how that code looks and Laravel, which this project is following (for now?). Let's explain and help him. @ryanhungate So there are a few PSRs (PHP Standards Recommendations) and they go from 0 to 4 (for now). Laravel (and this package) uses PSR-1, but Laravel (and this package) have made the executive decision to not use PSR-2 - which you have accidentally done. PHPStorm will probably default to PSR-2, so thats not your fault. Laravel follows PSR-2 ISH, but brackets always go on a new line, tabs are used instead of spaces (which is why you were initially having formatting problems) and... well thats about it. You should reverse the formatting and try to manually fix the indentation (by using tabs not spaces). If you successfully sort that out, @jasonlewis might switch the whole project so nobody else gets confused about it. :) |
|
Oh yeah, also:
What do you mean? My dingo config looks like this: |
|
FYI i read your book on APIs that dont suck, and reading about how Fractal / Transformers would solve major changes... Im definitely on board with your train of thought, and then this package embraced Fractal... Now Im a little baffled at something, so just answer when you can... but the other day, you had said you needed to add Sentry to this package... Sentry would be for an app code / api code living in the same house, or guest house... and i kind of got the impression from a few comments ago that you are not supporting that train of thought now. Can you explain a little on this man, or did I completely misunderstand you this time? |
|
The API itself uses Sentry, based on access tokens provided via the OAuth system, which has nothing to do with the actual application. The application MIGHT be a PHP application which uses Sentry and has its very own cookie, it might be a JS app which doesn’t even know what PHP is. Because the API enforces no concept of user state it doesn’t matter. Sentry itself is awesome on APIs and we can check user permissions and whatnot. Logged in state makes it not RESTful. If you don’t care about that, or about any of the use-cases mentioned then fair enough. I’ve worked on a few projects where APIs had logged in state and it was only kinda gross, but if you can avoid it you should. |
|
@ryanhungate back to the code though. :) I'm confused about a few things. /* That is a lot of different bits of config in a lot of different places, and auth and sentry are both referenced too much. What is auth? What is basic? As I said earlier, the only way I could get this package to look for access tokens was by using this: Taking out that basic and using only the following...: I get: Why is it checking username and password? :) How do I make any of this work vaguely sensibly with OAuth 2. |
|
Hey Phil, I've never written an oauth server type api ( so far )... and the class : |
|
@philsturgeon that setUser() method shoud be stateless()... let me know if
|
Signed-off-by: Ryan Hungate <[email protected]>
|
Hey @philsturgeon I just did something in a hurry, hopefully I didnt jack it all up, but I did want you to know that I was able to get the Auth Bearer with the token to work correctly using postman... lets pick this thing up tomorrow, and If I need to re-fork this, and start all over to push up my changes, I have no problem doing that. |
|
I'm wondering, what do these changes actually add? I'm running Sentry using the LeagueOAuth2Provider perfectly fine using the 'password' grant configuration of the package to validate the user credentials provided when requesting an access_token. Side note: I did make a custom User model to handle the persist_code and UserInterface problems. Am I missing something amazing here? |
|
I’ve not had a chance to play with this yet sadly. I really need OAuth 2.0 working with Access Tokens and Sentry, but I have a huge workload with Fractal at the moment and a client project that needs it. I can’t do both. |
|
@philsturgeon I think I had this working but you should check that out too… the only issue I had is I think I fubared the git repo by doing an init + push. Hurrying is never good. On May 28, 2014, at 12:28 PM, Kevin Dierkx [email protected] wrote:
|
|
@philsturgeon As mentioned in my previous comment using the LeagueOAuth2Provider with lucadegasperi's OAuth server everything works fine without changing any Dingo/Api related stuff. If you are talking Client Application you could always use SentrySocial. With a small modification it even works with the password_credentials grant. Edit: Still wondering what these changes should do? |
|
@kevindierkx I am certainly not an expert in this, but i dont think that what @philsturgeon wants is currently going to be accomplished with what you just recommended. The problem from what I understand, ( and the whole reason this is open ) is because the auth class that stores everything uses the Laravel auth... Sentry uses its own method, meaning yes, you can authenticate, but calling - does not return the sentry user... and if you are trying to use groups with OAuth as well, it wouldnt work correctly. If Im wrong, just say so. :) |
|
@ryanhungate Almost correct you would get a User model object instead of a Sentry User model object. However these are still the same thing. Sentry uses relationships for the groups, therefore when extending the default Sentry User model and implementing the missing Adding groups would also work. This is just a helper function in the Sentry User model. And for the access_tokens generated using the OAuth2 password grant. Returning something different when using Sentry would be inconsistent. Therefore remains the question "?" |
|
@ryanhungate In addition, I currently have the following response when using |
|
@kevindierkx Quick question on old topic. I have successfully implemented Sentry 2 and lucadegasperi's OAuth2 server just like you have (also using Dingo's API packages) and can issue and authorize tokens. However, how do you manage the authentication renewal of the Sentry user? The token authorization is set by default to 604800 seconds, but the Sentry user is "forgotten" much earlier than that. If the token is still valid, do you check and re-authenticate the user in a beforeFilter (e.g. auth.sentry) based on the user ID returned by the OAuth2 server? |
|
@jonaholsson I don't actually use Sentry for anything during authentication. All user authentication is handled with The laravel oauth server package. Check the session tables. |
|
@kevindierkx OK, so if I would want to verify user group permissions in a controller __construct, I would just verify that user ID (from OAuth2 check) is in given group (preferably using Sentry for simplicity)? |
|
@jonaholsson Actually using the Dingo auth something like this should work also: $user = $this->auth->user(); // Returns instance extending \Cartalyst\Sentry\Users\Eloquent\User
$userGroups = $user->groups;
foreach ($userGroups as $group) {
if ($group->hasAccess('admin', false))
return true;
}
return false; // False when permissions are not foundYou can take a look at my implementation in this bootstrap Application: https://github.com/kevindierkx/api Ignore the Dutch ReadMe ;) |
|
That makes sense. Thanks! :) |
|
@jonaholsson No problem. By the way if you want to use this for validating if the authenticated user is an admin I would suggest implementing this as a filter. This way you can easily reuse it in other places or even before any Controllers are handled by adding it as a before attribute on your route! |
|
@kevindierkx Yeah that's how I implemented it when using Sentry: But I'm probably going to move this to the route rules. Thanks again! :) |
|
What is the status of this exactly? @ryanhungate? |
|
Hey @jasonlewis - sorry i have been super busy lately but as far as I know, the only issue was the PSR code formatting. To be honest, Sentry keeps changing and because of compatibility issues with the Laravel versioning, I kind of moved on… but Ill try to put some time into it asap. I was under the impression that it was working now though.
|
|
Okay no worries. I'll close this for now. Thanks! |
|
@ryanhungate any update on this? |
|
Any news? |
|
Sorry but I'm not working with Sentry anymore. On Tuesday, February 3, 2015, olso [email protected] wrote:
|


Added the ability to use Sentry by a config item.