-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[Gekidou] Get all existing server credentials on init #5468
Conversation
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.
just not sure why Podfile.lock updated
@@ -106,7 +149,7 @@ export const getServerCredentials = async (serverUrl: string): Promise<ServerCre | |||
|
|||
// TODO: Create client and set token / CSRF |
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.
should this be removed?
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.
Can do in the PR that brings in the network lib
- libwebp/mux (= 1.1.0) | ||
- libwebp/webp (= 1.1.0) | ||
- libwebp/demux (1.1.0): | ||
- libwebp (1.2.0): |
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.
why is this changing?
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.
Not sure :/
diff --git a/node_modules/react-native-keychain/RNKeychainManager/RNKeychainManager.m b/node_modules/react-native-keychain/RNKeychainManager/RNKeychainManager.m | ||
index 38ccdf3..2875032 100644 | ||
--- a/node_modules/react-native-keychain/RNKeychainManager/RNKeychainManager.m | ||
+++ b/node_modules/react-native-keychain/RNKeychainManager/RNKeychainManager.m |
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.
do we want to submit this patch upstream?
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.
Yep, just gotta fix the patch.
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.
@@ -4,16 +4,56 @@ | |||
import {Platform} from 'react-native'; | |||
import AsyncStorage from '@react-native-community/async-storage'; |
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.
When sorting imports, the @react-native-community/async-storage
should be treated as the character a
and sorted amongst the first modules.
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.
LGTM
app/init/credentials.ts
Outdated
|
||
setServerCredentials(serverUrl, credentials!.userId, credentials!.token); | ||
|
||
const normalizedServerUrl = normalizeServerUrl(serverUrl); |
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'm having my doubts about normalizing.. let's chat about this
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.
Removed normalization. We're now OK with the the server URL containing the protocol in both the keychain and in the db. If we want to match against a deeplink URL, we can always use a LIKE
query or replace the deeplink protocol prior to querying.
Summary
Gets all existing server credentials on init for use with initializing the DatabaseManager and the NetworkClientManager. The keychain library on the iOS side needed patching to support extracting all server URLs from internet credential entries.
Release Note