Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The fixed-port server is exposed on all interfaces, and AWS login provides invalid recovery guidance when binding fails.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds configurable OAuth callback ports across login and SSO workflows.
Changes:
- Adds and validates
--redirect-port. - Passes the port to the local callback server.
- Adds tests and changelog entries.
File summaries
| File | Description |
|---|---|
awscli/customizations/sso/utils.py |
Defines, validates, and binds redirect ports. |
awscli/customizations/sso/login.py |
Supports the option for SSO login. |
awscli/customizations/login/login.py |
Supports the option for AWS login. |
awscli/customizations/configure/sso_commands.py |
Supports the option during SSO configuration. |
tests/unit/customizations/sso/test_utils.py |
Tests validation and server binding. |
tests/unit/customizations/configure/test_sso.py |
Tests configure SSO propagation and validation. |
tests/functional/sso/test_login.py |
Tests SSO login behavior. |
tests/functional/login/test_login.py |
Tests AWS login behavior. |
.changes/next-release/feature-sso-34535.json |
Documents the SSO enhancement. |
.changes/next-release/feature-login-34536.json |
Documents the login enhancement. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| try: | ||
| handler = partial(OAuthCallbackHandler, self) | ||
| self.http_server = HTTPServer(('', 0), handler) | ||
| self.http_server = HTTPServer(('', port), handler) |
| auth_code_fetcher=AuthCodeFetcher( | ||
| redirect_port=parsed_args.redirect_port | ||
| ), |
|
change looks fine to me. can you check if copilots comments are valid or not? |
|
Hi @ashovlin , Thanks for referencing #10462 here. I opened #10433 and then opened #10462 after discussing it with the maintainers My preference would be for #10462 to be reviewed directly, with any requested That said, if this PR is intended to supersede #10462 while substantially |
|
Hey @ixxq , thanks for replying, sorry about jumping ahead here. We have a few competing PRs, so my instinct was to pick what we're initially comfortable with. If you're willing to update #10462 with these, we can proceed over there:
|
|
@ashovlin Thanks for being open to moving forward with #10462. I'm happy to update it to support Just to confirm the scope: should I also include |
Issue #, if available: #9148, #10433
Description of changes: This begins addressing redirect port configurablity by adding
--redirect-portto:aws loginaws sso loginaws configure ssoIt pulls from some open PRs:
loginI still think we should work towards a profile config option, but that triggers more internal work (amending spec so AWS Tools for PowerShell honors it too, dev guide updates), so starting with this.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.