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

Documentation not clear about optional parameters for authentificatio #43

Closed
cderv opened this issue Jul 11, 2017 · 11 comments
Closed

Documentation not clear about optional parameters for authentificatio #43

cderv opened this issue Jul 11, 2017 · 11 comments
Milestone

Comments

@cderv
Copy link

cderv commented Jul 11, 2017

In the documentation about crul::auth() is said to have all its argument optional.

However, crul::auth(auth = "any) send back an error

Error in assert(user, "character") : 
  argument "user" is missing, with no default

it is because crul:::make_up does a check abour argument user and pwd which should not be empty

make_up <- function (user, pwd) 
{
  assert(user, "character")
  assert(pwd, "character")
  if (!is.null(user) || !is.null(pwd)) {
    return(paste0(user, ":", pwd))
  }
  NULL
}

It seems that it is expected to be set to NULL considering the code above the the crul:::cpp function.
I check : with auth(user = NULL, pwd = NULL, auth = "any") it is working.

It could be useful if this is explained in the documentation help page or maybe if the arguments are set to NULL by default.

Either way, I can help with a PR if you want to change thing

@sckott
Copy link
Collaborator

sckott commented Jul 11, 2017

thanks @cderv for the issue

Sorry about that, you are right that we need to fix this. I think we replace "optional" with "required". Unless you know of scenarios in which e.g., a username is required, but a pwd is not

@cderv
Copy link
Author

cderv commented Jul 11, 2017

No I don't know this kind of scenarios. Mine was to not provide any username and password (for negotiate authentification) and it was not cleared if it was to be empty or NULL.

So we probably should say it is required but can be set to NULL. And if it is required, shouldn't we put default value?

@sckott
Copy link
Collaborator

sckott commented Jul 11, 2017

Mine was to not provide any username and password (for negotiate authentification)

what do you mean by "negotiate authentication"?

@cderv
Copy link
Author

cderv commented Jul 11, 2017

I meant type = "gssnegotiate"

@sckott
Copy link
Collaborator

sckott commented Jul 11, 2017

okay, in that case you still would need to provide a username and pwd, correct?

@cderv
Copy link
Author

cderv commented Jul 11, 2017

No. I don't need any user or password. It is handle directly by libcurl and windows (I work on windows).

On httr, I managed a get request with authentificate("", "", "gssnegotiate").
On crul, I used a get with auth(NULL, NULL, "gssnegotiate").

It worked in both case

@sckott
Copy link
Collaborator

sckott commented Jul 11, 2017

hmm, i kind of like that both httr::authenticate and crul::auth have the same defaults - that is, no values - it makes a nice error message telling user they need to pass something - though I think we should add example for gssnegotiate as you gave above

@cderv
Copy link
Author

cderv commented Jul 11, 2017

Not sure to follow. httr needs empty string as default and crul needs NULL as default. So not the same really. I could try tomorrow with empty string in crul::auth see if it works the same as httr::authentificate.

At minimum, we need to document this to tell is required to fill user and pwd and show how to fill crul::authentificate with NULL if we don't want to provide any user or password.

@sckott
Copy link
Collaborator

sckott commented Jul 12, 2017

What I mean by the same is that they don't set a default. look at the function definitions:

httr::authenicate

function (user, password, type = "basic") 
{
    stopifnot(is.character(user), length(user) == 1)
    stopifnot(is.character(password), length(password) == 1)
    stopifnot(is.character(type), length(type) == 1)
    config(httpauth = auth_flags(type), userpwd = paste0(user, 
        ":", password))
}

crul::auth

function (user, pwd, auth = "basic") 
{
    structure(ccp(list(userpwd = make_up(user, pwd), httpauth = auth_type(auth))), 
        class = "auth", type = auth)
}

notice how the first two params in each fxn don't set a default value

httr checks to make sure character strings are passed in, so does crul, but crul allows NULL's to be passed through as well


having said that, yes, will document it better, and give egs for passing NULL to each if user doesn't want to provide user and/or pwd

@cderv
Copy link
Author

cderv commented Jul 13, 2017

Oh I see. You are right. I missed this.
So just a little more documentation on that! thanks!

@sckott
Copy link
Collaborator

sckott commented Jul 13, 2017

will do soon

@sckott sckott added this to the v0.5 milestone Jan 19, 2018
@sckott sckott closed this as completed in 267a831 Jan 19, 2018
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

No branches or pull requests

2 participants