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(lib-auth): @zooniverse/auth package #6376

Closed
wants to merge 11 commits into from

Conversation

eatyourgreens
Copy link
Contributor

@eatyourgreens eatyourgreens commented Oct 12, 2024

This revives the work on a new Panoptes auth client, from 2019, if it's useful. It implements client-side user registration and sign-in in a new, tested package: @zooniverse/auth.

Package

  • lib-auth

Linked Issue and/or Talk Post

How to Review

See the package Readme for docs, and the auth-client-test branch for an example implementation of Panoptes sign-in in cca57bb.

Checklist

PR Creator - Please cater the checklist to fit the review needed for your code changes.
PR Reviewer - Use the checklist during your review. Each point should be checkmarked or discussed before PR approval.

General

  • Tests are passing locally and on Github
  • Documentation is up to date and changelog has been updated if appropriate
  • You can yarn panic && yarn bootstrap or docker-compose up --build and FEM works as expected
  • FEM works in all major desktop browsers: Firefox, Chrome, Edge, Safari (Use Browserstack account as needed)
  • FEM works in a mobile browser

General UX

Example Staging Project: i-fancy-cats

  • All pages of a FEM project load: Home Page, Classify Page, and About Pages
  • Can submit a classification
  • Can sign-in and sign-out
  • The component is accessible

New Feature

  • The PR creator has listed user actions to use when testing the new feature
  • Unit tests are included for the new feature
  • A storybook story has been created or updated
    • Example of SlideTutorial component with docgen comments, and its story

@eatyourgreens eatyourgreens changed the title feat: packages/lib-auth feat(lib-auth): @zooniverse/auth package Oct 12, 2024
@coveralls
Copy link

Coverage Status

coverage: 78.346% (-0.2%) from 78.542%
when pulling 36df376 on eatyourgreens:auth-client
into 4062f39 on zooniverse:master.

@eatyourgreens
Copy link
Contributor Author

I closed the original PR because there was no time to finish this off but maybe it's worth picking up again.

Comment on lines +100 to +102
const { cookieName } = this._config
const cookies = cookie.parse(document.cookie)
return cookies[cookieName] || ''
Copy link
Contributor Author

@eatyourgreens eatyourgreens Oct 12, 2024

Choose a reason for hiding this comment

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

The Panoptes session cookie is a HttpOnly cookie, so it can't be read from JS like this. This function will always return an empty string.

Panoptes session cookies in Firefox, showing that they are secure and HttpOnly.

Copy link
Contributor Author

@eatyourgreens eatyourgreens Oct 12, 2024

Choose a reason for hiding this comment

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

The auth client can only read the Panoptes session cookie on the server, when the client is running in Node. The cookie can't be used when the auth client is running in a browser. Instead, you have to make a POST request to the Panoptes auth API, which returns the access token and refresh token for your session cookie.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternatively, use a third-party OAuth library to interact with the Panoptes OAuth APIs, so that you don't have to worry about the details of implementing an OAuth client here.

Comment on lines +137 to +144
this._state = {
accessToken: tokenData['access_token'],
accessTokenCreatedAt: tokenData['created_at'],
accessTokenExpiresAt: tokenData['created_at'] + tokenData['expires_in'],
accessTokenExpiresIn: tokenData['expires_in'],
refreshToken: tokenData['refresh_token'],
scope: tokenData.scope
}
Copy link
Contributor Author

@eatyourgreens eatyourgreens Oct 12, 2024

Choose a reason for hiding this comment

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

client._state is a public variable so this makes the refresh token visible via client._state.refreshToken. Your refresh token should be private just to you. _state should be private, so that it's not available outside the instantiated client object. PJC hides private state, like the refresh token, by using the module pattern for exports. Only methods and properties on the following list can be accessed from outside the client, to keep personal data secure.
https://github.com/zooniverse/panoptes-javascript-client/blob/8157794dfacfbc1f5d41c5730b2f47aae6fc013a/lib/auth.js#L341-L354

@eatyourgreens
Copy link
Contributor Author

I've left a couple of comments that I remember from when I last worked on this.

{
"name": "@zooniverse/auth",
"version": "1.0.0",
"main": "src/index.js",
Copy link
Contributor Author

@eatyourgreens eatyourgreens Oct 14, 2024

Choose a reason for hiding this comment

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

This new package was originally written as a CJS package. Nowadays, it would make more sense to write it as an ES Module. CJS has never been supported in browsers, and ESM is well-supported in Node now.

@goplayoutside3
Copy link
Contributor

See #6375 (comment)

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.

4 participants