-
-
Notifications
You must be signed in to change notification settings - Fork 3
Sessionkey dafuga #101
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
Sessionkey dafuga #101
Changes from 8 commits
76a3318
2608c61
061e05f
7fa4738
aa0f0c3
6e3920a
a85708f
2b035f7
e9bddf5
3f9a9e6
bc5682b
f2801f9
ed4f124
cc02d1b
b3c84e0
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 |
|---|---|---|
|
|
@@ -170,12 +170,15 @@ export class SessionKit { | |
| // Initialize session key support if configured | ||
| if (options.sessionKey) { | ||
| this.sessionKeyManager = new SessionKeyManager(options.sessionKey, this.ui) | ||
| this.walletPlugins = [ | ||
| ...this.walletPlugins, | ||
| new SessionKeyWalletPlugin({ | ||
| walletPlugins: this.walletPlugins, | ||
| }), | ||
| ] | ||
| // Only add SessionKeyWalletPlugin to wallet picker if not disabled | ||
| if (!options.sessionKey.disableWalletPlugin) { | ||
| this.walletPlugins = [ | ||
| ...this.walletPlugins, | ||
| new SessionKeyWalletPlugin({ | ||
| walletPlugins: this.walletPlugins, | ||
| }), | ||
| ] | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -502,6 +505,15 @@ export class SessionKit { | |
| // Call the `afterLogin` hooks that were registered by the LoginPlugins | ||
| for (const hook of context.hooks.afterLogin) await hook(context) | ||
|
|
||
| // Offer session key setup if configured and session does not have one yet | ||
| if (this.sessionKeyManager && !session.hasSessionKey()) { | ||
| try { | ||
| await session.setupSessionKey() | ||
|
Member
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. What is this for? There's a login plugin already to set one up. We don't want this to trigger on transact otherwise.
Contributor
Author
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. TBH, I had completely missed the login plugin that you added 😂 But since we're making session keys a core feature, wouldn't it be better if the session key setup worked out of the box (without needing to include the login plugin)?
Contributor
Author
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 gave it some thought and I actually like the idea of this being setup through a login plugin 👍 Please disregard my last comment 😄 |
||
| } catch (e) { | ||
| // User cancelled or setup failed - continue with regular session | ||
| // This is not a fatal error, the session is still valid | ||
| } | ||
| } | ||
| // Save the session to storage if it has a storage instance. | ||
| this.persistSession(session, options?.setAsDefault) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,7 @@ import { | |
| Transaction, | ||
| } from '@wharfkit/antelope' | ||
| import type {Fetch, LocaleDefinitions} from '@wharfkit/common' | ||
| import {SigningRequest} from '@wharfkit/signing-request' | ||
| import {PlaceholderAuth, SigningRequest} from '@wharfkit/signing-request' | ||
| import {TransactArgs, TransactPlugin} from './transact' | ||
| import {WalletPlugin} from './wallet' | ||
|
|
||
|
|
@@ -139,6 +139,8 @@ export function extractActions(args: TransactArgs): AnyAction[] { | |
|
|
||
| /** | ||
| * Check if an action has an authorization matching a given permission level. | ||
| * Also matches PlaceholderAuth (actor: '............1', permission: '............2') | ||
| * since placeholders will be resolved to the actual permission level. | ||
| * | ||
| * @param action AnyAction | ||
| * @param permissionLevel PermissionLevel | ||
|
|
@@ -148,17 +150,19 @@ export function actionMatchesPermission( | |
| action: AnyAction, | ||
| permissionLevel: PermissionLevel | ||
| ): boolean { | ||
| return action.authorization.some((auth: PermissionLevelType) => permissionLevel.equals(auth)) | ||
| return action.authorization.some( | ||
| (auth: PermissionLevelType) => permissionLevel.equals(auth) || PlaceholderAuth.equals(auth) | ||
| ) | ||
|
Member
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. Doesn't the placeholder resolve to the proper permission? If it doesn't, then the fix I believe is to make sure the session key permission is in place before the signing request gets resolved. That might be the bug (if you ran into one here)
Contributor
Author
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. The problem is that the placeholder needs to get resolved afterwards since the way that it gets resolved depends on whether or not we are using a session key. We could I guess resolve the signing request twice, but this is a cleaner way to do it IMO. |
||
| } | ||
|
|
||
| function rewriteAuthIfMatches( | ||
| auth: PermissionLevelType, | ||
| permissionLevel: PermissionLevel, | ||
| newPermission: Name | ||
| ): PermissionLevelType { | ||
| if (permissionLevel.equals(auth)) { | ||
| if (permissionLevel.equals(auth) || PlaceholderAuth.equals(auth)) { | ||
| return PermissionLevel.from({ | ||
| actor: auth.actor, | ||
| actor: permissionLevel.actor, | ||
| permission: newPermission, | ||
| }) | ||
| } | ||
|
|
||
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 I know what you're trying to solve for here (the wallet showing up in selectable wallets), but I don't think this is the solution. The solution is probably just hiding it somewhere else, rather than excluding it.
Doesn't this break the restore function?
Uh oh!
There was an error while loading. Please reload this page.
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, I added this so you could have a way to use the session kit without it showing up as an option in the wallet plugin picker. You're absolutely right that it breaks the restore function though so I'm gonna try something different.