This repository has been archived by the owner on Jan 24, 2023. It is now read-only.
Remove all tokens associated with a cnsi on unregister, also fix e2e #2821
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
richard-cox
commented
Aug 10, 2018
•
richard-cox
commented
- Fixes v2-master e2e
- e2e tests were failing after endpoint guid --> url hash change
- when an endpoint was disconnected only the tokens associated with itself and the requesting user were deleted
- previously this wasn't an issue, as registering the same endpoint resulted in a different guid and any tokens left in the db were not used
- with the guid --> url hash change re-registering the same endpoint results in the old tokens being used
- this broke e2e tests that expected every user to have to connect after all endpoints were unregistered
- not sure if there's any clean up of the tokens table over time, so for that reason and security we now remove all tokens associated with an unregistered endpoint
- other e2e fixes make tests more robust
- e2e tests were failing after endpoint guid --> url hash change - when an endpoint was disconnected only the tokens associated with itself and the requesting user were deleted - previously this wasn't an issue, as registering the same endpoint resulted in a different guid and any tokens left in the db were not used - with the guid --> url hash change re-registering the same endpoint results in the old tokens being used - this broke e2e tests that expected every user to have to connect after all endpoints were unregistered - not sure if there's any clean up of the tokens table over time, so for that reason and security we now remove all tokens associated with an unregistered endpoint - other e2e fixes make tests more robust
|
Hey richard-cox! Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA. |
Codecov Report
@@ Coverage Diff @@
## v2-master #2821 +/- ##
=============================================
- Coverage 71.3% 71.29% -0.01%
=============================================
Files 604 604
Lines 25844 25846 +2
Branches 5854 5854
=============================================
+ Hits 18427 18428 +1
- Misses 7417 7418 +1 |
nwmac
reviewed
Aug 13, 2018
| @@ -20,6 +22,8 @@ describe('Endpoints', () => { | |||
| describe('Connect/Disconnect endpoints -', () => { | |||
|
|
|||
| beforeAll(() => { | |||
| // Ran independently these tests are fine. However stacked with other spec files they'll fail without a pause | |||
| browser.sleep(1000); | |||
There was a problem hiding this comment.
I'm not a fan of using sleep unless we know why we're doing it... if we're waiting for something to load, complete or appear, I'd prefer to explicitly wait on that
- Needed before, ran several times now without and not needed
- Stale = type=cnsi and no corresponding endpoint
- if user can only see one cf, org and space the create app cf/org/space selections will be autopopulated - this means the `next` but is enabled on enter
- (test) the incorrect space was fetched if there were multiple with same name - (test) on delete app it's route would not be deleted due to the app entity having an empty routes array - Beefed up check for space in application service
…r-token-on-unregister
- Also tidy up space e2e tests
nwmac
reviewed
Aug 16, 2018
| @@ -20,6 +22,7 @@ export class ApplicationSummary extends Page { | |||
| expect(urlParts[3]).toBe('summary'); | |||
| const cfGuid = urlParts[1]; | |||
| const appGuid = urlParts[2]; | |||
| e2e.log(`Creating App Summary object using cfGuid: '${cfGuid}' and appGuid: '${appGuid}'`); | |||
nwmac
reviewed
Aug 16, 2018
src/test-e2e/helpers/cf-helpers.ts
Outdated
| return json.total_results; | ||
| }); | ||
| } | ||
|
|
||
| createApp(cnsiGuid: string, spaceGuid: string, appName: string) { | ||
| // For fully fleshed out fetch see application-e2e-helpers | ||
| baseFetchApp(cnsiGuid: string, spaceGuid: string, appName: string) { |
There was a problem hiding this comment.
I was wrong, these are used externally. Have updated name to better reflect type of method
nwmac
reviewed
Aug 16, 2018
|
|
||
| sendCfPost = (cfGuid: string, url: string, body: any): promise.Promise<CFResponse> => | ||
| this.sendCfRequest(cfGuid, url, 'POST', body).then(JSON.parse) | ||
|
|
||
| sendCfPut = (cfGuid: string, url: string, body?: any): promise.Promise<CFResponse> => |
There was a problem hiding this comment.
Add typing. group 3 http request methods
…r-token-on-unregister
This matches jasmine's defaultTimeoutInterval of 30000 Docs for allScriptsTimeout * The timeout in milliseconds for each script run on the browser. This * should be longer than the maximum time your application needs to * stabilize between tasks.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.