feat: minimal implementation of Auth and WebApp clients #51
Conversation
…deprecated field descriptions.
santhoshramaraj
requested review from
mure,
spanglerco and
cameronwaterman
as code owners
mure
changed the title
|
FYI I updated your PR title to match the commit format needed for our automated release workflow. |
| The login name of the user. This the "username" or equivalent entered when | ||
| the user authenticates with the identity provider. | ||
| """ | ||
| accepted_to_s: Optional[bool] = Field(None, alias="acceptedToS", example=True) |
There was a problem hiding this comment.
I know we're not using this model for anything yet, but accepted_to_s is a weird name. Probably should just be accepted_tos
| from . import models | ||
|
|
||
|
|
||
| class AuthClient(BaseClient): |
There was a problem hiding this comment.
I think it's very likely we'll want to combine the /niuser and /niauth methods into a single client. There's not really a meaningful distinction between them and they're both implemented in the same service anyway. With that in mind, is AuthClient the name we want to go with? I could see UserClient, AdminClient, AccessControlClient off the top of my head.
@spanglerco, @kjohn1922 , @cameronwaterman any opinions?
…ditional_models.py names
| user: Optional[User] | ||
| """Details of authenticated caller""" | ||
| org: Optional[Org] | ||
| """Organization of authenticated caller""" | ||
| workspaces: Optional[List[Workspace]] | ||
| """List of workspaces the authenticated caller has access""" | ||
| policies: Optional[List[AuthPolicy]] | ||
| """List of policies for the authenticated caller""" | ||
| properties: Optional[Dict[str, str]] = Field(None, example={"key1": "value1"}) | ||
| """A map of key value properties""" |
There was a problem hiding this comment.
Are these fields actually Optional? They should only be optional if it's possible for the JSON response to have null for a property. Now that I'm looking at the rest of the models, it looks like the generator is overly pessimistic and marks every field as optional. Is this something we can configure?
There was a problem hiding this comment.
It looks like the Swagger doc has modeled these as optional fields.
https://test-api.lifecyclesolutions.ni.com/niapis/?urls.primaryName=Auth%20Service
https://test-api.lifecyclesolutions.ni.com/niauth/swagger/v1/niauth.yaml
AuthResponse:
description: Auth Response
schema:
type: object
title: Auth Response
properties:
user:
$ref: '#/definitions/User'
org:
$ref: '#/definitions/Org'
workspaces:
type: array
items:
$ref: '#/definitions/Workspace'
policies:
type: array
items:
$ref: '#/definitions/AuthPolicy'
properties:
type: object
description: A map of key value properties
additionalProperties:
type: string
example:
key1: value1
| def test_webapps(client: WebappClient) -> List[str]: | ||
| """Fixture to return the pre-created WebApp IDs for testing.""" | ||
| webapp_ids = [ | ||
| "452bc11b-6a8a-4c3e-8eb1-721270157eb8", # 'PyAPI_Test1' dataspace | ||
| "0345a956-3747-42e1-9296-6ed06545bd0d", # 'PyAPI_Test2' dataspace | ||
| ] | ||
| return webapp_ids |
There was a problem hiding this comment.
I thought I had left a comment on this, but I can't seem to find it anymore 😕 These tests should be able to create the fake webapps if they don't exist. We wouldn't want the tests to break if the cluster's data gets wiped. Here's an example fixture from the data frame tests:
What does this Pull Request accomplish?
Adds support for Auth and WebApp services with limited scope
Why should this Pull Request be merged?
Auth and WebApp clients are a dependency for DataSpace client
What testing has been done?
Tested with main-test using Pytest