Skip to content

Conversation

@spetrescu84
Copy link
Contributor

Proposed changes

Add the business logic along with the dispatching and state creation for JIT

Type of change

  • Feature work
  • Bug fix
  • Documentation
  • Engineering change
  • Test
  • Logging/Telemetry

Risk

  • High – Errors could cause MAJOR regression of many scenarios. (Example: new large features or high level infrastructure changes)
  • Medium – Errors could cause regression of 1 or more scenarios. (Example: somewhat complex bug fixes, small new features)
  • Small – No issues are expected. (Example: Very small bug fixes, string changes, or configuration settings changes)

Additional information

Unit tests to follow

@spetrescu84 spetrescu84 self-assigned this Apr 16, 2025
// TODO: This we need to clarify how we handle
Task {
let jitController = createJITController()
let jitIntrospectResponse = await jitController.getJITAuthMethods(continuationToken: continuationToken,
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually the name of "onJITRequired" is not accurate. With this name I thought that the caller of this function is notified if "registrationRequired" error is returned. While, in this case we call the "verification/introspect" API here. So, here I see two paths:

  1. We let the caller decide what to do once "verificationRequired" is called. Most probably it will do something similar you did in line 856 - 870
  2. We rename the callback to "onJITAuthMethodsSelectionRequired", and we let the JIT controller handle this logic internally. So, the signIn controller delegate to the JIT controller the handling of "registrationRequired" as much as possible and just call the "onJITAuthMethodsSelectionRequired" or "onError".

I prefer approach 2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the controller response to return the state directly and moved that logic to the jit controller.
Renamed to onJITAuthMethodsSelectionRequired

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to not use withCheckedContinuation except for the MSIDHandler

func convertToRegisterStrongAuthChallengeError(correlationId: UUID) -> RegisterStrongAuthChallengeError {
switch self {
case .redirect:
return .init(type: .generalError, correlationId: correlationId)
Copy link
Contributor

Choose a reason for hiding this comment

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

I though we want to convert this to "browser required" error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be browserRequired, fixed

func convertToRegisterStrongAuthSubmitChallengeError(correlationId: UUID) -> RegisterStrongAuthSubmitChallengeError {
switch self {
case .redirect:
return .init(type: .generalError, correlationId: correlationId)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be browserRequired, fixed

MSALLogger.logPII(
level: .error,
context: context,
format: "register/challenge: Invalid response with challenge type preverified, response: \(MSALLogMask.maskPII(response))")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can you mention that "continuation token was expected"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@spetrescu84 spetrescu84 marked this pull request as ready for review April 17, 2025 22:06
@spetrescu84 spetrescu84 changed the base branch from spetrescu/jit-network-implementation to feature/just-in-time-registration April 22, 2025 08:58
@spetrescu84 spetrescu84 merged commit 2286125 into feature/just-in-time-registration Apr 22, 2025
1 of 10 checks passed
spetrescu84 added a commit that referenced this pull request Apr 22, 2025
* Native auth: Just in time SDK mock interface  (#2555)

* add new state and delegate for JIT

* add mock implementation and update for signIn after singUp and SSPR delegates

* Add callback also to signIn password delegate

* make jit state methods public

* use right callback method name

* [iOS SDK] JIT Registration, Network implementation (#2576)

* Added JIT classes

* Jit classes

* Controllers

* New controllers

* Moved controller

* Changes for PR

* Cleanup

* Revert controller changes

* Reverted changes on dispatchers

* Fixed unit tests

* remove non network files

* Unit tests

* Removed failing tests

* PR comments

* PR Comments

* Swiftlint

* Unit tests

* Add error validation for invalid verification contact

* Update MSAL/src/native_auth/public/state_machine/delegate_dispatcher/SignInDelegateDispatchers.swift

Co-authored-by: Danilo Raspa <105228698+nilo-ms@users.noreply.github.com>

* Update MSAL/src/native_auth/public/state_machine/delegate_dispatcher/SignInDelegateDispatchers.swift

Co-authored-by: Danilo Raspa <105228698+nilo-ms@users.noreply.github.com>

* Moved code to KnonwnESTSAPIErrorCodes

* Moved to error case

* Integration tests

* Added comment

---------

Co-authored-by: Danilo Raspa <105228698+nilo-ms@users.noreply.github.com>

* [iOS SDK] JIT Registration, Business implementation (#2583)

* Added JIT classes

* Jit classes

* Controllers

* New controllers

* Moved controller

* Changes for PR

* Cleanup

* Revert controller changes

* Reverted changes on dispatchers

* Fixed unit tests

* remove non network files

* Unit tests

* Removed failing tests

* PR comments

* PR Comments

* Swiftlint

* Unit tests

* Add error validation for invalid verification contact

* Update MSAL/src/native_auth/public/state_machine/delegate_dispatcher/SignInDelegateDispatchers.swift

Co-authored-by: Danilo Raspa <105228698+nilo-ms@users.noreply.github.com>

* Update MSAL/src/native_auth/public/state_machine/delegate_dispatcher/SignInDelegateDispatchers.swift

Co-authored-by: Danilo Raspa <105228698+nilo-ms@users.noreply.github.com>

* Moved code to KnonwnESTSAPIErrorCodes

* Moved to error case

* Controller code

* Linked controllers

* Fix infinite allocation loop and empty verification contact

* New SignIn method

* Fix Unit tests

* Removed not needed code

* Split functions to handlers for responses

* Update MSAL/src/native_auth/controllers/jit/MSALNativeAuthJITController.swift

Co-authored-by: Danilo Raspa <105228698+nilo-ms@users.noreply.github.com>

* Removed unused parameter

* Removed not needed swiftlint

* Removed not needed username

* Changed general error to browser required

* removed not needed config

* Renamed jitRequired to jitAuthMethodsSelectionRequired
Moved logic to jitcontroller

* changed incorrect error

* Added preverified path

* refactor, remove task inside of task

* Finished moving away from the on... parameters
Combined both signIn functions from the protocol into one

* Removed redirect case from introspect endpoints

* Renamed to HandleTokenResult

* Integration tests

* Added comment

* Unit tests

* Unit tests

* PR Comments

---------

Co-authored-by: Danilo Raspa <105228698+nilo-ms@users.noreply.github.com>
Co-authored-by: Danilo Raspa <daniloraspa@microsoft.com>

* IC

* Removed incorrect unit test host

* Removed incorrect extra msal.framework

* Swiftlint

* Preverified

---------

Co-authored-by: Danilo Raspa <105228698+nilo-ms@users.noreply.github.com>
Co-authored-by: Danilo Raspa <daniloraspa@microsoft.com>
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.

3 participants