fix api urls change auth behavior by davixcky · Pull Request #65 · signadot/browser-extension

davixcky · 2025-05-01T01:30:40Z

Issue:
The sessionn were expired and it was not reseting the cookies in case they were needed. Also internalstate was not being reset after change.

So i created a utlity function that resets to it's original state whenever there is a apiurl change

foxish

This doesn't work when trying to log out of staging. Please test your code for bugs: https://www.loom.com/share/ec7dc29941544111bfa5388a1fb8a174

foxish · 2025-05-01T15:28:29Z

src/contexts/AuthContext.tsx


+  const resetAuth = () => {
+    setAuthState(undefined);
+    setIsLoading(true);


This seems odd to me, does isLoading actually need to be a state variable, or can it actually be computed on the fly?

It's a state so we can show a loading while we try to fetch the user

Ok - makes sense. I think it's just feeling a bit overloaded when we use the same state to mean: signed out as well as "sign-in in progress". I don't feel strongly here, but it might be nice to actually separate out more states like:

type AuthStatus = | 'clear' // no auth creds | 'loading' // in the middle of a login request | 'success' // authed and valid | 'error'; // any failure during login or logout

Another idea is to do what we do on the web app side - that also seems to do some grouping:

https://github.com/signadot/signadot/blob/625996dbccf4e4318be1b733cbb0918a449854ff/sandboxes/www/src/contexts/AuthContext.tsx#L33-L41

foxish · 2025-05-01T15:33:01Z

src/components/Settings/Settings.tsx

    const cleanPreviewUrl = unsavedValues.previewUrl.replace(/\/+$/, "");
    const cleanDashboardUrl = unsavedValues.dashboardUrl.replace(/\/+$/, "");

+    // If there is a new apiUrl, we need to reset the auth state


I think we can use a more robust mechanism like https://github.com/braintree/sanitize-url rather than just trimming the string. WDYT?

davixcky · 2025-05-01T15:45:27Z

This doesn't work when trying to log out of staging. Please test your code for bugs:

I actually tested this case too and seems fine to me, not sure what race condition is happening, but i tested before sending to you and that's why i added e0dddc2

foxish · 2025-05-01T15:57:40Z

I saw that the icon doesn't update when I don't actually click reload on the extensions browser. So, maybe that was why it was harder to test. I think using a higher res icon should help.

foxish · 2025-05-01T15:58:48Z

I saw that the icon doesn't update when I don't actually click reload on the extensions browser. So, maybe that was why it was harder to test. I think using a higher res icon should help.

Oh, sorry, wrong issue - you can't repro the staging issue? Let me check what's up on my end then.

foxish · 2025-05-01T16:01:07Z

Yeah, it seems to repro for me still. I can post any debug output if you want - let me know. From the debug panel, I see that it says: isAuthenticated: true but I logged out of staging already.

EDIT: sometimes it randomly goes to isAuthenticated: false without me changing anything, but it still renders nothing. I think it's something to do with the various auth states being overloaded and refactoring that might help.

When it is false:

It seems to just load nothing in the top panel.

davixcky · 2025-05-01T16:16:40Z

Fixed the logged issue. Was rare in my case but able to reproduce after multiple tries.

Was not reproducible when happened in this order
State -> Set default value -> Load from memory

But when it some case (could be to the stack/queeu js runtime)
State -> Load from memory -> Set default value (with prod url)

Was causing that the latter succedded as user were still with it's cookie, but whenever it remember what url to use (in this case staging), was breaking for the staging url, meaning that indeed was authenticated but for the wrong environment.

My fix is to make sure the authContext do any query after the storage is completly set from whatever is set in the chrome.local

foxish · 2025-05-01T16:25:18Z

Nice, that fixes the issue for me - thanks!

…-urls-change-page-break

davixcky · 2025-05-01T16:34:51Z

@foxish ready for another pass

foxish · 2025-05-01T16:44:12Z

Waiting for a response on #65 (comment) @davixcky - I want to try and see if there's a clearer way to set these auth and loading vars. I think we're separating the loading and authstate which is prone to bugs, and it might make more sense to create a single set of auth states that we track and derive when to set isLoading from that.

davixcky · 2025-05-01T16:52:29Z

Waiting for a response on #65 (comment) @davixcky - I want to try and see if there's a clearer way to set these auth and loading vars. I think we're separating the loading and authstate which is prone to bugs, and it might make more sense to create a single set of auth states that we track and derive when to set isLoading from that.

Sgtm, i will do the latter

davixcky · 2025-05-01T17:02:34Z

@foxish done

foxish · 2025-05-01T17:29:51Z

After the latest change, both staging and prod are not working for me just like before the fix you did in #65 (comment).

It is authenticated, but shows just loading...
Debug panel says: isStoreAuthenticated=true, isAuthenticated=true.
Something weird is going on with the saving of the state as well. Loom: https://www.loom.com/share/ade192b380c54c47ae87332406a62c7d

davixcky · 2025-05-01T17:41:44Z

@foxish i will revert my last commit and do it in a separate PR, agree?

foxish · 2025-05-01T17:48:29Z

sgtm, that's ok with me - lmk once it's reverted, I'll review once again and then we can merge this.

davixcky · 2025-05-01T17:57:44Z

@foxish i think i was able to manage and fix the switch, LMK if that works to you. However i saw once issue that is when going from env to custom, the custom appears as blank but if you open and close, will appear correctly

So looks like a state is hanging

foxish · 2025-05-01T18:13:10Z

Works as expected! Checking the code

davixcky · 2025-05-01T18:14:18Z

@foxish i have to change a little bit the urls to add / cause the sanitize was appending "/" and that was partially causing the issue of not seeing anything at all too

foxish

LGTM after comments

foxish · 2025-05-01T18:14:53Z

src/contexts/StorageContext/defaults.ts

-export const PROD_SIGNADOT_PREVIEW_URL = "https://browser-extension-auth-redirect.preview.signadot.com";
-export const PROD_SIGNADOT_DASHBOARD_URL = "https://app.signadot.com";
-export const PROD_SIGNADOT_API_URL = "https://api.signadot.com";
+export const PROD_SIGNADOT_PREVIEW_URL = "https://browser-extension-auth-redirect.preview.signadot.com/";


nit: the trailing slashes seem surprising to me - we'll forget to add it. I'd rather change the calling site to construct URLs safely. I don't feel too strongly about this however.

Don't worries

Sanitize added automatically, but i have to added here also so we get the match in the toggles, otherwise if you select staging it will set to custom, because the missing /

foxish · 2025-05-01T18:15:31Z

src/contexts/AuthContext.tsx


+type AuthState = 
+  | { state: "loading"; user: undefined; org: undefined }
+  | { state: "not-authenticated"; user: undefined; org: undefined }


should say "unauthenticated"

foxish · 2025-05-01T18:16:25Z

src/components/ListRouteEntries/hooks.ts

+      if (!settings.signadotUrls.apiUrl) {
+        throw new Error("API URL not configured");
+      }
+      return fetchSandboxes(settings.signadotUrls.apiUrl, authState.org.name);


This is much nicer to not have invalid calls made, thanks for the fix.

foxish · 2025-05-01T18:18:18Z

src/contexts/AuthContext.tsx

 }

+type AuthState = 
+  | { state: "loading"; user: undefined; org: undefined }


This field could be called "status" to avoid AuthState.state that reads weirdly.

foxish · 2025-05-01T18:20:15Z

@foxish i have to change a little bit the urls to add / cause the sanitize was appending "/" and that was partially causing the issue of not seeing anything at all too

sanitize shouldn't be appending anything based the examples they have here: https://github.com/braintree/sanitize-url - I'm surprised by that.

davixcky · 2025-05-01T18:22:02Z

@foxish suggestions applied

foxish

LGTM

foxish

Found an issue, this extra slashes is breaking the links to sandboxes RGs etc:

When I click on that, I get sent to a 404 page:

The trailing slashes are affecting other things, we shouldn't have them, please fix them a different way.

davixcky · 2025-05-01T19:26:11Z

@foxish PTAL

foxish · 2025-05-01T20:23:20Z

I'm not happy about the trailing slashes in the URLs here: #65 (comment) - can you fix that instead and if sanitize is a problem, use a different library. Otherwise, given it's all manual testing, I don't know where else we might see some subtle breakage.

davixcky · 2025-05-01T20:43:08Z

I haven't see a similar library to sanitize, anyways we should keep in mind we are going to be the ones doing the custom urls

foxish

LGTM, but please file a follow up issue to address the URLs

reset auth state on env change

3db69aa

davixcky requested a review from foxish May 1, 2025 01:30

davixcky added 2 commits April 30, 2025 20:32

add all the urls to be checked

4a2281d

reset auth state on logout

e0dddc2

foxish requested changes May 1, 2025

View reviewed changes

foxish changed the title ~~fix urls change page break~~ fix api urls change auth behavior May 1, 2025

logged out user were still seing as logged in

c0fe1a7

sanitize urls

aad1d7a

Merge branch 'main' of github.com:signadot/browser-extension into fix…

75f7d99

…-urls-change-page-break

move loading to authstate

76fdc81

add multiple state levels in authstate

aa61a8c

foxish approved these changes May 1, 2025

View reviewed changes

rename state to status

4a86ee9

foxish approved these changes May 1, 2025

View reviewed changes

foxish requested changes May 1, 2025

View reviewed changes

fix rg/sb links

082abca

foxish approved these changes May 1, 2025

View reviewed changes

davixcky merged commit b6e298b into main May 1, 2025

davixcky deleted the fix-urls-change-page-break branch May 1, 2025 20:47

Conversation

davixcky commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

foxish left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davixcky commented May 1, 2025

Uh oh!

foxish commented May 1, 2025

Uh oh!

foxish commented May 1, 2025

Uh oh!

foxish commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davixcky commented May 1, 2025

Uh oh!

foxish commented May 1, 2025

Uh oh!

davixcky commented May 1, 2025

Uh oh!

foxish commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davixcky commented May 1, 2025

Uh oh!

davixcky commented May 1, 2025

Uh oh!

foxish commented May 1, 2025

Uh oh!

davixcky commented May 1, 2025

Uh oh!

foxish commented May 1, 2025

Uh oh!

davixcky commented May 1, 2025

Uh oh!

foxish commented May 1, 2025

Uh oh!

davixcky commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

foxish left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

foxish commented May 1, 2025

Uh oh!

davixcky commented May 1, 2025

Uh oh!

foxish left a comment

Choose a reason for hiding this comment

Uh oh!

foxish left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davixcky commented May 1, 2025

Uh oh!

foxish commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

davixcky commented May 1, 2025 •

edited

Loading

foxish commented May 1, 2025 •

edited

Loading

foxish commented May 1, 2025 •

edited

Loading

davixcky commented May 1, 2025 •

edited

Loading

foxish left a comment •

edited

Loading

foxish commented May 1, 2025 •

edited

Loading