Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changed API deletion to instead set key as inactive #54

Closed
wants to merge 1 commit into from

Conversation

billkalter
Copy link
Contributor

Github Issue

49

What Are We Doing Here?

This PR implements #49 as described. API keys are no longer deleted but put into an inactive state. An inactive API key exists but cannot be used to authorize or authenticate any permissions. In addition there is a third state, migrated, to track API keys which have been migrated.

Existing API keys are grandfathered in as active.

How to Test and Verify

  1. Check out this PR

  2. Create an API key:

    $ curl -XPOST 'localhost:8081/tasks/api-key?action=create&owner=me&description=desc&role=standard&APIKey=local_admin'
    API key: rbhroxstezbhrbnagekfplr2iw6vzgb5x9myesf8rykh7maj
    
    Warning:  This is your only chance to see this key.  Save it somewhere now.
    
  3. Verify the API key is working:

    $ curl -XPUT "localhost:8080/bus/1/test-sub" -H "X-BV-API-Key: rbhroxstezbhrbnagekfplr2iw6vzgb5x9myesf8rykh7maj"  -H "Content-Type: application/x.json-condition" -d 'alwaysTrue()'
    {"success":true}
    
  4. Delete the API key:

    $ curl -XPOST 'localhost:8081/tasks/api-key?action=delete&key=rbhroxstezbhrbnagekfplr2iw6vzgb5x9myesf8rykh7maj&APIKey=local_admin'
    API key deleted
    
  5. Verify the API key is inactive:

    $ curl -XPOST 'localhost:8081/tasks/api-key?action=view&key=rbhroxstezbhrbnagekfplr2iw6vzgb5x9myesf8rykh7maj&APIKey=local_admin'
    owner: me
    description: desc
    state: INACTIVE
    roles: standard
    issued: 10/20/16 4:49 PM
    

6: Verify the API can no longer authenticate:

$ curl -XPUT "localhost:8080/bus/1/test-sub" -H "X-BV-API-Key: rbhroxstezbhrbnagekfplr2iw6vzgb5x9myesf8rykh7maj"  -H "Content-Type: application/x.json-condition" -d 'alwaysTrue()'
{"reason":"not authenticated"}

Risk

Most the risk involves backwards compatibility. If there is an issue with backwards compatibility then existing API keys may not be able to access Emo.

Level

Medium

Required Testing

Manual

Risk Summary

In my opinion the riskiest area is the interplay between migrating and invalidating keys. Although I've tested this in detail I'd recommend a close review of the code to migrate API keys and then lookup API keys by internal ID: here and here.

Code Review Checklist

  • Tests are included. If not, make sure you leave us a line or two for the reason.
  • Pulled down the PR and performed verification of at least being able to
    build and run.
  • Well documented, including updates to any necessary markdown files. When
    we inevitably come back to this code it will only take hours to figure out, not
    days.
  • Consistent/Clear/Thoughtful? We are better with this code. We also aren't
    a victim of rampaging consistency, and should be using this course of action.
    We don't have coding standards out yet for this project, so please make sure to address any feedback regarding STYLE so the codebase remains consistent.
  • PR has a valid summary, and a good description.

@billkalter
Copy link
Contributor Author

Looks like DTQA may need updating.

@billkalter billkalter force-pushed the emo-6138 branch 2 times, most recently from eda9aaf to 11f9665 Compare October 21, 2016 19:23
@bv-jenkins
Copy link

This PR was tested by custom branch build on jenkins

@bv-jenkins
Copy link

This PR was tested by custom branch build on jenkins

@bdevore17 bdevore17 closed this Aug 30, 2018
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.

3 participants