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

Improve Builder interface methods #150

Closed
lcobucci opened this issue Jan 7, 2017 · 20 comments
Closed

Improve Builder interface methods #150

lcobucci opened this issue Jan 7, 2017 · 20 comments

Comments

@lcobucci
Copy link
Owner

lcobucci commented Jan 7, 2017

As per @henriquemoody and @Ocramius reviews on #129 we can make things more clear.
So let's discuss here to find the best names for with(), canOnlyBeUsedBy() and other method we would like to discuss about.

@lcobucci lcobucci added this to the 4.0.0 milestone Jan 7, 2017
@lcobucci
Copy link
Owner Author

lcobucci commented Jan 7, 2017

withClaim() is clearly better than with()

@lcobucci
Copy link
Owner Author

@Ocramius would allowedTo() be better than canOnlyBeUsedBy()? I think is pretty logical and also compatible with the constraints we created.

@Ocramius
Copy link
Collaborator

allowedTo... do what? (that's the question I ask myself when reading allowedTo.

@lcobucci
Copy link
Owner Author

allowed to as in "allowed to client A"

@Ocramius
Copy link
Collaborator

Ocramius commented Feb 14, 2017

Still lacks expressiveness...

@lcobucci
Copy link
Owner Author

😭

@Ocramius
Copy link
Collaborator

@lcobucci ask the twitters?

@lcobucci
Copy link
Owner Author

@Ocramius can you do it plz?

@rdohms
Copy link

rdohms commented Feb 14, 2017

clearedFor as in "security clearance for client a"

@rdohms
Copy link

rdohms commented Feb 14, 2017

or restrictedTo if the only is still relevant.

@greg0ire
Copy link

grantedTo, like a right? I like restrictedTo too.

@nikolaposa
Copy link

nikolaposa commented Feb 14, 2017

I find allow more appropriate as a term than restrict because with every subsequent invocation of a method you are widening scope instead of narrowing it.

@nikolaposa
Copy link

... that being said, here are my suggestions:

  • allow()
  • validFor()
  • permit()
  • permittedFor()
  • grant()

@rdohms
Copy link

rdohms commented Feb 14, 2017

@nikolaposa but the original name canOnlyBeUsedBy implies that its replacing all other grants with one restricted to the passed user. That's why i asked if the only is relevant.

@nikolaposa
Copy link

@rdohms Concrete implementation tells me that the actual idea of how method should behave is different: https://github.com/lcobucci/jwt/blob/master/src/Token/Builder.php#L63

@rdohms
Copy link

rdohms commented Feb 14, 2017

that's just bad naming then, i blame @lcobucci :D

@lcobucci
Copy link
Owner Author

lcobucci commented Feb 14, 2017

that's just bad naming then, i blame @lcobucci :D

@rdohms that's why this issue exists (so that people can come and blame me ofc 😄)

@lcobucci
Copy link
Owner Author

Thanks for the suggestions guys!

The idea of the aud claim is to have a list of clients that are allowed to use the token (basically a whitelist).

I'd say that permittedFor() is the closest one for that concept but I'm not sure it expresses what we want. Maybe dropping canBeUsedBy() is also a good idea (since it's causing the whole confusion), what you think @Ocramius?

@Ocramius
Copy link
Collaborator

@lcobucci "permission" is probably the closest to what the claim expresses (thanks for nothing, abbreviations, DUH.).

I'd go with permittedFor

@lcobucci
Copy link
Owner Author

@Ocramius cool let's go with that then. Do you think we should rename Lcobucci\JWT\Validation\Constraint\AllowedTo to make it consistent?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants