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

Panoptes JS: add lib-panoptes-js dev server, add experimental auth #6375

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

shaunanoordin
Copy link
Member

PR Overview

Package: lib-panoptes-js
Part of: replacing PJC with Panoptes JS

This PR is part of an experiment to remove PJC from FEM, replacing its functionality fully with making Panoptes JS (PanoptesJS? panoptes.js?)

This PR focuses on some small steps:

  • an experimental-auth.js file has been added to lib-panoptes-js
    • The plan: we'll copy PJC's auth, one bit of functionality at a time, into PanoptesJS's experimentalAuth. We'll eventually turn experimentalAuth into auth once we're confident all the features are working.
    • Features in this PR:
      • ✔️ event listener system is functional (addEventListener(), removeEventListener(), broadcastEvent() ; these are analogous to PJC's listen(), stopListening(), and emit())
      • 🛠️ basic sign in action is a WIP (goal: signIn() should, if given a valid username & password, return a Panoptes User resource. That's it.)
  • a dev server has been added to lib-panoptes-js
    • Run with yarn dev
    • Dev server serves a very basic form to test login.

Dev Notes

  • As noted on Slack discussions, I have issues with how Panoptes JS is supposed to (based on the documentation) be "stateless", which conflicts incredibly with how PJC's auth functionality handles login state. (My argument is that if we're moving login state handling to modules/packages that call Panoptes JS, then we're just basically moving the burden up to the modules/packages.)
  • Nonetheless, I'm trying a hybrid to see if I can stay (semi-)true to the Panoptes JS goals:
    • experimentalAuth exports functions, not a single instantiated object. (cf PJC's auth object)
    • each function in experimentalAuth has an optional store variable that can be specified by the calling module/function.
    • if a store isn't specified (which I imagine is the case 99% of the time), the functions use a default, shared, global store.

Status

Experimental WIP.

@shaunanoordin shaunanoordin marked this pull request as draft October 11, 2024 20:48
@eatyourgreens
Copy link
Contributor

eatyourgreens commented Oct 12, 2024

(My argument is that if we're moving login state handling to modules/packages that call Panoptes JS, then we're just basically moving the burden up to the modules/packages.)

But also remember that a stateful client has major bugs like zooniverse/panoptes-javascript-client#207 and zooniverse/panoptes-javascript-client#250.

This is old now, but have a look at packages/lib-auth on these branches:

@eatyourgreens
Copy link
Contributor

eatyourgreens commented Oct 12, 2024

Worth keeping this in mind for any new auth work, given that the password flow code in PJC is ten years old, and not very secure.

The Password grant type is a legacy way to exchange a user's credentials for an access token. Because the client application has to collect the user's password and send it to the authorization server, it is not recommended that this grant be used at all anymore.

https://oauth.net/2/grant-types/password/

The refresh token flow used to get access tokens for authenticated API requests is still secure.

https://oauth.net/2/grant-types/refresh-token/

@goplayoutside3
Copy link
Contributor

@eatyourgreens this is a draft PR and not ready for code review. Please refrain from commenting on draft PRs. While Zooniverse's frontend repos are open source, this PR is marked as a draft because Shaun, myself, and the frontend team are in ongoing discussions about auth and lib-panoptes-js. The context of those discussions is not always available in a draft/experimental PR such as this one, and commenting beyond the scope of this PR becomes unhelpful.

I see why you've opened #6376 based on auth development years ago, but I'm going to close it in favor of keeping this PR focused. Please trust that Shaun, myself, and the rest of the dev team have a plan for FEM, including whether or not to keep using oauth + passwords.

When commenting on Zooniverse Issues or PRs marked as ready for review, please keep comments focused on blocking code such as bugs or best practices that might have been missed while being mindful we're a very small dev team. We do our best to address website bugs and respond to comments on Github, but are working with limited resources and Zooniverse is a large web platform.

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