-
Notifications
You must be signed in to change notification settings - Fork 41
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
feat: invite code in guardian auth #431
feat: invite code in guardian auth #431
Conversation
WalkthroughWalkthroughThe recent changes refactor the Changes
Recent review detailsConfiguration used: CodeRabbit UI Files ignored due to path filters (2)
Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Tip Early access features: disabledWe are currently testing the following features in early access:
Note:
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
apps/guardian-ui/src/admin/FederationAdmin.tsx (1)
Line range hint
31-31
: ReplaceparseInt
withNumber.parseInt
for better clarity and consistency.- parseInt(id, 10) + Number.parseInt(id, 10)Also applies to: 34-34
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.
Actionable comments posted: 2
|
||
const password = sessionStorage.getItem('guardian-ui-key'); | ||
if (password) { | ||
params.append('password', password); | ||
params.password = password; |
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.
Consider encrypting the password before storing it in sessionStorage for enhanced security.
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.
yeah... probably okay, but it's just a lil bit sketchy
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.
@alexlwn123, you're right to point out the potential security issues with storing passwords in sessionStorage. While it provides convenience, it's indeed vulnerable to client-side attacks such as cross-site scripting (XSS). A more secure approach would be to avoid storing sensitive information directly in the browser's storage. Instead, consider using server-side sessions or encrypted tokens that can be validated without directly exposing sensitive details. Let me know if you'd like a deeper dive into these alternatives or help with implementing them.
} | ||
|
||
return `guardian:authenticate?${params.toString()}`; | ||
console.log(`fedimint:guardian:${JSON.stringify(params)}`); |
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.
Remove or replace the console log with a proper logging mechanism.
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.
couple non-blocking things, otherwise LGTM. feel free to merge
} | ||
|
||
export const DangerZone: React.FC<DangerZoneProps> = ({ config, ourPeer }) => { | ||
export const DangerZone: React.FC<DangerZoneProps> = ({ |
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.
nit: Would it make sense to collapse these options by default?
Just thinking the buttons should be hidden from the main dashboard
|
||
const password = sessionStorage.getItem('guardian-ui-key'); | ||
if (password) { | ||
params.append('password', password); | ||
params.password = password; |
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.
yeah... probably okay, but it's just a lil bit sketchy
90a8a82
to
482ea3d
Compare
Was working on this today and not putting the inviteCode in the auth means you have to separately connect as a client then auth as a guardian.
Summary by CodeRabbit
New Features
DangerZone
component to accept aninviteCode
for improved user authentication and configuration handling.Improvements
GuardianAuthenticationCode
component to use a more detailedGuardianAuth
type, enhancing security and functionality.Refactor
DangerZone
andGuardianAuthenticationCode
components to align with new prop and type structures.