Skip to content
This repository has been archived by the owner on Jun 6, 2018. It is now read-only.

Checks when adding APIs #350

Closed
PerMalmberg opened this issue Apr 2, 2016 · 9 comments
Closed

Checks when adding APIs #350

PerMalmberg opened this issue Apr 2, 2016 · 9 comments

Comments

@PerMalmberg
Copy link

@Adarnof we've discussed this before and it was then concluded there was a bug that was later fixed.

We got a new application from a prospective member, but the API provided only included basic info, kills and character sheet and only a single character, despite the settings as follows ():

IS_CORP = 'True' == os.environ.get('AA_IS_CORP', 'True')
MEMBER_API_MASK = os.environ.get('AA_MEMBER_API_MASK', 1073741823)
MEMBER_API_ACCOUNT = 'True' == os.environ.get('AA_MEMBER_API_ACCOUNT', 'True')
BLUE_API_MASK = os.environ.get('AA_BLUE_API_MASK', 8388608)
BLUE_API_ACCOUNT = 'True' == os.environ.get('AA_BLUE_API_ACCOUNT', 'False')

I tried this myself and the system gladly accepts APIs with less than the set requirements.

Am I missing something or is has this bug resurfaced?

@ghost
Copy link

ghost commented Apr 2, 2016

@PerMalmberg I found the same issue. When I had members refresh their API key though or if the API refresh task ran, it gladly removed those API keys that did not match requirements. I don't know if that is the case for you.

@PerMalmberg
Copy link
Author

No, this is when they first add the API.

@Adarnof
Copy link
Member

Adarnof commented Apr 2, 2016

The validation of keys works as follows:

  • a list of character states are compiled, for each character on the key plus the user's current state
  • the key is then checked against requirements for each state found

What's happening is the user is not member nor blue, and the character on the key is not member nor blue, so no checks are performed.

Would you prefer if the validation defaulted to member if no states are found?

@PerMalmberg
Copy link
Author

If I set the system to require a certain access mask, then it should not accept anything less.

And of course the user is not a member yet, s/he is about to do the application. So, we need a way to enforce all apis to the required mask prior to application creation.

@Adarnof
Copy link
Member

Adarnof commented Apr 3, 2016

Yet another reason I want to remove the blue/member distinction...

Easiest solution: Require all keys to meet the same mask.
Harder solution: Default to member requirements if not blue.

Still no idea why some keys are slipping through for members.

@orbitroom
Copy link
Collaborator

@Adarnof option 3 would be to validate the keys when their added and raise and exception so that it shows up on the api key form it self and then it will tell the user what their major malfunction was

@PerMalmberg
Copy link
Author

I'm not sure how it should work when used in alliance mode, but when IS_CORP is true, the validation logic is simple: any API not matching MEMBER_API_MASK should be rejected.

Also, the text (lings to API creation etc.) for "blue" shouldn't be visible at all when IS_CORP is true.

@Nutbolt52
Copy link

I have seen this occurring a lot with my install. Its not a ground breaking bug because people should learn to read, but it does result in a lot of people being confused. Not really sure what else I can do to help though. I run AllianceAuth in Corp mode and have a minimum required API set to the max. People can register/add API key with a lower mask, it accepts it and grants them access to the appropriate services, but then on the next celery run it removes their access as it notices the API key doesn't have the right mask.

@Adarnof Adarnof mentioned this issue Sep 10, 2016
16 tasks
Adarnof added a commit that referenced this issue Sep 10, 2016
Remove unused error count on API key model.
Restructure API key refresh task to queue all keys per user and await completion.
Closes #350
@Adarnof
Copy link
Member

Adarnof commented Sep 11, 2016

For those wondering:

This would only occur the first time a new user added a key which was not up to mask requirements. This was caused by incorrectly comparing corp/alliance IDs from models (strings) to those from settings.py (ints). This would determine non-blue characters were none, rather than members. After the key was added, a separate logic function would correctly identify the users were members/blues and apply the correct API checks during the 3-hour api refresh cycle. After adding one key, the user would be identified as the correct member/blue state so any subsequent key additions would be correctly checked.

Adarnof added a commit that referenced this issue Sep 11, 2016
Adarnof added a commit that referenced this issue Oct 16, 2016
* Port to Django 1.10
Initial migrations for current states of all models. Requires faking to retain data.
Removed all references to render_to_response, replacing with render shortcut.
Same for HttpResponseRedirect to render shortcut.
Corrected notification signal import to wait for app registry to finish loading.

* Correct typos from render conversion

* Modify models to suppress Django field warnings

* Script for automatic database conversion
 - fakes initial migrations to preserve data
Include LOGIN_URL setting

* Correct context processor import typo

* Removed pathfinder support.
Current pathfinder versions require SSO, not APIs added to database.
Conditionally load additional database definitions only if services are enabled.
Prevents errors when running auth without creating all possible databases.

* Condense context processors

* Include Django 1.10 installation in migrate script
Remove syncdb/evolve, replace with migrate for update script

* Replaced member/blue perms with user state system
Removed sigtracker
Initial migrations for default perms and groups
Removed perm bootstrapping on first run

* Clean up services list

* Remove fleet fittings page

* Provide action feedback via django messaging
Display unread notification count
Correct left navbar alignment

* Stop storing service passwords.
Provide them one time upon activation or reset.
Closes #177

* Add group sync buttons to admin site
Allow searcing of AuthServicesInfo models
Display user main character

* Correct button CSS to remove underlines on hover

* Added bulk actions to notifications
Altered notification default ordering

* Centralize API key validation.
Remove unused error count on API key model.
Restructure API key refresh task to queue all keys per user and await completion.
Closes #350

* Example configuration files for supervisor.
Copy to /etc/supervisor/conf.d and restart to take effect.
Closes #521
Closes #266

* Pre-save receiver for member/blue state switching
Removed is_blue field
Added link to admin site

* Remove all hardcoded URLs from views and templates
Correct missing render arguments
Closes #540

* Correct celeryd process directory

* Migration to automatically set user states.
Runs instead of waiting for next API refresh cycle. Should make the transition much easier.

* Verify service accounts accessible to member state

* Restructure project to remove unnecessary apps.
(celerytask, util, portal, registraion apps)
Added workarounds for python 3 compatibility.

* Correct python2 compatibility

* Check services against state being changed to

* Python3 compatibility fixes

* Relocate x2bool py3 fix

* SSO integration for logging in to existing accounts.

* Add missing url names for fleetup reverse

* Sanitize groupnames before syncing.

* Correct trailing slash preventing url resolution

* Alter group name sanitization to allow periods and hyphens

* Correct state check on pre_save model for corp/alliance group assignment

* Remove sigtracker table from old dbs to allow user deletion

* Include missing celery configuration

* Teamspeak error handling

* Prevent celery worker deadlock on async group result wait

* Correct active navbar links for translated urls.
Correct corp status url resolution for some links.
Remove DiscordAuthToken model.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants