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

feat: SSO Improvement - alter user_sessions table to include access token, implement CRUD ops, GET, POST, PATCH APIs and CLIs #9867

Open
wants to merge 40 commits into
base: main
Choose a base branch
from

Conversation

ShreyaLnuHpe
Copy link
Contributor

@ShreyaLnuHpe ShreyaLnuHpe commented Aug 26, 2024

Ticket

DET-10397
DET-10396
DET-10454
DET-10403
DET-10455
DET-10398
DET-10405
DET-10455

Description

Allowing users to create long lived access tokens that they can use for authentication.

Altering 'user_sessions' table for tracking access tokens that can be used for authentication & association with the appropriate user. Including CRUD operation functions on this table, leveraging the token id (id).

Updated table fields:

token_type AS ENUM (
'USER_SESSION', 
'ACCESS_TOKEN'
);

public.user_sessions (
    id integer GENERATED ALWAYS AS IDENTITY PRIMARY KEY,
    user_id integer NOT NULL,
    expiry TIMESTAMP NOT NULL,
    created_at TIMESTAMP DEFAULT NULL,
    token_type token_type NOT NULL DEFAULT 'USER_SESSION',
    revoked boolean NOT NULL DEFAULT false,
    description TEXT DEFAULT NULL,
)

APIs:

POST /api/v1/users/{user_Id}/token - Create and get a user's access token
GET /api/vi/user/tokens - Get list of all access token info
GET /api/v1/users/{user_Id}/token - Get user's access token info
PATCH /api/v1/users/token/{tokenId} - Patch an access tokens mutable fields.

CLI commands:

  1. det user token list/ls --(a)ll
% det user token ls --all                                                                   

   ID |   User ID | Description      | Created At                  | Expires At                  | Revoked   | Token Type
------+-----------+------------------+-----------------------------+-----------------------------+-----------+------------------
 5656 |      2305 |                  | 2024-09-18T05:12:48.424089Z | 2024-09-18T05:12:50.424089Z | True      | ACCESS_TOKEN
 5662 |         1 |                  | 2024-09-18T05:41:55.342039Z | 2024-09-18T05:41:57.342039Z | True      | ACCESS_TOKEN
 5673 |      2305 |                  | 2024-09-18T17:50:02.784704Z | 2024-09-18T17:50:04.784704Z | False     | ACCESS_TOKEN
 5675 |         1 |                  | 2024-09-18T17:59:52.688643Z | 2024-09-18T17:59:54.688643Z | False     | ACCESS_TOKEN
...
  1. det user token create --username --lifespan --description
 % det user token create --yaml --lifespan 2h --username 5a6e47d6-97a3-40dc-a45e-6f99b5062ad5

token: 
  v2.public.eyJpZCI6NTcxNSwidXNlcl9pZCI6MjMwNSwiZXhwaXJ5IjoiMjAyNC0wOS0yMFQwMjoxMjowNy42Mzg0NDRaIiwiY3JlYXRlZF9hdCI6IjIwMjQtMDktMjBUMDA6MTI6MDcuNjM4NDQ0WiIsInRva2VuX3R5cGUiOiJMT05HX0xJVkVEX1RPS0VOIiwicmV2b2tlZCI6ZmFsc2UsImRlc2NyaXB0aW9uIjpudWxsLCJJbmhlcml0ZWRDbGFpbXMiOm51bGx9DjNQpQv4Y-U11LaFEp2p06IzzTKIuiEGQSR-k9SXbMctBn3L-Y_U33J8XDdip8K24EsdaDps7RCnHzRDpAZgCw.bnVsbA
  1. det user token describe <username>
% det user token describe admin
   ID |   User ID | Description      | Created At                  | Expires At                  | Revoked   | Token Type
------+-----------+------------------+-----------------------------+-----------------------------+-----------+------------------
 5714 |         1 |                  | 2024-09-18T05:12:48.424089Z | 2024-09-18T05:12:50.424089Z | False     | ACCESS_TOKEN

Multiple usernames to describe:

% det user token describe admin 5a6e47d6-97a3-40dc-a45e-6f99b5062ad5
   ID |   User ID | Description      | Created At                  | Expires At                  | Revoked   | Token Type
------+-----------+------------------+-----------------------------+-----------------------------+-----------+------------------
 5714 |         1 |                  | 2024-09-18T05:12:48.424089Z | 2024-09-18T05:12:50.424089Z | False     | ACCESS_TOKEN
 5713 |      2305 |                  | 2024-09-18T05:41:55.342039Z | 2024-09-18T05:41:57.342039Z | False     | ACCESS_TOKEN
  1. det user token revoke <token_id>
% det user token revoke 5715                 
{
  "tokenInfo": {
    "id": 5715,
    "userId": 2305,
    "createdAt": "2024-09-20T00:12:07.638444Z",
    "description": "test_description",
    "expiry": "2024-09-20T02:12:07.638444Z",
    "revoked": true,
    "tokenType": "TOKEN_TYPE_ACCESS_TOKEN"
  }
}
Successfully revoked token with ID: 5715
  1. det user token update <token_id> <description_str>
 % det user token update 5715 test_description
{
  "tokenInfo": {
    "id": 5715,
    "userId": 2305,
    "createdAt": "2024-09-20T00:12:07.638444Z",
    "description": "test_description",
    "expiry": "2024-09-20T02:12:07.638444Z",
    "revoked": false,
    "tokenType": "TOKEN_TYPE_ACCESS_TOKEN"
  }
}
Successfully updated token with ID: 5715

Test Plan

  1. Login to latest-main Swagger as Admin and try all 7 APIs
  2. Should notice permissions kick in when trying with non-Admin user

Completed local tests

  • Table alter & Integration Tests of CRUD Ops
  • API creations & Integration Tests
  • CLI for CREATE, GET, PATCH API

After migration, you can see the altered 'user_sessions' table, along with CRUD operations to support POST, GET and PATCH operations.

Migration to create a table of the given schema:

$ cd master/static/migrations
$ ./migration-create.sh add-long-lived-tokens-table
$ cd ~/Projects/determined/
$ master/build/determined-master --config-file /Users/shreya/Projects/determined/.circleci/devcluster/master.yaml migrate up 

To build and generate files

$ make -C proto build
$ make -C harness build
$ make -C bindings build
$ make -C master build

To mock

$ make -C master clean
$ make -C master check OR $ make -C master mocks

otherwise:

$ make clean
$ make all

To check:

$ make -C proto fmt
$ make -C master fmt
$ make -C proto check
$ make -C master check

Checklist

  • Changes have been manually QA'd
  • New features have been approved by the corresponding PM
  • User-facing API changes have the "User-facing API Change" label
  • Release notes have been added as a separate file under docs/release-notes/
    See Release Note for details.
  • Licenses have been included for new code which was copied and/or modified from any external code

@cla-bot cla-bot bot added the cla-signed label Aug 26, 2024
Copy link

netlify bot commented Aug 26, 2024

Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit 784e69d
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/66fd8baf04703500085839c1
😎 Deploy Preview https://deploy-preview-9867--determined-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Aug 26, 2024

Codecov Report

Attention: Patch coverage is 19.84127% with 101 lines in your changes missing coverage. Please review.

Project coverage is 51.46%. Comparing base (88a4c67) to head (784e69d).
Report is 48 commits behind head on main.

Files with missing lines Patch % Lines
harness/determined/cli/user.py 14.45% 71 Missing ⚠️
harness/determined/common/experimental/user.py 33.33% 18 Missing ⚠️
...rness/determined/common/experimental/determined.py 9.09% 10 Missing ⚠️
harness/determined/experimental/client.py 60.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9867      +/-   ##
==========================================
- Coverage   54.52%   51.46%   -3.07%     
==========================================
  Files        1252      710     -542     
  Lines      156688   102412   -54276     
  Branches     3600     3601       +1     
==========================================
- Hits        85435    52706   -32729     
+ Misses      71120    49573   -21547     
  Partials      133      133              
Flag Coverage Δ
backend 43.80% <ø> (-1.42%) ⬇️
harness 44.13% <19.84%> (-28.62%) ⬇️
web 54.27% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
harness/determined/experimental/client.py 55.12% <60.00%> (-0.14%) ⬇️
...rness/determined/common/experimental/determined.py 25.28% <9.09%> (-21.43%) ⬇️
harness/determined/common/experimental/user.py 26.96% <33.33%> (-37.55%) ⬇️
harness/determined/cli/user.py 15.45% <14.45%> (-37.79%) ⬇️

... and 695 files with indirect coverage changes

@ShreyaLnuHpe ShreyaLnuHpe changed the title feat: create table and crud ops feat: create long_lived_tokens table and implement CRUD operations Aug 28, 2024
@ShreyaLnuHpe ShreyaLnuHpe marked this pull request as ready for review August 28, 2024 09:17
@ShreyaLnuHpe ShreyaLnuHpe requested a review from a team as a code owner August 28, 2024 09:17
@ShreyaLnuHpe ShreyaLnuHpe changed the title feat: create long_lived_tokens table and implement CRUD operations feat: create long_lived_tokens table, implement CRUD operations, create APIs Aug 30, 2024
@ShreyaLnuHpe ShreyaLnuHpe changed the title feat: create long_lived_tokens table, implement CRUD operations, create APIs feat: create long_lived_tokens table, implement CRUD operations, create GET, POST, DELETE APIs Sep 4, 2024
@ShreyaLnuHpe ShreyaLnuHpe changed the title feat: alter user_sessions table to include long lived token, implement CRUD ops, GET, POST, DELETE APIs and CLIs feat: alter user_sessions table to include access token, implement CRUD ops, GET, POST, PATCH APIs and CLIs Sep 19, 2024
@ShreyaLnuHpe ShreyaLnuHpe changed the title feat: alter user_sessions table to include access token, implement CRUD ops, GET, POST, PATCH APIs and CLIs feat: SSO Improvement - alter user_sessions table to include access token, implement CRUD ops, GET, POST, PATCH APIs and CLIs Sep 24, 2024


@_require_singleton
def list_tokens(includeInactive: Optional[bool] = None) -> List[TokenInfo]:
Copy link
Member

Choose a reason for hiding this comment

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

no camel casing python, please

Copy link
Member

Choose a reason for hiding this comment

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

(well, except for ClassNames)

@@ -313,6 +443,46 @@ def edit(args: argparse.Namespace) -> None:
help="grant/remove user admin permissions",
),
]),
cli.Cmd("token", None, "manage access tokens", [
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this belongs as a whole subtree of det user. Nested subcommands makes the CLI harder to learn for end users.

Can we do something more like:

  • det token login (instead of det user login --token)
  • det token describe
  • det token list
  • det token revoke
  • det token create
  • det token update

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was a conversation with @azhou-determined here. Let us know your thoughts

Copy link
Contributor

Choose a reason for hiding this comment

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

so @rb-determined-ai and i discussed this. TL;DR: we compromised to det user list-tokens, det user describe-token, etc. it shouldn't be a big change but sorry for proposing this now. 😞

his main points:

  • nested subcommands are bad, requires users to know various entity hierarchies
  • it's better to have a lot of top-level commands than nested subcommands

my main points:

  • nested subcommands aren't bad if they make sense, i'd assume this token functionality would be in the user management domain
  • the args for the commands are important, since we're basically binding user/username to token, det user token is more representative of the system

we agreed that since user:token is 1:1, det user makes more sense. and to avoid having too many subcommands, hyphenating the subcommands, likedet user list-tokens, seems like a great compromise. we also already do this for other commands (e.g. det e list-checkpoints/list-trials) so i'm happy with this.

@@ -582,3 +582,21 @@ def stream_trials_validation_metrics(
stacklevel=2,
)
return trial._stream_validation_metrics(self._session, trial_ids)

def list_tokens(self, includeInactive: Optional[bool] = None) -> List[user.TokenInfo]:
Copy link
Member

Choose a reason for hiding this comment

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

more camel casing on parameters

)

resps = api.read_paginated(get_with_offset)
userSessions = []
Copy link
Member

Choose a reason for hiding this comment

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

Please also avoid camel casing in our code. This isn't as critical as it isn't a user API.

I suppose I should also mention that the generated bindings have weird casing that isn't pythonic because generated bindings are just worse than hand-written code, at least from an ergonomic perspective. But prefer snake_case in the python code that you hand write.

Copy link
Member

Choose a reason for hiding this comment

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

Also, why is this variable called "user sessions" instead of like, "user tokens" or something?

),
]),
cli.Cmd("update", update_token, "update token info", [
cli.Arg("token_id", help="revoke given access token"),
Copy link
Member

Choose a reason for hiding this comment

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

help text seems wrong

self.revoked: Optional[bool] = None
self.description: Optional[str] = None

def _hydrate(self, tokenInfo: bindings.v1TokenInfo) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this is the right application of the _hydrate() pattern. Maybe @azhou-determined can comment on it.

Are there cases where you have just the token_id from a response, and you might not want to fetch the whole thing from the API? It seems like the answer is "probably not" and you should always have every field. And only "revoked" and "description" seem like they might be mutable, even though the docstring says that most fields are mutable.

Copy link
Contributor

Choose a reason for hiding this comment

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

agree with @rb-determined-ai here. the hydrate pattern is to support caching: attributes that may change can get explicitly refreshed without incurring the round-trip network cost of refreshing everything, every time. for this reason, hydrate is always paired with def reload().

this class right now is basically a pure data class that just packages some fields together. just set these in _from_bindings

@@ -118,3 +119,56 @@ def _from_bindings(cls, user_bindings: bindings.v1User, session: api.Session) ->
user = cls(session=session, user_id=user_bindings.id)
user._hydrate(user_bindings)
return user


class TokenType(enum.Enum):
Copy link
Member

Choose a reason for hiding this comment

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

If you do go with the det token * cli pattern instead of det user token *, I'd say this should probably be a file named token.py, not part of user.py. Especially since you can only do Determined.list_tokens() (implying that you can only list your own tokens) and not User.list_tokens() (which would imply that like, an admin could list other users' tokens).

Actually... is it intended to allow admins to list tokens for their users?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, only ClusterAdmins has permission to list the token info of other users

Copy link
Member

Choose a reason for hiding this comment

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

How would they do that with this SDK design? Or with this CLI design?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CLI design would look like this for an Admin:

% det user token ls --all                                                                   

   ID |   User ID | Description      | Created At                  | Expires At                  | Revoked   | Token Type
------+-----------+------------------+-----------------------------+-----------------------------+-----------+------------------
 5656 |      2305 |                  | 2024-09-18T05:12:48.424089Z | 2024-09-18T05:12:50.424089Z | True      | ACCESS_TOKEN
 5662 |         1 |                  | 2024-09-18T05:41:55.342039Z | 2024-09-18T05:41:57.342039Z | True      | ACCESS_TOKEN
 5673 |      2305 |                  | 2024-09-18T17:50:02.784704Z | 2024-09-18T17:50:04.784704Z | False     | ACCESS_TOKEN
 5675 |         1 |                  | 2024-09-18T17:59:52.688643Z | 2024-09-18T17:59:54.688643Z | False     | ACCESS_TOKEN
...

Comment on lines +247 to +258
values = []
for ti in token_info:
value = [
ti.id,
ti.userId,
ti.description,
ti.createdAt,
ti.expiry,
ti.revoked,
ti.tokenType,
]
values.append(value)
Copy link
Member

Choose a reason for hiding this comment

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

note on writing more pythonic code: use a list comprehension here, rather that a for loop with appending.

values = [
  [t.id, t.userId, t.description, t.createdAt, t.expiry, t.revoked, t.tokenType]
  for t in token_info
]

Comment on lines +263 to +268
if data:
json_data = [elem.to_json() for elem in data]
if len(data) == 1:
json_data = json_data[0]
else:
json_data = {}
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this is only called when people call det token list --json or --yaml.

That means people are probably intending to consume the output in a script.

That means that you should be consistent about the data format that you emit. Right now:

  • if there are no tokens, you emit an empty dict
  • if there is one token, you emit a non-empty dict
  • if there are multiple tokens, you emit a list of dicts

Instead, just always emit a list of dicts, which may be length 0, or 1, or >1.

Also, you probably don't need a helper function anymore with that change.

if args.json or args.yaml:
   json_data = [t.to_json() for t in resp.tokenInfo]
   if args.json:
       render.print_json(json_data)
   else:
       print(util.yaml_safe_dump(json_data, default_flow_style=False))
else:
    render_token_info(resp.tokenInfo)

result = []
for arg in args.username:
userID = bindings.get_GetUserByUsername(session=sess, username=arg).user.id
resp = bindings.get_GetAccessToken(session=sess, userId=userID)
Copy link
Member

Choose a reason for hiding this comment

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

random question: what is the difference between GetAcessToken and GetAllAccessTokens?

Why would det token describe my_user show one token but det token list show multiple tokens? How do you pick the one token if you only show one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GetAcessToken - Intends to return given user's token info. A non-admin will only be able to view their own token info, whereas admin can view any user's token info. Command run by Non-Admin : det user token describe <their_own_username>. Command run by Admin : det user token describe <username1> <username2> .. intends to return for the given usernames.

GetAllAccessTokens API - Intends to return all (active/revoked) token info. This is only accessible by admins. det user token list -(a)/ll CLI calls this API

try:
if args.description and args.token_id:
request = bindings.v1PatchAccessTokenRequest(
tokenId=args.token_id, description=args.description, setRevoked=False
Copy link
Member

Choose a reason for hiding this comment

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

can this accidentally un-revoke a token? That would feel like maybe not a security bug, but like a security "surprise".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

user will not be able to Patch any already revoked tokens. They would receive that error before Patch takes place.

Copy link
Contributor

Choose a reason for hiding this comment

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

if this is the case and an exception is thrown, we should catch whatever exception that'll be

Copy link
Member

@rb-determined-ai rb-determined-ai left a comment

Choose a reason for hiding this comment

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

OK, I know this is late (I was probably on paternity leave when this was originally discussed) but I found out talking to Anda that we have a limitation of only one active token per user (which answers some of my confusion about the cli design).

Why are we ok with that limitation? I've never heard of a service that limited users to one api token.

(I'm not sure who to CC, maybe @corban-beaird or @maxrussell?)

@@ -71,6 +83,19 @@ def log_in_user(args: argparse.Namespace) -> None:
else:
username = args.username

if args.token is not None:
token_store = authentication.TokenStore(args.master)
token_store.set_token(username, args.token)
Copy link
Contributor

Choose a reason for hiding this comment

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

we should do the validation check that the args.user = the token's user before setting it on the token store. as is, we'd be persisting invalid tokens before "logging in" with them. this would probably also cause problems later when trying to fetch auth from it if they didn't match.

token_store.set_token(username, args.token)
token_store.set_active(username)

d = client.Determined._from_session(
Copy link
Contributor

Choose a reason for hiding this comment

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

to get the user for a token, i think it's better to just call the "whoami" endpoint directly. seems unnecessary to construct a Determined client here. are the backend changes to support passing in these new tokens as auth in yet? ideally we'd just call the "/v1/me" endpoint directly with the "Bearer TOKEN" header, and get the user from there.

unauth_session = api.UnauthSession(master=master_address, cert=cli.cert)
auth_header = {"Authentication": f"Bearer {token}"}
user = unauth_session.get("/users/me", headers=auth_header).json()
username = user.get("username")

api.Session(master=args.master, username=username, token=args.token, cert=cli.cert)
)
user = d.whoami()
if user.username != username:
Copy link
Contributor

Choose a reason for hiding this comment

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

does the user need to pass in a username at all to det user login TOKEN? seems to me like the token should be all that's needed to authenticate?

user = d.whoami()
if user.username != username:
raise errors.CliError("Token does not match the provided username")
return
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this entire block is basically another way to "login", and therefore should live in determined.common.api.authentication.py, probably something like def login_with_token(token: str) -> "api.Session", which handles all the session/tokenstore logic.

ti.createdAt,
ti.expiry,
ti.revoked,
ti.tokenType,
Copy link
Contributor

Choose a reason for hiding this comment

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

would we maybe want to render "username" as well?

help="name of user to describe token"),
cli.Group(
cli.output_format_args["json"],
cli.output_format_args["yaml"],
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm not sure we need YAML here or in the other commands. YAMLs are largely for configs, and i can't imagine why you'd want to parse a YAML if you're using this as part of an automated workflow.

@@ -313,6 +443,46 @@ def edit(args: argparse.Namespace) -> None:
help="grant/remove user admin permissions",
),
]),
cli.Cmd("token", None, "manage access tokens", [
Copy link
Contributor

Choose a reason for hiding this comment

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

so @rb-determined-ai and i discussed this. TL;DR: we compromised to det user list-tokens, det user describe-token, etc. it shouldn't be a big change but sorry for proposing this now. 😞

his main points:

  • nested subcommands are bad, requires users to know various entity hierarchies
  • it's better to have a lot of top-level commands than nested subcommands

my main points:

  • nested subcommands aren't bad if they make sense, i'd assume this token functionality would be in the user management domain
  • the args for the commands are important, since we're basically binding user/username to token, det user token is more representative of the system

we agreed that since user:token is 1:1, det user makes more sense. and to avoid having too many subcommands, hyphenating the subcommands, likedet user list-tokens, seems like a great compromise. we also already do this for other commands (e.g. det e list-checkpoints/list-trials) so i'm happy with this.


@classmethod
def _from_bindings(
cls, tokenInfo_bindings: bindings.v1TokenInfo, session: api.Session
Copy link
Contributor

Choose a reason for hiding this comment

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

camelcase

@@ -582,3 +582,21 @@ def stream_trials_validation_metrics(
stacklevel=2,
)
return trial._stream_validation_metrics(self._session, trial_ids)

def list_tokens(self, includeInactive: Optional[bool] = None) -> List[user.TokenInfo]:
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason we are only introducing this single command into the SDK and not any of the others?

Copy link
Contributor

Choose a reason for hiding this comment

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

i feel like this "include inactive" bool isn't very SDK-like. usually the SDK is catered more towards developers integrating the methods in existing determined workflows, and since it's in code rather than command line, we can offer much more robust APIs.

here, "show all" suggests to me that what we really want is a filter on token status. it'd be nice if the backend APIs supported this as a parameter so filtering could happen master-side, but barring that, we could still accomplish it client-side.

i think this SDK command would be more flexible to users as: list_tokens(revoked: boolean = False, expired: boolean = False)

self.revoked: Optional[bool] = None
self.description: Optional[str] = None

def _hydrate(self, tokenInfo: bindings.v1TokenInfo) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

agree with @rb-determined-ai here. the hydrate pattern is to support caching: attributes that may change can get explicitly refreshed without incurring the round-trip network cost of refreshing everything, every time. for this reason, hydrate is always paired with def reload().

this class right now is basically a pure data class that just packages some fields together. just set these in _from_bindings

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants