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

CSS-9575 Use new error for session token login #1269

Closed
wants to merge 1 commit into from

Conversation

kian99
Copy link
Contributor

@kian99 kian99 commented Jul 12, 2024

Description

This PR is marked as a draft until juju/juju#17726 lands. NB: This change should only land after the Juju PR lands and a new Juju release is made including the change - likely in Juju 3.5.3 3.5.4.

This PR intends to fix a behaviour of the Juju CLI when interacting with JIMM. Copy-pasting from the above PR:

The current implementation of the Juju CLI uses the sessionTokenLoginProvider to login to JAAS. This login method uses the device code login flow, i.e. it:

Tries to login with the client's session token.
If the error returned is a specific code, it starts the device flow login described below.

  • The server returns a URL where the user must go to login.
  • The server polls the server waiting for the user to login.
  • The server returns a session token to the client.
  • The client tries to login again with the session token.

Currently, the specific error code is CodeUnauthorized. This is a poor choice because it is possible for Juju to return this code as a valid error unrelated to login. In cases like these the user is suddenly asked to login again when in fact they should simply be presented with the error.

This PR changes the expected error to a unique code. The code is only used in tests/the login provider. JAAS will use this error in its implementation.

NB! Unfortunately this change will cause a breakage between the Juju CLI and JAAS but because JAAS is not in widespread usage yet, it is better to fix the issue before then.

In JIMM, we ensure that whenever a call to VerifySessionToken fails, we return the appropriate error for the client to start the device flow. This handles cases where:

  • A user is logging in for the first time and presents an empty session token.
  • The user's session token is expired.
  • The user has modified their existing session token and it is no longer valid.
  • Covers both controller and model level calls, as they both return the error from VerifySessionToken direcltly.

Fixes CSS-9575

Engineering checklist

Check only items that apply

  • Documentation updated
  • Covered by unit tests
  • Covered by integration tests

Notes for code reviewers

This PR is marked as a draft until the Juju changes land at which point the Juju dependency in JIMM must be updated. It will likely also require some updates to tests.

Copy link
Member

@babakks babakks left a comment

Choose a reason for hiding this comment

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

Thanks for this. I know we just test the errors against a regexp/message, but can we have a test for VerifySessionToken method that checks the error code as well?

@kian99
Copy link
Contributor Author

kian99 commented Jul 12, 2024

can we have a test for VerifySessionToken method that checks the error code as well?

I think I've added what you're after in #1268, but once that lands and the Juju change lands, I'll make sure this PR has tests to verify the error codes and login.
Edit: Nevermind, I see what you mean, just a unit test for the error code. I'll make sure that is added before merging this.

@kian99 kian99 mentioned this pull request Jul 31, 2024
3 tasks
@kian99
Copy link
Contributor Author

kian99 commented Jul 31, 2024

These changes have been incorporated into #1251. Closing in favor of that PR.

@kian99 kian99 closed this Jul 31, 2024
@kian99 kian99 mentioned this pull request Sep 3, 2024
3 tasks
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