Skip to content

fix: add token endpoint URL to credentials file#1177

Merged
craicoverflow merged 1 commit intomainfrom
fix-add-token-url
Oct 4, 2021
Merged

fix: add token endpoint URL to credentials file#1177
craicoverflow merged 1 commit intomainfrom
fix-add-token-url

Conversation

@craicoverflow
Copy link
Contributor

Closes #

Verification Steps

  1. Do x
  2. Do y

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation change
  • Other (please specify)

Checklist

  • Documentation added for the feature
  • CI and all relevant tests are passing
  • Code Review completed
  • Verified independently by reviewer

@craicoverflow craicoverflow merged commit a9b6340 into main Oct 4, 2021
@craicoverflow craicoverflow deleted the fix-add-token-url branch October 4, 2021 16:15
@craicoverflow
Copy link
Contributor Author

Quick bug fix.

@wtrocki
Copy link
Collaborator

wtrocki commented Oct 4, 2021

That was fast :)

Was reviewing it :D

creds := &credentials.Credentials{
ClientID: updatedServiceAccount.GetClientId(),
ClientSecret: updatedServiceAccount.GetClientSecret(),
TokenURL: cfg.MasAuthURL + "/protocol/openid-connect/token",
Copy link
Collaborator

@wtrocki wtrocki Oct 4, 2021

Choose a reason for hiding this comment

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

Suggested change
TokenURL: cfg.MasAuthURL + "/protocol/openid-connect/token",
TokenURL: cfg.MasAuthURL + serviceaccount.TOKEN_URL_SUFFIX,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, realistically we should not be concatenating the URL like this, rather use something like ResolveReference (or something else), in case there are extra characters.

u := baseURL.ResolveReference(&rel)

Just wanted a quick fix so I copy/pasted it for now :)

@wtrocki
Copy link
Collaborator

wtrocki commented Oct 4, 2021

nice catch! I totally forgot about reset.

craicoverflow pushed a commit to craicoverflow/app-services-cli that referenced this pull request Oct 7, 2021
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