search

cloudfoundry / notifications

The notifications service component of Cloud Foundry
Apache License 2.0
17 stars 22 forks source link

Bugs when sending to everyone #11

Open ArthurHlt opened 7 years ago

ArthurHlt commented 7 years ago

First I'm not sure if I should V1 or V2, the second version seems not ready to send emails, is that correct ?

In the v1 I can't send emails to /everyone, I have this error: {"errors":["UAA Failure: 401 {\"error\":\"invalid_token\",\"error_description\":\"Invalid access token value, must be at least 30 characters:\"}"]}

I first thought about how I handled client in my UAA, but everything seems correct so I started digging inside the code.

It appears that in uaa package token is never given to the uaa client, see: https://github.com/cloudfoundry-incubator/notifications/blob/master/uaa/uaa.go#L94-L96

But token is passed in UsersEmailsByIDs function: https://github.com/cloudfoundry-incubator/notifications/blob/master/uaa/uaa.go#L58

By looking in vendor https://github.com/cloudfoundry-incubator/notifications/blob/master/vendor/github.com/pivotal-cf/uaa-sso-golang/uaa/all_users.go it looks like it will never tried to have a token if it's not already set. So obviously trying to retrieve all users will always fail.

I've patched myself from my side by giving the token to client which make it works.

Do you want a PR for this ? Or is it kind of wanted ?

cf-gitbot commented 7 years ago

We have created an issue in Pivotal Tracker to manage this. Unfortunately, the Pivotal Tracker project is private so you may be unable to view the contents of the story.

The labels on this github issue will be updated when the story is started.

iplay88keys commented 6 years ago

Hey @ArthurHlt!

We are currently supporting v1. PRs are welcome! We'll try to merge it in as soon as possible! Please reference this issue in your PR. We highly recommend adding a test to reproduce this behavior.

Thank you! Jeremy