Skip to content

Conversation

@yadutaf
Copy link
Contributor

@yadutaf yadutaf commented Mar 30, 2016

Following discussion with @VincentCasse on php-ovh-sms, here is a design proposal for improved consumer key helper. See Readme and test for examples. But, in a nutshell, it allows to build autorization requests for a given api endpoint prefix and and all sub-path or only the specific route.

ReadOnly is mapped to "GET". ReadWrite to all HTTP Methods.

This PR is about the design itself. If we all agree, we'll implement this in the other wrappers.

At a later time, we may consider human readable aliases for sections of /me and /auth like

  • "identity"
  • "billing"
  • "credentials"
    ...

ping @gregdel, @VincentCasse

README.md Outdated

// Allow GET method on /me
ckReq.AddRule("GET", "/me")
ckReq.GenerateRulesSingle("/xdsl", ovh.ReadOnly)
Copy link
Member

Choose a reason for hiding this comment

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

Typo, xdsl -> me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Thx.

@yadutaf
Copy link
Contributor Author

yadutaf commented Mar 31, 2016

Adding @LukeMarlin to the discussion for C# wrapper

@LukeMarlin
Copy link

Cool feature. Only problem for me is that it can be too restrictive (RW contains too much). I'd add an overloaded method taking custom verbs as a String[]

@gregdel
Copy link
Member

gregdel commented Mar 31, 2016

It's only helpers, the goal is to make the developer life easier. There is still a "AddRule" method for fine-grained right control.

@LukeMarlin
Copy link

Sure, but AddRule does not support the nice recurse param.

@yadutaf
Copy link
Contributor Author

yadutaf commented Mar 31, 2016

Only problem for me is that it can be too restrictive (RW contains too much)

Would it be better with like an intermediate ReadWriteDelete?

@VincentCasse
Copy link
Member

yep, distinct delete could ba a good thing.. Maybe distinct write and others actions ? but i don't know how

@yadutaf
Copy link
Contributor Author

yadutaf commented Mar 31, 2016

Adding ReadWriteSafe without the Delete

@yadutaf
Copy link
Contributor Author

yadutaf commented Mar 31, 2016

v2 pushed with Typo fixes from @gregdel and @PouuleT and intermediate authorization level suggested by @LukeMarlin.

@PouuleT
Copy link
Member

PouuleT commented Mar 31, 2016

Why not do something like
AddRules([]string{"GET","POST","PUT"}, "/sms")
and
AddRecursiveRules([]string{"GET","POST","PUT", "/sms")

@yadutaf
Copy link
Contributor Author

yadutaf commented Apr 1, 2016

API could be like:

const ( 
    ReadOnly      = []string{"GET"}
    ReadWrite     = []string{"GET", "POST", "PUT", "DELETE"}
    ReadWriteSafe = []string{"GET", "POST", "PUT"}
)

func AddRule(verb, pattern string)                  {}
func AddRules(verbs []string, pattern string)       {}    
func AddRecursiveRules(verbs []string, path string) {}

Go ?

@LukeMarlin
Copy link

Exactly, +1

@yadutaf
Copy link
Contributor Author

yadutaf commented Apr 1, 2016

@cygy does is look good to you too for Swift wrapper ?

@cygy
Copy link

cygy commented Apr 1, 2016

I have already some helpers but I can update if needed.
Seems good.

@yadutaf
Copy link
Contributor Author

yadutaf commented Apr 1, 2016

And there it is. I much prefer this new API anyway. Less code, less magic. As far as I'm concerned, we're ready to go.

README.md Outdated

// Allow GET method on /xdsl and all its sub routes
ckReq.AddRule("GET", "/xdsl/*")
ckReq.AddRules(ovh.ReadOnly, "/xdsl")
Copy link
Member

Choose a reason for hiding this comment

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

This should be:

ckReq.AddRecursiveRules(ovh.ReadOnly, "/xdsl")

To match the comment above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Damn it. Ur right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

yadutaf added 2 commits April 3, 2016 12:09
Signed-off-by: Jean-Tiare Le Bigot <jt@yadutaf.fr>
Signed-off-by: Jean-Tiare Le Bigot <jt@yadutaf.fr>
@yadutaf
Copy link
Contributor Author

yadutaf commented Apr 3, 2016

As far as I'm concerned: Ready.

@gregdel gregdel merged commit 5d36b61 into master Apr 4, 2016
@gregdel
Copy link
Member

gregdel commented Apr 4, 2016

Merged, thanks @yadutaf. Good work !

@gregdel gregdel deleted the jt-ck branch April 4, 2016 21:59
@yadutaf
Copy link
Contributor Author

yadutaf commented Apr 5, 2016

\o/ Starting Python version.

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.

7 participants