feat: Expo Web support (utilising localStorage) #8
Conversation
WalkthroughThe pull request introduces a platform-specific storage mechanism for the Kinde authentication provider. A new optional Changes
Sequence DiagramsequenceDiagram
participant App
participant KindeAuthProvider
participant StorageProvider
participant WebStorageProvider
participant NativeStorageProvider
App->>KindeAuthProvider: Initialize with platform config
KindeAuthProvider->>StorageProvider: Create storage provider
StorageProvider-->>KindeAuthProvider: Return platform-specific provider
KindeAuthProvider->>WebStorageProvider/NativeStorageProvider: Use setStorage/getStorage
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
lib/constants.ts (2)
2-2: Consider validating runtime platform values against known options.
Although"native"is set as the default platform, consider introducing a union or an enum for all supported platform values (e.g.,"native"|"web") at runtime to ensure that any external usage doesn't pass an unknown platform string.
4-8: Enum string values can be more descriptive.
Currently,accessToken,idToken, andstateare numeric enumerations (implicitly). Converting them to string-based values (e.g.,accessToken = "accessToken") may simplify debugging and usage.lib/storage/index.ts (1)
6-10: Correct the JSDoc parameter and return type in the docstring.
You have JSDoc lines specifying@param {StorageKeys}forplatformand@returns {Promise<void>}, but the function accepts a string union and returnsIStorageProviderinstead of a promise./** * Storage provider factory - * @param {StorageKeys} platform Key to switch the storage provider - * @returns {Promise<void>} + * @param {"web" | "native"} platform - Key to switch the storage provider + * @returns {IStorageProvider} */lib/storage/nativeProvider.ts (1)
1-3: Consider re-exporting or grouping imports for clarity.You're importing
deleteItemAsync,getItemAsync, andsetItemAsyncfromexpo-secure-store, plus local modules. This is perfectly fine; however, verifying that you only expose what is needed and grouping them in a single barrel file could improve maintainability in larger projects.lib/useKindeAuth.ts (1)
Line range hint
22-23: Add optional default values or fallback logic forgetFlag.Currently,
getFlagreturnsnullif a flag isn't found. Some use cases might benefit from a default value. Consider adding an optional parameter or fallback logic in case the requested flag doesn't exist.export interface KindeAuthHook { ... getFlag: <T = string | boolean | number>(name: string, defaultValue?: T) => Promise<T>; isAuthenticated: boolean; } ... async function getFlag<T = string | boolean | number>( name: string, + defaultValue?: T ): Promise<T> { ... return value?.v ?? defaultValue ?? null; }lib/KindeAuthProvider.tsx (2)
14-18: Keep track of constants and enumerations together.These constants, including
DEFAULT_PLATFORM,DEFAULT_TOKEN_SCOPES, andStorageKeys, are logically related to each other. Consider grouping them in a dedicatedconstants.tsfile or an index to maintain clarity.
Line range hint
85-152: Consolidate authentication error handling.Your
authenticatefunction returns partial error messages:
"This library only works on a mobile device"ifredirectUriis missing."No discovery document"ifdiscoveryis nonexistent.For a smoother UI, consider centralizing these error messages or documenting them for consistent consumer handling, especially if thrown from different points in the flow.
readme.md (1)
22-22: Clarify platform detection logic in the README.Using
Platform.OS !== "web"to default to"native"is correct but might need clarification for developers. Some platforms (like Windows, macOS, or other consolidated frameworks) may also appear differently. A short note or table explaining possible values ofPlatform.OScould help developers understand edge cases.Also applies to: 32-33
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package.jsonis excluded by!**/*.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/*.yaml
📒 Files selected for processing (9)
lib/KindeAuthProvider.tsx(15 hunks)lib/constants.ts(1 hunks)lib/storage.ts(0 hunks)lib/storage/index.ts(1 hunks)lib/storage/nativeProvider.ts(1 hunks)lib/storage/storageProvider.interface.ts(1 hunks)lib/storage/webProvider.ts(1 hunks)lib/useKindeAuth.ts(1 hunks)readme.md(2 hunks)
💤 Files with no reviewable changes (1)
- lib/storage.ts
🔇 Additional comments (8)
lib/storage/storageProvider.interface.ts (1)
3-18: Interface documentation is clear and consistent.
The doc comments match the method signatures, specifying the parameter types (StorageKeys, string, and null) and the returned promise. This is well-structured and follows good practices for TypeScript interfaces.
lib/storage/index.ts (1)
11-20: Implementation logic is straightforward and maintainable.
The factory method uses a switch statement for "web" vs. "native" and defaults to NativeStorageProvider. Future platforms can easily be added here.
lib/storage/nativeProvider.ts (2)
9-19: Efficient cleanup for null values.
The logic for removing chunks when value is null is correct and thorough. Once a null value is passed, you delete every chunk previously stored under the same key. This ensures no stale data remains. Good job!
32-42: Efficient retrieval approach.
The method that dynamically fetches and concatenates chunks until none remain is a solid approach. This ensures that larger values, split into multiple chunks, are successfully reconstructed.
lib/KindeAuthProvider.tsx (4)
49-49: Platform-based storage is a neat approach.
Allowing users to specify "web" or "native" for storage logic is a clean design. This is a great way to provide platform-specific handling without complicating the consumer’s code. Great job!
Also applies to: 60-63
Line range hint 176-187: Register and login share similar logic.
Both login and register wrap the authenticate function with slightly different prompt parameters. This is efficient and DRY. Good job on reusing logic.
Line range hint 202-215: Ensure cancellation scenarios are handled gracefully.
In logout, you prompt a web session logout and attempt to revoke the token. If the user cancels the open web session (e.g., closes the browser quickly), ensure everything from resolve({ success: false }) to internal cleanup still responds gracefully.
Line range hint 275-370: Comprehensive token-based utility methods.
Your utility methods getPermission, getPermissions, getClaims, getClaim, getFlag, etc., cover a wide range of security scenarios. This is well-structured and keeps your code flexible for future expansions.
| export class WebStorageProvider implements IStorageProvider { | ||
| setStorage(key: StorageKeys, value: string | null): Promise<void> { | ||
| if (!value || value.length === 0) { | ||
| localStorage.removeItem(`${key}`); | ||
| return Promise.resolve(); | ||
| } | ||
|
|
||
| localStorage.setItem(`${key}`, value); | ||
| return Promise.resolve(); | ||
| } | ||
|
|
||
| getStorage(key: StorageKeys): Promise<string | null> { | ||
| const item = localStorage.getItem(`${key}`); | ||
| return Promise.resolve(item); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Security concern confirmed: Authentication tokens are stored in localStorage
The verification confirms that sensitive authentication data (access tokens and ID tokens) from Kinde authentication are being stored directly in localStorage:
StorageKeys.accessTokenandStorageKeys.idTokenare used to store authentication tokens- These tokens are stored unencrypted in localStorage through the WebStorageProvider
- This implementation is vulnerable to XSS attacks as any malicious script running on the page can access these tokens
Recommendations:
- Consider using HttpOnly cookies for token storage instead of localStorage
- If localStorage must be used, implement additional security measures:
- Encrypt tokens before storage
- Implement token rotation
- Add security headers like CSP to mitigate XSS risks
- For the state parameter, which is less sensitive, localStorage usage is acceptable
🔗 Analysis chain
Be mindful of storing tokens in localStorage.
Storing access tokens, ID tokens, or refresh tokens in localStorage can introduce XSS vulnerabilities if malicious scripts gain access to the browser storage. Evaluate whether you need additional security measures, like secure cookies or token encryption, especially for production deployments.
Let me gather more information about how this storage is being used.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching project files for usage of WebStorageProvider to confirm in which contexts we rely on localStorage
rg -A 5 "WebStorageProvider"
Length of output: 959
Script:
#!/bin/bash
# Check what types of data are being stored using StorageKeys enum
rg -A 5 "enum StorageKeys"
# Search for actual usage of setStorage to understand what values are being stored
rg "setStorage\(" -A 3
Length of output: 1817
| chunks.forEach(async (chunk, index) => { | ||
| await setItemAsync(`${key}-${index}`, chunk); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Avoid using forEach with async callbacks.
Inside the forEach(async (chunk, index) => {...}), the calls to await setItemAsync won't resolve in a predictable sequence since Array.forEach doesn't await each iteration. If the order of execution matters, consider using a traditional for loop or Promise.all with map.
- chunks.forEach(async (chunk, index) => {
- await setItemAsync(`${key}-${index}`, chunk);
- });
+ for (let i = 0; i < chunks.length; i++) {
+ await setItemAsync(`${key}-${i}`, chunks[i]);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| chunks.forEach(async (chunk, index) => { | |
| await setItemAsync(`${key}-${index}`, chunk); | |
| }); | |
| } | |
| for (let i = 0; i < chunks.length; i++) { | |
| await setItemAsync(`${key}-${i}`, chunks[i]); | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
lib/enums.ts (1)
1-6: Consider using string-based enum values for clarity.
By default, these enum values will be numeric (0,1,2,3). Using string values (e.g.,accessToken = "accessToken") can help prevent confusion if these enum members are stored or transmitted as raw strings.export enum StorageKeys { - accessToken, - idToken, - state, - refreshToken, + accessToken = "accessToken", + idToken = "idToken", + state = "state", + refreshToken = "refreshToken", }lib/KindeAuthProvider.tsx (5)
56-56: Add platform value validationWhile the platform-specific storage setup is good, consider adding validation for the platform value to prevent potential runtime errors.
- const platform = config.platform ?? DEFAULT_PLATFORM; + const platform = config.platform; + if (platform && !["web", "native"].includes(platform)) { + throw new Error('Platform must be either "web" or "native"'); + } + const resolvedPlatform = platform ?? DEFAULT_PLATFORM; - const { getStorage, setStorage } = StorageProvider(platform); + const { getStorage, setStorage } = StorageProvider(resolvedPlatform);Also applies to: 67-69
74-74: Enhance error handling and loggingThe authentication state management is well-implemented, but consider improving error handling and logging:
- Remove or replace console.log statements with proper logging
- Add error logging for failed token validations
if (!refresh.success) { + console.error('Token refresh failed:', refresh.errorMessage); setIsAuthenticated(false); setIsReady(true); return; }Also applies to: 87-108
156-158: Security consideration for web storageGood practice not storing refresh tokens on web platform. Consider adding a comment explaining the security implications of this decision.
- // Dont store refresh token on web. + // Don't store refresh tokens in web localStorage due to XSS vulnerability risks. + // Web applications should request new tokens through the standard OAuth flow. if (platform !== "web" && response.refreshToken) await setStorage(StorageKeys.refreshToken, response.refreshToken);
261-261: Remove debug console.logRemove the debug console.log statement in production code.
- console.log("exchangeCodeResponse", exchangeCodeResponse);
Line range hint
439-467: Enhance method documentationThe new methods would benefit from more detailed JSDoc documentation including:
- Return type descriptions
- Error scenarios
- Usage examples
/** * Get the current organization code + * @description Returns the organization code for the current user's context * @returns { Promise<string | null> } + * @example + * const orgCode = await getCurrentOrganization(); + * if (orgCode) { + * console.log(`Current organization: ${orgCode}`); + * } */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/*.yaml
📒 Files selected for processing (8)
lib/KindeAuthProvider.tsx(18 hunks)lib/constants.ts(1 hunks)lib/enums.ts(1 hunks)lib/storage/index.ts(1 hunks)lib/storage/nativeProvider.ts(1 hunks)lib/storage/storageProvider.interface.ts(1 hunks)lib/storage/webProvider.ts(1 hunks)lib/useKindeAuth.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/constants.ts
- lib/storage/storageProvider.interface.ts
- lib/useKindeAuth.ts
🔇 Additional comments (5)
lib/storage/index.ts (1)
1-19: JSDoc @returns mismatch
The current JSDoc states @returns {Promise<void>}, but the function actually returns an IStorageProvider instance. Please update the JSDoc to match the actual return type.
/**
* Storage provider factory
* @param {PlatformKeys} platform Key to switch the storage provider
- * @returns {Promise<void>}
+ * @returns {IStorageProvider}
*/✅ Verification successful
Let me verify the JSDoc return type mismatch by checking the interface definition and any potential asynchronous operations.
JSDoc @returns type is incorrect and needs to be updated
The review comment is correct. The StorageProvider function synchronously returns an instance of IStorageProvider. While the interface methods (setStorage and getStorage) are asynchronous and return Promises, the factory function itself is synchronous.
- The JSDoc should be updated to
@returns {IStorageProvider}as suggested in the review comment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Check the interface definition
ast-grep --pattern 'interface IStorageProvider {
$$$
}'
# Check for any async operations in the providers
ast-grep --pattern 'class WebStorageProvider implements IStorageProvider {
$$$
}'
ast-grep --pattern 'class NativeStorageProvider implements IStorageProvider {
$$$
}'
Length of output: 4560
lib/storage/webProvider.ts (1)
7-22: Storing tokens in localStorage can introduce XSS vulnerabilities
(Refer to the previous security concern for more details)
lib/storage/nativeProvider.ts (1)
23-26: Async iteration in forEach() may not behave as intended
(Refer to the previous comment about avoiding forEach with async callbacks)
lib/KindeAuthProvider.tsx (2)
11-18: LGTM: Web platform initialization is properly set up
The new imports and initialization code correctly set up the foundation for web platform support. The maybeCompleteAuthSession() call is essential for handling web redirects in Expo's auth flow.
Also applies to: 20-21, 36-37
Line range hint 56-533: Verify native platform compatibility
While the web platform support looks well-implemented, we should verify that these changes don't affect existing native platform functionality.
✅ Verification successful
Native platform functionality remains intact and properly handled
The implementation maintains full compatibility with native platforms:
- The native storage provider (
NativeStorageProvider) is properly implemented usingexpo-secure-storewith chunking support for large tokens - The platform selection logic in
StorageProvidercorrectly defaults to native implementation when not explicitly set to "web" - The dependency on
expo-secure-storeis properly maintained inpackage.json - The token storage and retrieval logic in
KindeAuthProvideris platform-agnostic, delegating to the appropriate storage provider
The changes have successfully added web platform support while preserving the existing native functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential breaking changes in native platform usage
# Look for existing native platform implementations
echo "Checking for native platform usage..."
rg -l "expo-secure-store" .
# Check for potential breaking changes in test files
echo "Checking test files for platform-specific tests..."
rg -l "jest.mock.*expo-secure-store" .
Length of output: 345
Script:
#!/bin/bash
# Let's examine the native storage provider implementation and its tests
echo "=== Native Storage Provider Implementation ==="
cat ./lib/storage/nativeProvider.ts
# Check package.json for expo-secure-store dependency
echo -e "\n=== Package.json expo-secure-store Entry ==="
cat package.json | grep -A 2 -B 2 "expo-secure-store"
# Check for platform-specific logic in the provider
echo -e "\n=== Platform-specific Storage Provider Usage ==="
rg "StorageProvider\(" -A 5 -B 5
Length of output: 3301
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
lib/KindeAuthProvider.tsx (4)
241-258: Consider adding a retry mechanism for token refreshThe token refresh implementation is solid, but consider adding a retry mechanism with exponential backoff for transient failures. This would improve reliability, especially in poor network conditions.
Example implementation:
const refreshToken = async (retries = 3): Promise<LoginResponse> => { const refreshToken = await getStorage(StorageKeys.refreshToken); if (!refreshToken) return { success: false, errorMessage: "No refresh token" }; if (!discovery) return { success: false, errorMessage: "No discovery document" }; for (let attempt = 0; attempt < retries; attempt++) { try { const exchangeCodeResponse = await refreshAsync( { clientId, refreshToken, }, discovery ); return processTokenResponse(exchangeCodeResponse); } catch (error) { if (attempt === retries - 1) throw error; await new Promise(resolve => setTimeout(resolve, Math.pow(2, attempt) * 1000)); } } return { success: false, errorMessage: "Max retries exceeded" }; };
152-154: Document the security consideration for web refresh tokensGood security practice to avoid storing refresh tokens on web platform. Consider adding a comment explaining this security consideration.
- // Dont store refresh token on web. + // Don't store refresh tokens on web platform as localStorage is vulnerable to XSS attacks. + // This follows OAuth 2.0 security best practices for browser-based applications. if (platform !== "web" && response.refreshToken) await setStorage(StorageKeys.refreshToken, response.refreshToken);
Line range hint
432-497: Consider implementing caching for user profile and organization dataThe new methods for retrieving user profile and organization data are well-implemented, but they make token decoding calls on every invocation. Consider implementing a caching mechanism with appropriate invalidation to improve performance.
Example implementation:
private userProfileCache: { data: UserProfile | null; timestamp: number; } | null = null; private readonly CACHE_TTL = 5 * 60 * 1000; // 5 minutes async function getUserProfile(): Promise<UserProfile | null> { // Return cached data if valid if (this.userProfileCache && Date.now() - this.userProfileCache.timestamp < this.CACHE_TTL) { return this.userProfileCache.data; } const idToken = await getDecodedToken<{ sub: string; given_name: string; family_name: string; email: string; picture: string; }>("idToken"); if (!idToken) { return null; } const profile = { id: idToken.sub, givenName: idToken.given_name, familyName: idToken.family_name, email: idToken.email, picture: idToken.picture, }; // Update cache this.userProfileCache = { data: profile, timestamp: Date.now() }; return profile; }
Line range hint
1-534: Solid architectural implementation for web platform supportThe implementation successfully adds web platform support while maintaining good separation of concerns and security best practices. The storage abstraction and platform-specific handling are well-designed, making it easy to maintain and extend in the future.
A few architectural considerations:
- The platform-specific storage implementation isolates web/native differences effectively
- Security considerations for web platform (like refresh token handling) are properly addressed
- The code maintains backward compatibility for existing native platform users
Consider the following architectural improvements for future iterations:
- Add an event system for token refresh and authentication state changes
- Implement a plugin system for custom storage providers
- Consider adding a request interceptor system for automatic token refresh on API calls
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/KindeAuthProvider.tsx(18 hunks)
🔇 Additional comments (2)
lib/KindeAuthProvider.tsx (2)
11-18: LGTM: Web platform initialization is properly configured
The new imports and initialization code properly set up the foundation for web platform support. The maybeCompleteAuthSession() call is correctly placed to handle web redirects.
Also applies to: 20-21, 36-37
67-69: LGTM: Clean platform-specific storage initialization
The storage provider initialization is well-implemented with proper fallback to the default platform.
|
Hi @SpaceDux, I thank for you for raising this PR, this will be a fantastic update, apologies for the delay in commenting I have been away for the holidays. Since this SDK was released we have started moving many of the storage methods to our js-utils package. This package supports expo-secure-store and localStorage with code which is also extendable with own storage values in a typesafe way. The aim here is to take much of the responsibility away from the SDK to allow more custom implementations when needed. This also has a mechanism to use memory storage for session based storage and local storage as an insecure option, this will mean not storing the access/id tokens in the localStorage and store in memory with only storing the refreshToken in the localStorage. (ideally we would want to sort a more secure way to store as option) The package will also give access to all our token helpers like I can look at updating your PR or I am happy to arrange a call to go over how this will look. |
Hey Daniel, don't worry, hope you had a good break! That sounds good, I'm happy for you too take over this PR if you have the capacity and want to do it, equally I'm willing to make the changes to, but I'm unsure when I'll get onto it as I'm also back in work now. If you'd like me to make the changes, perhaps it would be good for us to schedule something in, just so I know exactly what is expected. Cheers, Ryan |
Explain your changes
I'm working on a side project, and wanted to try out Kinde, as it's an Auth provider I've yet to use, thus-far very happy with it. As I'm using Expo for all platforms (Web, iOS, Android), this package was great to get going with, but as expo-secure-store is not supported on web (see expo-secure-store GH for reasoning), I thought I'd raise this feature PR, without web support, this package doesn't quite fulfil my needs, and I imagine others may run into this issue too!
With this, I thought I'd implement a very basic storage provider based on platform, anything other than web at this point is considered native, so theres a simple switch happening, instantiating a storage provider class. Very basic stuff.
I have also implemented token refresh (using refreshAsync), I'm purposely not storing a refresh token on web.
I've also done some minor refactoring of other functionality.
Hope this helps.
Thanks & great job!
Checklist