Fix AWS Console apps logins not being provided in all places#44992
Fix AWS Console apps logins not being provided in all places#44992gabrielcorado merged 1 commit intomasterfrom
Conversation
gabrielcorado
added
the
no-changelog
| var checkableResource services.AccessCheckable = resource | ||
| if appServer, ok := resource.(types.AppServer); ok { | ||
| checkableResource = appServer.GetApp() | ||
| } | ||
|
|
There was a problem hiding this comment.
should this be part of GetAllowedLoginsForResource?
| if err != nil { | ||
| return nil, trace.Wrap(err) | ||
| } | ||
| if !req.IncludeLogins && (r.GetKind() != types.KindNode || r.GetKind() != types.KindWindowsDesktop || r.GetKind() != types.KindAppServer) { |
There was a problem hiding this comment.
nit: not sure how i feel about this check. Do we have to keep expanding this when new types is added to GetAllowedLoginsForResource? Could we maybe always call GetAllowedLoginsForResource for req.IncludeLogins and skip certain errors like trace.NotImplemented
It must use an endpoint that returns enriched resources ( |
I thought the current |
It does, but it does through the |
Seems to me a good opportunity to switch |
public-teleport-github-review-bot
Bot
removed request for
gzdunek and
probakowski
gabrielcorado
deleted the
gabrielcorado/fix-app-logins-enriched-resources
branch
|
@gabrielcorado See the table below for backport results.
|
3 participants
Closes #45086
#44611 Changed the app resources to return the
Loginsfield (from enriched resources) for AWS Console apps. However, the change needed to be completed, and the field needed to be added in some places.TLDR: Tests weren't covering this scenario, so this PR also updates them.
Two tests cover those scenarios. However, none of them caught this issue:
TestListResources_WithLogins:AppServerwasn't added to the list of resources fetched by the test.TestListUnifiedResources_WithLogins: This listing process was hard coded to fetch only a limited number of resources, meaning the resulting list would usually never return the apps. (The flakiness on this test was a case where the apps were included on the final list and returned). To improve it, we now consume all resources and assert the final number of resources, so if we add more resources, the test will fail. Hard coding the number of resources could be better. Still, a more scalable solution would require refactoring other tests, so I kept the changes scoped only for the affected tests.