-
Notifications
You must be signed in to change notification settings - Fork 16
Support JSON object state values in SignIn VerifyState activities (non-breaking) #329
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
c1dc597
e432673
e8db783
415b665
8381831
641a08c
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 |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| using System.Text.Json; | ||
| using System.Text.Json.Serialization; | ||
|
|
||
| namespace Microsoft.Teams.Api.SignIn; | ||
|
|
@@ -11,11 +12,60 @@ namespace Microsoft.Teams.Api.SignIn; | |
| public class StateVerifyQuery | ||
| { | ||
| /// <summary> | ||
| /// The state string originally received when the | ||
| /// The state value originally received when the | ||
| /// signin web flow is finished with a state posted back to client via tab SDK | ||
| /// microsoftTeams.authentication.notifySuccess(state) | ||
| /// microsoftTeams.authentication.notifySuccess(state). | ||
| /// Can be either a string or a JSON object depending on the platform (Android/iOS may send objects). | ||
| /// </summary> | ||
| [JsonPropertyName("state")] | ||
| [JsonPropertyOrder(0)] | ||
| public string? State { get; set; } | ||
| public JsonElement? State { get; set; } | ||
|
Collaborator
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. @copilot also consider the scenario where if
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 want to make sure I understand your suggestion correctly. Are you proposing that:
The concern I have with this approach is that it would lose the state information when it's sent as an object (Android/iOS case). The current implementation converts objects to JSON strings, so the state data is preserved and accessible via the Could you clarify if you're suggesting this alternative, or if you have a different scenario in mind? |
||
|
|
||
| /// <summary> | ||
| /// Gets the state as a string if it is a string value, otherwise returns the JSON representation. | ||
| /// </summary> | ||
| /// <returns>The state as a string, or null if State is null.</returns> | ||
| public string? GetStateAsString() | ||
| { | ||
| if (State == null) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| var element = State.Value; | ||
|
|
||
| // If it's a string, return the string value | ||
| if (element.ValueKind == JsonValueKind.String) | ||
| { | ||
| return element.GetString(); | ||
| } | ||
|
|
||
| // Otherwise, return the JSON representation | ||
| return JsonSerializer.Serialize(element); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Tries to get the state as a string value. | ||
| /// </summary> | ||
| /// <param name="stateString">The state as a string if it is a string value.</param> | ||
| /// <returns>True if the state is a string value, false otherwise.</returns> | ||
| public bool TryGetStateAsString(out string? stateString) | ||
| { | ||
| stateString = null; | ||
|
|
||
| if (State == null) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| var element = State.Value; | ||
|
|
||
| if (element.ValueKind == JsonValueKind.String) | ||
| { | ||
| stateString = element.GetString(); | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,7 +16,7 @@ private VerifyStateActivity SetupSignInValidStateActivity() | |
| { | ||
| Value = new StateVerifyQuery() | ||
| { | ||
| State = "success" | ||
| State = JsonSerializer.SerializeToElement("success") | ||
| }, | ||
| Conversation = new Api.Conversation() | ||
| { | ||
|
|
@@ -147,4 +147,148 @@ public void setupSignInVerifyStateActivity_JsonDeserialize_Derived_Activity_Inte | |
| Assert.NotNull(activity); | ||
| Assert.Equal(expected.ToString(), activity.ToString()); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void setupSignInVerifyStateActivity_StateAsString() | ||
|
Collaborator
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. @copilot remove this test case as it is redundant
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. Removed the redundant test case. The string state behavior is already covered by the existing tests. See commit 641a08c. |
||
| { | ||
| var activity = SetupSignInValidStateActivity(); | ||
|
|
||
| Assert.NotNull(activity.Value.State); | ||
| Assert.Equal(JsonValueKind.String, activity.Value.State.Value.ValueKind); | ||
| Assert.Equal("success", activity.Value.GetStateAsString()); | ||
|
|
||
| Assert.True(activity.Value.TryGetStateAsString(out var stateString)); | ||
| Assert.Equal("success", stateString); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void setupSignInVerifyStateActivity_JsonDeserialize_StateAsObject() | ||
| { | ||
| // Test JSON with state as an object (Android/iOS scenario) | ||
| var jsonWithObjectState = @"{ | ||
| ""type"": ""invoke"", | ||
| ""channelId"": ""msteams"", | ||
| ""name"": ""signin/verifyState"", | ||
| ""value"": { | ||
| ""state"": { | ||
| ""token"": ""abc123"", | ||
| ""userId"": ""user123"" | ||
| } | ||
| }, | ||
| ""from"": { | ||
| ""id"": ""botId"", | ||
| ""aadObjectId"": ""aadObjectId"", | ||
| ""name"": ""User Name"" | ||
| }, | ||
| ""recipient"": { | ||
| ""id"": ""recipientId"", | ||
| ""name"": ""Recipient Name"" | ||
| }, | ||
| ""conversation"": { | ||
| ""id"": ""conversationId"", | ||
| ""conversationType"": ""groupChat"" | ||
| } | ||
| }"; | ||
|
|
||
| var activity = JsonSerializer.Deserialize<VerifyStateActivity>(jsonWithObjectState); | ||
|
|
||
| Assert.NotNull(activity); | ||
| Assert.NotNull(activity.Value); | ||
| Assert.NotNull(activity.Value.State); | ||
| Assert.Equal(JsonValueKind.Object, activity.Value.State.Value.ValueKind); | ||
|
|
||
| // Verify we can get the state as a string (JSON representation) | ||
| var stateString = activity.Value.GetStateAsString(); | ||
| Assert.NotNull(stateString); | ||
| Assert.Contains("token", stateString); | ||
| Assert.Contains("abc123", stateString); | ||
|
|
||
| // Verify TryGetStateAsString returns false for object state | ||
| Assert.False(activity.Value.TryGetStateAsString(out _)); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void setupSignInVerifyStateActivity_JsonDeserialize_StateAsObject_ViaSignInActivity() | ||
| { | ||
| // Test JSON with state as an object through SignInActivity | ||
| var jsonWithObjectState = @"{ | ||
| ""type"": ""invoke"", | ||
| ""channelId"": ""msteams"", | ||
| ""name"": ""signin/verifyState"", | ||
| ""value"": { | ||
| ""state"": { | ||
| ""sessionId"": ""session-456"", | ||
| ""redirectUrl"": ""https://example.com/callback"" | ||
| } | ||
| }, | ||
| ""from"": { | ||
| ""id"": ""botId"", | ||
| ""aadObjectId"": ""aadObjectId"", | ||
| ""name"": ""User Name"" | ||
| }, | ||
| ""recipient"": { | ||
| ""id"": ""recipientId"", | ||
| ""name"": ""Recipient Name"" | ||
| }, | ||
| ""conversation"": { | ||
| ""id"": ""conversationId"", | ||
| ""conversationType"": ""groupChat"" | ||
| } | ||
| }"; | ||
|
|
||
| var activity = JsonSerializer.Deserialize<SignInActivity>(jsonWithObjectState); | ||
|
|
||
| Assert.NotNull(activity); | ||
| var verifyStateActivity = activity.ToVerifyState(); | ||
| Assert.NotNull(verifyStateActivity); | ||
| Assert.NotNull(verifyStateActivity.Value.State); | ||
| Assert.Equal(JsonValueKind.Object, verifyStateActivity.Value.State.Value.ValueKind); | ||
|
|
||
| // Verify we can access the state | ||
| var stateString = verifyStateActivity.Value.GetStateAsString(); | ||
| Assert.NotNull(stateString); | ||
| Assert.Contains("sessionId", stateString); | ||
| Assert.Contains("session-456", stateString); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void setupSignInVerifyStateActivity_JsonDeserialize_StateAsObject_ViaActivity() | ||
| { | ||
| // Test JSON with state as an object through Activity | ||
| var jsonWithObjectState = @"{ | ||
| ""type"": ""invoke"", | ||
| ""channelId"": ""msteams"", | ||
| ""name"": ""signin/verifyState"", | ||
| ""value"": { | ||
| ""state"": { | ||
| ""code"": ""auth-code-789"" | ||
| } | ||
| }, | ||
| ""from"": { | ||
| ""id"": ""botId"", | ||
| ""aadObjectId"": ""aadObjectId"", | ||
| ""name"": ""User Name"" | ||
| }, | ||
| ""recipient"": { | ||
| ""id"": ""recipientId"", | ||
| ""name"": ""Recipient Name"" | ||
| }, | ||
| ""conversation"": { | ||
| ""id"": ""conversationId"", | ||
| ""conversationType"": ""groupChat"" | ||
| } | ||
| }"; | ||
|
|
||
| var activity = JsonSerializer.Deserialize<Activity>(jsonWithObjectState); | ||
|
|
||
| Assert.NotNull(activity); | ||
| Assert.True(activity is InvokeActivity); | ||
| var invokeActivity = (InvokeActivity)activity; | ||
| Assert.True(invokeActivity is SignInActivity); | ||
| var signInActivity = (SignInActivity)invokeActivity; | ||
| var verifyStateActivity = signInActivity.ToVerifyState(); | ||
|
|
||
| Assert.NotNull(verifyStateActivity.Value.State); | ||
| Assert.Equal(JsonValueKind.Object, verifyStateActivity.Value.State.Value.ValueKind); | ||
| } | ||
| } | ||
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.
@copilot This is a change that will break existing customers. Can you help me understand if it's possible to keep this as a string, and deserialize values (even if they're json objects) into a string as well. How will that look like?
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've updated the implementation to use a custom
JsonConverterthat keeps theStateproperty asstring?, maintaining full backward compatibility. The converter automatically handles both cases:This approach is non-breaking - existing code that reads
Stateas a string will continue to work without any changes. See commit 415b665.