Snowflake-Labs / schemachange

A Database Change Management tool for Snowflake
Apache License 2.0
481 stars 219 forks source link

add logic if password and okta authenticator are passed as args #188

Closed AnilBakuru closed 10 months ago

AnilBakuru commented 11 months ago

Adding logic to handle issue # https://github.com/Snowflake-Labs/schemachange/issues/181

AnilBakuru commented 11 months ago

Please take code from most recent commit

sfc-gh-jhansen commented 11 months ago

Hey there @AnilBakuru, any update here after seeing @danielmdubois's comments?

AnilBakuru commented 11 months ago

Hey there @AnilBakuru, any update here after seeing @danielmdubois's comments?

Hi Jhansen, Update the code per comments

AnilBakuru commented 11 months ago

Hi - Any update on this?

Thanks Anil

sfc-gh-jhansen commented 11 months ago

Hey there @AnilBakuru, I'm not sure what happened with your latest changes but now you're introducing a ton of formatting changes to the script. Did you mean to do that? It would be nice to just include the OKTA related changes here.

AnilBakuru commented 11 months ago

Hey there @AnilBakuru, I'm not sure what happened with your latest changes but now you're introducing a ton of formatting changes to the script. Did you mean to do that? It would be nice to just include the OKTA related changes here.

Hi Jeremiah,

Tried to not change formatting but guess it is auto formatting for some reason. Please see if most recent commit has any formatting issues.

If it does, here are the code changes in def authenticate(self) module:

Existing Line 274-278:

Replace block:

if snowflake_password:
      if self.verbose:
        print(_log_auth_type %  'password' )
      self.conArgs['password'] = snowflake_password
      self.conArgs['authenticator'] = 'snowflake'

With:

# if password and not okta authenticator, then default the authenticator
if snowflake_password and not os.getenv("SNOWFLAKE_AUTHENTICATOR").lower()[:8] == 'https://' \
  and not os.getenv("SNOWFLAKE_AUTHENTICATOR"):
      if self.verbose:
        print(_log_auth_type %  'password' )
      self.conArgs['password'] = snowflake_password
      self.conArgs['authenticator'] = 'snowflake'

# if password and okta authenticator
elif snowflake_password and os.getenv("SNOWFLAKE_AUTHENTICATOR").lower()[:8] == 'https://' \
  and os.getenv("SNOWFLAKE_AUTHENTICATOR"):
      okta = os.getenv("SNOWFLAKE_AUTHENTICATOR")
      self.conArgs['password'] = snowflake_password
      self.conArgs['authenticator'] = okta
      if self.verbose:
        print(_log_auth_type %  'Password and Okta' )
        print(_log_okta_ep % okta)

Existing Line 320-328:

Replace block:

elif os.getenv("SNOWFLAKE_AUTHENTICATOR").lower()[:8]=='https://' \
  and os.getenv("SNOWFLAKE_AUTHENTICATOR"):
  okta = os.getenv("SNOWFLAKE_AUTHENTICATOR")
  self.conArgs['authenticator'] = okta
  if self.verbose:
    print(_log_auth_type % 'Okta')
    print(_log_okta_ep % okta)
else:
  raise NameError(_err_no_auth_mthd)

With

# If no password and okta authentication
elif not snowflake_password and os.getenv("SNOWFLAKE_AUTHENTICATOR").lower()[:8]=='https://' \
  and os.getenv("SNOWFLAKE_AUTHENTICATOR"):
  okta = os.getenv("SNOWFLAKE_AUTHENTICATOR")
  self.conArgs['authenticator'] = okta
  if self.verbose:
    print(_log_auth_type % 'Okta')
    print(_log_okta_ep % okta)
else:
  raise NameError(_err_no_auth_mthd)
AnilBakuru commented 10 months ago

Hi Jeremiah, Checking to see if this has been merged.

Thanks Anil

sfc-gh-tmathew commented 10 months ago

@AnilBakuru Can you test if this change will help you instead ? #189

In the PR referred here, It seems to be a cleaner solution to getting okta authentication to work compared to the number of changes you have in this PR.

Can you verify if #189 will solve your okta issue ?

sfc-gh-tmathew commented 10 months ago

Thank you for your contribution @AnilBakuru. We have merged the fix into v3.5.4.

AnilBakuru commented 10 months ago

Glad to help. Thanks so much.