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

warden: rename authorized / allowed endpoints to something more meaningful #162

Closed
4 tasks done
aeneasr opened this issue Jul 24, 2016 · 14 comments
Closed
4 tasks done
Assignees
Labels
breaking change Changes behavior in a breaking manner. feat New feature or request. help wanted We are looking for help on this one.
Milestone

Comments

@aeneasr
Copy link
Member

aeneasr commented Jul 24, 2016

  • check if /warden/authorized should be renamed
  • check if /warden/allowed should be renamed
  • check how to do anonymous policy checking
  • check how to do strict token validation
@aeneasr aeneasr added feat New feature or request. breaking change Changes behavior in a breaking manner. labels Jul 24, 2016
@aeneasr aeneasr added this to the 0.2.0 milestone Jul 24, 2016
@aeneasr aeneasr self-assigned this Jul 24, 2016
@aeneasr
Copy link
Member Author

aeneasr commented Jul 24, 2016

@matteosuppo @boyvinall @ericdouglas @janekolszak do you think the endpoints /warden/authorized and /warden/allowed are semantically easy to understand?

  • /warden/authorized: Check if a token + scope is ok
  • /warden/allowed: Check if token + scope + action + resource is ok

Additionally those two functionalities should be added:

  • strict mode: Only return a 200 result if audience of introspection token and audience of requester token match
  • anonymous mode: Expose an endpoint which does not require an introspection token, but a subject.

Do you think those should be merged to one endpoint? Or do you have naming ideas?

@aeneasr
Copy link
Member Author

aeneasr commented Jul 24, 2016

Additionally, I want to implement a strict mode which will only return the data if the introspection token audience (from post body) and the authorization token (from header) match to prevent token substitution attacks.

@aeneasr
Copy link
Member Author

aeneasr commented Jul 24, 2016

On top of that one API should be able to check for anonymous users on the warden endpoint. This would also need either a dedicated endpoint or maybe some query parameter?

@aeneasr aeneasr added the help wanted We are looking for help on this one. label Jul 25, 2016
@aeneasr
Copy link
Member Author

aeneasr commented Jul 25, 2016

Here's my proposal. In total we have these capabilities:

Warden: Endpoints for resource providers (usually private)

  1. /warden/token/valid: Check if a token + scope is valid, returns context data (subject, scopes, ...) if valid
  2. /warden/token/allowed: Check if a token + scope is valid and the token's subject is allowed to do something, returns context data (subject, scopes, ...) if valid
  3. /warden/allowed Check if a subject (e.g. anonymous user, service) is allowed to do something

OAuth2 endpoints (usually public)
4. oauth2/introspect Check if a token + scope is valid, using strict mode (implementation of rfc7662)

What do you think?

@matteosuppo
Copy link
Contributor

/token/valid and /token allowed are much more understandable than /authorized and /allowed.

I like the 4 endpoints, they sounds very clear. Do you need help implementing them? Next week I should put some efforts in our migration to hydra, so I can help.

@aeneasr
Copy link
Member Author

aeneasr commented Jul 25, 2016

Great! Let me know on Gitter when you have time. :)

@aeneasr
Copy link
Member Author

aeneasr commented Aug 1, 2016

I think the naming makes sense and I'll implement it in the next days

@aeneasr
Copy link
Member Author

aeneasr commented Aug 1, 2016

I think this makes much more sense:

type Firewall interface {
    // InspectToken checks if the given token is valid and if the requested scopes are satisfied. Returns
    // a context if the token is valid and an error if not.
    InspectToken(ctx context.Context, token string, scopes ...string) (*Context, error)

    // InspectTokenFromHTTP uses the HTTP request to decide weather a token is valid or not. If not, an error
    // is returned.
    InspectTokenFromHTTP(ctx context.Context, r *http.Request, scopes ...string) (*Context, error)

    // IsAllowed uses policies to return nil if the access request can be fulfilled or an error if not.
    IsAllowed(ctx context.Context, accessRequest *ladon.Request) error
}

@matteosuppo
Copy link
Contributor

Do you mean instead of having http endpoints?

@aeneasr
Copy link
Member Author

aeneasr commented Aug 1, 2016

No, it's just the client SDK

@aeneasr
Copy link
Member Author

aeneasr commented Aug 1, 2016

I should have said "instead of the current client sdk" -sorry for that :)

@aeneasr
Copy link
Member Author

aeneasr commented Aug 2, 2016

there will be some more changes. it would be awesome if you could review this

@matteosuppo
Copy link
Contributor

It seems pretty clear to me. There's only a typo in /warden/allowed. It says "This endpoint requires a token, a scope, a resource name, an action name and a context." While it clearly need a subject, not a token.

@aeneasr
Copy link
Member Author

aeneasr commented Aug 3, 2016

Right, thanks!

@aeneasr aeneasr closed this as completed in a297f7e Aug 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Changes behavior in a breaking manner. feat New feature or request. help wanted We are looking for help on this one.
Projects
None yet
Development

No branches or pull requests

2 participants