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

[PY] feat: SSO - auth-sign-in #1976

Merged
merged 7 commits into from
Sep 20, 2024

Conversation

BMS-geodev
Copy link
Collaborator

@BMS-geodev BMS-geodev commented Sep 3, 2024

Linked issues

closes: #1318

Details

Added the get_token_or_sign_in_user() method. It is the interaction entrypoint for the sso authentication.

It is modeled after the JS version of this functionality.

If the user is signed in, get the access token.
If not, triggers the sign in flow for the provided authentication setting name and returns.
In this case, the bot should end the turn until the sign in flow is completed.

Change details

Describe your changes, with screenshots and code snippets as appropriate

code snippets:

screenshots:

Attestation Checklist

  • My code follows the style guidelines of this project

  • I have checked for/fixed spelling, linting, and other errors

  • I have commented my code for clarity

  • I have made corresponding changes to the documentation (updating the doc strings in the code is sufficient)

  • My changes generate no new warnings

  • I have added tests that validates my changes, and provides sufficient test coverage. I have tested with:

    • Local testing
    • E2E testing in Teams
  • New and existing unit tests pass locally with my changes

Additional information

Feel free to add other relevant information below

@BMS-geodev
Copy link
Collaborator Author

BMS-geodev commented Sep 3, 2024

#1318 (comment)

Added getTokenOrSignInUser method in Application class. It is meant to be the single catch-all method to sign a user in and get the token. If the user is signed in it simply returns the token. If not, it triggers the sign in flow. Starting the sign in flow will only work if the incoming activity supports it (ex. message, messageExtension/fetch...etc). This method will work in action handlers as well.

@BMS-geodev
Copy link
Collaborator Author

BMS-geodev commented Sep 3, 2024

in application.ts, getTokenOrStartSignIn(),... does deleteUserInSignInFlow and setUserInSignInFlow, etc..

In app.py, get_token_or_sign_in_user(),... does not directly do these things. Here the "insigninflow" is captured at the top. IN_SIGN_IN_KEY = "__InSignInFlow__". IN_SIGN_IN_KEY is used in the

    async def _authenticate_user(self, context: TurnContext, state):
        if self.options.auth and self._auth:
            auth_condition = (
                isinstance(self.options.auth.auto, bool) and self.options.auth.auto
            ) or (callable(self.options.auth.auto) and self.options.auth.auto(context))
            user_in_sign_in = IN_SIGN_IN_KEY in state.user
            if auth_condition or user_in_sign_in:
                key: Optional[str] = state.user.get(IN_SIGN_IN_KEY, self.options.auth.default)

                if key is not None:
                    state.user[IN_SIGN_IN_KEY] = key
                    res = await self._auth.sign_in(context, state, key=key)
                    if res.status == "complete":
                        del state.user[IN_SIGN_IN_KEY]

                    if res.status == "pending":
                        await state.save(context, self._options.storage)
                        return False

                    if res.status == "error" and res.reason != "invalid-activity":
                        del state.user[IN_SIGN_IN_KEY]
                        raise ApplicationError(f"[{res.reason}] => {res.message}")

        return True

In the python get_token_or_sign_in(), _authenticate_user() is called when a token is not already available. So I think we are aligned with the notes from the TS getTokenOrStartSignIn() on this topic.

If the user is signed in, get the access token. If not, triggers the sign in flow for the provided authentication setting name and returns. In this case, the bot should end the turn until the sign in flow is completed.

@BMS-geodev
Copy link
Collaborator Author

I think the method is looking alright, but I still need to write tests @lilyydu

@BMS-geodev BMS-geodev marked this pull request as ready for review September 9, 2024 16:19
@corinagum
Copy link
Collaborator

corinagum commented Sep 11, 2024

@BMS-geodev could you update the PR style (update title of PR) to follow our contribution guidelines? Thanks!

@BMS-geodev BMS-geodev changed the title bms/auth-sign-in-1318 [PY] SSO - auth-sign-in Sep 12, 2024
@BMS-geodev
Copy link
Collaborator Author

@corinagum done, apologies

@corinagum corinagum changed the title [PY] SSO - auth-sign-in [PY] feat: SSO - auth-sign-in Sep 12, 2024
@BMS-geodev
Copy link
Collaborator Author

@lilyydu wrote 4 tests that circle around get_token_or_sign_in_user(), and added them to test_app.py

test_get_token_or_sign_in_user_token_exists
test_get_token_or_sign_in_user_no_token_sign_in_flow
test_get_token_or_sign_in_user_successful_authentication
test_get_token_or_sign_in_user_no_auth_manager

@lilyydu lilyydu merged commit 0388d37 into microsoft:lilyydu/py-sso Sep 20, 2024
7 checks passed
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.

4 participants