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

Adds guided authentication #10 #15

Merged
merged 1 commit into from
Nov 7, 2022
Merged

Conversation

JBGruber
Copy link
Contributor

@JBGruber JBGruber commented Nov 7, 2022

Here is my attempt to make a guided authentication function. You can test the function by, e.g., removing tokens from tools::R_user_dir("rtoot", "config") and then running a function like get_status() without a token or auth_setup() directly. All functions should now guide through authentication if no token is provided or found.

A couple of things still need to be decided:

  1. I renamed create_bearer() to create_token() since this seems to make more sense to me, but maybe there was a reason for the name that I didn't get.
  2. I changed verify_credentials() to error when the verification fails. This worked last night, but doesn't seem to work at the moment (status is 422 Unprocessable Entity). I did not change anything about the request, I'm not running into a rate limit. Maybe it's not a good idea to make it so strict? On the other hand, average users probably don't understand the response.
  3. I run verify_credentials() in auth_setup(), but since it currently fails, you will get an error message. The function still does the right thing though. Maybe it would be better to not run it since it errors anyway if
  4. I think if you like auth_setup(), it would make sense to not expose so many authentication functions. I think auth_setup() and verify_credentials() are probably the only ones the user needs.
  5. I think it would be a good idea to store the token path either in an option (which I currently do) or in an environment variable. I'm not sure yet which works better. The advantage is that one can set options("rtoot_token" = "path/to/token1.rds") or options("rtoot_token" = "path/to/token2.rds") if a user has multiple tokens and wants to use a specific one without naming it in every function (the default is to use the first one in tools::R_user_dir("rtoot", "config"); auth_setup() has a name argument where you can give the token a name).

@schochastics
Copy link
Member

I skimmed through the code (will check more thorough later) and am fine with the changes and renaming (create_bearer was a random choice and create_token is definitely better). @chainsawriot what are your thoughts?

@chainsawriot
Copy link
Collaborator

Sounds okay to me, but please also change the Vignette.

Regarding envvars, as now we have the instance parameter, maybe we can now somehow allow RTOOT_DEFAULT_INSTANCE and RTOOT_DEFAULT_TOKEN in e.g. /etc/environment or .Renviron. But that's a bit over-engineering for now.

@schochastics
Copy link
Member

schochastics commented Nov 7, 2022

I will change the vignette once merged. That was hastily written by me.

The environment stuff we can keep in mind for later

@schochastics schochastics merged commit 776f74c into gesistsa:main Nov 7, 2022
@JBGruber
Copy link
Contributor Author

JBGruber commented Nov 7, 2022

I just finished writing it 😉

@schochastics
Copy link
Member

Ah oops justs send another pr then 😅

schochastics added a commit that referenced this pull request Nov 7, 2022
changed auth vignette after #15
@JBGruber
Copy link
Contributor Author

JBGruber commented Nov 7, 2022

Regarding envvars, as now we have the instance parameter, maybe we can now somehow allow RTOOT_DEFAULT_INSTANCE and RTOOT_DEFAULT_TOKEN in e.g. /etc/environment or .Renviron. But that's a bit over-engineering for now.

I think that would make sense. It could also look if these variables are set at startup and set them otherwise.

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.

3 participants