Skip to content
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

[SDK-1473][B2B] Add exchange method to session client #233

Merged
merged 1 commit into from
May 23, 2024

Conversation

nidal-stytch
Copy link
Contributor

@nidal-stytch nidal-stytch commented May 22, 2024

Linear Ticket: SDK-1473: Add exchange method to session client

Notes:

  • The session exchange function works if there is a user that is authenticated by email magic links or oauth and is a member of multiple organizations. You can then call the session exchange function with the id of the org they wish to authenticate with and it will create and return a new member session.

Changes:

  1. Extend Sessions to include an exchnage method. And add the ExchangeParameters type.
  2. In the b2b sample app remove all usage of a defined property for private let defaults: UserDefaults = .standard and instead refer directly to UserDefaults.standard. I feel that this is confusing and would imply custom usage of user default and not the standard usage.
  3. Fix password reset flow in b2b sample app in SceneDelegate, wrong logic was being used to parse out the PasswordsViewController.
  4. Remove logic in b2b sample app RootViewController to stop popping to the root view controller every time we got a notification about the session info changing. The RootViewController only ever set the project id and that almost never changes. And with the improved reporting throughout the app, we know when the user needs to be authenticated.

Checklist:

  • I have verified that this change works in the relevant demo app, or N/A
  • I have added or updated any tests relevant to this change, or N/A
  • I have updated any relevant README files for this change, or N/A

@nidal-stytch nidal-stytch added the in-progress-do-not-review Still working on the changes, not ready for review yet label May 22, 2024
@nidal-stytch nidal-stytch changed the base branch from main to feature-parity May 22, 2024 15:50
@nidal-stytch nidal-stytch force-pushed the SDK-1473-session-client-parity branch 13 times, most recently from 8d7527d to d450152 Compare May 23, 2024 16:01
@nidal-stytch nidal-stytch marked this pull request as ready for review May 23, 2024 16:02
@nidal-stytch nidal-stytch requested a review from a team as a code owner May 23, 2024 16:02
@nidal-stytch nidal-stytch requested review from jhaven-stytch and removed request for a team May 23, 2024 16:02
@nidal-stytch nidal-stytch removed the in-progress-do-not-review Still working on the changes, not ready for review yet label May 23, 2024
@nidal-stytch nidal-stytch mentioned this pull request May 23, 2024
public extension Sessions {
// sourcery: AsyncVariants, (NOTE: - must use /// doc comment styling)
/// Use this endpoint to exchange a Member's existing session for another session in a different Organization.
func exchnage(parameters: ExchangeParameters) async throws -> B2BAuthenticateResponse {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: exchange

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, just an fyi, I am super dyslexic!

}
Task {
do {
let parameters = Sessions<B2BAuthenticateResponse>.ExchangeParameters(organizationID: organizationID)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whats with the <B2BAuthenticateResponse>? Is it because it's using the Session client defined in StytchClientCommon? Should we go ahead and break out the session clients into separate ones for B2B and B2C? that way we could do StytchB2BClient.Sessions.ExchangeParameters(), right? Just for consistency with all the other products/methods. I think this current approach might lead to developer confusion.

Copy link
Contributor Author

@nidal-stytch nidal-stytch May 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally feel that it is good the way it is and a good use of swift generics. If I can figure out something with sorcery where I can add a "where" clause to the protocol extensions then we can remove this.

This is the way around this:
public extension Sessions where AuthResponseType == B2BAuthenticateResponse

Then:
Sessions<B2BAuthenticateResponse>.ExchangeParameters
turns into:
Sessions.ExchangeParameters

But currently I can't figure out how to do that with sorcery and I posted a question about how to do that for the generated files here: krzysztofzablocki/Sourcery#1337.

I would only think we need to name space Session if it was to be used in tandem with the B2C session.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we split out the sessions clients (which we do on Android), would that avoid the unknown sourcery stuff? If so, that plus being able to namespace the parameters seems like a win to me. We already have some customer confusion with using two different clients, I just think that this might add to that cognitive load.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, after thinking about it and pair programming, I think we do need separate objects for b2b session and b2c session, lets make a ticket to capture this work.

@nidal-stytch nidal-stytch force-pushed the SDK-1473-session-client-parity branch 2 times, most recently from b73c0fc to 7b75f23 Compare May 23, 2024 16:49
@nidal-stytch nidal-stytch force-pushed the SDK-1473-session-client-parity branch from 7b75f23 to 6c0ea0f Compare May 23, 2024 16:50
@nidal-stytch nidal-stytch merged commit 834c9ce into feature-parity May 23, 2024
5 checks passed
@nidal-stytch nidal-stytch deleted the SDK-1473-session-client-parity branch May 23, 2024 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants