Added authentication, set up PDP types

[tjcouch-sil]

Requires paranext/paranext-core@main...2192-support-slingshot to work

This change is 

[lyonsil]

Reviewed 7 of 13 files at r1.
Reviewable status: 7 of 13 files reviewed, 6 unresolved discussions

---

src/scripture-forge/src/main.ts line 82 at r1 (raw file):

    logger.info('Scripture Forge is unable to use encryption. Will not save login tokens.');
  const authStorageManager = isEncryptionAvailable
    ? new SecureSerialStorageManager(

I don't understand what Serial means here, but not a big deal.

---

src/scripture-forge/src/main.ts line 187 at r1 (raw file):

      // Perform first startup actions
      await papi.webViews.openWebView(homeWebViewProvider.webViewType, undefined, {

Did you talk to UX about this? This doesn't feel like the kind of thing we'd want to show on first startup. We're already prompting for the PT registration code on startup. It doesn't seem like we should open this, too.

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 21 at r1 (raw file):

  scope: string;
  state: string;
  // TODO: Is this necessary?

Who are these TODOs directed at?

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 85 at r1 (raw file):

const SCOPES = ['openid', 'profile', 'email', 'sf_data', 'offline_access'];
/** Auth audience for Scripture Forge...? */
const AUDIENCE = 'https://scriptureforge.org/';

Seems like this might change between prod, QA, etc.

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 157 at r1 (raw file):

function base64UrlEncode(buffer: Buffer): string {
  return buffer.toString('base64').replace(/\+/g, '-').replace(/\//g, '_').replace(/=/g, '');

Why not use encodeURIComponent instead of writing your own normalizer?

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 166 at r1 (raw file):

function getExpireTimeMs(expiresInSec: number): number {
  // Now plus the token lifetime converted from seconds to milliseconds

This might be a better JSDOC-style comment on the function itself

[tjcouch-sil]

Reviewable status: 7 of 13 files reviewed, 4 unresolved discussions (waiting on @lyonsil)

---

src/scripture-forge/src/main.ts line 82 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I don't understand what Serial means here, but not a big deal.

One at a time :) I can take it off if that doesn't make sense haha

---

src/scripture-forge/src/main.ts line 187 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Did you talk to UX about this? This doesn't feel like the kind of thing we'd want to show on first startup. We're already prompting for the PT registration code on startup. It doesn't seem like we should open this, too.

Nope; I just did it since that's what they asked for. I just asked Alex, though, and he said it's fine for now until we have some better system for something along these lines.

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 21 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Who are these TODOs directed at?

Me :) My intention is to resolve these TODOs by the time I'm done working on this first pass of the extension or make some list of improvements for later.

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 85 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Seems like this might change between prod, QA, etc.

My inclination is to agree with you; it doesn't feel particularly clear to me either that this should always be the same. I'm not super clear on what this audience thing is, but they said it's the same for all three. The quick googling I did didn't really clear up why it's the same for all three. https://docs.google.com/document/d/1M2KjgHlJcUlFWLOlykzZiKsbDCRECrLn2Ykk78s2ZdA/edit?tab=t.0#heading=h.km3cs6nljm3q

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 157 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Why not use encodeURIComponent instead of writing your own normalizer?

It seems "base64 url encoding" is a particular way to encode base64 in a way that works with URLs, but it is not the same as URL encoding. For example, "base64 url encoding" removes the equals signs because they are just padding that can be reconstructed. I don['t know why people want to use "base64 url encoding" as opposed to normal url encoding (maybe to save a very negligible amount of internet traffic...? 😂), but this is what all examples I have seen use. I figure this is what the PKCE authentication flow requires. I will add a comment in the TSDoc.

Playground example

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 166 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

This might be a better JSDOC-style comment on the function itself

Done

[lyonsil]

Reviewed 6 of 13 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @tjcouch-sil)

---

src/scripture-forge/src/main.ts line 82 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

One at a time :) I can take it off if that doesn't make sense haha

My inclination would be to drop it. There isn't a "non-serial" version for devs to pick between, and it seems like the mutex used in the class is necessary for data safety, so I don't think it needs to be in the name.

---

src/scripture-forge/src/main.ts line 187 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Nope; I just did it since that's what they asked for. I just asked Alex, though, and he said it's fine for now until we have some better system for something along these lines.

Thanks for checking!

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 157 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

It seems "base64 url encoding" is a particular way to encode base64 in a way that works with URLs, but it is not the same as URL encoding. For example, "base64 url encoding" removes the equals signs because they are just padding that can be reconstructed. I don['t know why people want to use "base64 url encoding" as opposed to normal url encoding (maybe to save a very negligible amount of internet traffic...? 😂), but this is what all examples I have seen use. I figure this is what the PKCE authentication flow requires. I will add a comment in the TSDoc.

Playground example

Thanks for the details! It's weird that you need to make your own utility function for this, but 🤷 .

---

src/scripture-forge/src/home/home.web-view.tsx line 36 at r1 (raw file):

      sessionChangeListener;

      setIsLoginBusy(true);

Should isMounted be checked before this line, too?

---

webpack/webpack.config.main.ts line 23 at r1 (raw file):

    // Build for node since Platform.Bible loads this in node https://webpack.js.org/concepts/targets/
    // target: 'node',

Why did this get commented out?

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 0 at r1 (raw file):
This is a rather big file that feels a little unwieldy. But most of it comes from the SF team themselves, so if they are OK managing it long-term then it's probably fine.

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 83 at r1 (raw file):

];
/** Necessary auth scopes for accessing Slingshot drafts */
const SCOPES = ['openid', 'profile', 'email', 'sf_data', 'offline_access'];

Minor thing, but SCOPES is only used in one place below, and it is joined every time it is used. Maybe just call .join(' ') on it here so it doesn't get called repeatedly later.

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 182 at r1 (raw file):

 */
export default class ScriptureForgeAuthenticationProvider implements Dispose {
  private unsubscriberPromises: Promise<Unsubscriber | UnsubscriberAsync>[] = [];

Why are we mixing private and # here? Same with constructor variables below. It gives the impression that we expect the private variables to be accessed by something at runtime since we're distinguishing.

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 230 at r1 (raw file):

   */
  async handleAuthCallback(searchParams: URLSearchParams): Promise<void> {
    // About to test this type to make sure it is correct

This doesn't seem like a comment destined for merging into main.

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 262 at r1 (raw file):

      if (await this.#getAccessToken()) return true;
    } catch (e) {
      logger.debug(`Error trying to get access token to check if logged in: ${e}`);

There are a number of places in this file that should probably be changed to use getErrorMessage.

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 318 at r1 (raw file):

    // Successfully logged in! Get the access token
    const tokenResponse = await this.#requestAccessTokenUsingAuthorizationCode(authorizationCode);
    // TODO: Handle errors

Was this overlooked unintentionally or are you intending to put this off for a later time?

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 355 at r1 (raw file):

    if (response.ok) return response;

    if (response.status === 401) {

It would be nice to use a package like http-status-codes to give constants like this more meaningful names. I think this is the only one, though, so feel free to just ignore this comment. I just have an aversion to memorizing things like status code number meanings.

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 395 at r1 (raw file):

    // TODO: Catch errors
    const unsubscribers = await Promise.all(this.unsubscriberPromises);

It looks like this will always be an empty list. Were you intending to add things to it? If not, it seems like unnecessary code.

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 454 at r1 (raw file):

          const tokenResponse = await this.#requestAccessTokenUsingRefreshToken(refreshToken);

          // TODO: Handle errors

This looks like a sort of error that should probably be handled. Otherwise, it looks like if you have a refresh token that isn't going through, you'll get stuck in a loop of errors.

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 553 at r1 (raw file):

  }

  // eslint-disable-next-line @typescript-eslint/class-methods-use-this, @typescript-eslint/no-unused-vars

I don't think this eslint-disable-next-line applies anymore. Both conditions are met now.

---

src/scripture-forge/src/projects/scripture-forge-api.model.ts line 4 at r1 (raw file):

 * Information about a project as received from Scripture Forge's `paratext-api/projects` endpoint
 *
 * Projects can be in one of three states regarding connectivity:

You list 4 states below, not 3. Kind of minor, but this is so complicated that it probably should be fixed to avoid any confusion.

Separately, would it make sense to add a function that spits out an enum with values that map to these states?

[tjcouch-sil]

Reviewable status: 6 of 17 files reviewed, 12 unresolved discussions (waiting on @lyonsil)

---

src/scripture-forge/src/main.ts line 82 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

My inclination would be to drop it. There isn't a "non-serial" version for devs to pick between, and it seems like the mutex used in the class is necessary for data safety, so I don't think it needs to be in the name.

Done :) thanks for the feedback.

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 83 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Minor thing, but SCOPES is only used in one place below, and it is joined every time it is used. Maybe just call .join(' ') on it here so it doesn't get called repeatedly later.

Done

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 157 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Thanks for the details! It's weird that you need to make your own utility function for this, but 🤷 .

Indeed

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 182 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Why are we mixing private and # here? Same with constructor variables below. It gives the impression that we expect the private variables to be accessed by something at runtime since we're distinguishing.

(Removed the private field as a result of changes from another comment)

tl;dr for what's ahead: I generally tend to use # only when I need it because iirc there's some overhead to it, the details of which I'm not super familiar with. If we want to have a discussion and potentially decide to use one or the other, that's fine. We just haven't done so.

In general, I use private for things that we don't really intend for outsiders to use, and I use # for things that I need outsiders not to use. IIRC, depending on what version of TypeScript you're running or which environment you're targeting, it may translate the # variables into a WeakMap separate from the class object so that outsiders can't access the variables. I think, whne you're targeting a newer environment, TS leaves # variables in because I think newer versions of JS support # directly (or are planned to in the near future or something).

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 230 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

This doesn't seem like a comment destined for merging into main.

Reworded to be clearer what I meant

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 262 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

There are a number of places in this file that should probably be changed to use getErrorMessage.

Done

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 318 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Was this overlooked unintentionally or are you intending to put this off for a later time?

Same with other TODOs - meant for a later time.

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 355 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

It would be nice to use a package like http-status-codes to give constants like this more meaningful names. I think this is the only one, though, so feel free to just ignore this comment. I just have an aversion to memorizing things like status code number meanings.

I will be using status codes a lot when I start handling errors, so that's a good idea. Thanks :)

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 395 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

It looks like this will always be an empty list. Were you intending to add things to it? If not, it seems like unnecessary code.

Ah I was thinking I was going to listen to server config changes in this class at one point, but then I changed my mind. Removed.

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 454 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

This looks like a sort of error that should probably be handled. Otherwise, it looks like if you have a refresh token that isn't going through, you'll get stuck in a loop of errors.

I think I'll need some clarification on this as I don't currently see where there would be a loop.

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 553 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I don't think this eslint-disable-next-line applies anymore. Both conditions are met now.

Good catch; thanks!

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

This is a rather big file that feels a little unwieldy. But most of it comes from the SF team themselves, so if they are OK managing it long-term then it's probably fine.

Maybe I can split the oauth calls into a utility file...? I dunno; I was just writing code haha I'm open to suggestions. It's sometimes hard for me to zoom out and see the bigger picture, so it would be interesting to discuss and hear your perspective.

---

src/scripture-forge/src/home/home.web-view.tsx line 36 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Should isMounted be checked before this line, too?

Because everything before await runs synchronously, we know it is mounted if we got to this code. It won't run if not mounted.

---

src/scripture-forge/src/projects/scripture-forge-api.model.ts line 4 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

You list 4 states below, not 3. Kind of minor, but this is so complicated that it probably should be fixed to avoid any confusion.

Separately, would it make sense to add a function that spits out an enum with values that map to these states?

Good catch!

I plan to add code in the PDP that interprets these conditions and spits out a choice amongst a string union (basically the closest thing we have to enums in this strange environment we're running). That string union is SlingshotProjectConnectionState, and you can see all these conditions represented in there.

I figured I'd make this class do as little as possible beyond strictly accessing the API, then the PDP can interpret the API responses into meaningful information for our uses. So just the PDPF and PDP will touch ScriptureForgeProjectInfo; everything else will touch more focused data as distilled by the PDP.

---

webpack/webpack.config.main.ts line 23 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Why did this get commented out?

Adjusted to explain better what's going on. I have a note in my list of things to do to reconsider this and determine if it makes sense to keep and put in the templates, so I didn't think to tidy it up.

[irahopkinson]

Thanks for all this good work figuring out how to connect.

Reviewed 11 of 13 files at r1, 1 of 1 files at r2, 11 of 11 files at r3, all commit messages.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @lyonsil and @tjcouch-sil)

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Maybe I can split the oauth calls into a utility file...? I dunno; I was just writing code haha I'm open to suggestions. It's sometimes hard for me to zoom out and see the bigger picture, so it would be interesting to discuss and hear your perspective.

I'm surprised you aren't using one of the Auth0 libraries for all this. That is kinda the main point of using Auth0 so you don't have to write all of this.

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 204 at r2 (raw file):

  /**
   * @param redirectUri The full uri including path to which the authentication site will redirect

NIT: this TSDoc is missing 2 of the arguments.

---

src/scripture-forge/src/main.ts line 45 at r2 (raw file):

  const serverConfigurationValidatorPromise = papi.settings.registerValidator(
    'scriptureForge.serverConfiguration',
    // TODO: Localize these error messages. Do we do this in other validators?

IMHO errors should never be localized since then you could have trouble catching this specific error since the message would change. Also we don't want localized errors in the logs (which are for developers). However if you catch this error and want to tell the user then that message should be localized. Thrown error messages should not in general be shown to the user.

[tjcouch-sil]

Reviewable status: 16 of 17 files reviewed, 14 unresolved discussions (waiting on @irahopkinson and @lyonsil)

---

src/scripture-forge/src/main.ts line 45 at r2 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

IMHO errors should never be localized since then you could have trouble catching this specific error since the message would change. Also we don't want localized errors in the logs (which are for developers). However if you catch this error and want to tell the user then that message should be localized. Thrown error messages should not in general be shown to the user.

Validator errors are simply passed along and displayed in the Settings dialog. If you find this to be particularly concerning, we can always consider, find another solution, and deprecate this endpoint. For this PR and general time frame, though, I would like to continue on this path. If you feel particularly interested in finding another way, would you mind please filing an issue so we could take a bigger-picture look another time?

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

I'm surprised you aren't using one of the Auth0 libraries for all this. That is kinda the main point of using Auth0 so you don't have to write all of this.

See https://docs.google.com/document/d/1M2KjgHlJcUlFWLOlykzZiKsbDCRECrLn2Ykk78s2ZdA/edit?disco=AAABdXewoXc for explanation why I'm not using an auth0 package

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 204 at r2 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

NIT: this TSDoc is missing 2 of the arguments.

Fixed - good catch

[lyonsil]

Thanks for all the updates!

Reviewed 10 of 11 files at r3, 1 of 1 files at r4, 2 of 3 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @irahopkinson)

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 230 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Reworded to be clearer what I meant

Ah - that's not how I read it, but I see what you were getting at. Thanks for bearing with me!

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 454 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I think I'll need some clarification on this as I don't currently see where there would be a loop.

We talked through this - my mistake. Thanks!

---

src/scripture-forge/src/home/home.web-view.tsx line 36 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Because everything before await runs synchronously, we know it is mounted if we got to this code. It won't run if not mounted.

Agreed - I was thinking that usePromise might do this, but as we talked about it seems like that's not the case.

---

src/scripture-forge/src/projects/scripture-forge-api.model.ts line 4 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Good catch!

I plan to add code in the PDP that interprets these conditions and spits out a choice amongst a string union (basically the closest thing we have to enums in this strange environment we're running). That string union is SlingshotProjectConnectionState, and you can see all these conditions represented in there.

I figured I'd make this class do as little as possible beyond strictly accessing the API, then the PDP can interpret the API responses into meaningful information for our uses. So just the PDPF and PDP will touch ScriptureForgeProjectInfo; everything else will touch more focused data as distilled by the PDP.

Thanks for the explanation!

---

webpack/webpack.config.main.ts line 23 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Adjusted to explain better what's going on. I have a note in my list of things to do to reconsider this and determine if it makes sense to keep and put in the templates, so I didn't think to tidy it up.

Thanks for the description!

[irahopkinson]

Reviewed 2 of 3 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @tjcouch-sil)

---

src/scripture-forge/src/main.ts line 45 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Validator errors are simply passed along and displayed in the Settings dialog. If you find this to be particularly concerning, we can always consider, find another solution, and deprecate this endpoint. For this PR and general time frame, though, I would like to continue on this path. If you feel particularly interested in finding another way, would you mind please filing an issue so we could take a bigger-picture look another time?

Yep, that doesn't need to hold this up if that was a previous decision and so out of scope for this.

---

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

See https://docs.google.com/document/d/1M2KjgHlJcUlFWLOlykzZiKsbDCRECrLn2Ykk78s2ZdA/edit?disco=AAABdXewoXc for explanation why I'm not using an auth0 package

Gotcha