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

Update Cloud Client for new USER-scoped API tokens #1410

Closed
wants to merge 23 commits into from
Closed

Conversation

jlowin
Copy link
Member

@jlowin jlowin commented Aug 27, 2019

Thanks for contributing to Prefect!

Please describe your work and make sure your PR:

  • adds new tests (if appropriate)
  • updates CHANGELOG.md (if appropriate)
  • updates docstrings for any new functions or function arguments, including docs/outline.toml for API reference docs (if appropriate)

Note that your PR will not be reviewed unless all three boxes are checked.

What does this PR change?

This PR adds a new auth flow to the Python Cloud client, without changing its existing behavior. However, it does rename some private attributes.

The primary contribution of this PR is to support USER-scoped Cloud API tokens, which function as personal access tokens. USER-scoped tokens can not be used to interact with the Cloud API in a significant way, but are able to authenticate with the API and receive short-lived JWTs in return. These JWTs are full auth tokens representing a user's membership in a specific Cloud tenant.

The user flow works like this:

  1. User instantiates a Cloud client and passes it a USER-scoped API token
  2. User calls client.save_api_token() to write the token to local storage so it persists across Python sessions.
  3. User calls client.get_available_tenants() to see what tenants they can login to
  4. User calls client.log_to_tenant(tenant_id=<>, tenant_slug=<>) (with one or the other argument). When this happens, an ephemeral JWT is received as well as a refresh token. Note this step is also persisted across sessions.
  5. The User can now make API calls as if they were logged in to the tenant in question, for example client.graphql(...). The Client is managing all refreshing and exchanging of JWTs in the background.
  6. The User is finished, so they call client.logout_from_tenant(). This discards the JWT access token.

The TOKEN- and AGENT- scoped flows are unchanged:

  1. Client instantiated with an API token (or receives one via env var/config)
  2. Client includes the API token in all calls.

@jlowin
Copy link
Member Author

jlowin commented Aug 27, 2019

Note for review: many of the files are affected by the name changes (auth_token -> api_token), but almost the entire PR is contained in this commit: e5af2d6

@codecov
Copy link

codecov bot commented Aug 27, 2019

Codecov Report

Merging #1410 into master will increase coverage by 0.05%.
The diff coverage is 98.93%.

docs/cloud/cloud_concepts/api.md Outdated Show resolved Hide resolved
docs/cloud/cloud_concepts/api.md Outdated Show resolved Hide resolved
@@ -59,4 +58,7 @@ def login(token):
click.secho("Error attempting to communicate with Prefect Cloud", fg="red")
return

# save token
client.save_api_token()

Choose a reason for hiding this comment

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

Will this now work or will users still need to select a tenant? Should we make a CLI command to switch tenants?

Copy link
Member Author

Choose a reason for hiding this comment

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

This will work fine with current API tokens that don't require selecting a tenant; it writes the token to disk (and recovers it) without regard for what token it is.

We should 100% make a CLI interface for this, but I wanted to get the machinery in first.


[cloud]
# the Prefect Server address
api = "https://api.prefect.io"
graphql = "${cloud.api}/graphql/alpha"
api = "https://api.prefect.io/graphql/alpha"

Choose a reason for hiding this comment

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

In a lot of places deployment related this is provided only as https://api.prefect.io because it was placed into graphql with {}/graphql/alpha so you should also update the defaults in all the agent code.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would say the fallback should actually remain api.prefect.io so that it isn't versioned, and the config should include the versioned API (which it now does). That way the agent code doesn't need updating in most scenarios, and does have a sane fallback (since the Agent will always be hitting the most current API -- the main reason for changing the config is so that users could store a separate API token for each endpoint during testing)

I think I got all of the places where graphql/alpha was being added manually, the only place I still see it in the codebase is config.toml

src/prefect/agent/kubernetes/agent.py Outdated Show resolved Hide resolved
joshmeek
joshmeek previously approved these changes Aug 28, 2019
Copy link
Member

@cicdw cicdw left a comment

Choose a reason for hiding this comment

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

Overall this looks solid, two comments and I'm going to do another pass

docs/cloud/cloud_concepts/api.md Outdated Show resolved Hide resolved
- name: PREFECT__CLOUD__AUTH_TOKEN
value: PREFECT__CLOUD__AUTH_TOKEN
- name: PREFECT__CLOUD__API_TOKEN
value: PREFECT__CLOUD__API_TOKEN
Copy link
Member

Choose a reason for hiding this comment

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

For the record, this is a large breaking change - this means that once this agent is deployed, it can only deploy flows built off of the most recent version of Core.

Copy link
Member

Choose a reason for hiding this comment

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

This includes the local agent.

Copy link

@joshmeek joshmeek Aug 29, 2019

Choose a reason for hiding this comment

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

Oh yeah this hadn’t occurred to me. Should we make it so that it places both API_TOKEN and AUTH_TOKEN in the images to maintain current compatibility?

Co-Authored-By: Chris White <chris@prefect.io>
api = "https://api.prefect.io"
graphql = "${cloud.api}/graphql/alpha"
# the Prefect Cloud API
graphql = "https://api.prefect.io/graphql/alpha"

Choose a reason for hiding this comment

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

Why was api removed?

@jlowin
Copy link
Member Author

jlowin commented Aug 29, 2019

Closing for #1423 which does not have any breaking changes

@jlowin jlowin closed this Aug 29, 2019
@jlowin jlowin deleted the client-auth branch August 29, 2019 20:56
abrookins pushed a commit that referenced this pull request Mar 15, 2022
Doc: fix account name in cloud screenshot
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