Skip to content
This repository has been archived by the owner on Nov 10, 2024. It is now read-only.

Overhaul auth mechanism to support multiple tokens/auth mechanisms #542

Merged
merged 23 commits into from
Mar 30, 2021

Conversation

hadley
Copy link
Collaborator

@hadley hadley commented Mar 24, 2021

Fixes #469. Fixes #387.

I think this code is mostly done (although I do need to test it thoroughly). Most of the remaining work is to carefully deprecate the existing auth systems so that old code continues to work, but you are steered towards the new system.

@hadley
Copy link
Collaborator Author

hadley commented Mar 24, 2021

To do:

  • Test
  • Update token param documentation
  • Deprecate get_token()/get_tokens() in favour of auth_get()
  • Deprecate create_token() in favour of auth_type_app() and auth_type_bot(). Suggest auth_save() if set_renv() is TRUE.
  • Hard deprecate bearer_token()
  • Update auth.Rmd
  • Update section in README
  • Check if FAQ.Rmd still needed
  • Write NEWS section

@llrs
Copy link
Collaborator

llrs commented Mar 24, 2021

I can help with the several tokens I use. (Sorry I haven't reviewed the other PR, but I haven't found time to look at it in detail yet).

hadley added 6 commits March 24, 2021 12:29
Since the all questions were about auth and they should no longer come up
Removing a couple of now-uneeded uses along the way.
R/auth.R Outdated Show resolved Hide resolved
@hadley hadley marked this pull request as ready for review March 25, 2021 13:26
@hadley
Copy link
Collaborator Author

hadley commented Mar 25, 2021

@llrs I think this is in a good place to try out / review now. It's a big PR so I'd suggest starting in auth.Rmd which should hopefully explain how all the bits fit together

@alexpghayes would also be useful to get your eyes on auth.Rmd to see if this resolves your questions about using bearer tokens.

R/auth.R Outdated Show resolved Hide resolved
vignettes/auth.Rmd Outdated Show resolved Hide resolved
@alexpghayes
Copy link
Contributor

Is there a function that will display the available saved auth methods so that you can select between them / set a default one? In the auth vignette I currently see tools for creating and saving auth, but am less clear on how to manage multiple auth types at once.

@hadley
Copy link
Collaborator Author

hadley commented Mar 25, 2021

@alexpghayes what do you mean by manage multiple auth types? Do you mean multiple saved auth objects?

@alexpghayes
Copy link
Contributor

@alexpghayes what do you mean by manage multiple auth types? Do you mean multiple saved auth objects?

I mean whether you're authorizing as bot, app, or user. For example, suppose I have two apps, and I use app 1 exclusively for data collection, but I used app 2 both to post tweets to my timeline interactively and occasionally in an automated way. Then I would conceivably have three different auth objects, which I would probably want to name: app1-app, app2-user and app2-bot. And I would want to be able to select amongst these.

@hadley
Copy link
Collaborator Author

hadley commented Mar 25, 2021

You'd switch between them using auth_as("app1-app") etc.

Copy link
Collaborator

@llrs llrs left a comment

Choose a reason for hiding this comment

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

Overall it is much easier now to know what is happening and the different use cases. Haven't had time to fully test it (or look into tests), but there are many comments.

vignettes/auth.Rmd Outdated Show resolved Hide resolved
Comment on lines 18 to 23
- App authentication allows you to act as if you were a Twitter app.
You can't perform operations that a user would (like posting a tweet or reading a DM), but in return you get higher rate limits on operations like searching for tweets.

- Bot authentication allows you to create a fully automated Twitter bot that performs actions on its own behalf rather than on behalf of a human.

In either case, you'll need to start by creating a Twitter app, so this vignette will first walk you through that process.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is more general and probably nothing we can solve or mitigate on this vignette. But as a user I need to think twice each time Twitter app is mentioned. Is this my app/developer access or is Twitter default one.

Also it might be worth to add why having our own twitter app can be meaningful so I would add also the sentences of the README here on the vignete:

Using your personal account is fine for casual use, but if you are trying to collect a lot of data it's a good idea to create your own twitter "app".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think there's two things missing here:

  • An explanation of what a twitter app is
  • Mentioning that rtweet comes with a special one built-in

vignettes/auth.Rmd Outdated Show resolved Hide resolved
vignettes/auth.Rmd Outdated Show resolved Hide resolved
vignettes/auth.Rmd Outdated Show resolved Hide resolved
R/auth.R Show resolved Hide resolved
R/auth.R Show resolved Hide resolved
R/auth.R Show resolved Hide resolved
README.Rmd Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
hadley added 2 commits March 26, 2021 08:07
Conflicts:
	NAMESPACE
	NEWS.md
	tests/testthat/test-get-my-timeline.R
@hadley
Copy link
Collaborator Author

hadley commented Mar 26, 2021

Ok, I've taken another pass at it — auth.Rmd hopefully now emphasises that the name is arbitrary, and it just has to be meaningful to you. I've also switched the default auth to require an initial explicit call to auth_setup_default(), and it only ever looks for default.rds

vignettes/auth.Rmd Outdated Show resolved Hide resolved
@hadley
Copy link
Collaborator Author

hadley commented Mar 29, 2021

I've taken a couple more passes through the docs

NEWS.md Outdated Show resolved Hide resolved
NEWS.md Show resolved Hide resolved
Co-authored-by: alex hayes <[email protected]>
Copy link
Collaborator

@llrs llrs left a comment

Choose a reason for hiding this comment

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

This looks great! There are several function that do not have the #' @return tag, it would be nice if all of the functions would have it, but this can be added later.

Also the README hasn't been build with the latest changes, but as I'm sure it will change soon, it might not worth your time to spend on it.

NEWS.md Outdated
Comment on lines 53 to 54
- `get_token()` and `get_tokens()` have been deprecated in favour of
`auth_get()`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now that auth_list exists, the news should point to auth_list() + auth_as()

R/bearer_token.R Outdated
Comment on lines 22 to 24
#' This is something you should instead perform in the Twitter developer
#' portal.
#'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps it is worth linking to the dashboard: https://developer.twitter.com/en/portal/projects-and-apps

@hadley hadley merged commit 270733c into master Mar 30, 2021
@hadley hadley deleted the auth-overhaul branch March 30, 2021 11:54
@alexpghayes alexpghayes mentioned this pull request Apr 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support multiple tokens Confusion about bearer tokens
3 participants