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

Add basic member API #46

Merged
merged 3 commits into from
Apr 25, 2017
Merged

Add basic member API #46

merged 3 commits into from
Apr 25, 2017

Conversation

ZMYaro
Copy link
Member

@ZMYaro ZMYaro commented Apr 24, 2017

No description provided.

@dexw25
Copy link
Member

dexw25 commented Apr 24, 2017

@ZMYaro looks like this api allows complete access to the member database with no authentication, at least in a local test environment. Could allow a third party to access all member email addresses/DCEs for less than friendly purposes (probably just spam). Is that intended?

@dexw25
Copy link
Member

dexw25 commented Apr 24, 2017

Let me know if my concern is unwarranted, I just see a way 3rd parties could effectively fetch our entire distribution list, which may be an invasion of privacy depending on member preferences (Personally I would prefer that being a member of STAR not result in my email being listed publicly with my name and RIT DCE)

I do not know how hard it would be to implement some kind of authentication, if that is too much work then for the time being it may be better to simply not give out certain member fields, or at least limit it to information that is publicly available from other sources, depending on what is needed for the id scanner extension to function

@dexw25
Copy link
Member

dexw25 commented Apr 24, 2017

Example:
image

@ZMYaro
Copy link
Member Author

ZMYaro commented Apr 24, 2017

@derekw023: The short answer is yes, that could happen. IIRC, we discussed having some sort of API key system several meetings ago and decided it was not worth the effort considering how unlikely it would be that someone outside STAR would use the API.

One thing I might be able to do is, for the initial release, tie it to the existing authentication for the STAR site so it only returns e-mail addresses if you first sign into the STAR website, but that would mean whoever sets up the sign-in laptop would also need to log into the STAR site before the scanner would work. Removing e-mail addresses entirely is not an option because they get entered into the sign-in form.

@ZMYaro
Copy link
Member Author

ZMYaro commented Apr 24, 2017

@derekw023: Alternatively, since this is only being used for the ID card scanner right now, we could just limit it to only returning data to the Chrome extension for now.

@dexw25
Copy link
Member

dexw25 commented Apr 24, 2017

I think long term we need some kind of API key solution, I think the best (least hack-y) method would be an api key obtained from the website's admin page saved in the extension's local storage (correct me if this is not something that can be done)

For now I'd prefer the latter solution (access to the chrome extension only), less hack-y, more secure and maintainable than the other one from what I understand. We should have the STAR laptops signed into as little as possible given the way they are used.

@AlexFroio
Copy link

My concern is for bots which scan for websites with vulnerabilities like these. Even if we think no one outside STAR would use this, there is still a chance. More things have been lost to edge cases before and I believe we shouldn't play around with our member's info like this.

@ZMYaro
Copy link
Member Author

ZMYaro commented Apr 24, 2017

@derekw023, @AlexFroio: d40191f adds a basic API key system. It is not the prettiest, but it covers our current needs, and it should prevent unauthorized API access.

@dexw25
Copy link
Member

dexw25 commented Apr 25, 2017

Disclaimer: I am not a pentester, but looks fine to me. Good placeholder until the admin page is up to handle things like this. You have my
image

@ZMYaro ZMYaro requested a review from AlexFroio April 25, 2017 00:58
Copy link
Member

@dexw25 dexw25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

@ZMYaro ZMYaro merged commit 3f25ca3 into master Apr 25, 2017
@ZMYaro ZMYaro deleted the member-api branch April 25, 2017 01:07
@ZMYaro ZMYaro added this to the r5 milestone Sep 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants