feat(fronted): tenanted admin api credentials #3213
Conversation
- not removing yet because not sure if we might end up using it. could be useful if we want to make global redirect if this is not set.
…ll apollo requests Uses the assets and list asset query. This POC passes the request to the listAsset function. Which imports the apolloClient directly and passes the cookie from request headers in the context. To avoid having to set this on each query as we compose it, my intention is to create a new getApolloClient function and use that insteadof directly importing a single client. This enables us to form a link to handle setting the headers per request (as opposed to static links that are used across all requests as it is currently).
- enables authLink to get tenantId, apiSecret from cookie in request
- wondered if this was a performance concern
(maybe why we had single instance before?) but found
several things indicating this is OK and even recommended:
- apollographql/apollo-client#9520 (comment)
- https://www.apollographql.com/blog/how-to-use-apollo-client-with-remix
- see TODOs in apollo client in frontend. maybe need to remove some env vars and verify how no tenantid/secret are handled
| 'flex p-2 font-medium rounded-md', | ||
| !hasApiCredentials && | ||
| name !== 'Home' && | ||
| 'text-gray-400 pointer-events-none cursor-not-allowed' |
There was a problem hiding this comment.
disables nav links (Assets, Wallet Addresses, etc.) in nav menu when api credentials arent set. This keeps user on page w/ the form to submit the credentials and avoids an inevitable error (which you can see if manually forming the url).
There was a problem hiding this comment.
if there is more visual feedback we can give on the disabled state im all ears. With these changes it looks the same, they just arent clickable. See the pic here showing it.
Normally I'd make it a lighter gray but lighter gray already represents non-active tabs (and dont have any room to go lighter contrast wise). Perhaps make the text black on all of them (active or not) but keep the dark background to represent active. Then make the disabled links light gray?
Strikethrough, italics, etc. all seemd kinda meh too and a different color doesnt really seem to communicate disabled... could completely hide them but felt like showing what content there is still made more sense.
There was a problem hiding this comment.
To me the api credentials form is similar to a login form, so I believe hiding the contents from the sidebar is clearer for the user that he cannot navigate anywhere until they add their credentials.
There was a problem hiding this comment.
I agree w @oana-lolea, probably more straightforward to just hide everything until we are "logged in"
There was a problem hiding this comment.
Updated to hide if credentials not set, thanks for the feedback.
|
|
||
| const httpLink = createHttpLink({ | ||
| uri: process.env.GRAPHQL_URL | ||
| }) | ||
|
|
||
| if (!global.__apolloClient) { | ||
| global.__apolloClient = new ApolloClient({ | ||
| export async function getApolloClient(request: Request) { |
There was a problem hiding this comment.
Before this change, we re-used 1 server side apollo client for all requests.
This is no longer tenable because we need to support the concept of different requestors. Hence, a getApolloClient that is formed for each request with the correct apiSecret and tenantId.
I did wonder if this was a weird, not performant thing to do but actually I found it is preferred to avoid side effects between clients (someone else's credentials, cache issues, etc.). Some supporting evidence for this:
- pretty direct instruction to create a new client epr request when there is user specific data (like the tenantId, apiSecret): [Question] SSR + Apollo Client instances apollographql/apollo-client#9520 (comment)
- official apollo post about how to use remix. setup is a bit different but a new client is created in handleRequest (which is called for all page loads) https://www.apollographql.com/blog/how-to-use-apollo-client-with-remix
| defaultOptions: { | ||
| query: { | ||
| fetchPolicy: 'no-cache' | ||
| }, | ||
| mutate: { | ||
| fetchPolicy: 'no-cache' | ||
| } | ||
| } |
There was a problem hiding this comment.
no longer needed since clients arent shared between requests and we arent worried about accidentally sharing cache
original reasoning here: #969 (comment)
| httpOnly: true, | ||
| path: '/', | ||
| sameSite: 'lax', | ||
| secure: process.env.NODE_ENV === 'production', |
There was a problem hiding this comment.
@mkurapov Is there anything to test here? I recall some shenanigans with the snackbar message cookie and some scenario. Mobile perhaps? Didn't notice any actual problems in testing.
| To get started, please configure your API credentials | ||
| </p> | ||
| <div className='max-w-md mx-auto'> | ||
| <ApiCredentialsForm hasCredentials={hasCredentials} /> |
There was a problem hiding this comment.
New form to submit credentials.
I included it on the homepage because thats naturally where you land and you cant really see anything else without submitting these.
Form validates uuid client side. Saves to cookie based session on submit which you can also delete. Here are the views for credentials set in storage and not set.
| errorMessage = error.message.includes('401') | ||
| ? 'Unauthorized. Please set your API credentials.' | ||
| : error.message |
There was a problem hiding this comment.
On 401 (missing or bad credentials), shows our error page with this message and prompt to return to homepage where credentials can be set.
| SIGNATURE_VERSION: 1 | ||
| SIGNATURE_SECRET: iyIgCprjb9uL8wFckR+pLEkJWMB7FJhgkvqhTQR/964= |
There was a problem hiding this comment.
What do you think about having this still here (so we don't have to enter them in the local playground)?
If we want to use another tenant, we can just clear the credentials on the home screen
There was a problem hiding this comment.
Is that appropriate logic to have in the production image?
I was worried about having that functionality when using multiple tenants. For a single tenant it's good - it just works the way it always has. But if you have multiple tenants and set this then everyone is that tenant. That made it seem potentially risky to have so removing it was defensive. But thinking from the other side, it does streamline things not only in local development but for the single-tenant use-case which may be the most common one.
There was a problem hiding this comment.
Thinking about it another way, these env vars are required. So all existing deployments have them set.
If an existing deployment adds a new tenant, now how do we know which signature to use? How do we know not to use this one? I think the signature is just no longer a decision we can make at deployment.
There was a problem hiding this comment.
Im not against streamlining the localenv more. Currently it just logs the credentials and you copy-paste them. I did think a bit about alternatives.
I think its probably possible to simply trigger the action from the MASE with the env vars we are using there. The session with credentials are set in a remix action function which runs on the remix server at POST / (or could move to a dedicated /api/set-credentials. So I think in theory we could expose the port in docker and add a fetch call in the MASE to set them on startup. It does seem like a more appropriate thing to do from our development fixture than the frontend app itself.
There was a problem hiding this comment.
So all existing deployments have them set.
The SIGNATURE_SECRET and SIGNATURE_VERSION are optional in frontend I believe
There was a problem hiding this comment.
I think that makes sense in terms of the developer environment. Doesn't have to be in this PR.
Im giving it a shot... may not be too hard.
There was a problem hiding this comment.
I'm thinking more about allowing them to set SIGNATURE_SECRET and then using that if we can, otherwise requiring one from the session. I think the question is how do we know if the session one is required or if we can use the env var? I think maybe it's just if there is only 1 tenant in the backend or not?
I think we'd need an extra api call before making the gql request, and it may be difficult to use the gql api for it since we are assuming all requests will have the tenant details, and thats what we're trying to figure out.
export const loader = async ({ request }: LoaderFunctionArgs) => {
// ...
const shouldUseSessionSecret = await checkApiIfMultitenanted() // boolean
const assets = await listAssets(request, shouldUseSessionSecret, {
...pagination.data
})
// ...
}I guess this saves some intermittent input from operator (ie when there is no cookie with session) but how important is that? Dont really like this. And we cannot just always use the ENV var if set because new tenants would just use them.
There was a problem hiding this comment.
Ah shoot... setting from the MASE doesn't work because its a cookie... the browser wont have it.
There was a problem hiding this comment.
I changed the MASE to log a link which will prefill the form. So you can click the link and press save. This is something integrators could leverage as well when distributing credentials out of band.
In theory I think we should be able to automatically submit it in a useEffect hook but I wasn't able to get that working.
There was a problem hiding this comment.
nvm, got the auto submit working. MASE logs a link like http://localhost:3010/?tenantId=438fa74a-fa7d-4317-9ced-dde32ece1787&apiSecret=iyIgCprjb9uL8wFckR%2BpLEkJWMB7FJhgkvqhTQR%2F964%3D which you can use to set without any additional input.
Perhaps when an operator creates a new tenant we can form this sort of link for them to pass along to the tenant out of band.
| 'flex p-2 font-medium rounded-md', | ||
| !hasApiCredentials && | ||
| name !== 'Home' && | ||
| 'text-gray-400 pointer-events-none cursor-not-allowed' |
There was a problem hiding this comment.
I agree w @oana-lolea, probably more straightforward to just hide everything until we are "logged in"
- removes the action from the index. the intention is to expose the remix server port over docker and call this from the MASE to set the api credentials on start
requires changing intent to be set as an input. submitting form bypasses the button so the action didnt have the intent and failed when auto submitting.
Screen.Recording.2025-01-20.at.13.43.51.movGetting odd behaviour where 401 on the request is happening when navigating to the same page in succession |
Im seeing this on |
|
@BlairCurrey yep, seeing on main as well. Looking into it |
|
@BlairCurrey Ah, I think I got it. If you do it within one second of clicking, it looks like the signature validation fails because of the |
| @@ -30,6 +30,15 @@ async function callWithRetry(fn: () => any, depth = 0): Promise<void> { | |||
| } | |||
|
|
|||
| if (!global.__seeded) { | |||
| const tenantId = process.env.OPERATOR_TENANT_ID | |||
| const apiSecret = process.env.SIGNATURE_SECRET | |||
There was a problem hiding this comment.
It doesn't seem like SIGNATURE_SECRET is set in the environment anymore.
There was a problem hiding this comment.
It was removed from the frontend - is that what you're referring to? The docker compose sets them for the mock ase. When I spin up rafiki it doesnt error on the lines below for the secret, and it spits out a link with the apiSecret.
Changes proposed in this pull request
Context
fixes #3108
Checklist
fixes #numberuser-docslabel (if necessary)