-
Notifications
You must be signed in to change notification settings - Fork 3
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
chore: add CoderTokenAuth class #120
Changes from 4 commits
1d956bd
ca84e5b
df92b52
5a6d183
81bdbe9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
/** | ||
* @file Defines shared values and types used among any custom Coder auth | ||
* implementations for the frontend. | ||
*/ | ||
import { createApiRef } from '@backstage/core-plugin-api'; | ||
import { CODER_API_REF_ID_PREFIX } from '../typesConstants'; | ||
|
||
/** | ||
* Data about the auth that can safely be exposed throughout multiple parts of | ||
* the application without too much worry about security. | ||
* | ||
* All values should be JSON-serializable. | ||
*/ | ||
export type SafeAuthData = Readonly<{ | ||
isTokenValid: boolean; | ||
tokenHash: number | null; | ||
initialTokenHash: number | null; | ||
isInsideGracePeriod: boolean; | ||
}>; | ||
|
||
export type AuthSubscriptionCallback = (payload: SafeAuthData) => void; | ||
export type AuthValidatorDispatch = (newStatus: boolean) => void; | ||
|
||
/** | ||
* Shared set of properties among all Coder auth implementations | ||
*/ | ||
export type CoderAuthApi = SafeAuthData & { | ||
/** | ||
* Requests the token from the auth class. This may not always succeed (e.g., | ||
* auth doesn't think it's safe, there is no token to provide), in which case, | ||
* the value will be null. | ||
*/ | ||
requestToken: () => string | null; | ||
|
||
/** | ||
* Gives back a "state setter" that lets a different class dispatch a new auth | ||
* status to the auth class implementation. | ||
* | ||
* Use this to send the new status you think the auth should have. The auth | ||
* will decide whether it will let the dispatch go through and update state. | ||
*/ | ||
getAuthStateSetter: () => AuthValidatorDispatch; | ||
|
||
/** | ||
* Subscribes an external system to auth changes. | ||
* | ||
* Returns an pre-wired unsubscribe callback to remove fuss of needing to hold | ||
* onto the original callback if it's not directly needed anymore | ||
*/ | ||
subscribe: (callback: AuthSubscriptionCallback) => () => void; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, I am going to show some ignorance here because I think I am not seeing the bigger picture, but to show where my head is at, could it all be done with a context like this (well not sure this is exactly right, but this is the gist):
From here one has everything needed to show the token component if necessary and make API requests. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, I wish I could use Context more, but the main issue is that contexts aren't really the "Backstage way", and a lot of their APIs and tools are built on their own bespoke systems There were two things I was shooting for with the API classes (and I'm typing so much to get this out of my head and documented somewhere):
But maybe it doesn't have to be like that – maybe we could register the APIs from Backstage, and then handle everything else through React and There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But yeah, complexity is definitely a worry for me, too, and even if the code gets merged in as-is, I think there will still be some complexity to shave off I made an Excalidraw drawing to give a top-level view of how these things are supposed to fit together, but I feel like (1) the implementation isn't living up to how neat the drawing looks, and (2) maybe even some of the arrows linking things together don't need to be there I haven't done much OOP professionally, but this is where I wish I had more Go experience, because I feel like it'd get me into the habit of figuring out a solution that doesn't get too clever, and that's easier to maintain My gut feeling is that I want these interfaces to be as small as humanly possible (maybe even one method/value total), but I don't know what the most effective way of doing that is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Helpful chart! The picture does become clearer with the goal of being able to slot auth implementations into the system, although I keep feeling there should be a more Backstage-oriented way, similar to how their built-in auth providers work (which I think is what you are saying with Thinking about it more, and stealing from Roadie's pull request plugin, is this what Backstage means for it look like in the end?
Where the Coder auth API implements a Backstage auth provider. Although, not sure we can actually implement the current localstorage-based auth as an auth provider; the OAuth-based one should be fine though. I suppose it could be like this to start, really one could do anything as long as it spits out a token in the end:
My sense, from reading the code and the chart, is that much of this is mainly for tying the client and auth together (for the client to subscribe to changes to auth), and I think maybe they should actually be tied in a more top-down way, whether that is a context or a hook, which would simplify a few things. All this said, I might be completely missing the mark still haha, I think I need to actually get in there to really understand; looking forward to tomorrow! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is usually what I go by, but Kira is also trying to do the work for integrating OAuth anyway, so I figured that it'd be good to make the drop-in as easy for her as possible There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this file change also shows what I was hoping to achieve: old vs new My hope was that instead of having to manually pass every single dependency into every API function call, they would all be co-located into one single, thick client. I think I achieve that for this file – it's just, everything else around it has become a lot more chaotic There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just for posterity's sake, we talked about it, the part I was missing is that we want to end up with something like:
We could do it all React-based instead with hooks or contexts or whatever, but it is not the Backstage way with API factories. And, the OAuth provider has to be an API factory, so at the very least we are stuck with auth being an API reference, regardless of what we do with the client, which means because of 2 we still need this code. |
||
|
||
/** | ||
* Lets an external system unsubscribe from auth changes. | ||
*/ | ||
unsubscribe: (callback: AuthSubscriptionCallback) => void; | ||
|
||
/** | ||
* Lets an external system get a fully immutable snapshot of the current auth | ||
* state. | ||
*/ | ||
getStateSnapshot: () => SafeAuthData; | ||
}; | ||
|
||
/** | ||
* A single, shared auth API ref that can be used with any of the CoderAuth | ||
* API classes (CoderTokenAuth, eventually CoderOAuth, etc.) | ||
*/ | ||
export const coderAuthApiRef = createApiRef<CoderAuthApi>({ | ||
id: `${CODER_API_REF_ID_PREFIX}.auth`, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,255 @@ | ||
import { getMockLocalStorage } from '../testHelpers/mockBackstageData'; | ||
import { hashValue } from '../utils/crypto'; | ||
import type { SafeAuthData } from './Auth'; | ||
import { CoderTokenAuth, AUTH_SETTER_TIMEOUT_MS } from './CoderTokenAuth'; | ||
|
||
beforeEach(() => { | ||
jest.useFakeTimers(); | ||
}); | ||
|
||
afterEach(() => { | ||
jest.runOnlyPendingTimers(); | ||
jest.useRealTimers(); | ||
}); | ||
|
||
// Aggressively short time to ensure that the class can accept any arbitrary | ||
// timeout value. The auth logic is 90% synchronous, so this has no real risks | ||
const defaultGracePeriodTimeoutMs = 1_000; | ||
const defaultLocalStorageKey = 'backstage-plugin-coder/test'; | ||
|
||
type SetupAuthInputs = Readonly<{ | ||
initialData?: Record<string, string>; | ||
gracePeriodTimeoutMs?: number; | ||
localStorageKey?: string; | ||
}>; | ||
|
||
type SetupAuthResult = Readonly<{ | ||
localStorage: Storage; | ||
auth: CoderTokenAuth; | ||
}>; | ||
|
||
function setupAuth(inputs?: SetupAuthInputs): SetupAuthResult { | ||
const { | ||
initialData, | ||
localStorageKey = defaultLocalStorageKey, | ||
gracePeriodTimeoutMs = defaultGracePeriodTimeoutMs, | ||
} = inputs ?? {}; | ||
|
||
const localStorage = getMockLocalStorage(initialData); | ||
const auth = new CoderTokenAuth({ | ||
localStorage, | ||
localStorageKey, | ||
gracePeriodTimeoutMs, | ||
}); | ||
|
||
return { auth, localStorage }; | ||
} | ||
|
||
describe(`${CoderTokenAuth.name}`, () => { | ||
describe('Subscriptions', () => { | ||
it('Lets external systems subscribe to auth changes', () => { | ||
const onChange = jest.fn(); | ||
const { auth } = setupAuth(); | ||
auth.subscribe(onChange); | ||
|
||
auth.registerNewToken('blah'); | ||
expect(onChange).toHaveBeenCalled(); | ||
}); | ||
|
||
it('Lets external systems *un*subscribe to auth changes', () => { | ||
const onChange = jest.fn(); | ||
const { auth } = setupAuth(); | ||
auth.subscribe(onChange); | ||
auth.unsubscribe(onChange); | ||
|
||
auth.registerNewToken('blah'); | ||
expect(onChange).not.toHaveBeenCalled(); | ||
}); | ||
}); | ||
|
||
describe('Setting tokens', () => { | ||
it('Will reject empty strings', () => { | ||
const onChange = jest.fn(); | ||
const { auth } = setupAuth(); | ||
auth.subscribe(onChange); | ||
|
||
auth.registerNewToken(''); | ||
expect(onChange).not.toHaveBeenCalled(); | ||
}); | ||
|
||
it("Will reject new token if it's the same as the current one", () => { | ||
const onChange = jest.fn(); | ||
const { auth } = setupAuth(); | ||
auth.subscribe(onChange); | ||
|
||
auth.registerNewToken('blah'); | ||
auth.registerNewToken('blah'); | ||
expect(onChange).toHaveBeenCalledTimes(1); | ||
}); | ||
|
||
it('Will immediately notify subscriptions that the auth has been invalidated when a new token is set', () => { | ||
const onChange = jest.fn(); | ||
const { auth } = setupAuth(); | ||
auth.subscribe(onChange); | ||
|
||
auth.registerNewToken('blah'); | ||
expect(onChange).toHaveBeenCalledWith( | ||
expect.objectContaining<Partial<SafeAuthData>>({ | ||
isTokenValid: false, | ||
}), | ||
); | ||
}); | ||
}); | ||
|
||
describe('getAuthStateSetter', () => { | ||
it('Lets another system set the auth state', () => { | ||
const testToken = 'blah'; | ||
const hashed = hashValue(testToken); | ||
const { auth } = setupAuth(); | ||
|
||
auth.registerNewToken(testToken); | ||
const dispatchNewStatus = auth.getAuthStateSetter(); | ||
dispatchNewStatus(true); | ||
|
||
const snapshot = auth.getStateSnapshot(); | ||
expect(snapshot).toEqual( | ||
expect.objectContaining<Partial<SafeAuthData>>({ | ||
tokenHash: hashed, | ||
isTokenValid: true, | ||
}), | ||
); | ||
}); | ||
|
||
it('Rejects state changes if there is no token when the state setter is made', () => { | ||
const onChange = jest.fn(); | ||
const { auth } = setupAuth(); | ||
|
||
auth.subscribe(onChange); | ||
const dispatchNewStatus = auth.getAuthStateSetter(); | ||
dispatchNewStatus(true); | ||
|
||
expect(onChange).not.toHaveBeenCalled(); | ||
}); | ||
|
||
it('Disables the state setter if the token changes after the setter was created', () => { | ||
const onChange = jest.fn(); | ||
const { auth } = setupAuth(); | ||
|
||
auth.registerNewToken('dog'); | ||
const dispatchNewStatus = auth.getAuthStateSetter(); | ||
auth.registerNewToken('cat'); | ||
|
||
dispatchNewStatus(true); | ||
expect(onChange).not.toHaveBeenCalled(); | ||
}); | ||
|
||
it("Makes the state setter 'go inert' after a set amount of time (will start rejecting dispatches)", async () => { | ||
const { auth } = setupAuth(); | ||
auth.registerNewToken('blah'); | ||
const dispatchNewStatus = auth.getAuthStateSetter(); | ||
|
||
// Give an extra 100ms to give code time to flush state changes | ||
await jest.advanceTimersByTimeAsync(AUTH_SETTER_TIMEOUT_MS + 100); | ||
dispatchNewStatus(true); | ||
|
||
const snapshot = auth.getStateSnapshot(); | ||
expect(snapshot).toEqual( | ||
expect.objectContaining<Partial<SafeAuthData>>({ | ||
isTokenValid: false, | ||
}), | ||
); | ||
}); | ||
|
||
it("Will eventually leave 'grace period' state when auth validity flips from true to false", async () => { | ||
const { auth } = setupAuth(); | ||
auth.registerNewToken('blah'); | ||
const dispatchNewStatus = auth.getAuthStateSetter(); | ||
|
||
dispatchNewStatus(true); | ||
const snapshot1 = auth.getStateSnapshot(); | ||
expect(snapshot1).toEqual( | ||
expect.objectContaining<Partial<SafeAuthData>>({ | ||
isTokenValid: true, | ||
isInsideGracePeriod: true, | ||
}), | ||
); | ||
|
||
dispatchNewStatus(false); | ||
const snapshot2 = auth.getStateSnapshot(); | ||
expect(snapshot2).toEqual( | ||
expect.objectContaining<Partial<SafeAuthData>>({ | ||
isTokenValid: false, | ||
isInsideGracePeriod: true, | ||
}), | ||
); | ||
|
||
await jest.advanceTimersByTimeAsync(defaultGracePeriodTimeoutMs); | ||
const snapshot3 = auth.getStateSnapshot(); | ||
expect(snapshot3).toEqual( | ||
expect.objectContaining<Partial<SafeAuthData>>({ | ||
isTokenValid: false, | ||
isInsideGracePeriod: false, | ||
}), | ||
); | ||
}); | ||
}); | ||
|
||
describe('localStorage', () => { | ||
it('Will read from localStorage when first initialized', () => { | ||
const testValue = 'blah'; | ||
const hashed = hashValue(testValue); | ||
|
||
const { auth } = setupAuth({ | ||
initialData: { | ||
[defaultLocalStorageKey]: testValue, | ||
}, | ||
}); | ||
|
||
const initialStateSnapshot = auth.getStateSnapshot(); | ||
expect(initialStateSnapshot).toEqual( | ||
expect.objectContaining<Partial<SafeAuthData>>({ | ||
initialTokenHash: hashed, | ||
tokenHash: hashed, | ||
isTokenValid: false, | ||
}), | ||
); | ||
}); | ||
|
||
it('Will immediately update localStorage when token is cleared', () => { | ||
const { auth, localStorage } = setupAuth({ | ||
initialData: { | ||
[defaultLocalStorageKey]: 'blah', | ||
}, | ||
}); | ||
|
||
auth.clearToken(); | ||
expect(localStorage.getItem(defaultLocalStorageKey)).toEqual(''); | ||
}); | ||
|
||
it('Will write to localStorage when the token is confirmed to be valid', () => { | ||
const testToken = 'blah'; | ||
const { auth, localStorage } = setupAuth(); | ||
|
||
auth.registerNewToken(testToken); | ||
const dispatchNewStatus = auth.getAuthStateSetter(); | ||
dispatchNewStatus(true); | ||
|
||
expect(localStorage.getItem(defaultLocalStorageKey)).toEqual(testToken); | ||
}); | ||
|
||
it('Lets the user define a custom local storage key', () => { | ||
const customKey = 'blah'; | ||
const testToken = 'blah blah'; | ||
|
||
const { auth, localStorage } = setupAuth({ | ||
localStorageKey: customKey, | ||
}); | ||
|
||
auth.registerNewToken(testToken); | ||
const dispatchNewStatus = auth.getAuthStateSetter(); | ||
dispatchNewStatus(true); | ||
|
||
expect(localStorage.getItem(customKey)).toEqual(testToken); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this part is a little unconventional (it's kind of bringing React's
useState
into classes), but after playing around with a few different options, it felt like the easiest to use, and the easiest to maintain, while keeping the class completely decoupled from the API logicI originally had something like this:
But in practice, it led to weird, hard-to-reason-about behavior, because you had to worry about things like:
I don't have the PR open for it yet, but here's the method from the
CoderClient
, which I think is easier to reason about, because it mostly reads top-to-bottom:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems chill to me.