Skip to content

Conversation

@jessems
Copy link
Contributor

@jessems jessems commented Jul 1, 2025

OAuth Authentication Fix Summary

Problem: OAuth authentication was failing because AuthManager._get_oauth_token() was trying to access oauth_config.instance_url, but the OAuthConfig class only has a token_url field, not an instance_url field.

Solution: Pass the instance URL separately to the AuthManager constructor from the main server configuration.

Required Changes:

  1. src/servicenow_mcp/auth/auth_manager.py:

    • Add instance_url: str = None parameter to __init__()
    • Store as self.instance_url = instance_url
    • In _get_oauth_token(), replace oauth_config.instance_url with self.instance_url
    • Add proper error handling if instance_url is None
  2. src/servicenow_mcp/server.py:

    • Update AuthManager instantiation from:
      self.auth_manager = AuthManager(self.config.auth)
    • To:
      self.auth_manager = AuthManager(self.config.auth, self.config.instance_url)
  3. Enhanced OAuth flow (optional but recommended):

    • Implement both client_credentials and password grant types
    • Use proper Basic Auth headers for OAuth token requests
    • Add detailed logging for debugging OAuth issues

This allows the AuthManager to access the instance URL needed for constructing the OAuth token endpoint URL when token_url is not explicitly configured.

Copy link
Contributor

@anandsainath anandsainath left a comment

Choose a reason for hiding this comment

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

LGTM, fix it then ship it.

except requests.RequestException as e:
logger.error(f"Failed to get OAuth token: {e}")
raise ValueError(f"Failed to get OAuth token: {e}")
logger.info(f"password grant response status: {response.status_code}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove these?

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.

2 participants