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

Resource server errors #178

Merged
merged 4 commits into from
Jul 11, 2014

Conversation

shadowhand
Copy link
Member

Copies helper methods for exception handling from the Authorization server into the Resource server to allow for implementing RFC 6750, section 3.1:

  • getExceptionType
  • getExceptionMessage
  • getExceptionHttpHeaders

Adds $required parameter to hasScope to trigger exceptions when scopes are missing from the session.

Adds two new exceptions, MissingAccessToken and InsufficientScope to differentiate between the different error responses from the resource server.

Tests updated and new tests written to complete code coverage.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 18988ed on ushahidi:resource-server-errors into 8e164f4 on thephpleague:develop.

* @param string $error The error message key
* @return string The error message
*/
public static function getExceptionMessage($error = '')
Copy link
Member Author

Choose a reason for hiding this comment

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

This method is duplicated in Authorization server, along with getExceptionType and getExceptionHttpHeaders. Should I abstract these in some why? If so, do you have a preferred class name?

@shadowhand
Copy link
Member Author

Ping @alexbilbie, any chance you will look at this today/tomorrow? We'd like to start using it.

@@ -0,0 +1,19 @@
<?php
/**
* OAuth 2.0 Missing Access Token Exception
Copy link
Member Author

Choose a reason for hiding this comment

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

whoops, just noticed this was duplicated.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will look shortly —
Sent from my iPhone

On Tue, Jun 3, 2014 at 2:40 PM, Woody Gilk notifications@github.com
wrote:

@@ -0,0 +1,19 @@
+<?php
+/**

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling aa903b2 on ushahidi:resource-server-errors into 8e164f4 on thephpleague:develop.

@shadowhand
Copy link
Member Author

Anything else I should do to help move this along?

@alexbilbie
Copy link
Contributor

Hi,

There is a reason that I've held off on this - I've been working on proper documentation for v3 and v4 and it's been really helpful to spot where the holes are in the library. As soon as I've finished which will be in the next day or so I will merge this in.

Cheers,

Alex

@shadowhand
Copy link
Member Author

Alex, how goes the documentation?

@shadowhand
Copy link
Member Author

@alexbilbie I'm willing to help carry on the development of the current branch if you need to focus on docs and the next version... we're (@ushahidi) are getting to the point where the lack of movement on this is starting to be an issue, but we'd rather not fork.

@brianherbert
Copy link

Any movement on this?

@philsturgeon
Copy link
Member

At this point @alexbilbie it's probably better to merge and update docs when they eventually happen than it is to hold this up and annoy folks with waiting around. Can you give anyone an update on what is happening?

This v4 and the docs are both a long time overdue, but that is only because we mentioned it publicly. Whenever new versions are mentioned people want them nownownow and refuse to use any other version. :)

@shadowhand
Copy link
Member Author

@philsturgeon I don't know why Github thinks there are merge conflicts... I can merge this branch without any issues locally.

@shadowhand
Copy link
Member Author

@philsturgeon wait wait, I see now... I was merging to master, not develop. Fixing now.

alexbilbie added a commit that referenced this pull request Jul 11, 2014
@alexbilbie alexbilbie merged commit 4480aa3 into thephpleague:develop Jul 11, 2014
@alexbilbie
Copy link
Contributor

I've merged this in. Sorry for the unnecessarily long delay @shadowhand.

As I said on Twitter I know it seems like there isn't any activity on the codebase but I am working away on it locally, I'm not yet happy with the API and I've been trying some different approaches - I want to keep the library nimble and my concern is with v4 that quite a lot of code is required to implement the storage interfaces. I feel I may have gone a bit OTT with abstraction to the detriment of making the library easy to work with.

In addition I've come up with scenarios where I need to adapt sessions on the fly - e.g. auto-assigning scopes based on a user's role and permissions - and it's not currently possible to interact with the entities unless you implement customs grants which in itself requires lots of code copy and pasting.

I do also have most of the docs written, but until I've settled on the API it's not yet worth publishing them.

Again I am sorry for the delay, I genuinely care for the project and want to make sure it's the best implementation out there.

@philsturgeon @lucadegasperi @jasonlewis

@philsturgeon
Copy link
Member

Thanks for the update buddy. I'm wondering if "oauth2-server" and "oauth2-server-mysql" or.... something... could be achieved to solve the problems you are having?

Flexibility is good, but we've already seen from the lucadegasperi/oauth2-server-laravel package that if our stuff is tough to implement then people will need a wrapper to actually use it.

@alexbilbie
Copy link
Contributor

That's my plan.

I can easily abstract out an *sql and Redis package from the examples in the repo.

I've all but abandoned frameworks now so others will have to create implementations for those.

@shadowhand
Copy link
Member Author

@alexbilbie honestly, fwiw, I think that v3 API is very good... I was able to implement the storage interfaces in about a day and have a fully operational server a couple of days later.

I admit I have not followed v4 development very little, but can you point me at a document that explains the goals of a new version? I'm wondering if there would be some way to realize greater levels of scope injection, etc within the v3 branch.

@shadowhand
Copy link
Member Author

@alexbilbie also, can you tag this v3 for version 3.2.1, with this included? We'd like to start using it ASAP.

@shadowhand shadowhand deleted the resource-server-errors branch July 11, 2014 14:11
@alexbilbie
Copy link
Contributor

@shadowhand have tagged 3.2.1

v3 does a good job but it's a pain in the arse to implement a non-relational database backend, v4 addresses this as well as making hundreds more optimisations. There will be a 3.x upgrade guide.

@alexbilbie
Copy link
Contributor

@shadowhand actually I'm underselling v4 a bit - it's pretty much a complete rewrite of the codebase

@shadowhand
Copy link
Member Author

@alexbilbie well I hope you are not developing it in isolation... the more documentation you can write before writing code, outlining how the data flows through the system, the easier it will be for others to help you. FWIW, I use nomnoml to create DFD diagrams for @ushahidi and it has been tremendously helpful in getting the team to understand the architecture.

If you ever want to chat about stuff, I'm often on Freenode IRC as shadowhand in the #kohana, #ushahidi, and #cleancode channels, among others.

@alexbilbie
Copy link
Contributor

@shadowhand #139

@shadowhand
Copy link
Member Author

@alexbilbie v3.2.1 is not appearing on packagist... can you force update it? https://packagist.org/packages/league/oauth2-server

@alexbilbie
Copy link
Contributor

Fixed

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