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

personal key auth #156

Merged
merged 20 commits into from
Mar 15, 2024
Merged

personal key auth #156

merged 20 commits into from
Mar 15, 2024

Conversation

mj3cheun
Copy link
Contributor

@mj3cheun mj3cheun commented Mar 7, 2024

solves #80

includes feature code and documentation

example of pkey interface in use:

import {ClientPkey,} from '@graphistry/client-api';
const client = new ClientPkey('TQGURJJD42', '9XOES9I0C2QZDLEY', '', 'http', 'localhost');

@lmeyerov lmeyerov self-requested a review March 7, 2024 18:59
@mj3cheun mj3cheun marked this pull request as ready for review March 8, 2024 17:04
@mj3cheun mj3cheun changed the title personal key auth draft personal key auth Mar 8, 2024
@@ -312,7 +313,7 @@ export class File {

///////////////////////////////////////////////////////////////////////////////

private fillMetadata(data: any, client: Client): void {
private fillMetadata(data: any, client: Client | ClientPkey): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

same, AbstractClient, right?

Copy link
Contributor

@lmeyerov lmeyerov left a comment

Choose a reason for hiding this comment

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

Awesome, agreed w/ decision to separate out the base client from diff authentication entrypoints

  • js-upload-api is an internal class; we should export ClientPKey + AbstractClient to client-api, client-api-react, node-api for per-environment use. This may require a similar dual-client export, e.g., export class ClientPKey extends ClientPKeyBase:

    export class Client extends ClientBase {

  • instead of hard-coding Client | ClientPkey everywhere, we should be passing around AbstractClient where possible, or a new interface ClientI

  • I was a bit unsure about error handling, esp for explainability for devs + propagating to end-users

After this one goes in, we may want a quick follow-up on ensuring docs go through too (or as part, either way)

@mj3cheun
Copy link
Contributor Author

lets split this into two, this first part can go in alone and the second part which will include changes to graphistry/graphistry in order to allow POST to the pkey endpoint and re-add org to @graphistry/client-api can go in afterwards. i believe everything has been addressed.

clientProtocolHostname,
version
) {
console.debug('new client', { personalKeyId }, { personalKeySecret }, { protocol }, { host }, { clientProtocolHostname }, { version });
Copy link
Contributor

Choose a reason for hiding this comment

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

can we drop the debug statements, as they'll be in customer code vs ours?

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 esp thinking any that print the creds)

@lmeyerov
Copy link
Contributor

  • Approved, tho plz remove console.debug of creds

  • Splitting sounds good, tho I didn't understand what this is about, this is about using existing Graphistry pkey endpoints?

graphistry/graphistry in order to allow POST

  • RE:Graphistry upgrade follow-on to use the new client, great. We can pair th/f and I walk you through npm publishing, and after that, you have the client

FWIW, have you done live testing, e.g., in storybook + node, or elsewhere?

@mj3cheun
Copy link
Contributor Author

  1. credential debug statements dropped
  2. yes this is about not being able to utilise the existing pkey auth endpoint wrt sending org info to the backend because of fetch limitations
  3. i have done testing in node, thats fully working. i tried testing for react but there is an issue there with the api client-api-react is designed to work with, will carry on the convo in slack

@mj3cheun mj3cheun merged commit 10fd9ab into master Mar 15, 2024
3 checks passed
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.

2 participants